RE: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support

From: Pandey, Radhey Shyam
Date: Mon Jun 10 2024 - 14:53:52 EST

Next message: Andrew Lunn: "Re: [PATCH 1/1] arm: rust: Enable Rust support for ARMv7"
Previous message: Keith Busch: "Re: Kernel OOPS while creating a NVMe Namespace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> Sent: Thursday, June 6, 2024 11:54 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; javier.carrasco@xxxxxxxxxxxxxx;
> benjamin.bara@xxxxxxxxxxx; m.felsch@xxxxxxxxxxxxxx;
> jbrunet@xxxxxxxxxxxx; frieder.schrempf@xxxxxxxxxx;
> stefan.eichenberger@xxxxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > ---
> > 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
> >

Next message: Andrew Lunn: "Re: [PATCH 1/1] arm: rust: Enable Rust support for ARMv7"
Previous message: Keith Busch: "Re: Kernel OOPS while creating a NVMe Namespace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]