[PATCH V4] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com --- V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST; - } else if (is_mx7()) { + } else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status); @@ -573,9 +573,8 @@ static int ehci_usb_of_to_plat(struct udevice *dev) case USB_DR_MODE_PERIPHERAL: plat->init_type = USB_INIT_DEVICE; break; - case USB_DR_MODE_OTG: - case USB_DR_MODE_UNKNOWN: - return ehci_usb_phy_mode(dev); + default: + plat->init_type = USB_INIT_UNKNOWN; };
return 0; @@ -677,6 +676,20 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(1); #endif
+ /* + * If the device tree didn't specify host or device, + * the default is USB_INIT_UNKNOWN, so we need to check + * the register. For imx8mm and imx8mn, the clocks need to be + * running first, so we defer the check until they are. + */ + if (priv->init_type == USB_INIT_UNKNOWN) { + ret = ehci_usb_phy_mode(dev); + if (ret) + goto err_clk; + else + priv->init_type = plat->init_type; + } + #if CONFIG_IS_ENABLED(DM_REGULATOR) ret = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); @@ -741,8 +754,8 @@ err_regulator: #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) regulator_set_enable(priv->vbus_supply, false); -err_clk: #endif +err_clk: #if CONFIG_IS_ENABLED(CLK) clk_disable(&priv->clk); #else diff --git a/include/usb.h b/include/usb.h index f032de8af9..7e3796bd5b 100644 --- a/include/usb.h +++ b/include/usb.h @@ -163,7 +163,8 @@ struct int_queue; */ enum usb_init_type { USB_INIT_HOST, - USB_INIT_DEVICE + USB_INIT_DEVICE, + USB_INIT_UNKNOWN, };
/**********************************************************************
Hi Adam,
On Thu, Feb 3, 2022 at 6:20 PM Adam Ford aford173@gmail.com wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
The last time I tried to test your previous version I was having an issue which is solved by Marcel's patch: https://lists.denx.de/pipermail/u-boot/2022-February/474006.html
With Marcel's patch applied and your updated version, "ums 0 mmc 0" works fine on a imx7s-warp, thanks:
Tested-by: Fabio Estevam festevam@gmail.com
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
- } else if (is_mx7()) {
- } else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);
Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
U-Boot 2022.01-00678-ge83818c782-dirty (Feb 06 2022 - 18:39:41 -0600)
CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz Reset cause: POR Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit DRAM: 2 GiB Core: 99 devices, 21 uclasses, devicetree: separate WDT: Started watchdog@30280000 with servicing (60s timeout) MMC: FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2 Loading Environment from MMC... OK In: serial@30890000 Out: serial@30890000 Err: serial@30890000 Net: eth0: ethernet@30be0000 Hit any key to stop autoboot: 0 u-boot=> usb start starting USB... Bus usb@32e40000: imx8m_power_domain_on 0 imx8m_power_domain_on 2 USB EHCI 1.00 Bus usb@32e50000: imx8m_power_domain_on 0 imx8m_power_domain_on 3 USB EHCI 1.00 scanning bus usb@32e40000 for devices... imx8m_power_domain_on 0 imx8m_power_domain_on 2 1 USB Device(s) found scanning bus usb@32e50000 for devices... imx8m_power_domain_on 0 imx8m_power_domain_on 3 3 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found u-boot=>
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
(you can verify that by adding more printf()s)
On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
If you look at what this patch is doing, ehci_usb_of_to_plat is no longer making any calls to the USB registers. ehci_usb_of_to_plat is just an exercise in reading the device tree for the desired mode. If it's not host mode or peripheral mode it returns unknown. The part that reads the registers now is delayed until after the clocks are enabled inside the probe function which is called when USB is started and the power-domains are available.
adam
(you can verify that by adding more printf()s)
On 2/7/22 12:13, Adam Ford wrote:
On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
If you look at what this patch is doing, ehci_usb_of_to_plat is no longer making any calls to the USB registers. ehci_usb_of_to_plat is just an exercise in reading the device tree for the desired mode. If it's not host mode or peripheral mode it returns unknown. The part that reads the registers now is delayed until after the clocks are enabled inside the probe function which is called when USB is started and the power-domains are available.
Ah, I missed that part of the change.
One more thing, can you double-check that the "if (ctrl->init == USB_INIT_DEVICE)" checks in drivers/usb/host/ehci-hcd.c still work as they should ? If so, then this patch is fine.
On Mon, Feb 7, 2022 at 5:50 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 12:13, Adam Ford wrote:
On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote:
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
If you look at what this patch is doing, ehci_usb_of_to_plat is no longer making any calls to the USB registers. ehci_usb_of_to_plat is just an exercise in reading the device tree for the desired mode. If it's not host mode or peripheral mode it returns unknown. The part that reads the registers now is delayed until after the clocks are enabled inside the probe function which is called when USB is started and the power-domains are available.
Ah, I missed that part of the change.
One more thing, can you double-check that the "if (ctrl->init == USB_INIT_DEVICE)" checks in drivers/usb/host/ehci-hcd.c still work as they should ? If so, then this patch is fine.
Sure. If I run ums 0 mmc 2, I see the peripheral is enumerating on the U-Boot side:
U-Boot 2022.01-00678-ge83818c782-dirty (Feb 06 2022 - 18:55:39 -0600)
CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz Reset cause: POR Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit DRAM: 2 GiB Core: 99 devices, 21 uclasses, devicetree: separate WDT: Started watchdog@30280000 with servicing (60s timeout) MMC: FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2 Loading Environment from MMC... OK In: serial@30890000 Out: serial@30890000 Err: serial@30890000 Net: eth0: ethernet@30be0000 Hit any key to stop autoboot: 0 u-boot=> ums 0 mmc 2 UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000 imx8m_power_domain_on 0 imx8m_power_domain_on 2 -
And the power domains enable in the right order.
On the host side, I see:
usb 3-2: new high-speed USB device number 24 using xhci_hcd usb 3-2: New USB device found, idVendor=0525, idProduct=a4a5, bcdDevice= 2.21 usb 3-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 3-2: Product: USB download gadget usb 3-2: Manufacturer: U-Boot usb-storage 3-2:1.0: USB Mass Storage device detected usb-storage 3-2:1.0: Quirks match for vid 0525 pid a4a5: 10000 scsi host6: usb-storage 3-2:1.0 scsi 6:0:0:0: Direct-Access Linux UMS disk 0 ffff PQ: 0 ANSI: 2 sd 6:0:0:0: Attached scsi generic sg2 type 0 sd 6:0:0:0: [sdb] 61071360 512-byte logical blocks: (31.3 GB/29.1 GiB) sd 6:0:0:0: [sdb] Write Protect is off sd 6:0:0:0: [sdb] Mode Sense: 0f 00 00 00 sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sd 6:0:0:0: [sdb] Attached SCSI removable disk
On Mon, Feb 7, 2022 at 6:15 AM Adam Ford aford173@gmail.com wrote:
On Mon, Feb 7, 2022 at 5:50 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 12:13, Adam Ford wrote:
On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote:
On 2/3/22 22:20, Adam Ford wrote: > The imx8mm and imx8mn appear compatible with imx7d-usb > flags in the OTG driver. If the dr_mode is defined as > host or peripheral, the device appears to operate correctly, > however the auto host/peripheral detection results in an error. > > The solution isn't just adding checks for imx8mm and imx8mn to > the check for imx7, because the USB clock needs to be running > to read from the USBNC_PHY_STATUS_OFFSET register or it will hang. > > Marek requested that I not enable the clocks in ehci_usb_of_to_plat, > so I modified that function to return an unknown state if the > device tree does not explicitly state whether it is a host > or a peripheral. > > When the driver probes, it looks to see if it's in the unknown > state, and only then will it read the register to auto-detect. > > Signed-off-by: Adam Ford aford173@gmail.com > Tested-by: Tim Harvey tharvey@gateworks.com > --- > V4: Fix 'err_clk' reference, so it is not encapsulated in > an ifdef making it available at all times. > V3: Keep ehci_usb_of_to_plat but add the ability to return > and unknown state instead of reading the register. > If the probe determines the states is unknown, it will > query the register after the clocks have been enabled. > Because of the slight behavior change, I removed any > review or tested tags. > > V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it > from the probe after t > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > index 1bd6147c76..060b02accc 100644 > --- a/drivers/usb/host/ehci-mx6.c > +++ b/drivers/usb/host/ehci-mx6.c > @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) > plat->init_type = USB_INIT_DEVICE; > else > plat->init_type = USB_INIT_HOST; > - } else if (is_mx7()) { > + } else if (is_mx7() || is_imx8mm() || is_imx8mn()) { > phy_status = (void __iomem *)(addr + > USBNC_PHY_STATUS_OFFSET); > val = readl(phy_status);
Is the USB GPCv2 power domain always ON at this point ? If it isn't, then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
You can check that easily by adding printf() into drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and verify that printf() prints something before this ehci_usb_phy_mode() code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
If you look at what this patch is doing, ehci_usb_of_to_plat is no longer making any calls to the USB registers. ehci_usb_of_to_plat is just an exercise in reading the device tree for the desired mode. If it's not host mode or peripheral mode it returns unknown. The part that reads the registers now is delayed until after the clocks are enabled inside the probe function which is called when USB is started and the power-domains are available.
Ah, I missed that part of the change.
One more thing, can you double-check that the "if (ctrl->init == USB_INIT_DEVICE)" checks in drivers/usb/host/ehci-hcd.c still work as they should ? If so, then this patch is fine.
Sure. If I run ums 0 mmc 2, I see the peripheral is enumerating on the U-Boot side:
U-Boot 2022.01-00678-ge83818c782-dirty (Feb 06 2022 - 18:55:39 -0600)
CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz Reset cause: POR Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit DRAM: 2 GiB Core: 99 devices, 21 uclasses, devicetree: separate WDT: Started watchdog@30280000 with servicing (60s timeout) MMC: FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2 Loading Environment from MMC... OK In: serial@30890000 Out: serial@30890000 Err: serial@30890000 Net: eth0: ethernet@30be0000 Hit any key to stop autoboot: 0 u-boot=> ums 0 mmc 2 UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000 imx8m_power_domain_on 0 imx8m_power_domain_on 2
And the power domains enable in the right order.
On the host side, I see:
usb 3-2: new high-speed USB device number 24 using xhci_hcd usb 3-2: New USB device found, idVendor=0525, idProduct=a4a5, bcdDevice= 2.21 usb 3-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 3-2: Product: USB download gadget usb 3-2: Manufacturer: U-Boot usb-storage 3-2:1.0: USB Mass Storage device detected usb-storage 3-2:1.0: Quirks match for vid 0525 pid a4a5: 10000 scsi host6: usb-storage 3-2:1.0 scsi 6:0:0:0: Direct-Access Linux UMS disk 0 ffff PQ: 0 ANSI: 2 sd 6:0:0:0: Attached scsi generic sg2 type 0 sd 6:0:0:0: [sdb] 61071360 512-byte logical blocks: (31.3 GB/29.1 GiB) sd 6:0:0:0: [sdb] Write Protect is off sd 6:0:0:0: [sdb] Mode Sense: 0f 00 00 00 sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sd 6:0:0:0: [sdb] Attached SCSI removable disk
Marek,
Is this sufficient, or do you need explicit printf's inside the driver to show the device is in peripheral mode?
adam
On 2/10/22 15:49, Adam Ford wrote:
On Mon, Feb 7, 2022 at 6:15 AM Adam Ford aford173@gmail.com wrote:
On Mon, Feb 7, 2022 at 5:50 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 12:13, Adam Ford wrote:
On Mon, Feb 7, 2022 at 2:47 AM Marek Vasut marex@denx.de wrote:
On 2/7/22 01:51, Adam Ford wrote:
On Sun, Feb 6, 2022 at 3:59 PM Marek Vasut marex@denx.de wrote: > > On 2/3/22 22:20, Adam Ford wrote: >> The imx8mm and imx8mn appear compatible with imx7d-usb >> flags in the OTG driver. If the dr_mode is defined as >> host or peripheral, the device appears to operate correctly, >> however the auto host/peripheral detection results in an error. >> >> The solution isn't just adding checks for imx8mm and imx8mn to >> the check for imx7, because the USB clock needs to be running >> to read from the USBNC_PHY_STATUS_OFFSET register or it will hang. >> >> Marek requested that I not enable the clocks in ehci_usb_of_to_plat, >> so I modified that function to return an unknown state if the >> device tree does not explicitly state whether it is a host >> or a peripheral. >> >> When the driver probes, it looks to see if it's in the unknown >> state, and only then will it read the register to auto-detect. >> >> Signed-off-by: Adam Ford aford173@gmail.com >> Tested-by: Tim Harvey tharvey@gateworks.com >> --- >> V4: Fix 'err_clk' reference, so it is not encapsulated in >> an ifdef making it available at all times. >> V3: Keep ehci_usb_of_to_plat but add the ability to return >> and unknown state instead of reading the register. >> If the probe determines the states is unknown, it will >> query the register after the clocks have been enabled. >> Because of the slight behavior change, I removed any >> review or tested tags. >> >> V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it >> from the probe after t >> >> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c >> index 1bd6147c76..060b02accc 100644 >> --- a/drivers/usb/host/ehci-mx6.c >> +++ b/drivers/usb/host/ehci-mx6.c >> @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) >> plat->init_type = USB_INIT_DEVICE; >> else >> plat->init_type = USB_INIT_HOST; >> - } else if (is_mx7()) { >> + } else if (is_mx7() || is_imx8mm() || is_imx8mn()) { >> phy_status = (void __iomem *)(addr + >> USBNC_PHY_STATUS_OFFSET); >> val = readl(phy_status); > > Is the USB GPCv2 power domain always ON at this point ? If it isn't, > then this access will lock up the whole SoC.
The GPCv2 needs to be enabled if USB is going to be used, but I would expect that to happen even without this patch. I ran into that during my early testing.
> > You can check that easily by adding printf() into > drivers/power/domain/imx8m-power-domain.c imx8m_power_domain_on() , and > verify that printf() prints something before this ehci_usb_phy_mode() > code is called.
I added a printf to show when a power domain is enabled. It does appear that power domains for HSIO, USBOTG1 and USBOTG2 are all enabled before the scan on the respective USB bus happens, so I think we're safe from hanging.
Isn't the ehci_usb_of_to_plat() called way before you even initiate the scan, somewhere during U-Boot startup ? So no, you might end up hanging the hardware ?
If you look at what this patch is doing, ehci_usb_of_to_plat is no longer making any calls to the USB registers. ehci_usb_of_to_plat is just an exercise in reading the device tree for the desired mode. If it's not host mode or peripheral mode it returns unknown. The part that reads the registers now is delayed until after the clocks are enabled inside the probe function which is called when USB is started and the power-domains are available.
Ah, I missed that part of the change.
One more thing, can you double-check that the "if (ctrl->init == USB_INIT_DEVICE)" checks in drivers/usb/host/ehci-hcd.c still work as they should ? If so, then this patch is fine.
Sure. If I run ums 0 mmc 2, I see the peripheral is enumerating on the U-Boot side:
U-Boot 2022.01-00678-ge83818c782-dirty (Feb 06 2022 - 18:55:39 -0600)
CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz Reset cause: POR Model: Beacon EmbeddedWorks i.MX8M Mini Development Kit DRAM: 2 GiB Core: 99 devices, 21 uclasses, devicetree: separate WDT: Started watchdog@30280000 with servicing (60s timeout) MMC: FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2 Loading Environment from MMC... OK In: serial@30890000 Out: serial@30890000 Err: serial@30890000 Net: eth0: ethernet@30be0000 Hit any key to stop autoboot: 0 u-boot=> ums 0 mmc 2 UMS: LUN 0, dev mmc 2, hwpart 0, sector 0x0, count 0x3a3e000 imx8m_power_domain_on 0 imx8m_power_domain_on 2
And the power domains enable in the right order.
On the host side, I see:
usb 3-2: new high-speed USB device number 24 using xhci_hcd usb 3-2: New USB device found, idVendor=0525, idProduct=a4a5, bcdDevice= 2.21 usb 3-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 3-2: Product: USB download gadget usb 3-2: Manufacturer: U-Boot usb-storage 3-2:1.0: USB Mass Storage device detected usb-storage 3-2:1.0: Quirks match for vid 0525 pid a4a5: 10000 scsi host6: usb-storage 3-2:1.0 scsi 6:0:0:0: Direct-Access Linux UMS disk 0 ffff PQ: 0 ANSI: 2 sd 6:0:0:0: Attached scsi generic sg2 type 0 sd 6:0:0:0: [sdb] 61071360 512-byte logical blocks: (31.3 GB/29.1 GiB) sd 6:0:0:0: [sdb] Write Protect is off sd 6:0:0:0: [sdb] Mode Sense: 0f 00 00 00 sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sd 6:0:0:0: [sdb] Attached SCSI removable disk
Marek,
Is this sufficient, or do you need explicit printf's inside the driver to show the device is in peripheral mode?
It's in the MR queue.
Hi Adam,
it's nice to include people who made review comments in the follow-up patches. I had to pull this out of the mailinglist again.
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
- } else if (is_mx7()) {
- } else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);
@@ -573,9 +573,8 @@ static int ehci_usb_of_to_plat(struct udevice *dev) case USB_DR_MODE_PERIPHERAL: plat->init_type = USB_INIT_DEVICE; break;
- case USB_DR_MODE_OTG:
- case USB_DR_MODE_UNKNOWN:
Don't you want to propagate the OTG mode to the init_type field? Maybe it will be useful sometime and IMHO makes it easier to read the code...
return ehci_usb_phy_mode(dev);
default:
plat->init_type = USB_INIT_UNKNOWN;};
return 0;
@@ -677,6 +676,20 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(1); #endif
- /*
* If the device tree didn't specify host or device,* the default is USB_INIT_UNKNOWN, so we need to check* the register. For imx8mm and imx8mn, the clocks need to be* running first, so we defer the check until they are.*/- if (priv->init_type == USB_INIT_UNKNOWN) {
Because this implicitly also contains OTG mode, right? So despite the comment, if the device tree includes OTG mode, this branch is also taken.
Otherwise looks good to me:
Reviewed-by: Michael Walle michael@walle.cc
ret = ehci_usb_phy_mode(dev);if (ret)goto err_clk;elsepriv->init_type = plat->init_type;- }
#if CONFIG_IS_ENABLED(DM_REGULATOR) ret = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); @@ -741,8 +754,8 @@ err_regulator: #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) regulator_set_enable(priv->vbus_supply, false); -err_clk: #endif +err_clk: #if CONFIG_IS_ENABLED(CLK) clk_disable(&priv->clk); #else diff --git a/include/usb.h b/include/usb.h index f032de8af9..7e3796bd5b 100644 --- a/include/usb.h +++ b/include/usb.h @@ -163,7 +163,8 @@ struct int_queue; */ enum usb_init_type { USB_INIT_HOST,
- USB_INIT_DEVICE
- USB_INIT_DEVICE,
- USB_INIT_UNKNOWN,
};
/**********************************************************************
On Mon, Feb 7, 2022 at 5:00 AM Michael Walle michael@walle.cc wrote:
Hi Adam,
it's nice to include people who made review comments in the follow-up patches. I had to pull this out of the mailinglist again.
Sorry. I didn't purposefully leave you out. I have a little script I run to get the list of people involved, and I forgot to manually add you back in.
The imx8mm and imx8mn appear compatible with imx7d-usb flags in the OTG driver. If the dr_mode is defined as host or peripheral, the device appears to operate correctly, however the auto host/peripheral detection results in an error.
The solution isn't just adding checks for imx8mm and imx8mn to the check for imx7, because the USB clock needs to be running to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
Marek requested that I not enable the clocks in ehci_usb_of_to_plat, so I modified that function to return an unknown state if the device tree does not explicitly state whether it is a host or a peripheral.
When the driver probes, it looks to see if it's in the unknown state, and only then will it read the register to auto-detect.
Signed-off-by: Adam Ford aford173@gmail.com Tested-by: Tim Harvey tharvey@gateworks.com
V4: Fix 'err_clk' reference, so it is not encapsulated in an ifdef making it available at all times. V3: Keep ehci_usb_of_to_plat but add the ability to return and unknown state instead of reading the register. If the probe determines the states is unknown, it will query the register after the clocks have been enabled. Because of the slight behavior change, I removed any review or tested tags.
V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it from the probe after t
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 1bd6147c76..060b02accc 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) plat->init_type = USB_INIT_DEVICE; else plat->init_type = USB_INIT_HOST;
} else if (is_mx7()) {
} else if (is_mx7() || is_imx8mm() || is_imx8mn()) { phy_status = (void __iomem *)(addr + USBNC_PHY_STATUS_OFFSET); val = readl(phy_status);@@ -573,9 +573,8 @@ static int ehci_usb_of_to_plat(struct udevice *dev) case USB_DR_MODE_PERIPHERAL: plat->init_type = USB_INIT_DEVICE; break;
case USB_DR_MODE_OTG:case USB_DR_MODE_UNKNOWN:Don't you want to propagate the OTG mode to the init_type field? Maybe it will be useful sometime and IMHO makes it easier to read the code...
I was thinking the OTG case was included with the default, since this function is really only looking for whether or not we're host or peripheral. Every other case is considered unknown since we have to defer the reading of the USB state until later.
adam
return ehci_usb_phy_mode(dev);
default:plat->init_type = USB_INIT_UNKNOWN; }; return 0;@@ -677,6 +676,20 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(1); #endif
/** If the device tree didn't specify host or device,* the default is USB_INIT_UNKNOWN, so we need to check* the register. For imx8mm and imx8mn, the clocks need to be* running first, so we defer the check until they are.*/if (priv->init_type == USB_INIT_UNKNOWN) {Because this implicitly also contains OTG mode, right? So despite the comment, if the device tree includes OTG mode, this branch is also taken.
Yes, because the device tree didn't specify host or peripheral, all other instances are assumed to be OTG and deferred until we get here.
Otherwise looks good to me:
Reviewed-by: Michael Walle michael@walle.cc
ret = ehci_usb_phy_mode(dev);if (ret)goto err_clk;elsepriv->init_type = plat->init_type;}#if CONFIG_IS_ENABLED(DM_REGULATOR) ret = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); @@ -741,8 +754,8 @@ err_regulator: #if CONFIG_IS_ENABLED(DM_REGULATOR) if (priv->vbus_supply) regulator_set_enable(priv->vbus_supply, false); -err_clk: #endif +err_clk: #if CONFIG_IS_ENABLED(CLK) clk_disable(&priv->clk); #else diff --git a/include/usb.h b/include/usb.h index f032de8af9..7e3796bd5b 100644 --- a/include/usb.h +++ b/include/usb.h @@ -163,7 +163,8 @@ struct int_queue; */ enum usb_init_type { USB_INIT_HOST,
USB_INIT_DEVICE
USB_INIT_DEVICE,USB_INIT_UNKNOWN,};
/**********************************************************************
participants (4)
-
Adam Ford -
Fabio Estevam -
Marek Vasut -
Michael Walle