Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Hans de Goede]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Bin Liu]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Hans de Goede]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Bin Liu]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Hans de Goede]

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 
---
 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);


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Kishon Vijay Abraham I]

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 
> ---
>  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);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Hans de Goede]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Bin Liu]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] USB: Fix of_usb_get_dr_mode_by_phy with a shared phy block

[Hans de Goede]

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 
---
 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);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html