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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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
Attachment:
signature.asc
Description: PGP signature
