| Message ID | 1551438348-22119-2-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add USB-HOST support to cat874 | expand |
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@vger.kernel.org # v4.4+ > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > --- 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? 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
Hi Yoshihiro-san, Thank you for your feedback. > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > 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@vger.kernel.org # v4.4+ > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > > 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? 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
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@renesas.com> > > 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@vger.kernel.org # v4.4+ > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > --- > > > > 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. 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
Hi Yoshihiro-san, Thank you for your feedback. > -----Original Message----- > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Sent: 02 April 2019 12:10 > To: Fabrizio Castro <fabrizio.castro@bp.renesas.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Geert Uytterhoeven > <geert+renesas@glider.be> > Cc: Arnd Bergmann <arnd@arndb.de>; linux-usb@vger.kernel.org; Simon Horman <horms@verge.net.au>; Chris Paterson > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>; linux-renesas-soc@vger.kernel.org; stable@vger.kernel.org > 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@renesas.com> > > > 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@vger.kernel.org # v4.4+ > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > > > > > 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
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@renesas.com> > > > > 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@vger.kernel.org # v4.4+ > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > > --- > > > > > > > > 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@renesas.com> 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
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) {
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@vger.kernel.org # v4.4+ Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- drivers/usb/common/common.c | 2 ++ 1 file changed, 2 insertions(+)