[3/8] dt-bindings: phy: tegra-xusb-padctl: Add nvidia,usb3-port-fake entry

Add binding details regarding nvidia,usb3-port-fake

Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
---
 Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
 1 file changed, 3 insertions(+)

On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
> Add binding details regarding nvidia,usb3-port-fake
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> index 3742c15..21a5541 100644
> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> @@ -158,6 +158,9 @@ Optional properties:
>    is internal. In the absence of this property the port is considered to be
>    external.
>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
> +  is applicable only for Tegra210 based platforms which has USB2 only ports.

You don't provide a rationale fo why this fake USB3 port needs to be
specified. Doesn't the OTG port work without the fake USB3 port? Why
does it need to be specified and which one should be used as fake?

You mention elsewhere that an unused USB3 port is used on Jetson TX1,
but what if there are no unused ports on a platform? Can we use any
USB3 port for this purpose?

Thierry

On 04-02-2019 17:18, Thierry Reding wrote:
> On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
>> Add binding details regarding nvidia,usb3-port-fake
>>
>> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
>> ---
>>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>> index 3742c15..21a5541 100644
>> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>> @@ -158,6 +158,9 @@ Optional properties:
>>    is internal. In the absence of this property the port is considered to be
>>    external.
>>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
>> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
>> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
>> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
> 
> You don't provide a rationale fo why this fake USB3 port needs to be
> specified. Doesn't the OTG port work without the fake USB3 port? Why
> does it need to be specified and which one should be used as fake?
> 

Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work

> You mention elsewhere that an unused USB3 port is used on Jetson TX1,
> but what if there are no unused ports on a platform? Can we use any
> USB3 port for this purpose?
> 

If we dont have any unused port, we cannot get device mode working on USB2 only port.
We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
fake that corresponding USB2 only port as USB2 and USB3 port.

Will update documentation accordingly and move binding patch prior to code patch
to make easier to understand the changes

> Thierry
>

On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
> 
> 
> On 04-02-2019 17:18, Thierry Reding wrote:
> > On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
> >> Add binding details regarding nvidia,usb3-port-fake
> >>
> >> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> >> ---
> >>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >> index 3742c15..21a5541 100644
> >> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >> @@ -158,6 +158,9 @@ Optional properties:
> >>    is internal. In the absence of this property the port is considered to be
> >>    external.
> >>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
> >> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
> >> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
> >> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
> > 
> > You don't provide a rationale fo why this fake USB3 port needs to be
> > specified. Doesn't the OTG port work without the fake USB3 port? Why
> > does it need to be specified and which one should be used as fake?
> > 
> 
> Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
> 
> > You mention elsewhere that an unused USB3 port is used on Jetson TX1,
> > but what if there are no unused ports on a platform? Can we use any
> > USB3 port for this purpose?
> > 
> 
> If we dont have any unused port, we cannot get device mode working on USB2 only port.
> We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
> fake that corresponding USB2 only port as USB2 and USB3 port.

Hm... seems to me like we should be able to do this without an extra
device tree property. Surely we can determine at runtime which of the
ports are actually unused and pick one? If it doesn't matter which one,
we can just pick the first one that's unused, or an unused one at
random.

Thierry

On Wed, Feb 13, 2019 at 01:56:24PM +0100, Thierry Reding wrote:
> On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
> > 
> > 
> > On 04-02-2019 17:18, Thierry Reding wrote:
> > > On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
> > >> Add binding details regarding nvidia,usb3-port-fake
> > >>
> > >> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> > >> ---
> > >>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> > >> index 3742c15..21a5541 100644
> > >> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> > >> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> > >> @@ -158,6 +158,9 @@ Optional properties:
> > >>    is internal. In the absence of this property the port is considered to be
> > >>    external.
> > >>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
> > >> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
> > >> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
> > >> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
> > > 
> > > You don't provide a rationale fo why this fake USB3 port needs to be
> > > specified. Doesn't the OTG port work without the fake USB3 port? Why
> > > does it need to be specified and which one should be used as fake?
> > > 
> > 
> > Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
> > 
> > > You mention elsewhere that an unused USB3 port is used on Jetson TX1,
> > > but what if there are no unused ports on a platform? Can we use any
> > > USB3 port for this purpose?
> > > 
> > 
> > If we dont have any unused port, we cannot get device mode working on USB2 only port.
> > We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
> > fake that corresponding USB2 only port as USB2 and USB3 port.
> 
> Hm... seems to me like we should be able to do this without an extra
> device tree property. Surely we can determine at runtime which of the
> ports are actually unused and pick one? If it doesn't matter which one,
> we can just pick the first one that's unused, or an unused one at
> random.

It's slightly more complicated than I thought because there's an
optimization in tegra_xusb_add_usb3_port() that will cause only used
ports to get registered. But I think we can do something like this
(untested) to get hold of an unused one, since we only need the index
of the port:

	static int tegra_xusb_find_unused_usb3_port(struct tegra_xusb_padctl *padctl)
	{
		struct device_node *np;
		unsigned int i;

		for (i = 0; i < padctl->soc->ports.usb3.count; i++) {
			np = tegra_xusb_find_port_node(padctl, "usb3", i);
			if (!np || !of_device_is_available(np))
				return index;
		}

		return -ENODEV;
	}

Thierry

On 13-02-2019 18:46, Thierry Reding wrote:
> On Wed, Feb 13, 2019 at 01:56:24PM +0100, Thierry Reding wrote:
>> On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
>>>
>>>
>>> On 04-02-2019 17:18, Thierry Reding wrote:
>>>> On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
>>>>> Add binding details regarding nvidia,usb3-port-fake
>>>>>
>>>>> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>> index 3742c15..21a5541 100644
>>>>> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>> @@ -158,6 +158,9 @@ Optional properties:
>>>>>    is internal. In the absence of this property the port is considered to be
>>>>>    external.
>>>>>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
>>>>> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
>>>>> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
>>>>> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
>>>>
>>>> You don't provide a rationale fo why this fake USB3 port needs to be
>>>> specified. Doesn't the OTG port work without the fake USB3 port? Why
>>>> does it need to be specified and which one should be used as fake?
>>>>
>>>
>>> Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
>>>
>>>> You mention elsewhere that an unused USB3 port is used on Jetson TX1,
>>>> but what if there are no unused ports on a platform? Can we use any
>>>> USB3 port for this purpose?
>>>>
>>>
>>> If we dont have any unused port, we cannot get device mode working on USB2 only port.
>>> We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
>>> fake that corresponding USB2 only port as USB2 and USB3 port.
>>
>> Hm... seems to me like we should be able to do this without an extra
>> device tree property. Surely we can determine at runtime which of the
>> ports are actually unused and pick one? If it doesn't matter which one,
>> we can just pick the first one that's unused, or an unused one at
>> random.
> 
> It's slightly more complicated than I thought because there's an
> optimization in tegra_xusb_add_usb3_port() that will cause only used
> ports to get registered. But I think we can do something like this
> (untested) to get hold of an unused one, since we only need the index
> of the port:
> 
> 	static int tegra_xusb_find_unused_usb3_port(struct tegra_xusb_padctl *padctl)
> 	{
> 		struct device_node *np;
> 		unsigned int i;
> 
> 		for (i = 0; i < padctl->soc->ports.usb3.count; i++) {
> 			np = tegra_xusb_find_port_node(padctl, "usb3", i);
> 			if (!np || !of_device_is_available(np))
> 				return index;
> 		}
> 
> 		return -ENODEV;
> 	}
> 
> Thierry
> 

It not just about un-used USB3 port, we need to find which usb2 port is registered as
otg/device and then check if it doesnot have any companion usb3 port. On this condition
we need to assign unused usb3 port as fake port.
Above is possible, however port_fake need is must for T210 and only for device mode operation
and not for other SOC or USB mode.
So shall i place SOC data(tegra210_xusb_padctl_soc) to read if port fake is needed ?
If yes, and no un-used port available, will mark case as warning(not error) and make probe(tegra_xusb_padctl_probe)
success irrespective of this. Since port can be otg but can be designed to use only for Host mode.

I am not sure if above complication is better than an dt entry.

Please share your info.

- Nagarjuna

On Thu, Feb 14, 2019 at 10:58:13AM +0530, Nagarjuna Kristam wrote:
> 
> 
> On 13-02-2019 18:46, Thierry Reding wrote:
> > On Wed, Feb 13, 2019 at 01:56:24PM +0100, Thierry Reding wrote:
> >> On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
> >>>
> >>>
> >>> On 04-02-2019 17:18, Thierry Reding wrote:
> >>>> On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
> >>>>> Add binding details regarding nvidia,usb3-port-fake
> >>>>>
> >>>>> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
> >>>>>  1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >>>>> index 3742c15..21a5541 100644
> >>>>> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >>>>> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> >>>>> @@ -158,6 +158,9 @@ Optional properties:
> >>>>>    is internal. In the absence of this property the port is considered to be
> >>>>>    external.
> >>>>>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
> >>>>> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
> >>>>> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
> >>>>> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
> >>>>
> >>>> You don't provide a rationale fo why this fake USB3 port needs to be
> >>>> specified. Doesn't the OTG port work without the fake USB3 port? Why
> >>>> does it need to be specified and which one should be used as fake?
> >>>>
> >>>
> >>> Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
> >>>
> >>>> You mention elsewhere that an unused USB3 port is used on Jetson TX1,
> >>>> but what if there are no unused ports on a platform? Can we use any
> >>>> USB3 port for this purpose?
> >>>>
> >>>
> >>> If we dont have any unused port, we cannot get device mode working on USB2 only port.
> >>> We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
> >>> fake that corresponding USB2 only port as USB2 and USB3 port.
> >>
> >> Hm... seems to me like we should be able to do this without an extra
> >> device tree property. Surely we can determine at runtime which of the
> >> ports are actually unused and pick one? If it doesn't matter which one,
> >> we can just pick the first one that's unused, or an unused one at
> >> random.
> > 
> > It's slightly more complicated than I thought because there's an
> > optimization in tegra_xusb_add_usb3_port() that will cause only used
> > ports to get registered. But I think we can do something like this
> > (untested) to get hold of an unused one, since we only need the index
> > of the port:
> > 
> > 	static int tegra_xusb_find_unused_usb3_port(struct tegra_xusb_padctl *padctl)
> > 	{
> > 		struct device_node *np;
> > 		unsigned int i;
> > 
> > 		for (i = 0; i < padctl->soc->ports.usb3.count; i++) {
> > 			np = tegra_xusb_find_port_node(padctl, "usb3", i);
> > 			if (!np || !of_device_is_available(np))
> > 				return index;
> > 		}
> > 
> > 		return -ENODEV;
> > 	}
> > 
> > Thierry
> > 
> 
> It not just about un-used USB3 port, we need to find which usb2 port
> is registered as otg/device and then check if it doesnot have any
> companion usb3 port. On this condition we need to assign unused usb3
> port as fake port.

Yeah, we should only be running this code for the OTG port in the first
place. Once we know which one the OTG port is it's easy to check whether
it has a USB3 companion port.

> Above is possible, however port_fake need is must for T210 and only
> for device mode operation and not for other SOC or USB mode.
> So shall i place SOC data(tegra210_xusb_padctl_soc) to read if port
> fake is needed ?

Yeah, something like "needs_usb3_for_otg" should do the trick.

So to confirm whether I understand this correctly: if we want to do USB2
and USB3 device mode, then we'd usually have a USB2/USB3 port combo, in
which case we wouldn't need a fake USB3 port. But if we have a USB2 only
OTG port, then we need to program registers for the USB3 port in order
to make USB2 only OTG work, but only on Tegra210. With Tegra186 and
later we don't need this work around?

> If yes, and no un-used port available, will mark case as warning(not
> error) and make probe(tegra_xusb_padctl_probe) success irrespective of
> this. Since port can be otg but can be designed to use only for Host
> mode.

Yes, sounds good. Or perhaps we can use the mode property to determine
whether or not this should be a fatal error? For example if we've got an
OTG port but it is designed to be used only in host mode, then we could
require the mode to be set to "host". If it is set to "peripheral" or
"otg" and we don't find an un-used USB3 port to use as fake, it could
still be a fatal error so that people can fix their device tree. Right?

> I am not sure if above complication is better than an dt entry.

I understand that this may seem like a lot of complication, but there
are two reasons why I think it's worth it. On one hand we don't want to
add redundant information to device tree. Everything that can already be
derived from the compatible string doesn't need to be explicitly added
in device tree via extra properties. In this case we know that Tegra210
has this bug and we can identify the need for the workaround by looking
at the compatible string. Similarly we know exactly what to do in order
to work around the problem (i.e. find an unused port and use it as the
fake companion port).

On the other hand I think that this moves the complexity where it
belongs: in the driver. If we document this behaviour in the device
tree bindings, we need to be very explicit about what this means and
very careful about how to word it so as not to confuse readers. It's
also not entirely trivial for device tree authors to come up with the
fake port index. They have to carefully check the design to see which
one is unused. The driver can do a much better job at that, and then
we don't have to burden the device tree authors with that task.

Thierry

On 14-02-2019 14:29, Thierry Reding wrote:
> On Thu, Feb 14, 2019 at 10:58:13AM +0530, Nagarjuna Kristam wrote:
>>
>>
>> On 13-02-2019 18:46, Thierry Reding wrote:
>>> On Wed, Feb 13, 2019 at 01:56:24PM +0100, Thierry Reding wrote:
>>>> On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
>>>>>
>>>>>
>>>>> On 04-02-2019 17:18, Thierry Reding wrote:
>>>>>> On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
>>>>>>> Add binding details regarding nvidia,usb3-port-fake
>>>>>>>
>>>>>>> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>> index 3742c15..21a5541 100644
>>>>>>> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>> @@ -158,6 +158,9 @@ Optional properties:
>>>>>>>    is internal. In the absence of this property the port is considered to be
>>>>>>>    external.
>>>>>>>  - vbus-supply: phandle to a regulator supplying the VBUS voltage.
>>>>>>> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
>>>>>>> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
>>>>>>> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
>>>>>>
>>>>>> You don't provide a rationale fo why this fake USB3 port needs to be
>>>>>> specified. Doesn't the OTG port work without the fake USB3 port? Why
>>>>>> does it need to be specified and which one should be used as fake?
>>>>>>
>>>>>
>>>>> Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
>>>>>
>>>>>> You mention elsewhere that an unused USB3 port is used on Jetson TX1,
>>>>>> but what if there are no unused ports on a platform? Can we use any
>>>>>> USB3 port for this purpose?
>>>>>>
>>>>>
>>>>> If we dont have any unused port, we cannot get device mode working on USB2 only port.
>>>>> We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
>>>>> fake that corresponding USB2 only port as USB2 and USB3 port.
>>>>
>>>> Hm... seems to me like we should be able to do this without an extra
>>>> device tree property. Surely we can determine at runtime which of the
>>>> ports are actually unused and pick one? If it doesn't matter which one,
>>>> we can just pick the first one that's unused, or an unused one at
>>>> random.
>>>
>>> It's slightly more complicated than I thought because there's an
>>> optimization in tegra_xusb_add_usb3_port() that will cause only used
>>> ports to get registered. But I think we can do something like this
>>> (untested) to get hold of an unused one, since we only need the index
>>> of the port:
>>>
>>> 	static int tegra_xusb_find_unused_usb3_port(struct tegra_xusb_padctl *padctl)
>>> 	{
>>> 		struct device_node *np;
>>> 		unsigned int i;
>>>
>>> 		for (i = 0; i < padctl->soc->ports.usb3.count; i++) {
>>> 			np = tegra_xusb_find_port_node(padctl, "usb3", i);
>>> 			if (!np || !of_device_is_available(np))
>>> 				return index;
>>> 		}
>>>
>>> 		return -ENODEV;
>>> 	}
>>>
>>> Thierry
>>>
>>
>> It not just about un-used USB3 port, we need to find which usb2 port
>> is registered as otg/device and then check if it doesnot have any
>> companion usb3 port. On this condition we need to assign unused usb3
>> port as fake port.
> 
> Yeah, we should only be running this code for the OTG port in the first
> place. Once we know which one the OTG port is it's easy to check whether
> it has a USB3 companion port.
> 
>> Above is possible, however port_fake need is must for T210 and only
>> for device mode operation and not for other SOC or USB mode.
>> So shall i place SOC data(tegra210_xusb_padctl_soc) to read if port
>> fake is needed ?
> 
> Yeah, something like "needs_usb3_for_otg" should do the trick.
> 
> So to confirm whether I understand this correctly: if we want to do USB2
> and USB3 device mode, then we'd usually have a USB2/USB3 port combo, in
> which case we wouldn't need a fake USB3 port. But if we have a USB2 only
> OTG port, then we need to program registers for the USB3 port in order
> to make USB2 only OTG work, but only on Tegra210. With Tegra186 and
> later we don't need this work around?
> 
>> If yes, and no un-used port available, will mark case as warning(not
>> error) and make probe(tegra_xusb_padctl_probe) success irrespective of
>> this. Since port can be otg but can be designed to use only for Host
>> mode.
> 
> Yes, sounds good. Or perhaps we can use the mode property to determine
> whether or not this should be a fatal error? For example if we've got an
> OTG port but it is designed to be used only in host mode, then we could
> require the mode to be set to "host". If it is set to "peripheral" or
> "otg" and we don't find an un-used USB3 port to use as fake, it could
> still be a fatal error so that people can fix their device tree. Right?
> 
>> I am not sure if above complication is better than an dt entry.
> 
> I understand that this may seem like a lot of complication, but there
> are two reasons why I think it's worth it. On one hand we don't want to
> add redundant information to device tree. Everything that can already be
> derived from the compatible string doesn't need to be explicitly added
> in device tree via extra properties. In this case we know that Tegra210
> has this bug and we can identify the need for the workaround by looking
> at the compatible string. Similarly we know exactly what to do in order
> to work around the problem (i.e. find an unused port and use it as the
> fake companion port).
> 
> On the other hand I think that this moves the complexity where it
> belongs: in the driver. If we document this behaviour in the device
> tree bindings, we need to be very explicit about what this means and
> very careful about how to word it so as not to confuse readers. It's
> also not entirely trivial for device tree authors to come up with the
> fake port index. They have to carefully check the design to see which
> one is unused. The driver can do a much better job at that, and then
> we don't have to burden the device tree authors with that task.
> 
Thanks for details, considering the need for tegra 210 only, will put
logic to update port_fake as tegra210 specific code handling during port
enable API

- Nagarjuna
> Thierry
>

There won't be a case that all of the super-speed ports are in used but 
none of the ports can be paired with the OTG capable USB2 port. 
T210/T210B01(aka T214) has 4 super-speed ports and 4 USB2 ports. A USB 
3.0 connection (either host role or device role) must have one 
super-speed port pair with one USB2 port. If a platform has an USB 2.0 
OTG connector, there must be at least one super-speed port unused.

To make XUSB device mode working with the USB 2.0 OTG connector, driver 
just have to:
1. pair the designated unused super-speed port with the USB2 port that 
has OTG capability.
2. remove CLAMP_EN_EARLY/CLAMP_EN/VCORE_DOWN signals.

Please note the corresponding UPHY lane of the designated unused 
super-speed port can still be used by another IP (PCIE or SATA).

Agree that it'd be better to keep the complexity within driver, rather 
than confusing device tree authors. Thanks Thierry.

Thanks,
JC


On 2/14/19 11:01 PM, Nagarjuna Kristam wrote:
>
> On 14-02-2019 14:29, Thierry Reding wrote:
>> On Thu, Feb 14, 2019 at 10:58:13AM +0530, Nagarjuna Kristam wrote:
>>>
>>> On 13-02-2019 18:46, Thierry Reding wrote:
>>>> On Wed, Feb 13, 2019 at 01:56:24PM +0100, Thierry Reding wrote:
>>>>> On Wed, Feb 13, 2019 at 04:08:15PM +0530, Nagarjuna Kristam wrote:
>>>>>>
>>>>>> On 04-02-2019 17:18, Thierry Reding wrote:
>>>>>>> On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote:
>>>>>>>> Add binding details regarding nvidia,usb3-port-fake
>>>>>>>>
>>>>>>>> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
>>>>>>>> ---
>>>>>>>>   Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++
>>>>>>>>   1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>>> index 3742c15..21a5541 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
>>>>>>>> @@ -158,6 +158,9 @@ Optional properties:
>>>>>>>>     is internal. In the absence of this property the port is considered to be
>>>>>>>>     external.
>>>>>>>>   - vbus-supply: phandle to a regulator supplying the VBUS voltage.
>>>>>>>> +- nvidia,usb3-port-fake: A integer property whose presense informs that a
>>>>>>>> +  fake USB3 port needs to be mapped for corresponding USB2 port. This entry
>>>>>>>> +  is applicable only for Tegra210 based platforms which has USB2 only ports.
>>>>>>> You don't provide a rationale fo why this fake USB3 port needs to be
>>>>>>> specified. Doesn't the OTG port work without the fake USB3 port? Why
>>>>>>> does it need to be specified and which one should be used as fake?
>>>>>>>
>>>>>> Yes, on Tegra210 based platforms, USB2 only device/OTG ports don't work
>>>>>>
>>>>>>> You mention elsewhere that an unused USB3 port is used on Jetson TX1,
>>>>>>> but what if there are no unused ports on a platform? Can we use any
>>>>>>> USB3 port for this purpose?
>>>>>>>
>>>>>> If we dont have any unused port, we cannot get device mode working on USB2 only port.
>>>>>> We cannot use any USB3 port, we need to use an un-used USB3 port, so that we can
>>>>>> fake that corresponding USB2 only port as USB2 and USB3 port.
>>>>> Hm... seems to me like we should be able to do this without an extra
>>>>> device tree property. Surely we can determine at runtime which of the
>>>>> ports are actually unused and pick one? If it doesn't matter which one,
>>>>> we can just pick the first one that's unused, or an unused one at
>>>>> random.
>>>> It's slightly more complicated than I thought because there's an
>>>> optimization in tegra_xusb_add_usb3_port() that will cause only used
>>>> ports to get registered. But I think we can do something like this
>>>> (untested) to get hold of an unused one, since we only need the index
>>>> of the port:
>>>>
>>>> 	static int tegra_xusb_find_unused_usb3_port(struct tegra_xusb_padctl *padctl)
>>>> 	{
>>>> 		struct device_node *np;
>>>> 		unsigned int i;
>>>>
>>>> 		for (i = 0; i < padctl->soc->ports.usb3.count; i++) {
>>>> 			np = tegra_xusb_find_port_node(padctl, "usb3", i);
>>>> 			if (!np || !of_device_is_available(np))
>>>> 				return index;
>>>> 		}
>>>>
>>>> 		return -ENODEV;
>>>> 	}
>>>>
>>>> Thierry
>>>>
>>> It not just about un-used USB3 port, we need to find which usb2 port
>>> is registered as otg/device and then check if it doesnot have any
>>> companion usb3 port. On this condition we need to assign unused usb3
>>> port as fake port.
>> Yeah, we should only be running this code for the OTG port in the first
>> place. Once we know which one the OTG port is it's easy to check whether
>> it has a USB3 companion port.
>>
>>> Above is possible, however port_fake need is must for T210 and only
>>> for device mode operation and not for other SOC or USB mode.
>>> So shall i place SOC data(tegra210_xusb_padctl_soc) to read if port
>>> fake is needed ?
>> Yeah, something like "needs_usb3_for_otg" should do the trick.
>>
>> So to confirm whether I understand this correctly: if we want to do USB2
>> and USB3 device mode, then we'd usually have a USB2/USB3 port combo, in
>> which case we wouldn't need a fake USB3 port. But if we have a USB2 only
>> OTG port, then we need to program registers for the USB3 port in order
>> to make USB2 only OTG work, but only on Tegra210. With Tegra186 and
>> later we don't need this work around?
>>
>>> If yes, and no un-used port available, will mark case as warning(not
>>> error) and make probe(tegra_xusb_padctl_probe) success irrespective of
>>> this. Since port can be otg but can be designed to use only for Host
>>> mode.
>> Yes, sounds good. Or perhaps we can use the mode property to determine
>> whether or not this should be a fatal error? For example if we've got an
>> OTG port but it is designed to be used only in host mode, then we could
>> require the mode to be set to "host". If it is set to "peripheral" or
>> "otg" and we don't find an un-used USB3 port to use as fake, it could
>> still be a fatal error so that people can fix their device tree. Right?
>>
>>> I am not sure if above complication is better than an dt entry.
>> I understand that this may seem like a lot of complication, but there
>> are two reasons why I think it's worth it. On one hand we don't want to
>> add redundant information to device tree. Everything that can already be
>> derived from the compatible string doesn't need to be explicitly added
>> in device tree via extra properties. In this case we know that Tegra210
>> has this bug and we can identify the need for the workaround by looking
>> at the compatible string. Similarly we know exactly what to do in order
>> to work around the problem (i.e. find an unused port and use it as the
>> fake companion port).
>>
>> On the other hand I think that this moves the complexity where it
>> belongs: in the driver. If we document this behaviour in the device
>> tree bindings, we need to be very explicit about what this means and
>> very careful about how to word it so as not to confuse readers. It's
>> also not entirely trivial for device tree authors to come up with the
>> fake port index. They have to carefully check the design to see which
>> one is unused. The driver can do a much better job at that, and then
>> we don't have to burden the device tree authors with that task.
>>
> Thanks for details, considering the need for tegra 210 only, will put
> logic to update port_fake as tegra210 specific code handling during port
> enable API
>
> - Nagarjuna
>> Thierry
>>