diff mbox series

[v1,3/3] colibri-imx8x: configure usb hub to bypass mode

Message ID 20231220103811.228515-4-andrejs.cainikovs@gmail.com
State Accepted
Commit 8a72d193cf15cf0d6936886db4b2dd75956fb4d8
Delegated to: Fabio Estevam
Headers show
Series colibri-imx8x: configure module usb hub to bypass mode | expand

Commit Message

Andrejs Cainikovs Dec. 20, 2023, 10:38 a.m. UTC 
From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>

This change configures Toradex Colibri iMX8X SoM USB
hub to bypass mode, and brings out of the reset state.

Signed-off-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>
---
 arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi | 15 +++++++++++++++
 arch/arm/dts/fsl-imx8qxp-colibri.dts         |  2 --
 configs/colibri-imx8x_defconfig              |  1 +
 3 files changed, 16 insertions(+), 2 deletions(-)

Comments

Fabio Estevam Dec. 20, 2023, 10:48 a.m. UTC | #1 
Hi Andrejs,

On Wed, Dec 20, 2023 at 7:39 AM Andrejs Cainikovs
<andrejs.cainikovs@gmail.com> wrote:

> diff --git a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> index a6af4e5e2b7..6ab6b1f9ee6 100644
> --- a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> +++ b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> @@ -84,6 +84,21 @@
>         bootph-some-ram;
>  };
>
> +&gpio_expander_43 {
> +       usb-bypass-n-hog {
> +               gpio-hog;
> +               gpios = <5 GPIO_ACTIVE_LOW>;
> +               line-name = "usb-bypass-n";
> +               output-high;
> +       };
> +       usb-reset-n-hog {
> +               gpio-hog;
> +               gpios = <4 GPIO_ACTIVE_LOW>;
> +               line-name = "usb-reset-n";
> +               output-low;
> +       };
> +};

Patch looks good from a U-Boot perspective, but just want to
understand why the imx8qxp/imx8qm boards
in U-Boot use devicetrees based on downstream NXP instead of the
mainline kernel dts?

Shouldn't this 'hub out of reset' be added in mainline Linux first and
then just sync the dts in U-Boot?
Francesco Dolcini Dec. 20, 2023, 11:30 a.m. UTC | #2 
Hello Fabio,

On Wed, Dec 20, 2023 at 07:48:50AM -0300, Fabio Estevam wrote:
> On Wed, Dec 20, 2023 at 7:39 AM Andrejs Cainikovs
> <andrejs.cainikovs@gmail.com> wrote:
> 
> > diff --git a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> > index a6af4e5e2b7..6ab6b1f9ee6 100644
> > --- a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> > +++ b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
> > @@ -84,6 +84,21 @@
> >         bootph-some-ram;
> >  };
> >
> > +&gpio_expander_43 {
> > +       usb-bypass-n-hog {
> > +               gpio-hog;
> > +               gpios = <5 GPIO_ACTIVE_LOW>;
> > +               line-name = "usb-bypass-n";
> > +               output-high;
> > +       };
> > +       usb-reset-n-hog {
> > +               gpio-hog;
> > +               gpios = <4 GPIO_ACTIVE_LOW>;
> > +               line-name = "usb-reset-n";
> > +               output-low;
> > +       };
> > +};
> 
> Patch looks good from a U-Boot perspective, but just want to
> understand why the imx8qxp/imx8qm boards
> in U-Boot use devicetrees based on downstream NXP instead of the
> mainline kernel dts?
> 
> Shouldn't this 'hub out of reset' be added in mainline Linux first and
> then just sync the dts in U-Boot?

Valid point.

In general re-using the DTS for the kernel would not work.

The bypass and reset signals are part of the USB HUB node [1], and we
would need to add such a driver to U-Boot to be able to properly
implement it (despite the recent plans to make SPL a full blown
bootloader and U-Boot proper the operating system and kick out Linux I
would not go into that direction ;-)

Given that I believe that overriding the Linux DTS to use these 2
signals are simple GPIO HOG for U-Boot is the correct approach.

Francesco

[1] Linux: Documentation/devicetree/bindings/usb/smsc,usb3503.yaml
Fabio Estevam Dec. 20, 2023, 11:39 a.m. UTC | #3 
Hi Francesco,

On Wed, Dec 20, 2023 at 8:30 AM Francesco Dolcini <francesco@dolcini.it> wrote:

> Valid point.
>
> In general re-using the DTS for the kernel would not work.
>
> The bypass and reset signals are part of the USB HUB node [1], and we
> would need to add such a driver to U-Boot to be able to properly
> implement it (despite the recent plans to make SPL a full blown
> bootloader and U-Boot proper the operating system and kick out Linux I
> would not go into that direction ;-)
>
> Given that I believe that overriding the Linux DTS to use these 2
> signals are simple GPIO HOG for U-Boot is the correct approach.

Yes, this series can go as-is.

My main concern is why U-Boot still uses the NXP-based
fsl-imx8qxp-mek.dts, fsl-imx8qxp-colibri.dts
variant instead of the mainline versions.

The other i.MX SoCs in U-Boot are better synced with the Linux kernel DTs.

I don't have access to i.MX8QXP/i.MX8QM boards to help on this, but
for better long-term support it would be nice if someone could sync
the U-Boot i.MX8QXP/i.MX8QM DTs with Linux.
Francesco Dolcini Dec. 20, 2023, 11:44 a.m. UTC | #4 
On Wed, Dec 20, 2023 at 08:39:38AM -0300, Fabio Estevam wrote:
> Hi Francesco,
> 
> On Wed, Dec 20, 2023 at 8:30 AM Francesco Dolcini <francesco@dolcini.it> wrote:
> 
> > Valid point.
> >
> > In general re-using the DTS for the kernel would not work.
> >
> > The bypass and reset signals are part of the USB HUB node [1], and we
> > would need to add such a driver to U-Boot to be able to properly
> > implement it (despite the recent plans to make SPL a full blown
> > bootloader and U-Boot proper the operating system and kick out Linux I
> > would not go into that direction ;-)
> >
> > Given that I believe that overriding the Linux DTS to use these 2
> > signals are simple GPIO HOG for U-Boot is the correct approach.
> 
> Yes, this series can go as-is.
> 
> My main concern is why U-Boot still uses the NXP-based
> fsl-imx8qxp-mek.dts, fsl-imx8qxp-colibri.dts
> variant instead of the mainline versions.
> 
> The other i.MX SoCs in U-Boot are better synced with the Linux kernel DTs.
> 
> I don't have access to i.MX8QXP/i.MX8QM boards to help on this, but
> for better long-term support it would be nice if someone could sync
> the U-Boot i.MX8QXP/i.MX8QM DTs with Linux.

Understood. Yes, this is some real work that someone should take up.
Just doing the sync will likely break everything now.

At the moment the situation with i.MX8QXP/i.MX8QM is not as good as
i.MX8M* neither in U-Boot nor in Linux :-/

Francesco
Fabio Estevam Dec. 20, 2023, 11:49 a.m. UTC | #5 
On Wed, Dec 20, 2023 at 8:45 AM Francesco Dolcini <francesco@dolcini.it> wrote:

> Understood. Yes, this is some real work that someone should take up.
> Just doing the sync will likely break everything now.

Exactly.

> At the moment the situation with i.MX8QXP/i.MX8QM is not as good as
> i.MX8M* neither in U-Boot nor in Linux :-/

The NXP folks are on Cc here, so hopefully they could help improving this.

Thanks
diff mbox series

Patch

diff --git a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
index a6af4e5e2b7..6ab6b1f9ee6 100644
--- a/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
+++ b/arch/arm/dts/fsl-imx8qxp-colibri-u-boot.dtsi
@@ -84,6 +84,21 @@ 
 	bootph-some-ram;
 };
 
+&gpio_expander_43 {
+	usb-bypass-n-hog {
+		gpio-hog;
+		gpios = <5 GPIO_ACTIVE_LOW>;
+		line-name = "usb-bypass-n";
+		output-high;
+	};
+	usb-reset-n-hog {
+		gpio-hog;
+		gpios = <4 GPIO_ACTIVE_LOW>;
+		line-name = "usb-reset-n";
+		output-low;
+	};
+};
+
 &gpio0 {
 	bootph-some-ram;
 };
diff --git a/arch/arm/dts/fsl-imx8qxp-colibri.dts b/arch/arm/dts/fsl-imx8qxp-colibri.dts
index 295090ad711..b479921aff9 100644
--- a/arch/arm/dts/fsl-imx8qxp-colibri.dts
+++ b/arch/arm/dts/fsl-imx8qxp-colibri.dts
@@ -319,8 +319,6 @@ 
 		gpio-controller;
 		#gpio-cells = <2>;
 		reg = <0x43>;
-		initial_io_dir = <0xff>;
-		initial_output = <0x05>;
 	};
 };
 
diff --git a/configs/colibri-imx8x_defconfig b/configs/colibri-imx8x_defconfig
index 13c16bde3d8..ce60c509582 100644
--- a/configs/colibri-imx8x_defconfig
+++ b/configs/colibri-imx8x_defconfig
@@ -65,6 +65,7 @@  CONFIG_BOOTCOUNT_LIMIT=y
 CONFIG_BOOTCOUNT_ENV=y
 CONFIG_CLK_IMX8=y
 CONFIG_CPU=y
+CONFIG_GPIO_HOG=y
 CONFIG_FXL6408_GPIO=y
 CONFIG_MXC_GPIO=y
 CONFIG_DM_I2C=y