| Message ID | 1722440548-107682-2-git-send-email-radhey.shyam.pandey@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_usb_dev: add Microchip usb5744 SMBus support | expand |
On Wed, Jul 31, 2024 at 09:12:27PM +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> This constant addition of "platform data" seems to be duplicating what we did before with device tree, right? Why can't this information be there instead? thanks, greg k-h
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Wednesday, August 7, 2024 4:13 PM > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Cc: mka@chromium.org; javier.carrasco@wolfvision.net; > macpaul.lin@mediatek.com; jbrunet@baylibre.com; > stefan.eichenberger@toradex.com; linux-usb@vger.kernel.org; linux- > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH v3 1/2] usb: misc: onboard_dev: extend platform data to > add power on delay field > > On Wed, Jul 31, 2024 at 09:12:27PM +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> > > This constant addition of "platform data" seems to be duplicating what > we did before with device tree, right? Why can't this information be > there instead? > Yes, similar to existing 'reset_us' field i extended platform data to add 'power_on_delay_us' field. I can move it to device tree but think there could be question on why we need to add a new binding just for this property and not do it compatible based platform data considering this power_on_delay_us delay is fixed for USB5744? Thanks, Radhey
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,
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 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(+)