Re: [PATCH 2/3] phy: Add driver for ST-Ericsson AB8500 USB PHY

[Stephan Gerhold <stephan@gerhold.net>]

On Tue, Jul 06, 2021 at 10:10:54AM +0900, Jaehoon Chung wrote:
> Hi Stephan,
> 
> On 7/6/21 1:28 AM, Stephan Gerhold wrote:
> > The AB8500 PMIC contains an USB PHY that needs to be set up in
> > device or host mode to make USB work properly. Add a simple driver
> > for the generic PHY uclass that allows enabling it.
> > 
> > The if (CONFIG_IS_ENABLED(USB_MUSB_HOST)) might be a bit strange.
> > The USB PHY must be configured in either host or device mode and
> > somehow the USB PHY driver must be made aware of the mode.
> > 
> > Actually, the MUSB driver used together with this PHY does not
> > support dynamic selection of host/device mode in U-Boot at the moment.
> > Therefore, one very simple approach that works fine is to select
> > the mode to configure at compile time. When the MUSB driver is
> > configured in host mode the PHY is configured in host mode, and
> > similarly when the MUSB driver is configured in device/gadget mode.
> > 
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > 
> > Better suggestions to make the USB PHY driver aware of the mode
> > are very welcome. :) I'm not sure it's worth it though, the compile
> > time selection is not ideal but it does the job just fine for now.
> > ---
> >  drivers/phy/Kconfig          |  6 ++++
> >  drivers/phy/Makefile         |  1 +
> >  drivers/phy/phy-ab8500-usb.c | 54 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> >  create mode 100644 drivers/phy/phy-ab8500-usb.c
> > 
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 008186a10d..92c74b9d0b 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -64,6 +64,12 @@ config MIPI_DPHY_HELPERS
> >  	help
> >  	  Provides a number of helpers a core functions for MIPI D-PHY drivers.
> >  
> > +config AB8500_USB_PHY
> > +	bool "AB8500 USB PHY Driver"
> > +	depends on PHY && PMIC_AB8500
> > +	help
> > +	  Support for the USB OTG PHY in ST-Ericsson AB8500.
> > +
> >  config BCM6318_USBH_PHY
> >  	bool "BCM6318 USBH PHY support"
> >  	depends on PHY && ARCH_BMIPS
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 3c4a673a83..bf03d05d9b 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -6,6 +6,7 @@
> >  obj-$(CONFIG_$(SPL_)PHY) += phy-uclass.o
> >  obj-$(CONFIG_$(SPL_)NOP_PHY) += nop-phy.o
> >  obj-$(CONFIG_MIPI_DPHY_HELPERS) += phy-core-mipi-dphy.o
> > +obj-$(CONFIG_AB8500_USB_PHY) += phy-ab8500-usb.o
> >  obj-$(CONFIG_BCM6318_USBH_PHY) += bcm6318-usbh-phy.o
> >  obj-$(CONFIG_BCM6348_USBH_PHY) += bcm6348-usbh-phy.o
> >  obj-$(CONFIG_BCM6358_USBH_PHY) += bcm6358-usbh-phy.o
> > diff --git a/drivers/phy/phy-ab8500-usb.c b/drivers/phy/phy-ab8500-usb.c
> > new file mode 100644
> > index 0000000000..4ecb03a9cb
> > --- /dev/null
> > +++ b/drivers/phy/phy-ab8500-usb.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (C) 2019 Stephan Gerhold */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <generic-phy.h>
> > +#include <linux/bitops.h>
> > +#include <power/pmic.h>
> > +#include <power/ab8500.h>
> > +
> > +#define AB8500_USB_PHY_CTRL_REG			AB8500_USB(0x8A)
> > +#define AB8500_BIT_PHY_CTRL_HOST_EN		BIT(0)
> > +#define AB8500_BIT_PHY_CTRL_DEVICE_EN		BIT(1)
> > +#define AB8500_USB_PHY_CTRL_MASK		(AB8500_BIT_PHY_CTRL_HOST_EN |\
> > +						 AB8500_BIT_PHY_CTRL_DEVICE_EN)
> > +
> > +static int ab8500_usb_phy_power_on(struct phy *phy)
> > +{
> > +	struct udevice *dev = phy->dev;
> > +	uint set;
> > +
> 
> unit set = AB8500_BIT_PHY_CTRL_DEVICE_EN;
> 
> if (CONFIG_IS_ENABLED(USB)MUSB_HOST))
> 	set = ...;
> 
> how about that?
> 

Both looks fine to me, I can change it in v2.

> > +	if (CONFIG_IS_ENABLED(USB_MUSB_HOST))
> > +		set = AB8500_BIT_PHY_CTRL_HOST_EN;
> > +	else
> > +		set = AB8500_BIT_PHY_CTRL_DEVICE_EN;
> > +
> > +	return pmic_clrsetbits(dev->parent, AB8500_USB_PHY_CTRL_REG,
> > +			       AB8500_USB_PHY_CTRL_MASK, set);
> > +}

Thanks for the review!
Stephan