| Message ID | 20200729174918.321615-1-alexandre.belloni@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: ohci-nxp: add support for stotg04 phy | expand |
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
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
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.
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)); }
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(-)