lpc32xx and stotg04

lpc32xx and stotg04

[Trevor Woerner]

Hi Alexandre and James,

I too am working with a board that uses the lpc32xx SoC (the lpc3240, to be
specific) and has a stotg04 for the USB transceiver instead of the isp1301.

I can't get the USB to work.

My guess is that I don't have the device tree correct.

I could embarrass myself by showing you what combinations I've tried but I
thought maybe I'd ask and see if either of you could provide a DT snippet
describing how to hook up the stotg04 to the i2cusb. Admittedly I'm quite
fuzzy when it comes to device trees.

I'm also a bit fuzzy on USB. I want to plug usb sticks into my device (which,
by my understanding, is the opposite of OTG). So additionally I want to enable
ohci and not usbd?

In one DT incantation (the one showing the most promise so far) on startup
'lsusb' shows two usb devices. The moment I plug a USB drive into my device I
get:

	[  433.268009] usb-ohci 31020000.ohci: controller won't resume
	[  433.273603] usb-ohci 31020000.ohci: HC died; cleaning up
	[  433.280566] usb 1-1: USB disconnect, device number 2

And afterwards only one device is listed by 'lsusb'.

Currently I'm using a 5.0 kernel and a 5.4 kernel, but I could use any kernel
(upstream, ideally). In either case, it doesn't seem possible to deselect the
isp1301 from the kernel config? It gets selected automatically. If I'm using
the stotg04 instead of the isp1301, do I need a way to turn off the isp1301?

Best regards,
	Trevor

Re: lpc32xx and stotg04

[Alexandre Belloni]

Hi,

On 23/07/2020 17:27:43-0400, Trevor Woerner wrote:
> I too am working with a board that uses the lpc32xx SoC (the lpc3240, to be
> specific) and has a stotg04 for the USB transceiver instead of the isp1301.
> 
> I can't get the USB to work.
> 
> My guess is that I don't have the device tree correct.
> 
> I could embarrass myself by showing you what combinations I've tried but I
> thought maybe I'd ask and see if either of you could provide a DT snippet
> describing how to hook up the stotg04 to the i2cusb. Admittedly I'm quite
> fuzzy when it comes to device trees.
> 

This is what I had that is relevant:

&i2cusb {
	clock-frequency = <100000>;

	isp1301: usb-transceiver@2d {
		compatible = "nxp,isp1301";
		reg = <0x2d>;
	};
};

/* Here, choose exactly one from: ohci, usbd */
&usbd {
// &ohci {
	transceiver = <&isp1301>;
	status = "okay";
};

> I'm also a bit fuzzy on USB. I want to plug usb sticks into my device (which,
> by my understanding, is the opposite of OTG). So additionally I want to enable
> ohci and not usbd?
> 

Indeed, you want ohci (as you can see this is commented out in my device
tree)

> In one DT incantation (the one showing the most promise so far) on startup
> 'lsusb' shows two usb devices. The moment I plug a USB drive into my device I
> get:
> 
> 	[  433.268009] usb-ohci 31020000.ohci: controller won't resume
> 	[  433.273603] usb-ohci 31020000.ohci: HC died; cleaning up
> 	[  433.280566] usb 1-1: USB disconnect, device number 2
> 
> And afterwards only one device is listed by 'lsusb'.
> 
> Currently I'm using a 5.0 kernel and a 5.4 kernel, but I could use any kernel
> (upstream, ideally). In either case, it doesn't seem possible to deselect the
> isp1301 from the kernel config? It gets selected automatically. If I'm using
> the stotg04 instead of the isp1301, do I need a way to turn off the isp1301?
> 

The platform I was working on is on v5.1 and has an stotg04. Honnestly,
the whole thing is a mess and you have to use the isp1301 driver. Then
the difference between isp1301 and stotg04 is handled in lpc32xx_udc.c.

So I would say you are missing something similar to what I did in
2a60f5eafa74 ("usb: gadget: udc: lpc32xx: add support for stotg04 phy")
but in drivers/usb/host/ohci-nxp.c:isp1301_configure_lpc32xx()

I guess your issue is MC2_SPD_SUSP_CTRL line 75 ;) You can try to
remove it.

Regards,

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH] usb: ohci-nxp: add support for stotg04 phy

[Alexandre Belloni]

The STOTG04 phy is used as a drop-in replacement of the ISP1301 but some
bits doesn't have exactly the same meaning and this can lead to issues.
Detect the phy dynamically and avoid writing to reserved bits.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---

Hi Trevor, this is totally untested but at least it builds ;)

 drivers/usb/host/ohci-nxp.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
index 85878e8ad331..36ab1501c28f 100644
--- a/drivers/usb/host/ohci-nxp.c
+++ b/drivers/usb/host/ohci-nxp.c
@@ -55,6 +55,15 @@ static struct clk *usb_host_clk;
 
 static void isp1301_configure_lpc32xx(void)
 {
+	u8 value, atx_is_stotg = 0;
+	s32 vendor, product;
+
+	vendor = i2c_smbus_read_word_data(isp1301_i2c_client, 0x00);
+	product = i2c_smbus_read_word_data(isp1301_i2c_client, 0x02);
+
+	if (vendor == 0x0483 && product == 0xa0c4)
+		atx_is_stotg = 1;
+
 	/* LPC32XX only supports DAT_SE0 USB mode */
 	/* This sequence is important */
 
@@ -70,9 +79,11 @@ static void isp1301_configure_lpc32xx(void)
 	i2c_smbus_write_byte_data(isp1301_i2c_client,
 		  (ISP1301_I2C_MODE_CONTROL_2 | ISP1301_I2C_REG_CLEAR_ADDR),
 		  ~0);
+	value = MC2_BI_DI | MC2_PSW_EN;
+	if (!atx_is_stotg)
+		value |= MC2_SPD_SUSP_CTRL;
 	i2c_smbus_write_byte_data(isp1301_i2c_client,
-		ISP1301_I2C_MODE_CONTROL_2,
-		(MC2_BI_DI | MC2_PSW_EN | MC2_SPD_SUSP_CTRL));
+		ISP1301_I2C_MODE_CONTROL_2, value);
 	i2c_smbus_write_byte_data(isp1301_i2c_client,
 		(ISP1301_I2C_OTG_CONTROL_1 | ISP1301_I2C_REG_CLEAR_ADDR), ~0);
 	i2c_smbus_write_byte_data(isp1301_i2c_client,
@@ -91,10 +102,8 @@ static void isp1301_configure_lpc32xx(void)
 	i2c_smbus_write_byte_data(isp1301_i2c_client,
 		ISP1301_I2C_INTERRUPT_RISING | ISP1301_I2C_REG_CLEAR_ADDR, ~0);
 
-	printk(KERN_INFO "ISP1301 Vendor ID  : 0x%04x\n",
-	      i2c_smbus_read_word_data(isp1301_i2c_client, 0x00));
-	printk(KERN_INFO "ISP1301 Product ID : 0x%04x\n",
-	      i2c_smbus_read_word_data(isp1301_i2c_client, 0x02));
+	printk(KERN_INFO "ISP1301 Vendor ID  : 0x%04x\n", vendor);
+	printk(KERN_INFO "ISP1301 Product ID : 0x%04x\n", product);
 	printk(KERN_INFO "ISP1301 Version ID : 0x%04x\n",
 	      i2c_smbus_read_word_data(isp1301_i2c_client, 0x14));
 }
-- 
2.26.2

Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy

[Sergei Shtylyov]

Hello!

On 7/29/20 8:49 PM, Alexandre Belloni wrote:

> The STOTG04 phy is used as a drop-in replacement of the ISP1301 but some
> bits doesn't have exactly the same meaning and this can lead to issues.
> Detect the phy dynamically and avoid writing to reserved bits.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> 
> Hi Trevor, this is totally untested but at least it builds ;)
> 
>  drivers/usb/host/ohci-nxp.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
> index 85878e8ad331..36ab1501c28f 100644
> --- a/drivers/usb/host/ohci-nxp.c
> +++ b/drivers/usb/host/ohci-nxp.c
> @@ -55,6 +55,15 @@ static struct clk *usb_host_clk;
>  
>  static void isp1301_configure_lpc32xx(void)
>  {
> +	u8 value, atx_is_stotg = 0;

   Why the flag is not *bool*?

> +	s32 vendor, product;
> +
> +	vendor = i2c_smbus_read_word_data(isp1301_i2c_client, 0x00);
> +	product = i2c_smbus_read_word_data(isp1301_i2c_client, 0x02);
> +
> +	if (vendor == 0x0483 && product == 0xa0c4)
> +		atx_is_stotg = 1;
> +
>  	/* LPC32XX only supports DAT_SE0 USB mode */
>  	/* This sequence is important */
>  
[...]

MBR, Sergei

Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy

[Greg KH]

On Wed, Jul 29, 2020 at 09:00:04PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 7/29/20 8:49 PM, Alexandre Belloni wrote:
> 
> > The STOTG04 phy is used as a drop-in replacement of the ISP1301 but some
> > bits doesn't have exactly the same meaning and this can lead to issues.
> > Detect the phy dynamically and avoid writing to reserved bits.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> > 
> > Hi Trevor, this is totally untested but at least it builds ;)
> > 
> >  drivers/usb/host/ohci-nxp.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
> > index 85878e8ad331..36ab1501c28f 100644
> > --- a/drivers/usb/host/ohci-nxp.c
> > +++ b/drivers/usb/host/ohci-nxp.c
> > @@ -55,6 +55,15 @@ static struct clk *usb_host_clk;
> >  
> >  static void isp1301_configure_lpc32xx(void)
> >  {
> > +	u8 value, atx_is_stotg = 0;
> 
>    Why the flag is not *bool*?

That's not an issue so much as:

> 
> > +	s32 vendor, product;
> > +
> > +	vendor = i2c_smbus_read_word_data(isp1301_i2c_client, 0x00);
> > +	product = i2c_smbus_read_word_data(isp1301_i2c_client, 0x02);

Why are these signed 32bit numbers?  Shouldn't they be unsigned?

> > +
> > +	if (vendor == 0x0483 && product == 0xa0c4)

No endian flips anywhere?

thanks,

greg k-h

Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy

[Alexandre Belloni]

On 30/07/2020 08:43:03+0200, Greg KH wrote:
> > 
> > > +	s32 vendor, product;
> > > +
> > > +	vendor = i2c_smbus_read_word_data(isp1301_i2c_client, 0x00);
> > > +	product = i2c_smbus_read_word_data(isp1301_i2c_client, 0x02);
> 
> Why are these signed 32bit numbers?  Shouldn't they be unsigned?

Because i2c_smbus_read_word_data returns an s32 and should be checked
for errors but because the whole driver is never checking, I'll leave
that as an exercise for outreachy interns.

> > > +
> > > +	if (vendor == 0x0483 && product == 0xa0c4)
> 
> No endian flips anywhere?
> 

The whole driver makes the assumption that it will only run on lpc32xx
with an isp1301. I don't believe we will ever see an other platform
using it.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: lpc32xx and stotg04

[Trevor Woerner]

Hi Alexandre,

I so greatly appreciate your thoughts on this!

On Wed 2020-07-29 @ 07:28:29 PM, Alexandre Belloni wrote:
> Hi,
> 
> On 23/07/2020 17:27:43-0400, Trevor Woerner wrote:
> > I too am working with a board that uses the lpc32xx SoC (the lpc3240, to be
> > specific) and has a stotg04 for the USB transceiver instead of the isp1301.
> > 
> > I can't get the USB to work.
> > 
> > My guess is that I don't have the device tree correct.
> > 
> > I could embarrass myself by showing you what combinations I've tried but I
> > thought maybe I'd ask and see if either of you could provide a DT snippet
> > describing how to hook up the stotg04 to the i2cusb. Admittedly I'm quite
> > fuzzy when it comes to device trees.
> > 
> 
> This is what I had that is relevant:
> 
> &i2cusb {
> 	clock-frequency = <100000>;
> 
> 	isp1301: usb-transceiver@2d {
> 		compatible = "nxp,isp1301";
> 		reg = <0x2d>;
> 	};
> };
> 
> /* Here, choose exactly one from: ohci, usbd */
> &usbd {
> // &ohci {
> 	transceiver = <&isp1301>;
> 	status = "okay";
> };

It looks like my DTS is correct after all.

> > I'm also a bit fuzzy on USB. I want to plug usb sticks into my device (which,
> > by my understanding, is the opposite of OTG). So additionally I want to enable
> > ohci and not usbd?
> > 
> 
> Indeed, you want ohci (as you can see this is commented out in my device
> tree)
> 
> > In one DT incantation (the one showing the most promise so far) on startup
> > 'lsusb' shows two usb devices. The moment I plug a USB drive into my device I
> > get:
> > 
> > 	[  433.268009] usb-ohci 31020000.ohci: controller won't resume
> > 	[  433.273603] usb-ohci 31020000.ohci: HC died; cleaning up
> > 	[  433.280566] usb 1-1: USB disconnect, device number 2
> > 
> > And afterwards only one device is listed by 'lsusb'.
> > 
> > Currently I'm using a 5.0 kernel and a 5.4 kernel, but I could use any kernel
> > (upstream, ideally). In either case, it doesn't seem possible to deselect the
> > isp1301 from the kernel config? It gets selected automatically. If I'm using
> > the stotg04 instead of the isp1301, do I need a way to turn off the isp1301?
> > 
> 
> The platform I was working on is on v5.1 and has an stotg04. Honnestly,
> the whole thing is a mess and you have to use the isp1301 driver. Then
> the difference between isp1301 and stotg04 is handled in lpc32xx_udc.c.
> 
> So I would say you are missing something similar to what I did in
> 2a60f5eafa74 ("usb: gadget: udc: lpc32xx: add support for stotg04 phy")
> but in drivers/usb/host/ohci-nxp.c:isp1301_configure_lpc32xx()
> 
> I guess your issue is MC2_SPD_SUSP_CTRL line 75 ;) You can try to
> remove it.

Thank you very much for your insights and clarifications! Your patch will be
very useful to me.

Best regards,
	Trevor