* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 0:21 [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn Adam Ford
@ 2022-01-18 1:32 ` Tim Harvey
2022-01-18 1:45 ` Fabio Estevam
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Tim Harvey @ 2022-01-18 1:32 UTC (permalink / raw)
To: Adam Ford; +Cc: u-boot, Marek Vasut, Fabio Estevam, Adam Ford-BE
On Mon, Jan 17, 2022 at 4:21 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>
> ---
> 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 the clocks are enabled, but before
> the data is needed.
>
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 1bd6147c76..cf44e53ff7 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);
> diff --git a/include/usb.h b/include/usb.h
> index b3851fdb4f..47d738a786 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,
> };
>
> /**********************************************************************
> --
> 2.32.0
>
Adam,
For v3:
tested on imx8mm_venice_defconfig as USB host with USB_ETHER_ASIX and
as gadget with CMD_USB_MASS_STORAGE (ums 0 mmc 2).
Tested-By: Tim Harvey <tharvey@gateworks.com>
Best Regards,
Tim
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 0:21 [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn Adam Ford
2022-01-18 1:32 ` Tim Harvey
@ 2022-01-18 1:45 ` Fabio Estevam
2022-01-18 1:58 ` Adam Ford
2022-01-25 8:43 ` Marcel Ziswiler
2022-01-25 8:44 ` Marcel Ziswiler
3 siblings, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2022-01-18 1:45 UTC (permalink / raw)
To: Adam Ford
Cc: U-Boot-Denx, Marek Vasut, Tim Harvey, Fabio Estevam, Adam Ford-BE
Hi Adam,
On Mon, Jan 17, 2022 at 9:21 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>
> ---
> 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.
Unfortunately, v3 breaks 'ums 0 mmc 0' on the imx7s-warp board.
The eMMC is no longer mounted and the board hangs.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 1:45 ` Fabio Estevam
@ 2022-01-18 1:58 ` Adam Ford
2022-01-18 21:48 ` Fabio Estevam
0 siblings, 1 reply; 9+ messages in thread
From: Adam Ford @ 2022-01-18 1:58 UTC (permalink / raw)
To: Fabio Estevam
Cc: U-Boot-Denx, Marek Vasut, Tim Harvey, Fabio Estevam, Adam Ford-BE
On Mon, Jan 17, 2022 at 7:46 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Adam,
>
> On Mon, Jan 17, 2022 at 9:21 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>
> > ---
> > 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.
>
> Unfortunately, v3 breaks 'ums 0 mmc 0' on the imx7s-warp board.
Thanks for testing it.
I am not really sure what's significantly different between them. Do
you get any errors when you run UMS?
>
> The eMMC is no longer mounted and the board hangs.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 1:58 ` Adam Ford
@ 2022-01-18 21:48 ` Fabio Estevam
0 siblings, 0 replies; 9+ messages in thread
From: Fabio Estevam @ 2022-01-18 21:48 UTC (permalink / raw)
To: Adam Ford
Cc: U-Boot-Denx, Marek Vasut, Tim Harvey, Fabio Estevam, Adam Ford-BE
Hi Adam,
On Mon, Jan 17, 2022 at 10:58 PM Adam Ford <aford173@gmail.com> wrote:
> Thanks for testing it.
>
> I am not really sure what's significantly different between them. Do
> you get any errors when you run UMS?
Just realized the problem I am seeing is unrelated to your patch. I
will need to debug it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 0:21 [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn Adam Ford
2022-01-18 1:32 ` Tim Harvey
2022-01-18 1:45 ` Fabio Estevam
@ 2022-01-25 8:43 ` Marcel Ziswiler
2022-01-25 8:44 ` Marcel Ziswiler
3 siblings, 0 replies; 9+ messages in thread
From: Marcel Ziswiler @ 2022-01-25 8:43 UTC (permalink / raw)
To: u-boot
On Mon, 2022-01-17 at 18:21 -0600, 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>
> ---
> 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 the clocks are enabled, but before
> the data is needed.
>
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 1bd6147c76..cf44e53ff7 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;
Hm, unfortunately, that label is gated by an ifdef leading to the following on colibri_imx7:
drivers/usb/host/ehci-mx6.c: In function ‘ehci_usb_probe’:
drivers/usb/host/ehci-mx6.c:688:4: error: label ‘err_clk’ used but not defined
688 | goto err_clk;
| ^~~~
make[1]: *** [scripts/Makefile.build:254: drivers/usb/host/ehci-mx6.o] Error 1
I have to admit that maybe we should indeed run it with DM_REGULATOR but that won't change anything on this
issue. I guess one should really not have any gotos to a label err_clk being ifdefed from somewhere not
ifdefed.
> + else
> + priv->init_type = plat->init_type;
> + }
> +
> #if CONFIG_IS_ENABLED(DM_REGULATOR)
> ret = device_get_supply_regulator(dev, "vbus-supply",
> &priv->vbus_supply);
> diff --git a/include/usb.h b/include/usb.h
> index b3851fdb4f..47d738a786 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,
> };
>
> /**********************************************************************
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-18 0:21 [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn Adam Ford
` (2 preceding siblings ...)
2022-01-25 8:43 ` Marcel Ziswiler
@ 2022-01-25 8:44 ` Marcel Ziswiler
2022-02-03 20:58 ` Tim Harvey
3 siblings, 1 reply; 9+ messages in thread
From: Marcel Ziswiler @ 2022-01-25 8:44 UTC (permalink / raw)
To: aford173@gmail.com, u-boot@lists.denx.de
Cc: marex@denx.de, festevam@denx.de, tharvey@gateworks.com,
aford@beaconembedded.com
On Mon, 2022-01-17 at 18:21 -0600, 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>
> ---
> 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 the clocks are enabled, but before
> the data is needed.
>
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 1bd6147c76..cf44e53ff7 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;
Hm, unfortunately, that label is gated by an ifdef leading to the following on colibri_imx7:
drivers/usb/host/ehci-mx6.c: In function ‘ehci_usb_probe’:
drivers/usb/host/ehci-mx6.c:688:4: error: label ‘err_clk’ used but not defined
688 | goto err_clk;
| ^~~~
make[1]: *** [scripts/Makefile.build:254: drivers/usb/host/ehci-mx6.o] Error 1
I have to admit that maybe we should indeed run it with DM_REGULATOR but that won't change anything on this
issue. I guess one should really not have any gotos to a label err_clk being ifdefed from somewhere not
ifdefed.
> + else
> + priv->init_type = plat->init_type;
> + }
> +
> #if CONFIG_IS_ENABLED(DM_REGULATOR)
> ret = device_get_supply_regulator(dev, "vbus-supply",
> &priv->vbus_supply);
> diff --git a/include/usb.h b/include/usb.h
> index b3851fdb4f..47d738a786 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,
> };
>
> /**********************************************************************
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-01-25 8:44 ` Marcel Ziswiler
@ 2022-02-03 20:58 ` Tim Harvey
2022-02-03 21:13 ` Adam Ford
0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2022-02-03 20:58 UTC (permalink / raw)
To: aford173@gmail.com
Cc: u-boot@lists.denx.de, marex@denx.de, festevam@denx.de,
aford@beaconembedded.com, Marcel Ziswiler, Michael Walle
On Tue, Jan 25, 2022 at 12:44 AM Marcel Ziswiler
<marcel.ziswiler@toradex.com> wrote:
>
> On Mon, 2022-01-17 at 18:21 -0600, 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>
> > ---
> > 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 the clocks are enabled, but before
> > the data is needed.
> >
> > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > index 1bd6147c76..cf44e53ff7 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;
>
> Hm, unfortunately, that label is gated by an ifdef leading to the following on colibri_imx7:
>
> drivers/usb/host/ehci-mx6.c: In function ‘ehci_usb_probe’:
> drivers/usb/host/ehci-mx6.c:688:4: error: label ‘err_clk’ used but not defined
> 688 | goto err_clk;
> | ^~~~
> make[1]: *** [scripts/Makefile.build:254: drivers/usb/host/ehci-mx6.o] Error 1
>
> I have to admit that maybe we should indeed run it with DM_REGULATOR but that won't change anything on this
> issue. I guess one should really not have any gotos to a label err_clk being ifdefed from somewhere not
> ifdefed.
>
Adam,
Can you re-submit this with an ifdef to handle the correct goto when
DM_REGULATOR is undefined?
It looks like Fabio figured out his issue with his board and it was
unrelated to this (fixed by 'mmc: fsl_esdhc_imx: fix watermark level
in dma') so I think Marcel's point is the only blocker on this patch
unless there is more discussion needed with Michael or some feedback
needed from Marek?
Best regards,
Tim
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] usb: ehci-mx6: Enable OTG detection on imx8mm and imx8mn
2022-02-03 20:58 ` Tim Harvey
@ 2022-02-03 21:13 ` Adam Ford
0 siblings, 0 replies; 9+ messages in thread
From: Adam Ford @ 2022-02-03 21:13 UTC (permalink / raw)
To: Tim Harvey
Cc: u-boot@lists.denx.de, marex@denx.de, festevam@denx.de,
aford@beaconembedded.com, Marcel Ziswiler, Michael Walle
On Thu, Feb 3, 2022 at 2:59 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Tue, Jan 25, 2022 at 12:44 AM Marcel Ziswiler
> <marcel.ziswiler@toradex.com> wrote:
> >
> > On Mon, 2022-01-17 at 18:21 -0600, 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>
> > > ---
> > > 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 the clocks are enabled, but before
> > > the data is needed.
> > >
> > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> > > index 1bd6147c76..cf44e53ff7 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;
> >
> > Hm, unfortunately, that label is gated by an ifdef leading to the following on colibri_imx7:
> >
> > drivers/usb/host/ehci-mx6.c: In function ‘ehci_usb_probe’:
> > drivers/usb/host/ehci-mx6.c:688:4: error: label ‘err_clk’ used but not defined
> > 688 | goto err_clk;
> > | ^~~~
> > make[1]: *** [scripts/Makefile.build:254: drivers/usb/host/ehci-mx6.o] Error 1
> >
> > I have to admit that maybe we should indeed run it with DM_REGULATOR but that won't change anything on this
> > issue. I guess one should really not have any gotos to a label err_clk being ifdefed from somewhere not
> > ifdefed.
> >
>
> Adam,
>
> Can you re-submit this with an ifdef to handle the correct goto when
> DM_REGULATOR is undefined?
>
> It looks like Fabio figured out his issue with his board and it was
> unrelated to this (fixed by 'mmc: fsl_esdhc_imx: fix watermark level
> in dma') so I think Marcel's point is the only blocker on this patch
> unless there is more discussion needed with Michael or some feedback
> needed from Marek?
Tim,
Not a problem. I was waiting for feedback from Marek in case there
were more issues, but I can fix the goto reference and resubmit. I'm
build-testing the colibri_imx7 now.
I'm just moving the 'err_clk' reference out of the #ifdef so the same
reference can be used. It doesn't look like it should be inside the
#ifdef anyway.
You should see something shortly, and I'll add you to the CC list.
adam
>
> Best regards,
>
> Tim
^ permalink raw reply [flat|nested] 9+ messages in thread