Hi Yoshihiro-san,
Thank you for your feedback.
> -----Original Message-----
> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Sent: 02 April 2019 12:10
> To: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Simon Horman <horms@xxxxxxxxxxxx>; Chris Paterson
> <Chris.Paterson2@xxxxxxxxxxx>; Biju Das <biju.das@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/4] usb: common: Consider only available nodes for dr_mode
>
> Hi Fabrizio-san,
>
> > From: Fabrizio Castro, Sent: Tuesday, April 2, 2019 6:22 PM
> >
> > Hi Yoshihiro-san,
> >
> > Thank you for your feedback.
> >
> > > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > Sent: 02 April 2019 02:54
> > > Subject: RE: [PATCH 1/4] usb: common: Consider only available nodes for dr_mode
> > >
> > > Hi Fabrizio-san,
> > >
> > > Thank you for the patch!
> > >
> > > > From: Fabrizio Castro, Sent: Friday, March 1, 2019 8:06 PM
> > > >
> > > > There are cases where multiple device tree nodes point to the
> > > > same phy node by means of the "phys" property, but we should
> > > > only consider those nodes that are marked as available rather
> > > > than just any node.
> > > >
> > > > Fixes: 98bfb3946695 ("usb: of: add an api to get dr_mode by the phy node")
> > > > Cc: stable@xxxxxxxxxxxxxxx # v4.4+
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > > > ---
> > >
> > > I'm guessing this code needs for phy-rcar-gen3-usb2.c only because
> > > the phy driver only gets the dr_mode from index 0 like below:
> > >
> > > channel->dr_mode = of_usb_get_dr_mode_by_phy(dev->of_node, 0);
> > >
> > > Yesterday, I submitted patches to get multiple indexes from controller
> > > device nodes:
> > >
> > > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=99561
> > >
> > > So, would you check the phy patches can get the dr_mode without this changing common patch?
> >
> > I will go through your patch series, but I can't see why any driver would be interested
> > in considering a disabled node for getting dr_mode, can you?
> > Do you have a use case for this?
>
> This is not a use case though, the commit 98bfb3946695 is made by a TI engineer so that
> I checked TI's dts/dtsi file on armv7 arch (am335x-boneblue.dts) a little and then
> it has one usb controller node only. So, the commit didn't take care of a disabled node, I guess.
> However, as you know, on R-Car Gen3 and RZ/G2, they have multiple controller nodes. So,
> one of the controller can be disabled.
The real question is, are we going to need to read dr_mode from a disabled node?
Because this patch gets in the way of that, but looking through the code and
device trees it doesn't look like anybody needs that. I have tested this patch in isolation
on a few R-Car platforms, and it all seemed good to me back when I submitted the patch,
but it would probably need thorough testing, and more testing from your side is more than
welcome. Besides this point, of_usb_get_dr_mode_by_phy returns the dr_mode value that
matches the argument the first, which means what you get back from it depends from
the order in which you specify the nodes within the device tree in case you have multiple
nodes meeting the same requirements (from a of_usb_get_dr_mode_by_phy perspective),
a.k.a. "luck".
We should make sure there is only one node that can match our requirement at any one
time for this to work properly, and parsing only enabled nodes gets in that direction,
therefore I still think this patch is valid, and belongs to stable versions of the kernel
as well.
After reading your comment on patch:
"phy: renesas: rcar-gen3-usb2: No need to request IRQ for non-OTG"
I can now see that patch "arm64: dts: renesas: r8a77995: draak: Remove hsusb node" is wrong
for your case, and that we need to find a better solution for patch:
"phy: renesas: rcar-gen3-usb2: No need to request IRQ for non-OTG"
I am looking into your series now, I'll come back to you soon on the other patch.
Thanks,
Fab
>
> By the way, if we applied this patch, since the behavior will be changed at least,
> so all of this api user have to test whether this code doesn't break or not.
>
> Best regards,
> Yoshihiro Shimoda
>
> > Thanks,
> > Fab
> >
> > >
> > > Best regards,
> > > Yoshihiro Shimoda
> > >
> > > > drivers/usb/common/common.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > > > index 48277bb..73c8e65 100644
> > > > --- a/drivers/usb/common/common.c
> > > > +++ b/drivers/usb/common/common.c
> > > > @@ -145,6 +145,8 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0)
> > > >
> > > > do {
> > > > controller = of_find_node_with_property(controller, "phys");
> > > > + if (!of_device_is_available(controller))
> > > > + continue;
> > > > index = 0;
> > > > do {
> > > > if (arg0 == -1) {
> > > > --
> > > > 2.7.4
