Hi Fabrizio-san,
> From: Fabrizio Castro, Sent: Tuesday, April 2, 2019 8:47 PM
>
> Hi Yoshihiro-san,
>
> Thank you for your feedback.
>
> > 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".
Thank you very much for the detailed explanation. I agree with you. This API should not get
the dr_mode from a disabled node. For example, descriping the following device nodes are nonsense:
&host {
dr_mode = "peripheral"; // <--- here
status = "disabled";
}
&peripheral {
dr_mode = "peripheral";
status = "okay";
}
> 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.
I agree this patch is valid for both mainline and stable versions because all this API users
in v5.1-rc4 are called with the second argument -1 or 0 and then the behavior should be
the same as before. So,
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
Best regards,
Yoshihiro Shimoda
> 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
