dwc3: add support for hardware with multiple ports on USB2 hub enabled

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

Hello,

while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
come across something I did not know yet:
dwc3 has an internal USB2 hub (from what I can read in the code there
seem to be multiple USB3 ports supported as well).
When searching the web I did not come across any SoC that uses a
configuration with more than one port enabled.

On my Amlogic Meson GXM device (consumer device, no development board)
I see the following USB2 PHY register configuration (full register
dump from the kernel that was shipped with the device is attached):
GUSB2PHYCFG(0) = 0x40102500
GUSB2PHYCFG(1) = 0x40102540
GUSB2PHYCFG(2) = 0x40102540

Then vendor kernel sources (a 3.14 kernel) are adding the resets for
GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
from linux-usb) kernel works fine even with just applying the reset to
GUSB2PHYCFG(0).

That brings up two questions:
1. I guess it makes sense to adjust the upstream dwc3 to add the
resets for all available USB2 PHYs - is there a specific reason why
the current dwc3 driver does not do that (or is it simply because why
we find on Meson GXL/GXM is very exotic)?
2. would we also implement this for the USB3 "pipes" as well (without
being able to test this)?
3. from what I can see in the code we have to adjust dwc3_phy_setup()
and ulpi.c to add support for multiple ports, but how do we detect the
number of USB2 and USB3 ports (is this somewhere encoded in the
DWC3_GHWPARAMS registers)?

lsusb output is also attached, based on the PHY drivers for which the
patches can be found here: [0]


Best Regards,
Martin


[0] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001721.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sys_kernel_debug_dwc3_regdump_vendor_kernel
Type: application/octet-stream
Size: 8054 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20161127/9a64343e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lsusb-mainline-kernel
Type: application/octet-stream
Size: 4991 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20161127/9a64343e/attachment-0001.obj>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Sun, Nov 27, 2016 at 2:02 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hello,
>
> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
> come across something I did not know yet:
> dwc3 has an internal USB2 hub (from what I can read in the code there
> seem to be multiple USB3 ports supported as well).
> When searching the web I did not come across any SoC that uses a
> configuration with more than one port enabled.
>
> On my Amlogic Meson GXM device (consumer device, no development board)
> I see the following USB2 PHY register configuration (full register
> dump from the kernel that was shipped with the device is attached):
> GUSB2PHYCFG(0) = 0x40102500
> GUSB2PHYCFG(1) = 0x40102540
> GUSB2PHYCFG(2) = 0x40102540
>
> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
> from linux-usb) kernel works fine even with just applying the reset to
> GUSB2PHYCFG(0).
>
> That brings up two questions:
> 1. I guess it makes sense to adjust the upstream dwc3 to add the
> resets for all available USB2 PHYs - is there a specific reason why
> the current dwc3 driver does not do that (or is it simply because why
> we find on Meson GXL/GXM is very exotic)?
> 2. would we also implement this for the USB3 "pipes" as well (without
> being able to test this)?
> 3. from what I can see in the code we have to adjust dwc3_phy_setup()
> and ulpi.c to add support for multiple ports, but how do we detect the
> number of USB2 and USB3 ports (is this somewhere encoded in the
> DWC3_GHWPARAMS registers)?
>
> lsusb output is also attached, based on the PHY drivers for which the
> patches can be found here: [0]
>
>
> Best Regards,
> Martin
>
>
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001721.html
re-sending due to a typo in Felipe Balbi's email address (sorry for that)

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

Hello Felipe, Hello John,

On Sun, Nov 27, 2016 at 2:05 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Sun, Nov 27, 2016 at 2:02 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> Hello,
>>
>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>> come across something I did not know yet:
>> dwc3 has an internal USB2 hub (from what I can read in the code there
>> seem to be multiple USB3 ports supported as well).
>> When searching the web I did not come across any SoC that uses a
>> configuration with more than one port enabled.
I am still trying to figure out the logic behind the dwc3 USB hubs to
add USB support for the Amlogic Meson GXL and GXM SoCs.
Unfortunately I do not have access to the dwc3 documentation.

It would be great if you could share your thoughts how to add support
for dwc3 configurations with more than one port enabled on the
internal hub.
I am *not* expecting any finished code or a concept that just has to
be converted to "source code".
You can find *my* ideas on how to solve this below.

>> On my Amlogic Meson GXM device (consumer device, no development board)
>> I see the following USB2 PHY register configuration (full register
>> dump from the kernel that was shipped with the device is attached):
>> GUSB2PHYCFG(0) = 0x40102500
>> GUSB2PHYCFG(1) = 0x40102540
>> GUSB2PHYCFG(2) = 0x40102540
>>
>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>> from linux-usb) kernel works fine even with just applying the reset to
>> GUSB2PHYCFG(0).
>>
>> That brings up two questions:
>> 1. I guess it makes sense to adjust the upstream dwc3 to add the
>> resets for all available USB2 PHYs - is there a specific reason why
>> the current dwc3 driver does not do that (or is it simply because why
>> we find on Meson GXL/GXM is very exotic)?
>> 2. would we also implement this for the USB3 "pipes" as well (without
>> being able to test this)?
>> 3. from what I can see in the code we have to adjust dwc3_phy_setup()
>> and ulpi.c to add support for multiple ports, but how do we detect the
>> number of USB2 and USB3 ports (is this somewhere encoded in the
>> DWC3_GHWPARAMS registers)?
after reading more about how USB hubs are working internally it seems
to me that this design makes sense. I guess the answer for #1 and #2
is simply "yes".
it also seems to make sense that I have to enable all USB2 PHYs
(additional info: I cannot get dwc3 to detect any device as long as
any of the USB PHYs is disabled, even if the device is plugged into a
non-disabled port) due to the way an USB hub works. so it only seems
logical that we should add support for that to the dwc3 driver as
well.

my current idea is to describe the hub on the dwc3 via devicetree
(thanks to Rob for the idea back in November).
the result could look like this (very work in progress!):
roothub at 0 {
  compatible = "usb1d6b,2";
  #address-cells = <1>;
  #size-cells = <0>;
  reg = <0>;

  port at 1 {
    reg = <1>;
    phys = <&usb2_phy0>;
    phy-names = "usb2-phy";
  };

  port at 2 {
    reg = <2>;
    phys = <&usb2_phy1>;
    phy-names = "usb2-phy";
  };
};

(there are no usb3-phys configured because these are not supported on
the SoC I'm testing with, but the code would support these of course)
This would also allow us to move settings like
"snps,dis_u2_susphy_quirk" to specific ports as well.
If we don't find the root-hub as child-node of the dwc3 node then we
can simply fall back to parsing the dwc3 node itself (to ensure
backwards compatibility).
we could also use this in the future to prevent the following error
message when the dwc3 IP is configured without USB3 ports:
hub 2-0:1.0: USB hub found
hub 2-0:1.0: config failed, hub doesn't have any ports! (err -19)

PS: I found the  Genesys Logic GL850 and Cypress CY7C65642 datasheets helpful.
The "Block Diagrams" give a good basic overview.


Regards,
Martin

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
> come across something I did not know yet:
> dwc3 has an internal USB2 hub (from what I can read in the code there
> seem to be multiple USB3 ports supported as well).

no, that's not true. It has a roothub when working as host. But that's
it. When working as peripheral, it's always single-port AFAIR.

> When searching the web I did not come across any SoC that uses a
> configuration with more than one port enabled.
>
> On my Amlogic Meson GXM device (consumer device, no development board)
> I see the following USB2 PHY register configuration (full register
> dump from the kernel that was shipped with the device is attached):
> GUSB2PHYCFG(0) = 0x40102500
> GUSB2PHYCFG(1) = 0x40102540
> GUSB2PHYCFG(2) = 0x40102540

multiple PHYs are only used by the host block (xHCI). Don't touch these
and just let xHCI core handle the ports.

> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().

That shouldn't be necessary, actually. If it is, it means the HW was
poorly integrated. In that case, we _can_ add the other resets, but I
need confirmation that they are needed by means of a public errata
document.

> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
> from linux-usb) kernel works fine even with just applying the reset to
> GUSB2PHYCFG(0).

there you go

> That brings up two questions:
> 1. I guess it makes sense to adjust the upstream dwc3 to add the
> resets for all available USB2 PHYs - is there a specific reason why
> the current dwc3 driver does not do that (or is it simply because why
> we find on Meson GXL/GXM is very exotic)?

yeah, they're not needed :-)

> 2. would we also implement this for the USB3 "pipes" as well (without
> being able to test this)?

nope

> 3. from what I can see in the code we have to adjust dwc3_phy_setup()
> and ulpi.c to add support for multiple ports, but how do we detect the
> number of USB2 and USB3 ports (is this somewhere encoded in the
> DWC3_GHWPARAMS registers)?

also nope. xHCI can detect how many ports a roothub has and work with
it. From peripheral point of view, dwc3 is always single-port.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/d0420a8a/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

Hi Felipe,

On Mon, Jan 9, 2017 at 11:37 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>> come across something I did not know yet:
>> dwc3 has an internal USB2 hub (from what I can read in the code there
>> seem to be multiple USB3 ports supported as well).
>
> no, that's not true. It has a roothub when working as host. But that's
> it. When working as peripheral, it's always single-port AFAIR.
OK, I should have been more clear here: I am only talking about
host-mode since DWC3_GHWPARAMS0 on the GXL/GXM SoCs has a value of
0x20208009 which translates to "DWC3_GHWPARAMS0_MODE_HOST".

are you sure about the fact that it does not have an internal hub?
What I see in both, the vendor kernel's and my own patched mainline
kernel log is:
[   19.130331 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[   19.130385 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
assigned bus number 1
[   19.139666 at 3] xhci-hcd xhci-hcd.0.auto: irq 62, io mem 0xc9000000
[   19.145295 at 3] hub 1-0:1.0: USB hub found
[   19.148098 at 3] hub 1-0:1.0: 3 ports detected
[   19.152396 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
[   19.157813 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
assigned bus number 2
[   19.166598 at 3] hub 2-0:1.0: USB hub found
[   19.169452 at 3] hub 2-0:1.0: config failed, hub doesn't have any
ports! (err -19)
This is from a GXM SoC which also comes with 3x USB2 PHYs (these are
not Synopsys DesignWare PHYs but custom ones from Amlogic).
I see similar messages but with "2 ports detect" on a GXL SoC which
comes with 2x USB2 PHYs.

>> When searching the web I did not come across any SoC that uses a
>> configuration with more than one port enabled.
>>
>> On my Amlogic Meson GXM device (consumer device, no development board)
>> I see the following USB2 PHY register configuration (full register
>> dump from the kernel that was shipped with the device is attached):
>> GUSB2PHYCFG(0) = 0x40102500
>> GUSB2PHYCFG(1) = 0x40102540
>> GUSB2PHYCFG(2) = 0x40102540
>
> multiple PHYs are only used by the host block (xHCI). Don't touch these
> and just let xHCI core handle the ports.
could you be more specific with "xHCI core" - do you mean the core in
the dwc3 IP or drivers/usb/host/xhci-*?
However, we still have a "problem" here: the USB PHYs for each
"enabled" port have to be turned on (if I leave only one USB PHY
disabled then none of the ports is working). unfortunately the current
code (both, in dwc3 and drivers/usb/host/*) assumes that there's
either 0 or 1 PHY for each HCD.

>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>
> That shouldn't be necessary, actually. If it is, it means the HW was
> poorly integrated. In that case, we _can_ add the other resets, but I
> need confirmation that they are needed by means of a public errata
> document.
>
>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>> from linux-usb) kernel works fine even with just applying the reset to
>> GUSB2PHYCFG(0).
>
> there you go
does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
current dwc3 code in dwc3_phy_setup) is done only because of the
quirks/erratas? in other words: do you mean that one would not have to
reset GUSB2PHYCFG(0) if there were no erratas?

>> That brings up two questions:
>> 1. I guess it makes sense to adjust the upstream dwc3 to add the
>> resets for all available USB2 PHYs - is there a specific reason why
>> the current dwc3 driver does not do that (or is it simply because why
>> we find on Meson GXL/GXM is very exotic)?
>
> yeah, they're not needed :-)
>
>> 2. would we also implement this for the USB3 "pipes" as well (without
>> being able to test this)?
>
> nope
these two are probably covered with the question before

>> 3. from what I can see in the code we have to adjust dwc3_phy_setup()
>> and ulpi.c to add support for multiple ports, but how do we detect the
>> number of USB2 and USB3 ports (is this somewhere encoded in the
>> DWC3_GHWPARAMS registers)?
>
> also nope. xHCI can detect how many ports a roothub has and work with
> it. From peripheral point of view, dwc3 is always single-port.
let's postpone this until we have discussed the more important questions


Regards,
Martin

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>>> come across something I did not know yet:
>>> dwc3 has an internal USB2 hub (from what I can read in the code there
>>> seem to be multiple USB3 ports supported as well).
>>
>> no, that's not true. It has a roothub when working as host. But that's
>> it. When working as peripheral, it's always single-port AFAIR.
> OK, I should have been more clear here: I am only talking about
> host-mode since DWC3_GHWPARAMS0 on the GXL/GXM SoCs has a value of
> 0x20208009 which translates to "DWC3_GHWPARAMS0_MODE_HOST".
>
> are you sure about the fact that it does not have an internal hub?
> What I see in both, the vendor kernel's and my own patched mainline
> kernel log is:
> [   19.130331 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
> [   19.130385 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
> assigned bus number 1
> [   19.139666 at 3] xhci-hcd xhci-hcd.0.auto: irq 62, io mem 0xc9000000
> [   19.145295 at 3] hub 1-0:1.0: USB hub found

this is the roothub :-)

> [   19.148098 at 3] hub 1-0:1.0: 3 ports detected
> [   19.152396 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
> [   19.157813 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
> assigned bus number 2
> [   19.166598 at 3] hub 2-0:1.0: USB hub found
> [   19.169452 at 3] hub 2-0:1.0: config failed, hub doesn't have any
> ports! (err -19)
> This is from a GXM SoC which also comes with 3x USB2 PHYs (these are
> not Synopsys DesignWare PHYs but custom ones from Amlogic).
> I see similar messages but with "2 ports detect" on a GXL SoC which
> comes with 2x USB2 PHYs.

The fact that is claims that it has "no ports" hints at quirk caused,
likely, by poor RTL edits. coreConsultant should make sure that
MAX_PORTS (in xHCI HCC_PARAMS* register set) was set to correct value,
but somehow that field is set to 0 in one of your Amlogic SoC.

>>> When searching the web I did not come across any SoC that uses a
>>> configuration with more than one port enabled.
>>>
>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>> I see the following USB2 PHY register configuration (full register
>>> dump from the kernel that was shipped with the device is attached):
>>> GUSB2PHYCFG(0) = 0x40102500
>>> GUSB2PHYCFG(1) = 0x40102540
>>> GUSB2PHYCFG(2) = 0x40102540
>>
>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>> and just let xHCI core handle the ports.
> could you be more specific with "xHCI core" - do you mean the core in
> the dwc3 IP or drivers/usb/host/xhci-*?
> However, we still have a "problem" here: the USB PHYs for each
> "enabled" port have to be turned on (if I leave only one USB PHY
> disabled then none of the ports is working). unfortunately the current
> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
> either 0 or 1 PHY for each HCD.
>
>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>
>> That shouldn't be necessary, actually. If it is, it means the HW was
>> poorly integrated. In that case, we _can_ add the other resets, but I
>> need confirmation that they are needed by means of a public errata
>> document.
>>
>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>> from linux-usb) kernel works fine even with just applying the reset to
>>> GUSB2PHYCFG(0).
>>
>> there you go
> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
> current dwc3 code in dwc3_phy_setup) is done only because of the
> quirks/erratas? in other words: do you mean that one would not have to
> reset GUSB2PHYCFG(0) if there were no erratas?

no, it's done for peripheral configuration of dwc3. Look at the code
more carefully:

> /**
>  * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>  * @dwc: pointer to our context structure
>  */
> static int dwc3_core_soft_reset(struct dwc3 *dwc)
> {
> 	u32		reg;
> 	int		retries = 1000;
> 	int		ret;
>
> 	usb_phy_init(dwc->usb2_phy);
> 	usb_phy_init(dwc->usb3_phy);
> 	ret = phy_init(dwc->usb2_generic_phy);
> 	if (ret < 0)
> 		return ret;
>
> 	ret = phy_init(dwc->usb3_generic_phy);
> 	if (ret < 0) {
> 		phy_exit(dwc->usb2_generic_phy);
> 		return ret;
> 	}
>
> 	/*
> 	 * We're resetting only the device side because, if we're in host mode,
> 	 * XHCI driver will reset the host block. If dwc3 was configured for
> 	 * host-only mode, then we can return early.
> 	 */
> 	if (dwc->dr_mode == USB_DR_MODE_HOST)
> 		return 0;

see here ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> 	reg |= DWC3_DCTL_CSFTRST;
> 	dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>
> 	do {
> 		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> 		if (!(reg & DWC3_DCTL_CSFTRST))
> 			return 0;
>
> 		udelay(1);
> 	} while (--retries);
>
> 	return -ETIMEDOUT;
> }

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/7cb4f0f1/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Mon, Jan 9, 2017 at 12:18 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>>>> come across something I did not know yet:
>>>> dwc3 has an internal USB2 hub (from what I can read in the code there
>>>> seem to be multiple USB3 ports supported as well).
>>>
>>> no, that's not true. It has a roothub when working as host. But that's
>>> it. When working as peripheral, it's always single-port AFAIR.
>> OK, I should have been more clear here: I am only talking about
>> host-mode since DWC3_GHWPARAMS0 on the GXL/GXM SoCs has a value of
>> 0x20208009 which translates to "DWC3_GHWPARAMS0_MODE_HOST".
>>
>> are you sure about the fact that it does not have an internal hub?
>> What I see in both, the vendor kernel's and my own patched mainline
>> kernel log is:
>> [   19.130331 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>> [   19.130385 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>> assigned bus number 1
>> [   19.139666 at 3] xhci-hcd xhci-hcd.0.auto: irq 62, io mem 0xc9000000
>> [   19.145295 at 3] hub 1-0:1.0: USB hub found
>
> this is the roothub :-)
agreed :)

>> [   19.148098 at 3] hub 1-0:1.0: 3 ports detected
this is the one I wanted to point out: in this case there are 3 ports
on the (USB2) roothub.
the SoC has one USB2 for each of these ports, and all 3 PHYs have to
be initialized to get USB working (this seems to be my main issue).

>> [   19.152396 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>> [   19.157813 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>> assigned bus number 2
>> [   19.166598 at 3] hub 2-0:1.0: USB hub found
>> [   19.169452 at 3] hub 2-0:1.0: config failed, hub doesn't have any
>> ports! (err -19)
>> This is from a GXM SoC which also comes with 3x USB2 PHYs (these are
>> not Synopsys DesignWare PHYs but custom ones from Amlogic).
>> I see similar messages but with "2 ports detect" on a GXL SoC which
>> comes with 2x USB2 PHYs.
>
> The fact that is claims that it has "no ports" hints at quirk caused,
> likely, by poor RTL edits. coreConsultant should make sure that
> MAX_PORTS (in xHCI HCC_PARAMS* register set) was set to correct value,
> but somehow that field is set to 0 in one of your Amlogic SoC.
yes, it's a USB2-only configuration (so there are no USB3 ports on the
internal roothub)

>>>> When searching the web I did not come across any SoC that uses a
>>>> configuration with more than one port enabled.
>>>>
>>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>>> I see the following USB2 PHY register configuration (full register
>>>> dump from the kernel that was shipped with the device is attached):
>>>> GUSB2PHYCFG(0) = 0x40102500
>>>> GUSB2PHYCFG(1) = 0x40102540
>>>> GUSB2PHYCFG(2) = 0x40102540
>>>
>>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>>> and just let xHCI core handle the ports.
>> could you be more specific with "xHCI core" - do you mean the core in
>> the dwc3 IP or drivers/usb/host/xhci-*?
>> However, we still have a "problem" here: the USB PHYs for each
>> "enabled" port have to be turned on (if I leave only one USB PHY
>> disabled then none of the ports is working). unfortunately the current
>> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
>> either 0 or 1 PHY for each HCD.
>>
>>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>>
>>> That shouldn't be necessary, actually. If it is, it means the HW was
>>> poorly integrated. In that case, we _can_ add the other resets, but I
>>> need confirmation that they are needed by means of a public errata
>>> document.
>>>
>>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>>> from linux-usb) kernel works fine even with just applying the reset to
>>>> GUSB2PHYCFG(0).
>>>
>>> there you go
>> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
>> current dwc3 code in dwc3_phy_setup) is done only because of the
>> quirks/erratas? in other words: do you mean that one would not have to
>> reset GUSB2PHYCFG(0) if there were no erratas?
>
> no, it's done for peripheral configuration of dwc3. Look at the code
> more carefully:
sorry for the confusion - the word "reset" should be "configuration".
The function I am looking at is dwc3_phy_setup(): apart from toggling
some "errata bits" it also configures the PHY modes. I am wondering if
I need to do this setup (DWC3_GUSB2PHYCFG_ULPI_UTMI and
DWC3_GUSB2PHYCFG_PHYIF_MASK related bits) not only for the *first*
port ("DWC3_GUSB2PHYCFG(0)") but also for the "other" ports (index 1
and 2 in above case, where the roothub has 3 ports)

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> On Mon, Jan 9, 2017 at 12:18 PM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>>
>> Hi,
>>
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>>>>> come across something I did not know yet:
>>>>> dwc3 has an internal USB2 hub (from what I can read in the code there
>>>>> seem to be multiple USB3 ports supported as well).
>>>>
>>>> no, that's not true. It has a roothub when working as host. But that's
>>>> it. When working as peripheral, it's always single-port AFAIR.
>>> OK, I should have been more clear here: I am only talking about
>>> host-mode since DWC3_GHWPARAMS0 on the GXL/GXM SoCs has a value of
>>> 0x20208009 which translates to "DWC3_GHWPARAMS0_MODE_HOST".
>>>
>>> are you sure about the fact that it does not have an internal hub?
>>> What I see in both, the vendor kernel's and my own patched mainline
>>> kernel log is:
>>> [   19.130331 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>> [   19.130385 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>>> assigned bus number 1
>>> [   19.139666 at 3] xhci-hcd xhci-hcd.0.auto: irq 62, io mem 0xc9000000
>>> [   19.145295 at 3] hub 1-0:1.0: USB hub found
>>
>> this is the roothub :-)
> agreed :)
>
>>> [   19.148098 at 3] hub 1-0:1.0: 3 ports detected
> this is the one I wanted to point out: in this case there are 3 ports
> on the (USB2) roothub.
> the SoC has one USB2 for each of these ports, and all 3 PHYs have to
> be initialized to get USB working (this seems to be my main issue).
>
>>> [   19.152396 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>> [   19.157813 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>>> assigned bus number 2
>>> [   19.166598 at 3] hub 2-0:1.0: USB hub found
>>> [   19.169452 at 3] hub 2-0:1.0: config failed, hub doesn't have any
>>> ports! (err -19)
>>> This is from a GXM SoC which also comes with 3x USB2 PHYs (these are
>>> not Synopsys DesignWare PHYs but custom ones from Amlogic).
>>> I see similar messages but with "2 ports detect" on a GXL SoC which
>>> comes with 2x USB2 PHYs.
>>
>> The fact that is claims that it has "no ports" hints at quirk caused,
>> likely, by poor RTL edits. coreConsultant should make sure that
>> MAX_PORTS (in xHCI HCC_PARAMS* register set) was set to correct value,
>> but somehow that field is set to 0 in one of your Amlogic SoC.
> yes, it's a USB2-only configuration (so there are no USB3 ports on the
> internal roothub)

aha, makes sense.

>>>>> When searching the web I did not come across any SoC that uses a
>>>>> configuration with more than one port enabled.
>>>>>
>>>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>>>> I see the following USB2 PHY register configuration (full register
>>>>> dump from the kernel that was shipped with the device is attached):
>>>>> GUSB2PHYCFG(0) = 0x40102500
>>>>> GUSB2PHYCFG(1) = 0x40102540
>>>>> GUSB2PHYCFG(2) = 0x40102540
>>>>
>>>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>>>> and just let xHCI core handle the ports.
>>> could you be more specific with "xHCI core" - do you mean the core in
>>> the dwc3 IP or drivers/usb/host/xhci-*?
>>> However, we still have a "problem" here: the USB PHYs for each
>>> "enabled" port have to be turned on (if I leave only one USB PHY
>>> disabled then none of the ports is working). unfortunately the current
>>> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
>>> either 0 or 1 PHY for each HCD.

true. But even so, that PHY handle is only needed for devices which
weren't properly configured at coreConsultant time.

>>>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>>>
>>>> That shouldn't be necessary, actually. If it is, it means the HW was
>>>> poorly integrated. In that case, we _can_ add the other resets, but I
>>>> need confirmation that they are needed by means of a public errata
>>>> document.
>>>>
>>>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>>>> from linux-usb) kernel works fine even with just applying the reset to
>>>>> GUSB2PHYCFG(0).
>>>>
>>>> there you go
>>> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
>>> current dwc3 code in dwc3_phy_setup) is done only because of the
>>> quirks/erratas? in other words: do you mean that one would not have to
>>> reset GUSB2PHYCFG(0) if there were no erratas?
>>
>> no, it's done for peripheral configuration of dwc3. Look at the code
>> more carefully:
> sorry for the confusion - the word "reset" should be "configuration".
> The function I am looking at is dwc3_phy_setup(): apart from toggling
> some "errata bits" it also configures the PHY modes. I am wondering if
> I need to do this setup (DWC3_GUSB2PHYCFG_ULPI_UTMI and
> DWC3_GUSB2PHYCFG_PHYIF_MASK related bits) not only for the *first*
> port ("DWC3_GUSB2PHYCFG(0)") but also for the "other" ports (index 1
> and 2 in above case, where the roothub has 3 ports)

Ideally, that should've been set at coreConsultant (RTL instantiation)
time. If it wasn't, then we need to add a quirk for these SoCs
accordingly. We _do_ need documentation that this quirk is, indeed,
needed.


-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/66c864d6/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Mon, Jan 9, 2017 at 12:55 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>> On Mon, Jan 9, 2017 at 12:18 PM, Felipe Balbi
>> <felipe.balbi@linux.intel.com> wrote:
>>>
>>> Hi,
>>>
>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>>>> while adding USB support on the Amlogic Meson GXL / GXM SoCs I have
>>>>>> come across something I did not know yet:
>>>>>> dwc3 has an internal USB2 hub (from what I can read in the code there
>>>>>> seem to be multiple USB3 ports supported as well).
>>>>>
>>>>> no, that's not true. It has a roothub when working as host. But that's
>>>>> it. When working as peripheral, it's always single-port AFAIR.
>>>> OK, I should have been more clear here: I am only talking about
>>>> host-mode since DWC3_GHWPARAMS0 on the GXL/GXM SoCs has a value of
>>>> 0x20208009 which translates to "DWC3_GHWPARAMS0_MODE_HOST".
>>>>
>>>> are you sure about the fact that it does not have an internal hub?
>>>> What I see in both, the vendor kernel's and my own patched mainline
>>>> kernel log is:
>>>> [   19.130331 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>>> [   19.130385 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>>>> assigned bus number 1
>>>> [   19.139666 at 3] xhci-hcd xhci-hcd.0.auto: irq 62, io mem 0xc9000000
>>>> [   19.145295 at 3] hub 1-0:1.0: USB hub found
>>>
>>> this is the roothub :-)
>> agreed :)
>>
>>>> [   19.148098 at 3] hub 1-0:1.0: 3 ports detected
>> this is the one I wanted to point out: in this case there are 3 ports
>> on the (USB2) roothub.
>> the SoC has one USB2 for each of these ports, and all 3 PHYs have to
>> be initialized to get USB working (this seems to be my main issue).
>>
>>>> [   19.152396 at 3] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
>>>> [   19.157813 at 3] xhci-hcd xhci-hcd.0.auto: new USB bus registered,
>>>> assigned bus number 2
>>>> [   19.166598 at 3] hub 2-0:1.0: USB hub found
>>>> [   19.169452 at 3] hub 2-0:1.0: config failed, hub doesn't have any
>>>> ports! (err -19)
>>>> This is from a GXM SoC which also comes with 3x USB2 PHYs (these are
>>>> not Synopsys DesignWare PHYs but custom ones from Amlogic).
>>>> I see similar messages but with "2 ports detect" on a GXL SoC which
>>>> comes with 2x USB2 PHYs.
>>>
>>> The fact that is claims that it has "no ports" hints at quirk caused,
>>> likely, by poor RTL edits. coreConsultant should make sure that
>>> MAX_PORTS (in xHCI HCC_PARAMS* register set) was set to correct value,
>>> but somehow that field is set to 0 in one of your Amlogic SoC.
>> yes, it's a USB2-only configuration (so there are no USB3 ports on the
>> internal roothub)
>
> aha, makes sense.
>
>>>>>> When searching the web I did not come across any SoC that uses a
>>>>>> configuration with more than one port enabled.
>>>>>>
>>>>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>>>>> I see the following USB2 PHY register configuration (full register
>>>>>> dump from the kernel that was shipped with the device is attached):
>>>>>> GUSB2PHYCFG(0) = 0x40102500
>>>>>> GUSB2PHYCFG(1) = 0x40102540
>>>>>> GUSB2PHYCFG(2) = 0x40102540
>>>>>
>>>>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>>>>> and just let xHCI core handle the ports.
>>>> could you be more specific with "xHCI core" - do you mean the core in
>>>> the dwc3 IP or drivers/usb/host/xhci-*?
>>>> However, we still have a "problem" here: the USB PHYs for each
>>>> "enabled" port have to be turned on (if I leave only one USB PHY
>>>> disabled then none of the ports is working). unfortunately the current
>>>> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
>>>> either 0 or 1 PHY for each HCD.
>
> true. But even so, that PHY handle is only needed for devices which
> weren't properly configured at coreConsultant time.
I don't understand that - there are two separate modules involved
here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
choose Synopsys DesignWare PHYs)
(some background info: the USB2 PHYs are in "reset mode" by default)
So even if the dwc3 IP block is configured correctly at coreConsultant
time we still need to configure the PHYs. From "USB controller driver"
(could be dwc3, or some generic hcd.c code, etc.) this means that
phy_init() and phy_power_on() needs to be called for each of the three
"Amlogic USB2 PHYs". the current code however only calls these for the
first PHY (leaving the others in reset mode = disabled).

>>>>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>>>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>>>>
>>>>> That shouldn't be necessary, actually. If it is, it means the HW was
>>>>> poorly integrated. In that case, we _can_ add the other resets, but I
>>>>> need confirmation that they are needed by means of a public errata
>>>>> document.
>>>>>
>>>>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>>>>> from linux-usb) kernel works fine even with just applying the reset to
>>>>>> GUSB2PHYCFG(0).
>>>>>
>>>>> there you go
>>>> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
>>>> current dwc3 code in dwc3_phy_setup) is done only because of the
>>>> quirks/erratas? in other words: do you mean that one would not have to
>>>> reset GUSB2PHYCFG(0) if there were no erratas?
>>>
>>> no, it's done for peripheral configuration of dwc3. Look at the code
>>> more carefully:
>> sorry for the confusion - the word "reset" should be "configuration".
>> The function I am looking at is dwc3_phy_setup(): apart from toggling
>> some "errata bits" it also configures the PHY modes. I am wondering if
>> I need to do this setup (DWC3_GUSB2PHYCFG_ULPI_UTMI and
>> DWC3_GUSB2PHYCFG_PHYIF_MASK related bits) not only for the *first*
>> port ("DWC3_GUSB2PHYCFG(0)") but also for the "other" ports (index 1
>> and 2 in above case, where the roothub has 3 ports)
>
> Ideally, that should've been set at coreConsultant (RTL instantiation)
> time. If it wasn't, then we need to add a quirk for these SoCs
> accordingly. We _do_ need documentation that this quirk is, indeed,
> needed.
That is an explanation I'm fine with: we trust the (default) values
which are in hardware until we have documentation that we need a
quirk. I do not have access to Amlogic's documentation so I can't
provide that - but we can probably leave it as it is as it "worked for
me".

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>>>>> When searching the web I did not come across any SoC that uses a
>>>>>>> configuration with more than one port enabled.
>>>>>>>
>>>>>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>>>>>> I see the following USB2 PHY register configuration (full register
>>>>>>> dump from the kernel that was shipped with the device is attached):
>>>>>>> GUSB2PHYCFG(0) = 0x40102500
>>>>>>> GUSB2PHYCFG(1) = 0x40102540
>>>>>>> GUSB2PHYCFG(2) = 0x40102540
>>>>>>
>>>>>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>>>>>> and just let xHCI core handle the ports.
>>>>> could you be more specific with "xHCI core" - do you mean the core in
>>>>> the dwc3 IP or drivers/usb/host/xhci-*?
>>>>> However, we still have a "problem" here: the USB PHYs for each
>>>>> "enabled" port have to be turned on (if I leave only one USB PHY
>>>>> disabled then none of the ports is working). unfortunately the current
>>>>> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
>>>>> either 0 or 1 PHY for each HCD.
>>
>> true. But even so, that PHY handle is only needed for devices which
>> weren't properly configured at coreConsultant time.
> I don't understand that - there are two separate modules involved
> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
> choose Synopsys DesignWare PHYs)
> (some background info: the USB2 PHYs are in "reset mode" by default)
> So even if the dwc3 IP block is configured correctly at coreConsultant
> time we still need to configure the PHYs. From "USB controller driver"
> (could be dwc3, or some generic hcd.c code, etc.) this means that
> phy_init() and phy_power_on() needs to be called for each of the three
> "Amlogic USB2 PHYs". the current code however only calls these for the
> first PHY (leaving the others in reset mode = disabled).

argh, good point. In that case, we need to figure out the best way to
pass all these handles to xHCI-plat

>>>>>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>>>>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>>>>>
>>>>>> That shouldn't be necessary, actually. If it is, it means the HW was
>>>>>> poorly integrated. In that case, we _can_ add the other resets, but I
>>>>>> need confirmation that they are needed by means of a public errata
>>>>>> document.
>>>>>>
>>>>>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>>>>>> from linux-usb) kernel works fine even with just applying the reset to
>>>>>>> GUSB2PHYCFG(0).
>>>>>>
>>>>>> there you go
>>>>> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
>>>>> current dwc3 code in dwc3_phy_setup) is done only because of the
>>>>> quirks/erratas? in other words: do you mean that one would not have to
>>>>> reset GUSB2PHYCFG(0) if there were no erratas?
>>>>
>>>> no, it's done for peripheral configuration of dwc3. Look at the code
>>>> more carefully:
>>> sorry for the confusion - the word "reset" should be "configuration".
>>> The function I am looking at is dwc3_phy_setup(): apart from toggling
>>> some "errata bits" it also configures the PHY modes. I am wondering if
>>> I need to do this setup (DWC3_GUSB2PHYCFG_ULPI_UTMI and
>>> DWC3_GUSB2PHYCFG_PHYIF_MASK related bits) not only for the *first*
>>> port ("DWC3_GUSB2PHYCFG(0)") but also for the "other" ports (index 1
>>> and 2 in above case, where the roothub has 3 ports)
>>
>> Ideally, that should've been set at coreConsultant (RTL instantiation)
>> time. If it wasn't, then we need to add a quirk for these SoCs
>> accordingly. We _do_ need documentation that this quirk is, indeed,
>> needed.
> That is an explanation I'm fine with: we trust the (default) values
> which are in hardware until we have documentation that we need a
> quirk. I do not have access to Amlogic's documentation so I can't
> provide that - but we can probably leave it as it is as it "worked for
> me".

awesome, so we need *only* phy_init() / phy_power_on() for the other
PHYs, right?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/7482a1b7/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Mon, Jan 9, 2017 at 1:16 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>>>>>> When searching the web I did not come across any SoC that uses a
>>>>>>>> configuration with more than one port enabled.
>>>>>>>>
>>>>>>>> On my Amlogic Meson GXM device (consumer device, no development board)
>>>>>>>> I see the following USB2 PHY register configuration (full register
>>>>>>>> dump from the kernel that was shipped with the device is attached):
>>>>>>>> GUSB2PHYCFG(0) = 0x40102500
>>>>>>>> GUSB2PHYCFG(1) = 0x40102540
>>>>>>>> GUSB2PHYCFG(2) = 0x40102540
>>>>>>>
>>>>>>> multiple PHYs are only used by the host block (xHCI). Don't touch these
>>>>>>> and just let xHCI core handle the ports.
>>>>>> could you be more specific with "xHCI core" - do you mean the core in
>>>>>> the dwc3 IP or drivers/usb/host/xhci-*?
>>>>>> However, we still have a "problem" here: the USB PHYs for each
>>>>>> "enabled" port have to be turned on (if I leave only one USB PHY
>>>>>> disabled then none of the ports is working). unfortunately the current
>>>>>> code (both, in dwc3 and drivers/usb/host/*) assumes that there's
>>>>>> either 0 or 1 PHY for each HCD.
>>>
>>> true. But even so, that PHY handle is only needed for devices which
>>> weren't properly configured at coreConsultant time.
>> I don't understand that - there are two separate modules involved
>> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
>> choose Synopsys DesignWare PHYs)
>> (some background info: the USB2 PHYs are in "reset mode" by default)
>> So even if the dwc3 IP block is configured correctly at coreConsultant
>> time we still need to configure the PHYs. From "USB controller driver"
>> (could be dwc3, or some generic hcd.c code, etc.) this means that
>> phy_init() and phy_power_on() needs to be called for each of the three
>> "Amlogic USB2 PHYs". the current code however only calls these for the
>> first PHY (leaving the others in reset mode = disabled).
>
> argh, good point. In that case, we need to figure out the best way to
> pass all these handles to xHCI-plat
OK, I assume that we want to let xHCI-plat manage the PHYs in the
future instead of doing this in dwc3 (dwc3 may have to pass these to
xHCI-plat, but it should not do the logic on it's own)?

>>>>>>>> Then vendor kernel sources (a 3.14 kernel) are adding the resets for
>>>>>>>> GUSB2PHYCFG([1-3]) in dwc3_core_soft_reset().
>>>>>>>
>>>>>>> That shouldn't be necessary, actually. If it is, it means the HW was
>>>>>>> poorly integrated. In that case, we _can_ add the other resets, but I
>>>>>>> need confirmation that they are needed by means of a public errata
>>>>>>> document.
>>>>>>>
>>>>>>>> A mainline 4.9+(Meson GXL USB PHY patches + dwc3/xhci-plat DMA patches
>>>>>>>> from linux-usb) kernel works fine even with just applying the reset to
>>>>>>>> GUSB2PHYCFG(0).
>>>>>>>
>>>>>>> there you go
>>>>>> does that mean that the reset of GUSB2PHYCFG(0) (which is part of the
>>>>>> current dwc3 code in dwc3_phy_setup) is done only because of the
>>>>>> quirks/erratas? in other words: do you mean that one would not have to
>>>>>> reset GUSB2PHYCFG(0) if there were no erratas?
>>>>>
>>>>> no, it's done for peripheral configuration of dwc3. Look at the code
>>>>> more carefully:
>>>> sorry for the confusion - the word "reset" should be "configuration".
>>>> The function I am looking at is dwc3_phy_setup(): apart from toggling
>>>> some "errata bits" it also configures the PHY modes. I am wondering if
>>>> I need to do this setup (DWC3_GUSB2PHYCFG_ULPI_UTMI and
>>>> DWC3_GUSB2PHYCFG_PHYIF_MASK related bits) not only for the *first*
>>>> port ("DWC3_GUSB2PHYCFG(0)") but also for the "other" ports (index 1
>>>> and 2 in above case, where the roothub has 3 ports)
>>>
>>> Ideally, that should've been set at coreConsultant (RTL instantiation)
>>> time. If it wasn't, then we need to add a quirk for these SoCs
>>> accordingly. We _do_ need documentation that this quirk is, indeed,
>>> needed.
>> That is an explanation I'm fine with: we trust the (default) values
>> which are in hardware until we have documentation that we need a
>> quirk. I do not have access to Amlogic's documentation so I can't
>> provide that - but we can probably leave it as it is as it "worked for
>> me".
>
> awesome, so we need *only* phy_init() / phy_power_on() for the other
> PHYs, right?
correct!
That made me curious and I found these:
- ehci-platform.c has ehci_platform_power_{on,off}
- xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
- ohci-platform.c has ohci_platform_power_{on,off}
- (there are some more, for example ohci-exynos.c which are doing similar stuff)

all of them are parsing the "phys" property and build an array of "struct phy*":
of_count_phandle_with_args(np, "phys", "#phy-cells");
(afterwards they apply phy_{init,power_on,power_off,exit} in a loop on
all of the PHYs)

Maybe it would make sense to remove duplicated code from these drivers
and create some generic functions from it.
that would result in a few functions (function names are just to
illustrate my idea):
- devm_get_all_phys_from_of_node()
- all_phys_init_and_power_on() (phy_init and phy_power_on always seem
to be used together)
- all_phys_power_off_and_exit() (phy_power_off and phy_exit always
seem to be used together)

let me know what you think

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

[snip]

>>>> true. But even so, that PHY handle is only needed for devices which
>>>> weren't properly configured at coreConsultant time.
>>> I don't understand that - there are two separate modules involved
>>> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
>>> choose Synopsys DesignWare PHYs)
>>> (some background info: the USB2 PHYs are in "reset mode" by default)
>>> So even if the dwc3 IP block is configured correctly at coreConsultant
>>> time we still need to configure the PHYs. From "USB controller driver"
>>> (could be dwc3, or some generic hcd.c code, etc.) this means that
>>> phy_init() and phy_power_on() needs to be called for each of the three
>>> "Amlogic USB2 PHYs". the current code however only calls these for the
>>> first PHY (leaving the others in reset mode = disabled).
>>
>> argh, good point. In that case, we need to figure out the best way to
>> pass all these handles to xHCI-plat
> OK, I assume that we want to let xHCI-plat manage the PHYs in the
> future instead of doing this in dwc3 (dwc3 may have to pass these to
> xHCI-plat, but it should not do the logic on it's own)?

perhaps. Maybe that mode check for dwc3 to bail out if mode == Host
should be done earlier.

[snip]

>>> That is an explanation I'm fine with: we trust the (default) values
>>> which are in hardware until we have documentation that we need a
>>> quirk. I do not have access to Amlogic's documentation so I can't
>>> provide that - but we can probably leave it as it is as it "worked for
>>> me".
>>
>> awesome, so we need *only* phy_init() / phy_power_on() for the other
>> PHYs, right?
> correct!
> That made me curious and I found these:
> - ehci-platform.c has ehci_platform_power_{on,off}
> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
> - ohci-platform.c has ohci_platform_power_{on,off}
> - (there are some more, for example ohci-exynos.c which are doing similar stuff)
>
> all of them are parsing the "phys" property and build an array of "struct phy*":
> of_count_phandle_with_args(np, "phys", "#phy-cells");
> (afterwards they apply phy_{init,power_on,power_off,exit} in a loop on
> all of the PHYs)
>
> Maybe it would make sense to remove duplicated code from these drivers
> and create some generic functions from it.

makes sense to me. The difficulty is really just making sure no
regressions are caused along the way. Also, DWC3 creates xHCI
platform-device manually, so we need to figure out a clean way of
passing along PHY phandles to xHCI. Another thing to consider is
dual-role implementations of dwc3. In such cases, peripheral side also
needs to control PHY[0].

> that would result in a few functions (function names are just to
> illustrate my idea):
> - devm_get_all_phys_from_of_node()
> - all_phys_init_and_power_on() (phy_init and phy_power_on always seem
> to be used together)
> - all_phys_power_off_and_exit() (phy_power_off and phy_exit always
> seem to be used together)
>
> let me know what you think

I like that, specially so if it's done generically and all HCD drivers
just use the same piece of code.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/c247c758/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Mon, Jan 9, 2017 at 1:39 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
> [snip]
>
>>>>> true. But even so, that PHY handle is only needed for devices which
>>>>> weren't properly configured at coreConsultant time.
>>>> I don't understand that - there are two separate modules involved
>>>> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
>>>> choose Synopsys DesignWare PHYs)
>>>> (some background info: the USB2 PHYs are in "reset mode" by default)
>>>> So even if the dwc3 IP block is configured correctly at coreConsultant
>>>> time we still need to configure the PHYs. From "USB controller driver"
>>>> (could be dwc3, or some generic hcd.c code, etc.) this means that
>>>> phy_init() and phy_power_on() needs to be called for each of the three
>>>> "Amlogic USB2 PHYs". the current code however only calls these for the
>>>> first PHY (leaving the others in reset mode = disabled).
>>>
>>> argh, good point. In that case, we need to figure out the best way to
>>> pass all these handles to xHCI-plat
>> OK, I assume that we want to let xHCI-plat manage the PHYs in the
>> future instead of doing this in dwc3 (dwc3 may have to pass these to
>> xHCI-plat, but it should not do the logic on it's own)?
>
> perhaps. Maybe that mode check for dwc3 to bail out if mode == Host
> should be done earlier.
I guess your idea is to let xHCI-plat handle all phy_* logic when in host-mode?

> [snip]
>
>>>> That is an explanation I'm fine with: we trust the (default) values
>>>> which are in hardware until we have documentation that we need a
>>>> quirk. I do not have access to Amlogic's documentation so I can't
>>>> provide that - but we can probably leave it as it is as it "worked for
>>>> me".
>>>
>>> awesome, so we need *only* phy_init() / phy_power_on() for the other
>>> PHYs, right?
>> correct!
>> That made me curious and I found these:
>> - ehci-platform.c has ehci_platform_power_{on,off}
>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>> - ohci-platform.c has ohci_platform_power_{on,off}
>> - (there are some more, for example ohci-exynos.c which are doing similar stuff)
>>
>> all of them are parsing the "phys" property and build an array of "struct phy*":
>> of_count_phandle_with_args(np, "phys", "#phy-cells");
>> (afterwards they apply phy_{init,power_on,power_off,exit} in a loop on
>> all of the PHYs)
>>
>> Maybe it would make sense to remove duplicated code from these drivers
>> and create some generic functions from it.
>
> makes sense to me. The difficulty is really just making sure no
> regressions are caused along the way. Also, DWC3 creates xHCI
> platform-device manually, so we need to figure out a clean way of
> passing along PHY phandles to xHCI. Another thing to consider is
> dual-role implementations of dwc3. In such cases, peripheral side also
> needs to control PHY[0].
indeed, regression-testing is probably the hardest part here!

I think we already have the correct device_node (sysdev->of_node)
available in xHCI-plat, see the comment above the sysdev variable
assignment in xhci_plat_probe.

The Amlogic GXL and GXM SoCs also support dual-role mode, but I think
that is a bit more exotic:
like you mentioned PHY[0] is used for dual-role mode. There is an
additional USB3 PHY which also does mode-detection (in otg mode).
when that USB3 PHY/mode-detection block detects that it has to switch
to device mode it re-configures USB2 PHY[0] accordingly.
however it doesn't stop here: after that it goes ahead and turns on an
additional dwc2 (yes, dwc2, not dwc3!) IP block which is limited to
"device" (peripheral) mode only.
So on Amlogic GXL and GXM SoCs host-mode is handled by dwc3 while
device-mode is handled by dwc2

>> that would result in a few functions (function names are just to
>> illustrate my idea):
>> - devm_get_all_phys_from_of_node()
>> - all_phys_init_and_power_on() (phy_init and phy_power_on always seem
>> to be used together)
>> - all_phys_power_off_and_exit() (phy_power_off and phy_exit always
>> seem to be used together)
>>
>> let me know what you think
>
> I like that, specially so if it's done generically and all HCD drivers
> just use the same piece of code.
great!
how should we proceed: should I come up with a first RFC so we can
then discuss issues/improvements based on that RFC patch?

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Felipe Balbi]

Hi,

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> On Mon, Jan 9, 2017 at 1:39 PM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>>
>> Hi,
>>
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>> [snip]
>>
>>>>>> true. But even so, that PHY handle is only needed for devices which
>>>>>> weren't properly configured at coreConsultant time.
>>>>> I don't understand that - there are two separate modules involved
>>>>> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
>>>>> choose Synopsys DesignWare PHYs)
>>>>> (some background info: the USB2 PHYs are in "reset mode" by default)
>>>>> So even if the dwc3 IP block is configured correctly at coreConsultant
>>>>> time we still need to configure the PHYs. From "USB controller driver"
>>>>> (could be dwc3, or some generic hcd.c code, etc.) this means that
>>>>> phy_init() and phy_power_on() needs to be called for each of the three
>>>>> "Amlogic USB2 PHYs". the current code however only calls these for the
>>>>> first PHY (leaving the others in reset mode = disabled).
>>>>
>>>> argh, good point. In that case, we need to figure out the best way to
>>>> pass all these handles to xHCI-plat
>>> OK, I assume that we want to let xHCI-plat manage the PHYs in the
>>> future instead of doing this in dwc3 (dwc3 may have to pass these to
>>> xHCI-plat, but it should not do the logic on it's own)?
>>
>> perhaps. Maybe that mode check for dwc3 to bail out if mode == Host
>> should be done earlier.
> I guess your idea is to let xHCI-plat handle all phy_* logic when in host-mode?

right. I guess at the end of the day host-only dwc3 instances shouldn't
need dwc3.ko at all. Only peripheral-only and dual-role implementations
should rely on dwc3.ko.

>>> Maybe it would make sense to remove duplicated code from these drivers
>>> and create some generic functions from it.
>>
>> makes sense to me. The difficulty is really just making sure no
>> regressions are caused along the way. Also, DWC3 creates xHCI
>> platform-device manually, so we need to figure out a clean way of
>> passing along PHY phandles to xHCI. Another thing to consider is
>> dual-role implementations of dwc3. In such cases, peripheral side also
>> needs to control PHY[0].
> indeed, regression-testing is probably the hardest part here!
>
> I think we already have the correct device_node (sysdev->of_node)
> available in xHCI-plat, see the comment above the sysdev variable
> assignment in xhci_plat_probe.

cool

> The Amlogic GXL and GXM SoCs also support dual-role mode, but I think
> that is a bit more exotic:

okay, in that case this is less important for you. We should really make
sure we don't cause problems for a future dual-role support.

> like you mentioned PHY[0] is used for dual-role mode. There is an
> additional USB3 PHY which also does mode-detection (in otg mode).
> when that USB3 PHY/mode-detection block detects that it has to switch
> to device mode it re-configures USB2 PHY[0] accordingly.

okay

> however it doesn't stop here: after that it goes ahead and turns on an
> additional dwc2 (yes, dwc2, not dwc3!) IP block which is limited to
> "device" (peripheral) mode only.

hehe, that's interesting :-)

> So on Amlogic GXL and GXM SoCs host-mode is handled by dwc3 while
> device-mode is handled by dwc2

so you have host-only dwc3 (for all practical reasons, a standard xHCI)
and a peripheral only dwc2. Good to know. I wonder why it was done this
way. Maybe Amlogic didn't pay for dual-role dwc3 license?

>>> that would result in a few functions (function names are just to
>>> illustrate my idea):
>>> - devm_get_all_phys_from_of_node()
>>> - all_phys_init_and_power_on() (phy_init and phy_power_on always seem
>>> to be used together)
>>> - all_phys_power_off_and_exit() (phy_power_off and phy_exit always
>>> seem to be used together)
>>>
>>> let me know what you think
>>
>> I like that, specially so if it's done generically and all HCD drivers
>> just use the same piece of code.
> great!
> how should we proceed: should I come up with a first RFC so we can
> then discuss issues/improvements based on that RFC patch?

yeah, that's usually the way :-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20170109/0b476e39/attachment.sig>

dwc3: add support for hardware with multiple ports on USB2 hub enabled

[Martin Blumenstingl]

On Mon, Jan 9, 2017 at 2:05 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>> On Mon, Jan 9, 2017 at 1:39 PM, Felipe Balbi
>> <felipe.balbi@linux.intel.com> wrote:
>>>
>>> Hi,
>>>
>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>
>>> [snip]
>>>
>>>>>>> true. But even so, that PHY handle is only needed for devices which
>>>>>>> weren't properly configured at coreConsultant time.
>>>>>> I don't understand that - there are two separate modules involved
>>>>>> here: 1. the dwc3 IP block 2. Amlogic's own USB2 PHYs (they did not
>>>>>> choose Synopsys DesignWare PHYs)
>>>>>> (some background info: the USB2 PHYs are in "reset mode" by default)
>>>>>> So even if the dwc3 IP block is configured correctly at coreConsultant
>>>>>> time we still need to configure the PHYs. From "USB controller driver"
>>>>>> (could be dwc3, or some generic hcd.c code, etc.) this means that
>>>>>> phy_init() and phy_power_on() needs to be called for each of the three
>>>>>> "Amlogic USB2 PHYs". the current code however only calls these for the
>>>>>> first PHY (leaving the others in reset mode = disabled).
>>>>>
>>>>> argh, good point. In that case, we need to figure out the best way to
>>>>> pass all these handles to xHCI-plat
>>>> OK, I assume that we want to let xHCI-plat manage the PHYs in the
>>>> future instead of doing this in dwc3 (dwc3 may have to pass these to
>>>> xHCI-plat, but it should not do the logic on it's own)?
>>>
>>> perhaps. Maybe that mode check for dwc3 to bail out if mode == Host
>>> should be done earlier.
>> I guess your idea is to let xHCI-plat handle all phy_* logic when in host-mode?
>
> right. I guess at the end of the day host-only dwc3 instances shouldn't
> need dwc3.ko at all. Only peripheral-only and dual-role implementations
> should rely on dwc3.ko.
interesting thought, I guess that makes sense!

> okay, in that case this is less important for you. We should really make
> sure we don't cause problems for a future dual-role support.
>
>> like you mentioned PHY[0] is used for dual-role mode. There is an
>> additional USB3 PHY which also does mode-detection (in otg mode).
>> when that USB3 PHY/mode-detection block detects that it has to switch
>> to device mode it re-configures USB2 PHY[0] accordingly.
>
> okay
>
>> however it doesn't stop here: after that it goes ahead and turns on an
>> additional dwc2 (yes, dwc2, not dwc3!) IP block which is limited to
>> "device" (peripheral) mode only.
>
> hehe, that's interesting :-)
>
>> So on Amlogic GXL and GXM SoCs host-mode is handled by dwc3 while
>> device-mode is handled by dwc2
>
> so you have host-only dwc3 (for all practical reasons, a standard xHCI)
> and a peripheral only dwc2. Good to know. I wonder why it was done this
> way. Maybe Amlogic didn't pay for dual-role dwc3 license?
I don't know any details how this decision was made but licensing
costs may have been the reason (it may also be the same reason why
they chose to enable multiple USB ports on just one dwc3 instance).
Please note that this is just pure speculation and no official
information (as I don't have that)!

>>>> that would result in a few functions (function names are just to
>>>> illustrate my idea):
>>>> - devm_get_all_phys_from_of_node()
>>>> - all_phys_init_and_power_on() (phy_init and phy_power_on always seem
>>>> to be used together)
>>>> - all_phys_power_off_and_exit() (phy_power_off and phy_exit always
>>>> seem to be used together)
>>>>
>>>> let me know what you think
>>>
>>> I like that, specially so if it's done generically and all HCD drivers
>>> just use the same piece of code.
>> great!
>> how should we proceed: should I come up with a first RFC so we can
>> then discuss issues/improvements based on that RFC patch?
>
> yeah, that's usually the way :-)
I sent a first draft to the linux-usb list with the subject "[RFC
PATCH 0/2] initialize (multiple) PHYs in xhci-plat" (which does not
show up in the usual web-archives yet)


Regards,
Martin