Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* lpc32xx and stotg04
@ 2020-07-23 21:27 Trevor Woerner
  2020-07-29 17:28 ` Alexandre Belloni
  0 siblings, 1 reply; 7+ messages in thread
From: Trevor Woerner @ 2020-07-23 21:27 UTC (permalink / raw)
  To: alexandre.belloni, jamesg; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: lpc32xx and stotg04
  2020-07-23 21:27 lpc32xx and stotg04 Trevor Woerner
@ 2020-07-29 17:28 ` Alexandre Belloni
  2020-07-29 17:49   ` [PATCH] usb: ohci-nxp: add support for stotg04 phy Alexandre Belloni
  2020-07-30 18:51   ` lpc32xx and stotg04 Trevor Woerner
  0 siblings, 2 replies; 7+ messages in thread
From: Alexandre Belloni @ 2020-07-29 17:28 UTC (permalink / raw)
  To: Trevor Woerner; +Cc: jamesg, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] usb: ohci-nxp: add support for stotg04 phy
  2020-07-29 17:28 ` Alexandre Belloni
@ 2020-07-29 17:49   ` Alexandre Belloni
  2020-07-29 18:00     ` Sergei Shtylyov
  2020-07-30 18:51   ` lpc32xx and stotg04 Trevor Woerner
  1 sibling, 1 reply; 7+ messages in thread
From: Alexandre Belloni @ 2020-07-29 17:49 UTC (permalink / raw)
  To: Trevor Woerner; +Cc: jamesg, linux-usb, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy
  2020-07-29 17:49   ` [PATCH] usb: ohci-nxp: add support for stotg04 phy Alexandre Belloni
@ 2020-07-29 18:00     ` Sergei Shtylyov
  2020-07-30  6:43       ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Sergei Shtylyov @ 2020-07-29 18:00 UTC (permalink / raw)
  To: Alexandre Belloni, Trevor Woerner; +Cc: jamesg, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy
  2020-07-29 18:00     ` Sergei Shtylyov
@ 2020-07-30  6:43       ` Greg KH
  2020-07-30 11:35         ` Alexandre Belloni
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2020-07-30  6:43 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: Alexandre Belloni, Trevor Woerner, jamesg, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: ohci-nxp: add support for stotg04 phy
  2020-07-30  6:43       ` Greg KH
@ 2020-07-30 11:35         ` Alexandre Belloni
  0 siblings, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2020-07-30 11:35 UTC (permalink / raw)
  To: Greg KH; +Cc: Sergei Shtylyov, Trevor Woerner, jamesg, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: lpc32xx and stotg04
  2020-07-29 17:28 ` Alexandre Belloni
  2020-07-29 17:49   ` [PATCH] usb: ohci-nxp: add support for stotg04 phy Alexandre Belloni
@ 2020-07-30 18:51   ` Trevor Woerner
  1 sibling, 0 replies; 7+ messages in thread
From: Trevor Woerner @ 2020-07-30 18:51 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: jamesg, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 21:27 lpc32xx and stotg04 Trevor Woerner
2020-07-29 17:28 ` Alexandre Belloni
2020-07-29 17:49   ` [PATCH] usb: ohci-nxp: add support for stotg04 phy Alexandre Belloni
2020-07-29 18:00     ` Sergei Shtylyov
2020-07-30  6:43       ` Greg KH
2020-07-30 11:35         ` Alexandre Belloni
2020-07-30 18:51   ` lpc32xx and stotg04 Trevor Woerner

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git