RE: [PATCH 2/3] usb: renesas_usbhs: Enable support for more than two clks

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Adam,

> From: Adam Ford, Sent: Saturday, August 28, 2021 3:58 AM
> 
> On Thu, Jul 1, 2021 at 6:34 AM Yoshihiro Shimoda wrote:
> >
> > Hi Adam, Geert-san,
> >
> > > From: Geert Uytterhoeven, Sent: Thursday, July 1, 2021 6:07 PM
> > >
> > > Hi Adam,
> > >
> > > Thanks for your patch!
> > >
> > > On Wed, Jun 30, 2021 at 7:30 PM Adam Ford wrote:
> > <snip>
> > > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> > > > index 3af91b2b8f76..255e4bd68ed3 100644
> > > > --- a/drivers/usb/renesas_usbhs/common.c
> > > > +++ b/drivers/usb/renesas_usbhs/common.c
> > <snip>
> > > > @@ -309,11 +311,13 @@ static int usbhsc_clk_get(struct device *dev, struct usbhs_priv *priv)
> > > >          * To backward compatibility with old DT, this driver checks the return
> > > >          * value if it's -ENOENT or not.
> > > >          */
> > > > -       priv->clks[1] = of_clk_get(dev_of_node(dev), 1);
> > > > -       if (PTR_ERR(priv->clks[1]) == -ENOENT)
> > > > -               priv->clks[1] = NULL;
> > > > -       else if (IS_ERR(priv->clks[1]))
> > > > -               return PTR_ERR(priv->clks[1]);
> > > > +       for (i = 1; i < ARRAY_SIZE(priv->clks); i++) {
> > > > +               priv->clks[1] = of_clk_get(dev->of_node, i);
> >
> > s/clks[1]/[i]/
> >
> > > > +               if (PTR_ERR(priv->clks[i]) == -ENOENT)
> > > > +                       priv->clks[i] = NULL;
> > > > +               else if (IS_ERR(priv->clks[i]))
> > > > +                       return PTR_ERR(priv->clks[i]);
> > > > +       }
> > >
> > > This is identical to the current code, as ARRAY_SIZE(priv->clks) == 2.
> > > Probably you wanted to increase usbhs_priv.clks[], too?
> 
> Thanks for the review.  Sorry for my delayed response.  I broke my
> wrist on the evening on June 30, and I was on medical leave for 6
> weeks.  I am now trying to get caught up to where I was.

I'm sorry to hear that. I hope you recover soon.

> >
> > I think so.
> > # I realized the clks array is only 2, so that this driver cannot
> > # enable usb2_clksel...
> >
> > > Does it make sense to start using the clk_bulk*() API?
> >
> > clk_bulk*() API seems to need clock-names property.
> > Is my understanding correct? However, the hsusb nodes doesn't
> > have the property for now...
> 
> The usb2_clksel has the names "ehci_ohci" and "hs-usb-if" for <&cpg
> CPG_MOD 703> and <&cpg CPG_MOD 704>, respectively.
> 
> What if I add the clock-names property, then switch to the clk_bulk
> IO?  I think we can just turn them all on together if that's
> acceptable

According to an article [1],
=====================================================
forward and backward dts compatibility

Current practice is that
* new kernels work with old devicetrees
* old kernels may or may not work with new devicetrees
=====================================================
[1]
https://elinux.org/Device_Tree_Linux#forward_and_backward_dts_compatibility

So, IIUC, we have to modify binding doc, dts/dtsi files and the driver code step-by-step
to achieve the compatibility.
1) Modify binding doc to add clock-names.
2) Modify the renesas_usbhs driver to check clock-names.
3) Modify all dts/dtsi files to add clock-names.

We can make "1) and 2)" patches, and "3)" patch in parallel.
And, the 2) should take care of forward and backword dts compatibility,
IIUC.

Best regards,
Yoshihiro Shimoda