| Message ID | 1717676883-2876611-1-git-send-email-radhey.shyam.pandey@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: misc: add Microchip usb5744 SMBus programming support | expand |
Hi Radhey,
kernel test robot noticed the following build errors:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/usb-misc-add-Microchip-usb5744-SMBus-programming-support/20240606-203028
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1717676883-2876611-1-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240607/202406072046.cA1Mbg1K-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406072046.cA1Mbg1K-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406072046.cA1Mbg1K-lkp@intel.com/
All errors (new ones prefixed by >>):
>> aarch64-linux-ld: drivers/usb/misc/onboard_usb_dev_pdevs.o:(.rodata+0x1498): undefined reference to `onboard_dev_5744_i2c_init'
Hi Radhey, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.10-rc2 next-20240607] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/usb-misc-add-Microchip-usb5744-SMBus-programming-support/20240606-203028 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/1717676883-2876611-1-git-send-email-radhey.shyam.pandey%40amd.com patch subject: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support config: i386-randconfig-063-20240607 (https://download.01.org/0day-ci/archive/20240607/202406072332.qRphZq3E-lkp@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406072332.qRphZq3E-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406072332.qRphZq3E-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@ drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: expected unsigned short [usertype] value drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: got restricted __be16 [usertype] drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@ drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: expected unsigned short [usertype] value drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: got restricted __be16 [usertype] drivers/usb/misc/onboard_usb_dev.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, ...): include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false drivers/usb/misc/onboard_usb_dev.c: note: in included file (through include/linux/mutex.h, include/linux/notifier.h, include/linux/clk.h): include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true vim +311 drivers/usb/misc/onboard_usb_dev.c 299 300 int onboard_dev_5744_i2c_init(struct i2c_client *client) 301 { 302 struct device *dev = &client->dev; 303 int ret; 304 305 char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08}; 306 307 ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf); 308 if (ret) 309 return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n"); 310 > 311 ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700)); 312 if (ret) 313 return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n"); 314 315 /* Send SMBus command to boot hub. */ 316 ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600)); 317 if (ret < 0) 318 return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n"); 319 320 return ret; 321 } 322
> -----Original Message----- > From: Matthias Kaehlcke <mka@chromium.org> > Sent: Thursday, June 6, 2024 11:54 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Cc: gregkh@linuxfoundation.org; javier.carrasco@wolfvision.net; > benjamin.bara@skidata.com; m.felsch@pengutronix.de; > jbrunet@baylibre.com; frieder.schrempf@kontron.de; > stefan.eichenberger@toradex.com; Simek, Michal > <michal.simek@amd.com>; linux-usb@vger.kernel.org; linux- > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH] usb: misc: add Microchip usb5744 SMBus programming > support > > On Thu, Jun 06, 2024 at 05:58:03PM +0530, Radhey Shyam Pandey wrote: > > > PATCH] usb: misc: add Microchip usb5744 SMBus programming support > > usb: misc: onboard_usb_dev: ... Sure , will update in next version. > > > > > usb5744 supports SMBus Configuration and it may be configured via the > > SMBus slave interface during the hub’s start-up configuration stage. > > > > To program it introduce i2c initialization hook and set usb5744 > > platform data with function having required smbus initialization > > sequence. Core driver uses i2c-bus phandle (added in commit > '02be19e914b8 dt-bindings: > > usb: Add support for Microchip usb5744 hub controller') to get i2c > > client device and then calls usb5744 i2c default initialization sequence. > > > > Apart from the USB command attach, prevent the hub from suspend. > > when the “USB Attach with SMBus (0xAA56)” command is issued to the > > hub, the hub is getting enumerated and then it puts in a suspend mode. > > This causes the hub to NAK any SMBus access made by the SMBus Master > > during this period and not able to see the hub's slave address while > > running the "i2c probe" command. > > > > Prevent the MCU from the putting the HUB in suspend mode through > > register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the > > RuntimeFlags2 register at address 0x411D controls this aspect of the > > hub. The BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to > > ensure that the MCU is always enabled and ready to respond to SMBus > runtime commands. > > This register needs to be written before the USB attach command is issued. > > > > The byte sequence is as follows: > > Slave addr: 0x2d 00 00 05 00 01 41 1D 08 > > Slave addr: 0x2d 99 37 00 > > Slave addr: 0x2d AA 56 00 > > > > In addition to SMBus programming sequence also update post reset delay > > as without it there is a failure on first SMBus write. > > i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed > > > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > > --- > > --- > > drivers/usb/misc/onboard_usb_dev.c | 46 > > ++++++++++++++++++++++++++++++ drivers/usb/misc/onboard_usb_dev.h > | > > 8 +++++- > > 2 files changed, 53 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/misc/onboard_usb_dev.c > > b/drivers/usb/misc/onboard_usb_dev.c > > index f2bcc1a8b95f..5621c1273a12 100644 > > --- a/drivers/usb/misc/onboard_usb_dev.c > > +++ b/drivers/usb/misc/onboard_usb_dev.c > > @@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct > onboard_dev > > *onboard_dev) > > > > fsleep(onboard_dev->pdata->reset_us); > > gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0); > > + fsleep(onboard_dev->pdata->reset_us); > > This also impacts devices that don't require a delay, plus requirements for > this delay are not necessarily the same as the reset delay. > > Better add a dedicated field like 'power_on_delay_us'. It's a nice suggestion. I will create a separate preparatory patch to introduce 'power_on_delay_us' and then set its value in this usb5744 SMBus programming support patch. > > > > > onboard_dev->is_powered_on = true; > > > > @@ -296,10 +297,34 @@ static void > onboard_dev_attach_usb_driver(struct work_struct *work) > > pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err)); } > > > > +int onboard_dev_5744_i2c_init(struct i2c_client *client) > > static int > > We probably want to move hardware specific code to a dedicated file as > there is added more, but I for now it's ok to have it in the main driver. To make onboard_dev_5744_i2c_init as static we need to move microchip_usb5744_data and onboard_dev_match to onboard_usb_dev.c but then i noticed onboard_dev_match is also being used in onboard_usb_dev_pdevs.c source file. > > > +{ > > + struct device *dev = &client->dev; > > + int ret; > > + > > + char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08}; > > Please use constants for the different bits instead of magic values. I know the > magic values are explained in the commit message, but that's something > people have to dig up. Sure, will use defines for these values. > > > + > > + ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf); > > + if (ret) > > + return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit > > +configuration failed\n"); > > + > > + ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700)); > > ditto, no magic values please Yes, will fix it in next version. > > > + if (ret) > > + return dev_err_probe(dev, ret, "Configuration Register > Access > > +Command failed\n"); > > + > > + /* Send SMBus command to boot hub. */ > > + ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600)); > > ditto Sure, will fix it. > > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "USB Attach with SMBus > command > > +failed\n"); > > + > > + return ret; > > return 0; > > +} > > + > > static int onboard_dev_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; > > struct onboard_dev *onboard_dev; > > + struct device_node *i2c_node; > > int err; > > > > onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), > GFP_KERNEL); > > @@ -339,6 +364,23 @@ static int onboard_dev_probe(struct > platform_device *pdev) > > if (err) > > return err; > > > > + i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0); > > + if (i2c_node) { > > + struct i2c_client *client; > > + > > + client = of_find_i2c_device_by_node(i2c_node); > > + of_node_put(i2c_node); > > + > > + if (!client) { > > + err = -EPROBE_DEFER; > > + goto err_dev_power_off; > > nit: err_power_off Sure, will fix it. > > > + } > > + err = onboard_dev->pdata->onboard_dev_i2c_init(client); > > + put_device(&client->dev); > > + if (err < 0) > > + goto err_dev_power_off; > > + } > > + > > /* > > * The USB driver might have been detached from the USB devices by > > * onboard_dev_remove() (e.g. through an 'unbind' by userspace), > @@ > > -350,6 +392,10 @@ static int onboard_dev_probe(struct platform_device > *pdev) > > schedule_work(&attach_usb_driver_work); > > > > return 0; > > + > > +err_dev_power_off: > > + onboard_dev_power_off(onboard_dev); > > + return err; > > } > > > > static void onboard_dev_remove(struct platform_device *pdev) diff > > --git a/drivers/usb/misc/onboard_usb_dev.h > > b/drivers/usb/misc/onboard_usb_dev.h > > index fbba549c0f47..17311ea7bacd 100644 > > --- a/drivers/usb/misc/onboard_usb_dev.h > > +++ b/drivers/usb/misc/onboard_usb_dev.h > > @@ -6,6 +6,8 @@ > > #ifndef _USB_MISC_ONBOARD_USB_DEV_H > > #define _USB_MISC_ONBOARD_USB_DEV_H > > > > +#include <linux/i2c.h> > > + > > #define MAX_SUPPLIES 2 > > > > struct onboard_dev_pdata { > > @@ -13,6 +15,7 @@ struct onboard_dev_pdata { > > unsigned int num_supplies; /* number of supplies */ > > const char * const supply_names[MAX_SUPPLIES]; > > bool is_hub; > > + int (*onboard_dev_i2c_init)(struct i2c_client *client); > > }; > > > > static const struct onboard_dev_pdata microchip_usb424_data = { @@ > > -22,11 +25,14 @@ static const struct onboard_dev_pdata > microchip_usb424_data = { > > .is_hub = true, > > }; > > > > +int onboard_dev_5744_i2c_init(struct i2c_client *client); > > + > > static const struct onboard_dev_pdata microchip_usb5744_data = { > > - .reset_us = 0, > > + .reset_us = 10000, > > That's one reason why I don't think it's a good idea to use 'reset_us' > twice. In this case the total delay would go from formerly 0ms to 20ms, when > a delay of 10ms after the reset should be sufficient. Agree , will introduce 'power_on_delay_us' and it should address it. > > > .num_supplies = 2, > > .supply_names = { "vdd", "vdd2" }, > > .is_hub = true, > > + .onboard_dev_i2c_init = onboard_dev_5744_i2c_init, > > }; > > > > static const struct onboard_dev_pdata realtek_rts5411_data = { > > -- > > 2.34.1 > >
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c index f2bcc1a8b95f..5621c1273a12 100644 --- a/drivers/usb/misc/onboard_usb_dev.c +++ b/drivers/usb/misc/onboard_usb_dev.c @@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct onboard_dev *onboard_dev) fsleep(onboard_dev->pdata->reset_us); gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0); + fsleep(onboard_dev->pdata->reset_us); onboard_dev->is_powered_on = true; @@ -296,10 +297,34 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work) pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err)); } +int onboard_dev_5744_i2c_init(struct i2c_client *client) +{ + struct device *dev = &client->dev; + int ret; + + char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08}; + + ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf); + if (ret) + return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n"); + + ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700)); + if (ret) + return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n"); + + /* Send SMBus command to boot hub. */ + ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600)); + if (ret < 0) + return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n"); + + return ret; +} + static int onboard_dev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct onboard_dev *onboard_dev; + struct device_node *i2c_node; int err; onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL); @@ -339,6 +364,23 @@ static int onboard_dev_probe(struct platform_device *pdev) if (err) return err; + i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0); + if (i2c_node) { + struct i2c_client *client; + + client = of_find_i2c_device_by_node(i2c_node); + of_node_put(i2c_node); + + if (!client) { + err = -EPROBE_DEFER; + goto err_dev_power_off; + } + err = onboard_dev->pdata->onboard_dev_i2c_init(client); + put_device(&client->dev); + if (err < 0) + goto err_dev_power_off; + } + /* * The USB driver might have been detached from the USB devices by * onboard_dev_remove() (e.g. through an 'unbind' by userspace), @@ -350,6 +392,10 @@ static int onboard_dev_probe(struct platform_device *pdev) schedule_work(&attach_usb_driver_work); return 0; + +err_dev_power_off: + onboard_dev_power_off(onboard_dev); + return err; } static void onboard_dev_remove(struct platform_device *pdev) diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h index fbba549c0f47..17311ea7bacd 100644 --- a/drivers/usb/misc/onboard_usb_dev.h +++ b/drivers/usb/misc/onboard_usb_dev.h @@ -6,6 +6,8 @@ #ifndef _USB_MISC_ONBOARD_USB_DEV_H #define _USB_MISC_ONBOARD_USB_DEV_H +#include <linux/i2c.h> + #define MAX_SUPPLIES 2 struct onboard_dev_pdata { @@ -13,6 +15,7 @@ struct onboard_dev_pdata { unsigned int num_supplies; /* number of supplies */ const char * const supply_names[MAX_SUPPLIES]; bool is_hub; + int (*onboard_dev_i2c_init)(struct i2c_client *client); }; static const struct onboard_dev_pdata microchip_usb424_data = { @@ -22,11 +25,14 @@ static const struct onboard_dev_pdata microchip_usb424_data = { .is_hub = true, }; +int onboard_dev_5744_i2c_init(struct i2c_client *client); + static const struct onboard_dev_pdata microchip_usb5744_data = { - .reset_us = 0, + .reset_us = 10000, .num_supplies = 2, .supply_names = { "vdd", "vdd2" }, .is_hub = true, + .onboard_dev_i2c_init = onboard_dev_5744_i2c_init, }; static const struct onboard_dev_pdata realtek_rts5411_data = {
usb5744 supports SMBus Configuration and it may be configured via the SMBus slave interface during the hub’s start-up configuration stage. To program it introduce i2c initialization hook and set usb5744 platform data with function having required smbus initialization sequence. Core driver uses i2c-bus phandle (added in commit '02be19e914b8 dt-bindings: usb: Add support for Microchip usb5744 hub controller') to get i2c client device and then calls usb5744 i2c default initialization sequence. Apart from the USB command attach, prevent the hub from suspend. when the “USB Attach with SMBus (0xAA56)” command is issued to the hub, the hub is getting enumerated and then it puts in a suspend mode. This causes the hub to NAK any SMBus access made by the SMBus Master during this period and not able to see the hub's slave address while running the "i2c probe" command. Prevent the MCU from the putting the HUB in suspend mode through register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2 register at address 0x411D controls this aspect of the hub. The BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to ensure that the MCU is always enabled and ready to respond to SMBus runtime commands. This register needs to be written before the USB attach command is issued. The byte sequence is as follows: Slave addr: 0x2d 00 00 05 00 01 41 1D 08 Slave addr: 0x2d 99 37 00 Slave addr: 0x2d AA 56 00 In addition to SMBus programming sequence also update post reset delay as without it there is a failure on first SMBus write. i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> --- --- drivers/usb/misc/onboard_usb_dev.c | 46 ++++++++++++++++++++++++++++++ drivers/usb/misc/onboard_usb_dev.h | 8 +++++- 2 files changed, 53 insertions(+), 1 deletion(-)