Message ID | 20210412132619.7896-1-aford173@gmail.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [V4,1/2] dt-bindings: net: renesas,etheravb: Add additional clocks | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success | |
robh/dtbs-check | success |
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 12 Apr 2021 08:26:18 -0500 you wrote: > The AVB driver assumes there is an external crystal, but it could > be clocked by other means. In order to enable a programmable > clock, it needs to be added to the clocks list and enabled in the > driver. Since there currently only one clock, there is no > clock-names list either. > > Update bindings to add the additional optional clock, and explicitly > name both of them. > > [...] Here is the summary with links: - [V4,1/2] dt-bindings: net: renesas,etheravb: Add additional clocks https://git.kernel.org/netdev/net-next/c/6f43735b6da6 - [V4,2/2] net: ethernet: ravb: Enable optional refclk https://git.kernel.org/netdev/net-next/c/8ef7adc6beb2 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Hi Adam, On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > For devices that use a programmable clock for the AVB reference clock, > the driver may need to enable them. Add code to find the optional clock > and enable it when available. > > Signed-off-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > --- > V4: Eliminate the NULL check when disabling refclk, and add a line > to disable the refclk if there is a failure after it's been > initialized. Thanks for the update! > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > goto out_release; > } > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > + if (IS_ERR(priv->refclk)) { > + error = PTR_ERR(priv->refclk); > + goto out_release; Note that this will call clk_disable_unprepare() in case of failure, which is fine, as that function is a no-op in case of a failed clock. > + } > + clk_prepare_enable(priv->refclk); > + > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > ndev->min_mtu = ETH_MIN_MTU; > > @@ -2244,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev) > if (chip_id != RCAR_GEN2) > ravb_ptp_stop(ndev); > out_release: > + clk_disable_unprepare(priv->refclk); > free_netdev(ndev); > > pm_runtime_put(&pdev->dev); Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Tue, Apr 13, 2021 at 2:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Adam, > > On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > > For devices that use a programmable clock for the AVB reference clock, > > the driver may need to enable them. Add code to find the optional clock > > and enable it when available. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > --- > > V4: Eliminate the NULL check when disabling refclk, and add a line > > to disable the refclk if there is a failure after it's been > > initialized. > > Thanks for the update! > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > > goto out_release; > > } > > > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > > + if (IS_ERR(priv->refclk)) { > > + error = PTR_ERR(priv->refclk); > > + goto out_release; > > Note that this will call clk_disable_unprepare() in case of failure, which is > fine, as that function is a no-op in case of a failed clock. Geert, A bot reported that if I jump to out_release may try to free a clock if some instances where priv isn't defined. Currently, the priv->clk isn't freed either. I have heard some back-and-forth discussions in other threads on whether or not devm functions auto free or not. I'm fine with sending a V5 to make the free for the refclock happen only when the priv has successfully initialized. Should I also add one for freeing priv->clk and change all the other goto out_release commands to point to this new section? I am thinking it would like something like: free_refclk: clk_disable_unprepare(priv->refclk); free_clk; clk_disable_unprepare(priv->clk); out_release: free_netdev(ndev); .... adam > > > + } > > + clk_prepare_enable(priv->refclk); > > + > > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > > ndev->min_mtu = ETH_MIN_MTU; > > > > @@ -2244,6 +2251,7 @@ static int ravb_probe(struct platform_device *pdev) > > if (chip_id != RCAR_GEN2) > > ravb_ptp_stop(ndev); > > out_release: > > + clk_disable_unprepare(priv->refclk); > > free_netdev(ndev); > > > > pm_runtime_put(&pdev->dev); > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Adam, On Wed, Apr 14, 2021 at 3:08 PM Adam Ford <aford173@gmail.com> wrote: > On Tue, Apr 13, 2021 at 2:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Apr 12, 2021 at 3:27 PM Adam Ford <aford173@gmail.com> wrote: > > > For devices that use a programmable clock for the AVB reference clock, > > > the driver may need to enable them. Add code to find the optional clock > > > and enable it when available. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > > --- > > > V4: Eliminate the NULL check when disabling refclk, and add a line > > > to disable the refclk if there is a failure after it's been > > > initialized. > > > > Thanks for the update! > > > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -2148,6 +2148,13 @@ static int ravb_probe(struct platform_device *pdev) > > > goto out_release; > > > } > > > > > > + priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk"); > > > + if (IS_ERR(priv->refclk)) { > > > + error = PTR_ERR(priv->refclk); > > > + goto out_release; > > > > Note that this will call clk_disable_unprepare() in case of failure, which is > > fine, as that function is a no-op in case of a failed clock. > > Geert, > > A bot reported that if I jump to out_release may try to free a clock > if some instances where priv isn't defined. As priv is allocated using alloc_etherdev_mqs(), priv->refclk is NULL initially, but priv itself may indeed not be initialized when the first "goto out_release" is taken. Sorry for missing that. > Currently, the priv->clk isn't freed either. I have heard some > back-and-forth discussions in other threads on whether or not devm > functions auto free or not. The devm_clk_get_optional() will be undone automatically, so there is no need to handle that explicitly. > I'm fine with sending a V5 to make the free for the refclock happen > only when the priv has successfully initialized. Should I also add As this patch has been applied to net-next, you''ll have to send a follow-up fix patch, not a v5. > one for freeing priv->clk and change all the other goto out_release > commands to point to this new section? No, not for priv->clk, due to devm_*(). > I am thinking it would like something like: > > free_refclk: > clk_disable_unprepare(priv->refclk); OK. > free_clk; > clk_disable_unprepare(priv->clk); NAK, as priv->clk is not enabled in ravb_probe(). > out_release: > free_netdev(ndev); > .... OK. Thanks! Gr{oetje,eeting}s, Geert
diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml index 91ba96d43c6c..fe72a5598add 100644 --- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml +++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml @@ -50,7 +50,16 @@ properties: interrupt-names: true clocks: - maxItems: 1 + minItems: 1 + maxItems: 2 + items: + - description: AVB functional clock + - description: Optional TXC reference clock + + clock-names: + items: + - const: fck + - const: refclk iommus: maxItems: 1