| Message ID | 20201212165648.166220-1-aford173@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ravb: Add support for optional txc_refclk | expand |
On Sat, Dec 12, 2020 at 11:55 AM Sergei Shtylyov <sergei.shtylyov@gmail.com> wrote: > > Hello! > > On 12.12.2020 19:56, Adam Ford wrote: > > > The SoC expects the txv_refclk is provided, but if it is provided > > by a programmable clock, there needs to be a way to get and enable > > this clock to operate. It needs to be optional since it's only > > necessary for those with programmable clocks. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > index bd30505fbc57..4c3f95923ef2 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2148,6 +2148,18 @@ static int ravb_probe(struct platform_device *pdev) > > goto out_release; > > } > > > > + priv->ref_clk = devm_clk_get(&pdev->dev, "txc_refclk"); > > Why not devm_clk_get_optional()? I am not that familiar with the clock API. I'll look into that function. It looks like it makes more sense. I'll send a V2. adam > > > + if (IS_ERR(priv->ref_clk)) { > > + if (PTR_ERR(priv->ref_clk) == -EPROBE_DEFER) { > > + /* for Probe defer return error */ > > + error = PTR_ERR(priv->ref_clk); > > + goto out_release; > > + } > > + /* Ignore other errors since it's optional */ > > + } else { > > + (void)clk_prepare_enable(priv->ref_clk); > > + } > > + > > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > > ndev->min_mtu = ETH_MIN_MTU; > > > > MBR, Sergei
Hi Adam, On Sun, Dec 13, 2020 at 5:18 PM Adam Ford <aford173@gmail.com> wrote: > The SoC expects the txv_refclk is provided, but if it is provided > by a programmable clock, there needs to be a way to get and enable > this clock to operate. It needs to be optional since it's only > necessary for those with programmable clocks. > > Signed-off-by: Adam Ford <aford173@gmail.com> Thanks for your patch! > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -994,6 +994,7 @@ struct ravb_private { > struct platform_device *pdev; > void __iomem *addr; > struct clk *clk; > + struct clk *ref_clk; > 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 bd30505fbc57..4c3f95923ef2 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2148,6 +2148,18 @@ static int ravb_probe(struct platform_device *pdev) > goto out_release; > } > > + priv->ref_clk = devm_clk_get(&pdev->dev, "txc_refclk"); Please also update the DT bindings[1], to document the optional presence of the clock. > + if (IS_ERR(priv->ref_clk)) { > + if (PTR_ERR(priv->ref_clk) == -EPROBE_DEFER) { > + /* for Probe defer return error */ > + error = PTR_ERR(priv->ref_clk); > + goto out_release; > + } > + /* Ignore other errors since it's optional */ > + } else { > + (void)clk_prepare_enable(priv->ref_clk); This can fail. Does this clock need to be enabled all the time? At least it should be disabled in the probe failure path, and in ravb_remove(). [1] Documentation/devicetree/bindings/net/renesas,etheravb.yaml 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
On Mon, Dec 14, 2020 at 4:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Adam, > > On Sun, Dec 13, 2020 at 5:18 PM Adam Ford <aford173@gmail.com> wrote: > > The SoC expects the txv_refclk is provided, but if it is provided > > by a programmable clock, there needs to be a way to get and enable > > this clock to operate. It needs to be optional since it's only > > necessary for those with programmable clocks. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > Thanks for your patch! > > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -994,6 +994,7 @@ struct ravb_private { > > struct platform_device *pdev; > > void __iomem *addr; > > struct clk *clk; > > + struct clk *ref_clk; > > 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 bd30505fbc57..4c3f95923ef2 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2148,6 +2148,18 @@ static int ravb_probe(struct platform_device *pdev) > > goto out_release; > > } > > > > + priv->ref_clk = devm_clk_get(&pdev->dev, "txc_refclk"); > > Please also update the DT bindings[1], to document the optional > presence of the clock. I am not all that familiar with the YAML syntax, but right now, the clock-names property isn't in the binding, and the driver doesn't use a name when requesting the single clock it's expecting. Since the txc_refclk is optional, can the clock-names property allow for 0-2 names while the number of clocks be 1-2? clocks: minItems: 1 maxItems: 2 clock-names: minItems: 0 maxItems: 2 items: enum: - fck # AVB functional clock (optional if it is the only clock) - txc_refclk # TXC reference clock With the above proposal, the clock-names would only be necessary when using the txc_refclk. > > > + if (IS_ERR(priv->ref_clk)) { > > + if (PTR_ERR(priv->ref_clk) == -EPROBE_DEFER) { > > + /* for Probe defer return error */ > > + error = PTR_ERR(priv->ref_clk); > > + goto out_release; > > + } > > + /* Ignore other errors since it's optional */ > > + } else { > > + (void)clk_prepare_enable(priv->ref_clk); > > This can fail. > Does this clock need to be enabled all the time? > At least it should be disabled in the probe failure path, and in > ravb_remove(). I'll do that for the next rev. thanks, adam > > [1] Documentation/devicetree/bindings/net/renesas,etheravb.yaml > > 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, CC devicetree On Mon, Dec 28, 2020 at 2:49 PM Adam Ford <aford173@gmail.com> wrote: > On Mon, Dec 14, 2020 at 4:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Sun, Dec 13, 2020 at 5:18 PM Adam Ford <aford173@gmail.com> wrote: > > > The SoC expects the txv_refclk is provided, but if it is provided > > > by a programmable clock, there needs to be a way to get and enable > > > this clock to operate. It needs to be optional since it's only > > > necessary for those with programmable clocks. > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > Thanks for your patch! > > > > > --- a/drivers/net/ethernet/renesas/ravb.h > > > +++ b/drivers/net/ethernet/renesas/ravb.h > > > @@ -994,6 +994,7 @@ struct ravb_private { > > > struct platform_device *pdev; > > > void __iomem *addr; > > > struct clk *clk; > > > + struct clk *ref_clk; > > > 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 bd30505fbc57..4c3f95923ef2 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -2148,6 +2148,18 @@ static int ravb_probe(struct platform_device *pdev) > > > goto out_release; > > > } > > > > > > + priv->ref_clk = devm_clk_get(&pdev->dev, "txc_refclk"); > > > > Please also update the DT bindings[1], to document the optional > > presence of the clock. > > I am not all that familiar with the YAML syntax, but right now, the > clock-names property isn't in the binding, and the driver doesn't use > a name when requesting the single clock it's expecting. > Since the txc_refclk is optional, can the clock-names property allow > for 0-2 names while the number of clocks be 1-2? > > clocks: > minItems: 1 > maxItems: 2 > > clock-names: > minItems: 0 > maxItems: 2 > items: > enum: > - fck # AVB functional clock (optional if it is the only clock) > - txc_refclk # TXC reference clock With "enum", it accepts any order. But for compatibility, we want to force "fck" first. Something like: clocks: minItems: 1 items: - description: AVB functional clock - description: Optional TXC reference clock clock-names: items: - const: fck - const: txc_refclk > With the above proposal, the clock-names would only be necessary when > using the txc_refclk. I think that's difficult to express: either make clock-names optional (i.e. don't list it under "required"), or make it required in all cases (which need fixups for the existing users, and "minItems: 1" for "clock-names", too). > > > + if (IS_ERR(priv->ref_clk)) { > > > + if (PTR_ERR(priv->ref_clk) == -EPROBE_DEFER) { > > > + /* for Probe defer return error */ > > > + error = PTR_ERR(priv->ref_clk); > > > + goto out_release; > > > + } > > > + /* Ignore other errors since it's optional */ > > > + } else { > > > + (void)clk_prepare_enable(priv->ref_clk); > > > > This can fail. > > Does this clock need to be enabled all the time? > > At least it should be disabled in the probe failure path, and in > > ravb_remove(). > > I'll do that for the next rev. > > thanks, > > adam > > > > [1] Documentation/devicetree/bindings/net/renesas,etheravb.yaml 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
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 7453b17a37a2..ddf3bc5164d2 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -994,6 +994,7 @@ struct ravb_private { struct platform_device *pdev; void __iomem *addr; struct clk *clk; + struct clk *ref_clk; 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 bd30505fbc57..4c3f95923ef2 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2148,6 +2148,18 @@ static int ravb_probe(struct platform_device *pdev) goto out_release; } + priv->ref_clk = devm_clk_get(&pdev->dev, "txc_refclk"); + if (IS_ERR(priv->ref_clk)) { + if (PTR_ERR(priv->ref_clk) == -EPROBE_DEFER) { + /* for Probe defer return error */ + error = PTR_ERR(priv->ref_clk); + goto out_release; + } + /* Ignore other errors since it's optional */ + } else { + (void)clk_prepare_enable(priv->ref_clk); + } + ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); ndev->min_mtu = ETH_MIN_MTU;
The SoC expects the txv_refclk is provided, but if it is provided by a programmable clock, there needs to be a way to get and enable this clock to operate. It needs to be optional since it's only necessary for those with programmable clocks. Signed-off-by: Adam Ford <aford173@gmail.com>