* [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks @ 2021-04-12 13:26 Adam Ford 2021-04-12 13:26 ` [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk Adam Ford 2021-04-12 21:20 ` [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks patchwork-bot+netdevbpf 0 siblings, 2 replies; 6+ messages in thread From: Adam Ford @ 2021-04-12 13:26 UTC (permalink / raw) To: netdev Cc: aford, Adam Ford, Sergei Shtylyov, David S. Miller, Jakub Kicinski, Rob Herring, linux-renesas-soc, devicetree, linux-kernel 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. Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Acked-by: Rob Herring <robh@kernel.org> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com> --- V4: No Change V3: No Change V2: No Change 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 -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk 2021-04-12 13:26 [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks Adam Ford @ 2021-04-12 13:26 ` Adam Ford 2021-04-13 7:33 ` Geert Uytterhoeven 2021-04-12 21:20 ` [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks patchwork-bot+netdevbpf 1 sibling, 1 reply; 6+ messages in thread From: Adam Ford @ 2021-04-12 13:26 UTC (permalink / raw) To: netdev Cc: aford, Adam Ford, Sergei Shtylyov, David S. Miller, Jakub Kicinski, Rob Herring, linux-renesas-soc, devicetree, linux-kernel 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. V3: Change 'avb' to 'AVB' Remove unnessary else statement and pointer maniupluation when enabling the refclock. Add disable_unprepare call in remove funtion. V2: The previous patch to fetch the fclk was dropped. In its place is code to enable the refclk diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index cb47e68c1a3e..86a1eb0634e8 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -993,6 +993,7 @@ struct ravb_private { struct platform_device *pdev; void __iomem *addr; struct clk *clk; + struct clk *refclk; struct mdiobb_ctrl mdiobb; u32 num_rx_ring[NUM_RX_QUEUE]; u32 num_tx_ring[NUM_TX_QUEUE]; diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index eb0c03bdb12d..1409ae986aa2 100644 --- 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; + } + 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); @@ -2260,6 +2268,8 @@ static int ravb_remove(struct platform_device *pdev) if (priv->chip_id != RCAR_GEN2) ravb_ptp_stop(ndev); + clk_disable_unprepare(priv->refclk); + dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat, priv->desc_bat_dma); /* Set reset mode */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk 2021-04-12 13:26 ` [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk Adam Ford @ 2021-04-13 7:33 ` Geert Uytterhoeven 2021-04-14 13:07 ` Adam Ford 0 siblings, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2021-04-13 7:33 UTC (permalink / raw) To: Adam Ford Cc: netdev, Adam Ford-BE, Sergei Shtylyov, David S. Miller, Jakub Kicinski, Rob Herring, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List 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 -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk 2021-04-13 7:33 ` Geert Uytterhoeven @ 2021-04-14 13:07 ` Adam Ford 2021-04-14 17:59 ` Geert Uytterhoeven 0 siblings, 1 reply; 6+ messages in thread From: Adam Ford @ 2021-04-14 13:07 UTC (permalink / raw) To: Geert Uytterhoeven Cc: netdev, Adam Ford-BE, Sergei Shtylyov, David S. Miller, Jakub Kicinski, Rob Herring, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk 2021-04-14 13:07 ` Adam Ford @ 2021-04-14 17:59 ` Geert Uytterhoeven 0 siblings, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2021-04-14 17:59 UTC (permalink / raw) To: Adam Ford Cc: netdev, Adam Ford-BE, Sergei Shtylyov, David S. Miller, Jakub Kicinski, Rob Herring, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List 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 -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks 2021-04-12 13:26 [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks Adam Ford 2021-04-12 13:26 ` [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk Adam Ford @ 2021-04-12 21:20 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2021-04-12 21:20 UTC (permalink / raw) To: Adam Ford Cc: netdev, aford, sergei.shtylyov, davem, kuba, robh+dt, linux-renesas-soc, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-14 18:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-12 13:26 [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks Adam Ford 2021-04-12 13:26 ` [PATCH V4 2/2] net: ethernet: ravb: Enable optional refclk Adam Ford 2021-04-13 7:33 ` Geert Uytterhoeven 2021-04-14 13:07 ` Adam Ford 2021-04-14 17:59 ` Geert Uytterhoeven 2021-04-12 21:20 ` [PATCH V4 1/2] dt-bindings: net: renesas,etheravb: Add additional clocks patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).