* 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 related [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, other threads:[~2020-07-30 18:51 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).