Message-ID: <5ibu7s3ycks6wgchkjvvmf22w3rpx6ngyuiw6aoj5joj672piz@sfardf5qf4a5>
Date: Thu, 19 Sep 2024 10:12:26 +0000
From: Matthias Kaehlcke <matthias@...hlcke.net>
To: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>
Cc: Greg KH <gregkh@...uxfoundation.org>, 
	"mka@...omium.org" <mka@...omium.org>, "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>, 
	"wentong.wu@...el.com" <wentong.wu@...el.com>, 
	"javier.carrasco@...fvision.net" <javier.carrasco@...fvision.net>, 
	"stefan.eichenberger@...adex.com" <stefan.eichenberger@...adex.com>, "francesco.dolcini@...adex.com" <francesco.dolcini@...adex.com>, 
	"jbrunet@...libre.com" <jbrunet@...libre.com>, "macpaul.lin@...iatek.com" <macpaul.lin@...iatek.com>, 
	"frieder.schrempf@...tron.de" <frieder.schrempf@...tron.de>, "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "git (AMD-Xilinx)" <git@....com>
Subject: Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744
 SMBus programming support

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

> > -----Original Message-----
> > From: Matthias Kaehlcke <matthias@...hlcke.net>
> > Sent: Wednesday, September 18, 2024 10:37 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> > Cc: Greg KH <gregkh@...uxfoundation.org>; mka@...omium.org;
> > sakari.ailus@...ux.intel.com; wentong.wu@...el.com;
> > javier.carrasco@...fvision.net; stefan.eichenberger@...adex.com;
> > francesco.dolcini@...adex.com; jbrunet@...libre.com; macpaul.lin@...iatek.com;
> > frieder.schrempf@...tron.de; linux-usb@...r.kernel.org; linux-
> > kernel@...r.kernel.org; git (AMD-Xilinx) <git@....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@...uxfoundation.org>
> > > > Sent: Tuesday, September 3, 2024 12:10 PM
> > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> > > > Cc: mka@...omium.org; sakari.ailus@...ux.intel.com;
> > > > wentong.wu@...el.com; javier.carrasco@...fvision.net;
> > > > stefan.eichenberger@...adex.com; francesco.dolcini@...adex.com;
> > > > jbrunet@...libre.com; macpaul.lin@...iatek.com;
> > > > frieder.schrempf@...tron.de; linux-usb@...r.kernel.org; linux-
> > > > kernel@...r.kernel.org; git (AMD-Xilinx) <git@....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@....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@...gkh
> > > > >
> > > > > 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!

Powered by blists - more mailing lists