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

Matthias Kaehlcke
2024-09-18T18:19:00Z

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;

> +}