[PATCH v4 0/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus support

[PATCH v4 0/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus support

[Radhey Shyam Pandey]

This patchset adds usb5744 SMBus support in onboard usb driver.

Changes for v4:
- Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
  introducing a kconfig dependency on I2C_CONFIG. This error is reported
  by kernel test on v3 series and usb:usb-testing 20/25 branch.
  https://lore.kernel.org/all/2024082503-uncoated-chaperone-7f70@gregkh

Changes for v3:
- Modified power_on_delay_us comment.
- Add comment for UDC suspend sequence.
- Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and replace
  it with literal + comment.
- Move microchip defines to source file.

Changes in v2:
- Fix subsystem "usb: misc: onboard_usb_dev:..."
- Change implementation from introducing onboard_dev_i2c_init
  func pointer and do i2c initialization based on compatible string.
  This is to make onboard_dev_5744_i2c_init() as static.
- Use #define for different register bits instead of magic values.
- Use err_power_off label name.
- Modified commit description to be in sync with v2 changes.
- Move power on reset delay to separate patch.

Radhey Shyam Pandey (2):
  usb: misc: onboard_dev: extend platform data to add power on delay
    field
  usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming
    support

 drivers/usb/misc/Kconfig           |  2 +-
 drivers/usb/misc/onboard_usb_dev.c | 74 ++++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_dev.h |  2 +
 3 files changed, 77 insertions(+), 1 deletion(-)


base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
-- 
2.34.1

[PATCH v4 1/2] usb: misc: onboard_dev: extend platform data to add power on delay field

[Radhey Shyam Pandey]

Introduce dedicated field 'power_on_delay_us' in onboard platform data
and update its delay for USB5744 configuration. Hub itself requires some
delay after reset to get to state where configuration data is going to
be accepted. Without delay upcoming support for configuration via SMBUS
is reporting a failure on the first SMBus write.

i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed

Similar delay is likely also required for default configuration but
because there is enough time (code execution) between reset and usage
of the hub any issue is not visible but it doesn't mean delay shouldn't
be reflected.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Suggested-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes for v4:
- None

Changes for v3:
- Modified power_on_delay_us comment.

Changes for v2:
- New patch
---
 drivers/usb/misc/onboard_usb_dev.c | 1 +
 drivers/usb/misc/onboard_usb_dev.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index 56710e6b1653..da27c48fc11d 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->power_on_delay_us);
 
 	onboard_dev->is_powered_on = true;
 
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index fbba549c0f47..317b3eb99c02 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -10,6 +10,7 @@
 
 struct onboard_dev_pdata {
 	unsigned long reset_us;		/* reset pulse width in us */
+	unsigned long power_on_delay_us; /* power on delay in us */
 	unsigned int num_supplies;	/* number of supplies */
 	const char * const supply_names[MAX_SUPPLIES];
 	bool is_hub;
@@ -24,6 +25,7 @@ static const struct onboard_dev_pdata microchip_usb424_data = {
 
 static const struct onboard_dev_pdata microchip_usb5744_data = {
 	.reset_us = 0,
+	.power_on_delay_us = 10000,
 	.num_supplies = 2,
 	.supply_names = { "vdd", "vdd2" },
 	.is_hub = true,
-- 
2.34.1

[PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Radhey Shyam Pandey]

usb5744 supports SMBus Configuration and it may be configured via the
SMBus slave interface during the hub start-up configuration stage.

To program it 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 based on usb5744 compatible check 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 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

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes for v4:
- Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
  introducing a dependency on I2C_CONFIG. This error is reported
  by kernel test on v3 series and usb:usb-testing 20/25 branch.
  https://lore.kernel.org/all/2024082503-uncoated-chaperone-7f70@gregkh

Changes for v3:
- Add comment for UDC suspend sequence.
- Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and replace
  it with literal + comment.
- Move microchip defines to source file.

Changes for v2:
- Move power on reset delay to separate patch.
- Switch to compatible based check for calling usb5755
  onboard_dev_5744_i2c_init(). This is done to make
  onboard_dev_5744_i2c_init() as static.
- Fix subsystem "usb: misc: onboard_usb_dev:..."
- Use #define for different register bits instead of magic values.
- Use err_power_off label name.
- Modified commit description to be in sync with v2 changes.
---
 drivers/usb/misc/Kconfig           |  2 +-
 drivers/usb/misc/onboard_usb_dev.c | 73 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 50b86d531701..cb5e47d439ab 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
 
 config USB_ONBOARD_DEV
 	tristate "Onboard USB device support"
-	depends on OF
+	depends on OF && I2C
 	help
 	  Say Y here if you want to support discrete onboard USB devices
 	  that don't require an additional control bus for initialization,
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index da27c48fc11d..247600015bca 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -11,6 +11,7 @@
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/init.h>
+#include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -29,6 +30,17 @@
 
 #include "onboard_usb_dev.h"
 
+/* USB5744 register offset and mask */
+#define USB5744_CMD_ATTACH			0xAA
+#define USB5744_CMD_ATTACH_LSB			0x56
+#define USB5744_CMD_CREG_ACCESS			0x99
+#define USB5744_CMD_CREG_ACCESS_LSB		0x37
+#define USB5744_CREG_MEM_ADDR			0x00
+#define USB5744_CREG_WRITE			0x00
+#define USB5744_CREG_RUNTIMEFLAGS2		0x41
+#define USB5744_CREG_RUNTIMEFLAGS2_LSB		0x1D
+#define USB5744_CREG_BYPASS_UDC_SUSPEND		BIT(3)
+
 static void onboard_dev_attach_usb_driver(struct work_struct *work);
 
 static struct usb_device_driver onboard_dev_usbdev_driver;
@@ -297,10 +309,46 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
 		pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
 }
 
+static int onboard_dev_5744_i2c_init(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	int ret;
+
+	/*
+	 * Set BYPASS_UDC_SUSPEND bit to ensure MCU is always enabled
+	 * and ready to respond to SMBus runtime commands.
+	 * The command writes 5 bytes to memory and single data byte in
+	 * configuration register.
+	 */
+	char wr_buf[7] = {USB5744_CREG_MEM_ADDR, 5,
+			  USB5744_CREG_WRITE, 1,
+			  USB5744_CREG_RUNTIMEFLAGS2,
+			  USB5744_CREG_RUNTIMEFLAGS2_LSB,
+			  USB5744_CREG_BYPASS_UDC_SUSPEND};
+
+	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, USB5744_CMD_CREG_ACCESS,
+					USB5744_CMD_CREG_ACCESS_LSB);
+	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, USB5744_CMD_ATTACH,
+					USB5744_CMD_ATTACH_LSB);
+	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);
@@ -340,6 +388,27 @@ 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_power_off;
+		}
+
+		if (of_device_is_compatible(pdev->dev.of_node, "usb424,2744") ||
+		    of_device_is_compatible(pdev->dev.of_node, "usb424,5744"))
+			err = onboard_dev_5744_i2c_init(client);
+
+		put_device(&client->dev);
+		if (err < 0)
+			goto err_power_off;
+	}
+
 	/*
 	 * The USB driver might have been detached from the USB devices by
 	 * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
@@ -351,6 +420,10 @@ static int onboard_dev_probe(struct platform_device *pdev)
 	schedule_work(&attach_usb_driver_work);
 
 	return 0;
+
+err_power_off:
+	onboard_dev_power_off(onboard_dev);
+	return err;
 }
 
 static void onboard_dev_remove(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Greg KH]

On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> usb5744 supports SMBus Configuration and it may be configured via the
> SMBus slave interface during the hub start-up configuration stage.
> 
> To program it 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 based on usb5744 compatible check 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 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
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes for v4:
> - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
>   introducing a dependency on I2C_CONFIG. This error is reported
>   by kernel test on v3 series and usb:usb-testing 20/25 branch.
>   https://lore.kernel.org/all/2024082503-uncoated-chaperone-7f70@gregkh
> 
> Changes for v3:
> - Add comment for UDC suspend sequence.
> - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and replace
>   it with literal + comment.
> - Move microchip defines to source file.
> 
> Changes for v2:
> - Move power on reset delay to separate patch.
> - Switch to compatible based check for calling usb5755
>   onboard_dev_5744_i2c_init(). This is done to make
>   onboard_dev_5744_i2c_init() as static.
> - Fix subsystem "usb: misc: onboard_usb_dev:..."
> - Use #define for different register bits instead of magic values.
> - Use err_power_off label name.
> - Modified commit description to be in sync with v2 changes.
> ---
>  drivers/usb/misc/Kconfig           |  2 +-
>  drivers/usb/misc/onboard_usb_dev.c | 73 ++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 50b86d531701..cb5e47d439ab 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
>  
>  config USB_ONBOARD_DEV
>  	tristate "Onboard USB device support"
> -	depends on OF
> +	depends on OF && I2C

This feels wrong.

While a single device that this driver supports might need i2c, not all
of the devices do, so you have the potential to drag in a bunch of code
here for devices that do not have/need i2c at all, right?

Any way to "split this out" into a smaller chunk?  Or better yet, just
detect at runtime if you need/want to call those i2c functions (and they
should have no-ops for when i2c is not enabled, right?)

thanks,

greg k-h

RE: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, September 3, 2024 12:10 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: mka@chromium.org; sakari.ailus@linux.intel.com;
> wentong.wu@intel.com; javier.carrasco@wolfvision.net;
> stefan.eichenberger@toradex.com; francesco.dolcini@toradex.com;
> jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip
> usb5744 SMBus programming support
> 
> On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> > usb5744 supports SMBus Configuration and it may be configured via the
> > SMBus slave interface during the hub start-up configuration stage.
> >
> > To program it 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 based on usb5744 compatible check 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 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
> >
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > ---
> > Changes for v4:
> > - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
> >   introducing a dependency on I2C_CONFIG. This error is reported
> >   by kernel test on v3 series and usb:usb-testing 20/25 branch.
> >   https://lore.kernel.org/all/2024082503-uncoated-chaperone-
> 7f70@gregkh
> >
> > Changes for v3:
> > - Add comment for UDC suspend sequence.
> > - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and
> replace
> >   it with literal + comment.
> > - Move microchip defines to source file.
> >
> > Changes for v2:
> > - Move power on reset delay to separate patch.
> > - Switch to compatible based check for calling usb5755
> >   onboard_dev_5744_i2c_init(). This is done to make
> >   onboard_dev_5744_i2c_init() as static.
> > - Fix subsystem "usb: misc: onboard_usb_dev:..."
> > - Use #define for different register bits instead of magic values.
> > - Use err_power_off label name.
> > - Modified commit description to be in sync with v2 changes.
> > ---
> >  drivers/usb/misc/Kconfig           |  2 +-
> >  drivers/usb/misc/onboard_usb_dev.c | 73
> ++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > index 50b86d531701..cb5e47d439ab 100644
> > --- a/drivers/usb/misc/Kconfig
> > +++ b/drivers/usb/misc/Kconfig
> > @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
> >
> >  config USB_ONBOARD_DEV
> >  	tristate "Onboard USB device support"
> > -	depends on OF
> > +	depends on OF && I2C
> 
> This feels wrong.
> 
> While a single device that this driver supports might need i2c, not all
> of the devices do, so you have the potential to drag in a bunch of code
> here for devices that do not have/need i2c at all, right?
> 
> Any way to "split this out" into a smaller chunk?  Or better yet, just
> detect at runtime if you need/want to call those i2c functions (and they
> should have no-ops for when i2c is not enabled, right?)

In onboard driver I am calling onboard_dev_5744_i2c_init() by
detecting at runtime if "i2c-bus" phandle is present. But the 
problem is i2c_smbus_* APIs are declared and defined only for 
#if IS_ENABLED(CONFIG_I2C).

Do you think we should implement no-ops for I2C smbus APIs
(in linux/i2c.h)? OR a simpler alternative would be to
add #if IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init()
and return error code if CONFIG_I2C is not defined ? 
Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of 
drivers using this approach.

Thanks,
Radhey

Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Greg KH]

On Tue, Sep 03, 2024 at 07:19:09AM +0000, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Tuesday, September 3, 2024 12:10 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: mka@chromium.org; sakari.ailus@linux.intel.com;
> > wentong.wu@intel.com; javier.carrasco@wolfvision.net;
> > stefan.eichenberger@toradex.com; francesco.dolcini@toradex.com;
> > jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> > frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip
> > usb5744 SMBus programming support
> > 
> > On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> > > usb5744 supports SMBus Configuration and it may be configured via the
> > > SMBus slave interface during the hub start-up configuration stage.
> > >
> > > To program it 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 based on usb5744 compatible check 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 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
> > >
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > ---
> > > Changes for v4:
> > > - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
> > >   introducing a dependency on I2C_CONFIG. This error is reported
> > >   by kernel test on v3 series and usb:usb-testing 20/25 branch.
> > >   https://lore.kernel.org/all/2024082503-uncoated-chaperone-
> > 7f70@gregkh
> > >
> > > Changes for v3:
> > > - Add comment for UDC suspend sequence.
> > > - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and
> > replace
> > >   it with literal + comment.
> > > - Move microchip defines to source file.
> > >
> > > Changes for v2:
> > > - Move power on reset delay to separate patch.
> > > - Switch to compatible based check for calling usb5755
> > >   onboard_dev_5744_i2c_init(). This is done to make
> > >   onboard_dev_5744_i2c_init() as static.
> > > - Fix subsystem "usb: misc: onboard_usb_dev:..."
> > > - Use #define for different register bits instead of magic values.
> > > - Use err_power_off label name.
> > > - Modified commit description to be in sync with v2 changes.
> > > ---
> > >  drivers/usb/misc/Kconfig           |  2 +-
> > >  drivers/usb/misc/onboard_usb_dev.c | 73
> > ++++++++++++++++++++++++++++++
> > >  2 files changed, 74 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > > index 50b86d531701..cb5e47d439ab 100644
> > > --- a/drivers/usb/misc/Kconfig
> > > +++ b/drivers/usb/misc/Kconfig
> > > @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
> > >
> > >  config USB_ONBOARD_DEV
> > >  	tristate "Onboard USB device support"
> > > -	depends on OF
> > > +	depends on OF && I2C
> > 
> > This feels wrong.
> > 
> > While a single device that this driver supports might need i2c, not all
> > of the devices do, so you have the potential to drag in a bunch of code
> > here for devices that do not have/need i2c at all, right?
> > 
> > Any way to "split this out" into a smaller chunk?  Or better yet, just
> > detect at runtime if you need/want to call those i2c functions (and they
> > should have no-ops for when i2c is not enabled, right?)
> 
> In onboard driver I am calling onboard_dev_5744_i2c_init() by
> detecting at runtime if "i2c-bus" phandle is present. But the 
> problem is i2c_smbus_* APIs are declared and defined only for 
> #if IS_ENABLED(CONFIG_I2C).
> 
> Do you think we should implement no-ops for I2C smbus APIs
> (in linux/i2c.h)? OR a simpler alternative would be to
> add #if IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init()
> and return error code if CONFIG_I2C is not defined ? 
> Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of 
> drivers using this approach.

Yes, try doing this with simpler way first and see what that looks like.

thanks,

greg k-h

Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Matthias Kaehlcke]

El Tue, Sep 03, 2024 at 07:19:09AM GMT Pandey, Radhey Shyam ha dit:

> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Tuesday, September 3, 2024 12:10 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: mka@chromium.org; sakari.ailus@linux.intel.com;
> > wentong.wu@intel.com; javier.carrasco@wolfvision.net;
> > stefan.eichenberger@toradex.com; francesco.dolcini@toradex.com;
> > jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> > frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip
> > usb5744 SMBus programming support
> > 
> > On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> > > usb5744 supports SMBus Configuration and it may be configured via the
> > > SMBus slave interface during the hub start-up configuration stage.
> > >
> > > To program it 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 based on usb5744 compatible check 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 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
> > >
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > ---
> > > Changes for v4:
> > > - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
> > >   introducing a dependency on I2C_CONFIG. This error is reported
> > >   by kernel test on v3 series and usb:usb-testing 20/25 branch.
> > >   https://lore.kernel.org/all/2024082503-uncoated-chaperone-
> > 7f70@gregkh
> > >
> > > Changes for v3:
> > > - Add comment for UDC suspend sequence.
> > > - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and
> > replace
> > >   it with literal + comment.
> > > - Move microchip defines to source file.
> > >
> > > Changes for v2:
> > > - Move power on reset delay to separate patch.
> > > - Switch to compatible based check for calling usb5755
> > >   onboard_dev_5744_i2c_init(). This is done to make
> > >   onboard_dev_5744_i2c_init() as static.
> > > - Fix subsystem "usb: misc: onboard_usb_dev:..."
> > > - Use #define for different register bits instead of magic values.
> > > - Use err_power_off label name.
> > > - Modified commit description to be in sync with v2 changes.
> > > ---
> > >  drivers/usb/misc/Kconfig           |  2 +-
> > >  drivers/usb/misc/onboard_usb_dev.c | 73
> > ++++++++++++++++++++++++++++++
> > >  2 files changed, 74 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > > index 50b86d531701..cb5e47d439ab 100644
> > > --- a/drivers/usb/misc/Kconfig
> > > +++ b/drivers/usb/misc/Kconfig
> > > @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
> > >
> > >  config USB_ONBOARD_DEV
> > >  	tristate "Onboard USB device support"
> > > -	depends on OF
> > > +	depends on OF && I2C
> > 
> > This feels wrong.
> > 
> > While a single device that this driver supports might need i2c, not all
> > of the devices do, so you have the potential to drag in a bunch of code
> > here for devices that do not have/need i2c at all, right?
> > 
> > Any way to "split this out" into a smaller chunk?  Or better yet, just
> > detect at runtime if you need/want to call those i2c functions (and they
> > should have no-ops for when i2c is not enabled, right?)
> 
> In onboard driver I am calling onboard_dev_5744_i2c_init() by
> detecting at runtime if "i2c-bus" phandle is present. But the 
> problem is i2c_smbus_* APIs are declared and defined only for 
> #if IS_ENABLED(CONFIG_I2C).
> 
> Do you think we should implement no-ops for I2C smbus APIs
> (in linux/i2c.h)? OR a simpler alternative would be to
> add #if IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init()
> and return error code if CONFIG_I2C is not defined ? 
> Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of 
> drivers using this approach.

I expect using IS_ENABLED(CONFIG_I2C) would cause issues when
USB_ONBOARD_DEV=y and CONFIG_I2C=m.

Another option could be adding an option USB_ONBOARD_DEV_USB5744
(or similar) and make that dependent on CONFIG_I2C.

RE: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Matthias Kaehlcke <matthias@kaehlcke.net>
> Sent: Wednesday, September 18, 2024 10:37 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; mka@chromium.org;
> sakari.ailus@linux.intel.com; wentong.wu@intel.com;
> javier.carrasco@wolfvision.net; stefan.eichenberger@toradex.com;
> francesco.dolcini@toradex.com; jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744
> SMBus programming support
> 
> El Tue, Sep 03, 2024 at 07:19:09AM GMT Pandey, Radhey Shyam ha dit:
> 
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Tuesday, September 3, 2024 12:10 PM
> > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > Cc: mka@chromium.org; sakari.ailus@linux.intel.com;
> > > wentong.wu@intel.com; javier.carrasco@wolfvision.net;
> > > stefan.eichenberger@toradex.com; francesco.dolcini@toradex.com;
> > > jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> > > frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add
> > > Microchip
> > > usb5744 SMBus programming support
> > >
> > > On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> > > > usb5744 supports SMBus Configuration and it may be configured via
> > > > the SMBus slave interface during the hub start-up configuration stage.
> > > >
> > > > To program it 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 based on usb5744
> > > > compatible check 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 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
> > > >
> > > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > > ---
> > > > Changes for v4:
> > > > - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
> > > >   introducing a dependency on I2C_CONFIG. This error is reported
> > > >   by kernel test on v3 series and usb:usb-testing 20/25 branch.
> > > >   https://lore.kernel.org/all/2024082503-uncoated-chaperone-
> > > 7f70@gregkh
> > > >
> > > > Changes for v3:
> > > > - Add comment for UDC suspend sequence.
> > > > - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES
> and
> > > replace
> > > >   it with literal + comment.
> > > > - Move microchip defines to source file.
> > > >
> > > > Changes for v2:
> > > > - Move power on reset delay to separate patch.
> > > > - Switch to compatible based check for calling usb5755
> > > >   onboard_dev_5744_i2c_init(). This is done to make
> > > >   onboard_dev_5744_i2c_init() as static.
> > > > - Fix subsystem "usb: misc: onboard_usb_dev:..."
> > > > - Use #define for different register bits instead of magic values.
> > > > - Use err_power_off label name.
> > > > - Modified commit description to be in sync with v2 changes.
> > > > ---
> > > >  drivers/usb/misc/Kconfig           |  2 +-
> > > >  drivers/usb/misc/onboard_usb_dev.c | 73
> > > ++++++++++++++++++++++++++++++
> > > >  2 files changed, 74 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > > > index 50b86d531701..cb5e47d439ab 100644
> > > > --- a/drivers/usb/misc/Kconfig
> > > > +++ b/drivers/usb/misc/Kconfig
> > > > @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
> > > >
> > > >  config USB_ONBOARD_DEV
> > > >  	tristate "Onboard USB device support"
> > > > -	depends on OF
> > > > +	depends on OF && I2C
> > >
> > > This feels wrong.
> > >
> > > While a single device that this driver supports might need i2c, not
> > > all of the devices do, so you have the potential to drag in a bunch
> > > of code here for devices that do not have/need i2c at all, right?
> > >
> > > Any way to "split this out" into a smaller chunk?  Or better yet,
> > > just detect at runtime if you need/want to call those i2c functions
> > > (and they should have no-ops for when i2c is not enabled, right?)
> >
> > In onboard driver I am calling onboard_dev_5744_i2c_init() by
> > detecting at runtime if "i2c-bus" phandle is present. But the problem
> > is i2c_smbus_* APIs are declared and defined only for #if
> > IS_ENABLED(CONFIG_I2C).
> >
> > Do you think we should implement no-ops for I2C smbus APIs (in
> > linux/i2c.h)? OR a simpler alternative would be to add #if
> > IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init() and return
> > error code if CONFIG_I2C is not defined ?
> > Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of drivers
> > using this approach.
> 
> I expect using IS_ENABLED(CONFIG_I2C) would cause issues when
> USB_ONBOARD_DEV=y and CONFIG_I2C=m.
> 
> Another option could be adding an option USB_ONBOARD_DEV_USB5744 (or
> similar) and make that dependent on CONFIG_I2C.

Yes, theoretically this can also be valid build configuration.

I assume your suggestion is something like below:

+config USB_ONBOARD_DEV_USB5744
+       tristate "Onboard USB Microchip usb5744 hub with SMBus support"
+       depends on USB_ONBOARD_DEV && I2C
+       help
+         Say Y here if you want to support onboard USB Microchip usb5744
+         hub that requires SMBus initialization.
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index 560591e02d6a..6169780a18d9 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -311,7 +311,7 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)

 static int onboard_dev_5744_i2c_init(struct i2c_client *client)
 {
-#if IS_ENABLED(CONFIG_I2C)
+#if IS_ENABLED(CONFIG_USB_ONBOARD_DEV_USB5744)


<*>   Onboard USB device support                                                               
  <M>     Onboard USB Microchip usb5744 hub with SMBus support


Here IS_ENABLED will report true either onboard usb5744 is module/in-built.
So, we hit same issue as USB_ONBOARD_DEV=y and I2C=m.

FYI, as this series merged to -next based on our discussion I will send another
series fixing this particular build configuration and other coding style changes
based on v4 review comments.

Thanks,
Radhey

Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Matthias Kaehlcke]

El Wed, Sep 18, 2024 at 07:22:42PM GMT Pandey, Radhey Shyam ha dit:

> > -----Original Message-----
> > From: Matthias Kaehlcke <matthias@kaehlcke.net>
> > Sent: Wednesday, September 18, 2024 10:37 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: Greg KH <gregkh@linuxfoundation.org>; mka@chromium.org;
> > sakari.ailus@linux.intel.com; wentong.wu@intel.com;
> > javier.carrasco@wolfvision.net; stefan.eichenberger@toradex.com;
> > francesco.dolcini@toradex.com; jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> > frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744
> > SMBus programming support
> > 
> > El Tue, Sep 03, 2024 at 07:19:09AM GMT Pandey, Radhey Shyam ha dit:
> > 
> > > > -----Original Message-----
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Sent: Tuesday, September 3, 2024 12:10 PM
> > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > Cc: mka@chromium.org; sakari.ailus@linux.intel.com;
> > > > wentong.wu@intel.com; javier.carrasco@wolfvision.net;
> > > > stefan.eichenberger@toradex.com; francesco.dolcini@toradex.com;
> > > > jbrunet@baylibre.com; macpaul.lin@mediatek.com;
> > > > frieder.schrempf@kontron.de; linux-usb@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > > > Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add
> > > > Microchip
> > > > usb5744 SMBus programming support
> > > >
> > > > On Sun, Sep 01, 2024 at 05:38:39PM +0530, Radhey Shyam Pandey wrote:
> > > > > usb5744 supports SMBus Configuration and it may be configured via
> > > > > the SMBus slave interface during the hub start-up configuration stage.
> > > > >
> > > > > To program it 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 based on usb5744
> > > > > compatible check 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 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
> > > > >
> > > > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > > > ---
> > > > > Changes for v4:
> > > > > - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
> > > > >   introducing a dependency on I2C_CONFIG. This error is reported
> > > > >   by kernel test on v3 series and usb:usb-testing 20/25 branch.
> > > > >   https://lore.kernel.org/all/2024082503-uncoated-chaperone-
> > > > 7f70@gregkh
> > > > >
> > > > > Changes for v3:
> > > > > - Add comment for UDC suspend sequence.
> > > > > - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES
> > and
> > > > replace
> > > > >   it with literal + comment.
> > > > > - Move microchip defines to source file.
> > > > >
> > > > > Changes for v2:
> > > > > - Move power on reset delay to separate patch.
> > > > > - Switch to compatible based check for calling usb5755
> > > > >   onboard_dev_5744_i2c_init(). This is done to make
> > > > >   onboard_dev_5744_i2c_init() as static.
> > > > > - Fix subsystem "usb: misc: onboard_usb_dev:..."
> > > > > - Use #define for different register bits instead of magic values.
> > > > > - Use err_power_off label name.
> > > > > - Modified commit description to be in sync with v2 changes.
> > > > > ---
> > > > >  drivers/usb/misc/Kconfig           |  2 +-
> > > > >  drivers/usb/misc/onboard_usb_dev.c | 73
> > > > ++++++++++++++++++++++++++++++
> > > > >  2 files changed, 74 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > > > > index 50b86d531701..cb5e47d439ab 100644
> > > > > --- a/drivers/usb/misc/Kconfig
> > > > > +++ b/drivers/usb/misc/Kconfig
> > > > > @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
> > > > >
> > > > >  config USB_ONBOARD_DEV
> > > > >  	tristate "Onboard USB device support"
> > > > > -	depends on OF
> > > > > +	depends on OF && I2C
> > > >
> > > > This feels wrong.
> > > >
> > > > While a single device that this driver supports might need i2c, not
> > > > all of the devices do, so you have the potential to drag in a bunch
> > > > of code here for devices that do not have/need i2c at all, right?
> > > >
> > > > Any way to "split this out" into a smaller chunk?  Or better yet,
> > > > just detect at runtime if you need/want to call those i2c functions
> > > > (and they should have no-ops for when i2c is not enabled, right?)
> > >
> > > In onboard driver I am calling onboard_dev_5744_i2c_init() by
> > > detecting at runtime if "i2c-bus" phandle is present. But the problem
> > > is i2c_smbus_* APIs are declared and defined only for #if
> > > IS_ENABLED(CONFIG_I2C).
> > >
> > > Do you think we should implement no-ops for I2C smbus APIs (in
> > > linux/i2c.h)? OR a simpler alternative would be to add #if
> > > IS_ENABLED(CONFIG_I2C) check in the onboard_*_i2c_init() and return
> > > error code if CONFIG_I2C is not defined ?
> > > Did a grep on #if IS_ENABLED(CONFIG_I2C) and find couple of drivers
> > > using this approach.
> > 
> > I expect using IS_ENABLED(CONFIG_I2C) would cause issues when
> > USB_ONBOARD_DEV=y and CONFIG_I2C=m.
> > 
> > Another option could be adding an option USB_ONBOARD_DEV_USB5744 (or
> > similar) and make that dependent on CONFIG_I2C.
> 
> Yes, theoretically this can also be valid build configuration.
> 
> I assume your suggestion is something like below:
> 
> +config USB_ONBOARD_DEV_USB5744
> +       tristate "Onboard USB Microchip usb5744 hub with SMBus support"
> +       depends on USB_ONBOARD_DEV && I2C
> +       help
> +         Say Y here if you want to support onboard USB Microchip usb5744
> +         hub that requires SMBus initialization.
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index 560591e02d6a..6169780a18d9 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -311,7 +311,7 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
> 
>  static int onboard_dev_5744_i2c_init(struct i2c_client *client)
>  {
> -#if IS_ENABLED(CONFIG_I2C)
> +#if IS_ENABLED(CONFIG_USB_ONBOARD_DEV_USB5744)
> 
> 
> <*>   Onboard USB device support                                                               
>   <M>     Onboard USB Microchip usb5744 hub with SMBus support

Yes, something along these lines

> Here IS_ENABLED will report true either onboard usb5744 is module/in-built.
>
> So, we hit same issue as USB_ONBOARD_DEV=y and I2C=m.

Right, the problem is that USB_ONBOARD_DEV_USB5744 isn't an actual module,
otherwise Kconfig would prevent it from being builtin when I2C=m.

I think CONFIG_USB_ONBOARD_DEV_USB5744 should be boolean. It should only
be selectable when CONFIG_I2C=y or CONFIG_USB_ONBOARD_DEV=m and
CONFIG_I2C=m. That can be achieved with:

  depends on USB_ONBOARD_DEV && (I2C=y || I2C=USB_ONBOARD_DEV)

or

  depends on (USB_ONBOARD_DEV && I2C=y) || (USB_ONBOARD_DEV=m && I2C=m)

This doesn't look particularly elegant but should do the trick.

> FYI, as this series merged to -next based on our discussion

Ah, thanks for letting me know. Sorry for the delayed reply, I went
through an intercontinental relocation and it took some time to
get things settled.

> I will send another series fixing this particular build configuration and
> other coding style changes based on v4 review comments.

Thanks!

Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus programming support

[Matthias Kaehlcke]

El Sun, Sep 01, 2024 at 05:38:39PM GMT Radhey Shyam Pandey ha dit:

> usb5744 supports SMBus Configuration and it may be configured via the
> SMBus slave interface during the hub start-up configuration stage.
> 
> To program it 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 based on usb5744 compatible check 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 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
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes for v4:
> - Fix error: implicit declaration of function 'i2c_smbus_*' APIs by
>   introducing a dependency on I2C_CONFIG. This error is reported
>   by kernel test on v3 series and usb:usb-testing 20/25 branch.
>   https://lore.kernel.org/all/2024082503-uncoated-chaperone-7f70@gregkh
> 
> Changes for v3:
> - Add comment for UDC suspend sequence.
> - Drop USB5744_CREG_MEM_NBYTES and USB5744_CREG_NBYTES and replace
>   it with literal + comment.
> - Move microchip defines to source file.
> 
> Changes for v2:
> - Move power on reset delay to separate patch.
> - Switch to compatible based check for calling usb5755
>   onboard_dev_5744_i2c_init(). This is done to make
>   onboard_dev_5744_i2c_init() as static.
> - Fix subsystem "usb: misc: onboard_usb_dev:..."
> - Use #define for different register bits instead of magic values.
> - Use err_power_off label name.
> - Modified commit description to be in sync with v2 changes.
> ---
>  drivers/usb/misc/Kconfig           |  2 +-
>  drivers/usb/misc/onboard_usb_dev.c | 73 ++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 50b86d531701..cb5e47d439ab 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -318,7 +318,7 @@ config BRCM_USB_PINMAP
>  
>  config USB_ONBOARD_DEV
>  	tristate "Onboard USB device support"
> -	depends on OF
> +	depends on OF && I2C
>  	help
>  	  Say Y here if you want to support discrete onboard USB devices
>  	  that don't require an additional control bus for initialization,
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index da27c48fc11d..247600015bca 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -11,6 +11,7 @@
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/init.h>
> +#include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -29,6 +30,17 @@
>  
>  #include "onboard_usb_dev.h"
>  
> +/* USB5744 register offset and mask */
> +#define USB5744_CMD_ATTACH			0xAA
> +#define USB5744_CMD_ATTACH_LSB			0x56
> +#define USB5744_CMD_CREG_ACCESS			0x99
> +#define USB5744_CMD_CREG_ACCESS_LSB		0x37
> +#define USB5744_CREG_MEM_ADDR			0x00
> +#define USB5744_CREG_WRITE			0x00
> +#define USB5744_CREG_RUNTIMEFLAGS2		0x41
> +#define USB5744_CREG_RUNTIMEFLAGS2_LSB		0x1D
> +#define USB5744_CREG_BYPASS_UDC_SUSPEND		BIT(3)
> +
>  static void onboard_dev_attach_usb_driver(struct work_struct *work);
>  
>  static struct usb_device_driver onboard_dev_usbdev_driver;
> @@ -297,10 +309,46 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
>  		pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
>  }
>  
> +static int onboard_dev_5744_i2c_init(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	/*
> +	 * Set BYPASS_UDC_SUSPEND bit to ensure MCU is always enabled
> +	 * and ready to respond to SMBus runtime commands.
> +	 * The command writes 5 bytes to memory and single data byte in

nit: s/in/to the/

> +	 * configuration register.
> +	 */
> +	char wr_buf[7] = {USB5744_CREG_MEM_ADDR, 5,
> +			  USB5744_CREG_WRITE, 1,
> +			  USB5744_CREG_RUNTIMEFLAGS2,
> +			  USB5744_CREG_RUNTIMEFLAGS2_LSB,
> +			  USB5744_CREG_BYPASS_UDC_SUSPEND};
> +
> +	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, USB5744_CMD_CREG_ACCESS,
> +					USB5744_CMD_CREG_ACCESS_LSB);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n");

nit: s/Command/command/

Uppercase for the register name is ok, but command isn't a name.

> +
> +	/* Send SMBus command to boot hub. */

nit: from the code it's obvious that an SMBus command is sent,
no need to spell that out. I suggest to change the comment to
"Attach/boot the hub".

> +	ret = i2c_smbus_write_word_data(client, USB5744_CMD_ATTACH,
> +					USB5744_CMD_ATTACH_LSB);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n");

nit: s/.../USB Attach command failed/

> +
> +	return ret;

    return 0;

> +}