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

[PATCH v2 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 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/onboard_usb_dev.c | 57 ++++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_dev.h | 14 ++++++++
 2 files changed, 71 insertions(+)


base-commit: 51835949dda3783d4639cfa74ce13a3c9829de00
-- 
2.34.1

[PATCH v2 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 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 f2bcc1a8b95f..94d5424841fd 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..82c76a0b3346 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 pulse width 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 v2 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 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

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
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/onboard_usb_dev.c | 56 ++++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_dev.h | 12 +++++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index 94d5424841fd..4f3845f35ac4 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>
@@ -297,10 +298,40 @@ 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;
+
+	char wr_buf[7] = {USB5744_CREG_MEM_ADDR, USB5744_CREG_MEM_NBYTES,
+			  USB5744_CREG_WRITE, USB5744_CREG_NBYTES,
+			  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_CREG_ACCESS,
+					USB5744_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 +371,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 +403,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)
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index 82c76a0b3346..5744c589b90f 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -8,6 +8,18 @@
 
 #define MAX_SUPPLIES 2
 
+#define USB5744_CMD_ATTACH			0xAA
+#define USB5744_CMD_ATTACH_LSB			0x56
+#define USB5744_CREG_ACCESS			0x99
+#define USB5744_CREG_ACCESS_LSB			0x37
+#define USB5744_CREG_MEM_ADDR			0x00
+#define USB5744_CREG_MEM_NBYTES			0x05
+#define USB5744_CREG_WRITE			0x00
+#define USB5744_CREG_NBYTES			0x01
+#define USB5744_CREG_RUNTIMEFLAGS2		0x41
+#define USB5744_CREG_RUNTIMEFLAGS2_LSB		0x1D
+#define USB5744_CREG_BYPASS_UDC_SUSPEND		BIT(3)
+
 struct onboard_dev_pdata {
 	unsigned long reset_us;		/* reset pulse width in us */
 	unsigned long power_on_delay_us; /* power on pulse width in us */
-- 
2.34.1

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

[Matthias Kaehlcke]

Hi,

On Thu, Jul 18, 2024 at 12:53:43AM +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 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
> 
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> 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/onboard_usb_dev.c | 56 ++++++++++++++++++++++++++++++
>  drivers/usb/misc/onboard_usb_dev.h | 12 +++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index 94d5424841fd..4f3845f35ac4 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>
> @@ -297,10 +298,40 @@ 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;
> +

Please explain what the purpose of this sequence is. It is described
in the commit message, however that is not directly available to a
reader of the code.

> +	char wr_buf[7] = {USB5744_CREG_MEM_ADDR, USB5744_CREG_MEM_NBYTES,

IIUC USB5744_CREG_MEM_NBYTES is the number of registers written to the
configuration register by this specific command, it could be different
for other commands. If that is correct then I think this shouldn't be
a constant but a literal plus a comment.

> +			  USB5744_CREG_WRITE, USB5744_CREG_NBYTES,

Similar as above, USB5744_CREG_NBYTES should be a literal + comment, or
alternatively something like USB5744_CREG_SIZE, assuming the width of
all registers is one byte.

It would also be an option to add a wrapper onboard_dev_5744_write_creg(),
that encapsulates writing a configuration register. Not strictly need
since currently only one register is written.

> +			  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_CREG_ACCESS,
> +					USB5744_CREG_ACCESS_LSB);

This is a command ("Configuration Register Access command", this should be
reflected in the name, as for USB5744_CMD_ATTACH.

> +	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;

	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);
> @@ -340,6 +371,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 +403,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)
> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
> index 82c76a0b3346..5744c589b90f 100644
> --- a/drivers/usb/misc/onboard_usb_dev.h
> +++ b/drivers/usb/misc/onboard_usb_dev.h
> @@ -8,6 +8,18 @@
>  
>  #define MAX_SUPPLIES 2
>  
> +#define USB5744_CMD_ATTACH			0xAA
> +#define USB5744_CMD_ATTACH_LSB			0x56
> +#define USB5744_CREG_ACCESS			0x99
> +#define USB5744_CREG_ACCESS_LSB			0x37
> +#define USB5744_CREG_MEM_ADDR			0x00
> +#define USB5744_CREG_MEM_NBYTES			0x05
> +#define USB5744_CREG_WRITE			0x00
> +#define USB5744_CREG_NBYTES			0x01
> +#define USB5744_CREG_RUNTIMEFLAGS2		0x41
> +#define USB5744_CREG_RUNTIMEFLAGS2_LSB		0x1D
> +#define USB5744_CREG_BYPASS_UDC_SUSPEND		BIT(3)
> +

These defines are specific to the Microchip 5744 hub, there is no need
for them to be defined in the include of the onboard_usb_dev driver.
Please move them to the .c file

Thanks

Matthias

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

[Matthias Kaehlcke]

On Thu, Jul 18, 2024 at 12:53:42AM +0530, Radhey Shyam Pandey wrote:
> 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 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 f2bcc1a8b95f..94d5424841fd 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..82c76a0b3346 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 pulse width in us */

nit: it isn't really a pulse width, just a simple delay.

>  	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
>