Re: Re: [PATCH v2 1/1] arm64: dts: tqma8mpql: add USB DR support

From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Tim Harvey <tharvey@gateworks.com>, Jun Li <jun.li@nxp.com>
Subject: Re: Re: [PATCH v2 1/1] arm64: dts: tqma8mpql: add USB DR support
Date: Mon, 05 Sep 2022 08:28:29 +0200	[thread overview]
Message-ID: <5604780.DvuYhMxLoT@steina-w> (raw)
In-Reply-To: <20220905022228.GB1728671@dragon>

Hello Shawn,

thanks for your feedback.

Am Montag, 5. September 2022, 04:22:28 CEST schrieb Shawn Guo:
> On Wed, Aug 31, 2022 at 09:46:06AM +0200, Alexander Stein wrote:
> > Add support for USB DR on USB1 interface. Host/Device detection is done
> > using the usb-role-switch connector.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Split from previous series
> > * For now enable USB OTG only
> > 
> >  .../freescale/imx8mp-tqma8mpql-mba8mpxl.dts   | 40 +++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > b/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts index
> > d8ca52976170..b30d75b1fa47 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > @@ -459,6 +459,37 @@ &usdhc2 {
> > 
> >  	status = "okay";
> >  
> >  };
> > 
> > +&usb3_phy0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_usb0>;
> 
> Should the pinctrl be in USB controller instead of PHY node?

That seems reasonable. I'll move this to the USB glue layer node, where also 
the OC and PWR polarity can be changed.

> > +	status = "okay";
> > +};
> > +
> > +&usb3_0 {
> > +	fsl,over-current-active-low;
> > +	status = "okay";
> > +};
> > +
> > +&usb_dwc3_0 {
> 
> These nodes are not in alphabetic order.

Sure, this looks wrong, I'll fix that.

> > +	/* dual role is implemented, but not a full featured OTG */
> > +	hnp-disable;
> > +	srp-disable;
> > +	adp-disable;
> > +	dr_mode = "otg";
> > +	usb-role-switch;
> > +	role-switch-default-mode = "peripheral";
> > +	status = "okay";
> > +
> > +	connector {
> > +		compatible = "gpio-usb-b-connector", "usb-b-connector";
> > +		type = "micro";
> > +		label = "X29";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_usbcon0>;
> > +		id-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;
> > +	};
> > +};
> > +
> > 
> >  &iomuxc {
> >  
> >  	pinctrl_backlight: backlightgrp {
> >  	
> >  		fsl,pins = <MX8MP_IOMUXC_SAI5_RXFS__GPIO3_IO19		
0x14>;
> > 
> > @@ -666,6 +697,15 @@ pinctrl_uart4: uart4grp {
> > 
> >  			   <MX8MP_IOMUXC_UART4_TXD__UART4_DCE_TX	
0x140>;
> >  	
> >  	};
> > 
> > +	pinctrl_usbcon0: usb0congrp {
> > +		fsl,pins = <MX8MP_IOMUXC_GPIO1_IO10__GPIO1_IO10		
0x1c0>;
> > +	};
> > +
> > +	pinctrl_usb0: usb0grp {
> 
> pinctrl_usb0 should go before pinctrl_usbcon0?

Yes, I'll fix that.

Thanks and best regards,
Alexander

> Shawn
> 
> > +		fsl,pins = <MX8MP_IOMUXC_GPIO1_IO13__USB1_OTG_OC	
0x1c0>,
> > +			   <MX8MP_IOMUXC_GPIO1_IO12__USB1_OTG_PWR	
0x1c0>;
> > +	};
> > +
> > 
> >  	pinctrl_usdhc2: usdhc2grp {
> >  	
> >  		fsl,pins = <MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK		
0x192>,
> >  		
> >  			   <MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD		
0x1d2>,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel