Skip to contentSkip to conversation

lpc32xx and stotg04

PATCH In review Trevor Woerner 4 participants 7 messages Jul 23, 2020 – Jul 30, 2020

Subsystems (3)
USB Arm/LPC32XX SoC support USB ohci driver
lore
Share & tools
lore

Trevor Woerner

author
#0 lore raw

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

#1 lore raw

Hi,

Quoted 12 lines · Trevor Woerner

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";
};
Quoted 4 lines

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)

Quoted 8 lines

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
Quoted 7 lines

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,

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#2 lore raw

PATCH 1/1 usb: ohci-nxp: add support for stotg04 phy

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 <[email protected]>

---

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(-)
drivers/usb/host/ohci-nxp.c +15 -6
index 85878e8ad331..36ab1501c28f 100644
@@ -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));
 }
Signature · 1 line
2.26.2
#3 lore raw

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

Hello!

Quoted 17 lines · 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 <[email protected]>

---

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(-)
drivers/usb/host/ohci-nxp.c +0 -0
index 85878e8ad331..36ab1501c28f 100644

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 58

Quoted 4 lines
drivers/usb/host/ohci-nxp.c +1 -0
@@ -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*?

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 69

Quoted 11 lines
drivers/usb/host/ohci-nxp.c +8 -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 */

[...]

MBR, Sergei

maintainer
#4 lore raw

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

Quoted 22 lines · Sergei Shtylyov

Hello!

Quoted 18 lines · 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 <[email protected]>

---

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(-)
drivers/usb/host/ohci-nxp.c +0 -0
index 85878e8ad331..36ab1501c28f 100644
@@ -55,6 +55,15 @@ static struct clk *usb_host_clk;
Quoted 5 lines

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 58

Quoted 3 lines
drivers/usb/host/ohci-nxp.c +1 -0
 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:

Quoted 5 lines

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 62

Quoted 5 lines
drivers/usb/host/ohci-nxp.c +4 -0
+	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?

Quoted 2 lines

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 64

Quoted 2 lines
drivers/usb/host/ohci-nxp.c +2 -0
+
+	if (vendor == 0x0483 && product == 0xa0c4)

No endian flips anywhere?

thanks,

greg k-h

#5 lore raw

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

Quoted 7 lines · Greg KH
Quoted 5 lines

drivers/usb/host/ohci-nxp.c static struct clk *usb_host_clk; · line 62

Quoted 5 lines
drivers/usb/host/ohci-nxp.c +4 -0
+	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.

Quoted 5 lines
Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	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.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#6 lore raw
In reply to Alexandre Belloni

Hi Alexandre,

I so greatly appreciate your thoughts on this!

Quoted 27 lines · Alexandre Belloni

Hi,

Quoted 12 lines · Trevor Woerner

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>;
	};
};
Quoted 6 lines

/* Here, choose exactly one from: ohci, usbd */
&usbd {

// &ohci {
	transceiver = <&isp1301>;
	status = "okay";
};

It looks like my DTS is correct after all.

Quoted 28 lines
Quoted 4 lines

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)

Quoted 8 lines

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
Quoted 7 lines

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.

Quoted 6 lines

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