| Message ID | 1464888666-17728-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote: > Some SoCs have a single phy-hw-block with multiple phys, this is > modelled by a single phy dts node, so we end up with multiple > controller nodes with a phys property pointing to the phy-node > of the otg-phy. > > Only one of these controllers typically is an otg controller, yet we Is it guaranteed that only one of them will be otg? > were checking the first controller who uses a phy from the block and > then end up looking for a dr_mode property in e.g. the ehci controller. > > Instead of looking for nodes with a phy property, look for nodes > with a dr_mode property, so that we actually access the dr_mode property > in a node which has it. Quote from Documentation/devicetree/bindings/usb/generic.txt: "- dr_mode: ... In case this attribute isn't passed via DT, USB DRD controllers should default to OTG." So it is not mandatory to define dr_mode in DT, then you wouldn't be able to find the controller in such case. Regards, -Bin.
Hi, On 02-06-16 20:16, Bin Liu wrote: > Hi, > > On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote: >> Some SoCs have a single phy-hw-block with multiple phys, this is >> modelled by a single phy dts node, so we end up with multiple >> controller nodes with a phys property pointing to the phy-node >> of the otg-phy. >> >> Only one of these controllers typically is an otg controller, yet we > > Is it guaranteed that only one of them will be otg? I guess not, but if there are 2 then with my patch we are of no worse then today, we will then pick the first otg controller. Whereas of_usb_get_dr_mode_by_phy currently is broken even on setups with a shared phy dt-node and only 1 otg controller, which are quite common. I believe it is unlikely that there will be more then 1 otg controller, so I believe that we should not worry about this until we actually hit this problem. >> were checking the first controller who uses a phy from the block and>> then end up looking for a dr_mode property in e.g. the ehci controller. >> >> Instead of looking for nodes with a phy property, look for nodes >> with a dr_mode property, so that we actually access the dr_mode property >> in a node which has it. > > Quote from Documentation/devicetree/bindings/usb/generic.txt: > "- dr_mode: ... > In case this attribute isn't > passed via DT, USB DRD controllers should default to > OTG." > > So it is not mandatory to define dr_mode in DT, then you wouldn't be > able to find the controller in such case. If no controller is found then of_usb_get_dr_mode_by_phy will return USB_DR_MODE_UNKNOWN, just like it does today for controller nodes which do not set dr_mode. So in this case my patch does not change anything. Regards, Hans
Hi, On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote: > Some SoCs have a single phy-hw-block with multiple phys, this is > modelled by a single phy dts node, so we end up with multiple > controller nodes with a phys property pointing to the phy-node > of the otg-phy. Maybe we should try to model each phy with a separate dt node? Thanks Kishon > > Only one of these controllers typically is an otg controller, yet we > were checking the first controller who uses a phy from the block and > then end up looking for a dr_mode property in e.g. the ehci controller. > > Instead of looking for nodes with a phy property, look for nodes > with a dr_mode property, so that we actually access the dr_mode property > in a node which has it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/common/common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > index e3d0161..9806433 100644 > --- a/drivers/usb/common/common.c > +++ b/drivers/usb/common/common.c > @@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np) > int err; > > do { > - controller = of_find_node_with_property(controller, "phys"); > + controller = of_find_node_with_property(controller, "dr_mode"); > index = 0; > do { > phy = of_parse_phandle(controller, "phys", index); >
Hi, On 03-06-16 13:20, Kishon Vijay Abraham I wrote: > Hi, > > On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote: >> Some SoCs have a single phy-hw-block with multiple phys, this is >> modelled by a single phy dts node, so we end up with multiple >> controller nodes with a phys property pointing to the phy-node >> of the otg-phy. > > Maybe we should try to model each phy with a separate dt node? That seems like making things unnecessarily complicated. If we want to be 100% sure that of_usb_get_dr_mode_by_phy finds the right controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy and make it check that first argument specified to the phandle used in the controller node matches the passed in index. And use index == -1 to skip this test. Regards, Hans > > Thanks > Kishon >> >> Only one of these controllers typically is an otg controller, yet we >> were checking the first controller who uses a phy from the block and >> then end up looking for a dr_mode property in e.g. the ehci controller. >> >> Instead of looking for nodes with a phy property, look for nodes >> with a dr_mode property, so that we actually access the dr_mode property >> in a node which has it. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/usb/common/common.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c >> index e3d0161..9806433 100644 >> --- a/drivers/usb/common/common.c >> +++ b/drivers/usb/common/common.c >> @@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np) >> int err; >> >> do { >> - controller = of_find_node_with_property(controller, "phys"); >> + controller = of_find_node_with_property(controller, "dr_mode"); >> index = 0; >> do { >> phy = of_parse_phandle(controller, "phys", index); >>
Hi, On Fri, Jun 03, 2016 at 12:34:35PM +0200, Hans de Goede wrote: > Hi, > > On 02-06-16 20:16, Bin Liu wrote: > >Hi, > > > >On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote: > >>Some SoCs have a single phy-hw-block with multiple phys, this is > >>modelled by a single phy dts node, so we end up with multiple > >>controller nodes with a phys property pointing to the phy-node > >>of the otg-phy. > >> > >>Only one of these controllers typically is an otg controller, yet we > > > >Is it guaranteed that only one of them will be otg? > > I guess not, but if there are 2 then with my patch we are of no worse > then today, we will then pick the first otg controller. Whereas What if the first otg controller is not what we want? this patch does not solve the problem. I would think Kishon's suggestion in another email - seperate dt phy nodes - is a better option. > of_usb_get_dr_mode_by_phy currently is broken even on setups with > a shared phy dt-node and only 1 otg controller, which are quite > common. If that is the case, the model has to be changed. Otherwise, a single phy driver is unable to handle different operations from multiple controllers. Regards, -Bin.
Hi, On 03-06-16 15:04, Bin Liu wrote: > Hi, > > On Fri, Jun 03, 2016 at 12:34:35PM +0200, Hans de Goede wrote: >> Hi, >> >> On 02-06-16 20:16, Bin Liu wrote: >>> Hi, >>> >>> On Thu, Jun 02, 2016 at 07:31:03PM +0200, Hans de Goede wrote: >>>> Some SoCs have a single phy-hw-block with multiple phys, this is >>>> modelled by a single phy dts node, so we end up with multiple >>>> controller nodes with a phys property pointing to the phy-node >>>> of the otg-phy. >>>> >>>> Only one of these controllers typically is an otg controller, yet we >>> >>> Is it guaranteed that only one of them will be otg? >> >> I guess not, but if there are 2 then with my patch we are of no worse >> then today, we will then pick the first otg controller. Whereas > > What if the first otg controller is not what we want? this patch does > not solve the problem. I would think Kishon's suggestion in another > email - seperate dt phy nodes - is a better option. That is not possible as it will break the dt-bindings for existing devices. And that adds a lot of complexity without a good reason, as I mentioned in my reply to Kishon if we want to make 100% sure that we've the right controller, we should pass in the phy index argument to the phandle to of_usb_get_dr_mode_by_phy and make of_usb_get_dr_mode_by_phy check this. I'll submit a v2 which does this. Regards, Hans
Hi, On Fri, Jun 03, 2016 at 01:39:26PM +0200, Hans de Goede wrote: > Hi, > > On 03-06-16 13:20, Kishon Vijay Abraham I wrote: > >Hi, > > > >On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote: > >>Some SoCs have a single phy-hw-block with multiple phys, this is > >>modelled by a single phy dts node, so we end up with multiple > >>controller nodes with a phys property pointing to the phy-node > >>of the otg-phy. > > > >Maybe we should try to model each phy with a separate dt node? > > That seems like making things unnecessarily complicated. If we want > to be 100% sure that of_usb_get_dr_mode_by_phy finds the right I believe we have to. > controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy > and make it check that first argument specified to the phandle > used in the controller node matches the passed in index. Why we need the 'index'? Once the dt has seperate nodes for the phys, 'phy_np' passed in is the uniqe id of the phy node. > > And use index == -1 to skip this test. > > Regards, > > Hans Regards, -Bin.
Hi, On 03-06-16 15:12, Bin Liu wrote: > Hi, > > On Fri, Jun 03, 2016 at 01:39:26PM +0200, Hans de Goede wrote: >> Hi, >> >> On 03-06-16 13:20, Kishon Vijay Abraham I wrote: >>> Hi, >>> >>> On Thursday 02 June 2016 11:01 PM, Hans de Goede wrote: >>>> Some SoCs have a single phy-hw-block with multiple phys, this is >>>> modelled by a single phy dts node, so we end up with multiple >>>> controller nodes with a phys property pointing to the phy-node >>>> of the otg-phy. >>> >>> Maybe we should try to model each phy with a separate dt node? >> >> That seems like making things unnecessarily complicated. If we want >> to be 100% sure that of_usb_get_dr_mode_by_phy finds the right > > I believe we have to. We do not have to, things work fine as is, and the problem we're discussing can be solved much simpler by passing the otg-phy index to of_usb_get_dr_mode_by_phy Also doing this breaks the dt bindings and that simply is not allowed so not only do we not have to do this, we cannot do this! >> controller, we could add an "int index" parameter to of_usb_get_dr_mode_by_phy >> and make it check that first argument specified to the phandle >> used in the controller node matches the passed in index. > > Why we need the 'index'? Once the dt has seperate nodes for the phys, > 'phy_np' passed in is the uniqe id of the phy node. Because we cannot use separate nodes for the phys as that would break the devicetree bindings. Anyways please wait for v2 of my patch, I believe that will solve this properly without needing to break devicetree bindings. Regards, Hans
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index e3d0161..9806433 100644 --- a/drivers/usb/common/common.c +++ b/drivers/usb/common/common.c @@ -145,7 +145,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np) int err; do { - controller = of_find_node_with_property(controller, "phys"); + controller = of_find_node_with_property(controller, "dr_mode"); index = 0; do { phy = of_parse_phandle(controller, "phys", index);
Some SoCs have a single phy-hw-block with multiple phys, this is modelled by a single phy dts node, so we end up with multiple controller nodes with a phys property pointing to the phy-node of the otg-phy. Only one of these controllers typically is an otg controller, yet we were checking the first controller who uses a phy from the block and then end up looking for a dr_mode property in e.g. the ehci controller. Instead of looking for nodes with a phy property, look for nodes with a dr_mode property, so that we actually access the dr_mode property in a node which has it. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/common/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)