Re: [PATCH v2 3/6] usb: chipidea: imx: add HSIO Block Control wakeup setting

Xu Yang
2025-02-27T09:45:14Z

On Thu, Feb 27, 2025 at 02:32:35PM +0800, Xu Yang wrote:
> On Thu, Feb 27, 2025 at 11:15:40AM +0800, Peter Chen wrote:
> > On 25-02-25 13:39:52, Xu Yang wrote:
> > > On i.MX95 platform, USB wakeup setting is controlled by HSIO Block
> > > Control:
> > >
> > > HSIO Block Control Overview:
> > > - The HSIO block control include configuration and status registers that
> > > provide miscellaneous top-level controls for clocking, beat limiter
> > > enables, wakeup signal enables and interrupt status for the PCIe and USB
> > > interfaces.
> > >
> > > The wakeup function of HSIO blkctl is basically same as non-core, except
> > > improvements about power lost cases. This will add the wakeup setting for
> > > HSIO blkctl on i.MX95. It will firstly ioremap hsio blkctl memory, then do
> > > wakeup setting as needs.
> > >
> > > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > > Reviewed-by: Jun Li <jun.li@nxp.com>
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > >
> > > ---
> > > Changes in v2:
> > > - add Rb tag
> > > ---
> > > drivers/usb/chipidea/usbmisc_imx.c | 107 +++++++++++++++++++++++++++++
> > > 1 file changed, 107 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 1394881fde5f..f933fc70be66 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -139,6 +139,22 @@
> > > #define MX6_USB_OTG_WAKEUP_BITS (MX6_BM_WAKEUP_ENABLE | MX6_BM_VBUS_WAKEUP | \
> > > MX6_BM_ID_WAKEUP | MX6SX_BM_DPDM_WAKEUP_EN)
> > >
> > > +/*
> > > + * HSIO Block Control Register
> > > + */
> > > +
> > > +#define BLKCTL_USB_WAKEUP_CTRL 0x0
> > > +#define BLKCTL_OTG_WAKE_ENABLE BIT(31)
> > > +#define BLKCTL_OTG_VBUS_SESSVALID BIT(4)
> > > +#define BLKCTL_OTG_ID_WAKEUP_EN BIT(2)
> > > +#define BLKCTL_OTG_VBUS_WAKEUP_EN BIT(1)
> > > +#define BLKCTL_OTG_DPDM_WAKEUP_EN BIT(0)
> > > +
> > > +#define BLKCTL_WAKEUP_SOURCE (BLKCTL_OTG_WAKE_ENABLE | \
> > > + BLKCTL_OTG_ID_WAKEUP_EN | \
> > > + BLKCTL_OTG_VBUS_WAKEUP_EN | \
> > > + BLKCTL_OTG_DPDM_WAKEUP_EN)
> > > +
> > > struct usbmisc_ops {
> > > /* It's called once when probe a usb device */
> > > int (*init)(struct imx_usbmisc_data *data);
> > > @@ -159,6 +175,7 @@ struct usbmisc_ops {
> > >
> > > struct imx_usbmisc {
> > > void __iomem *base;
> > > + void __iomem *blkctl;
> > > spinlock_t lock;
> > > const struct usbmisc_ops *ops;
> > > };
> > > @@ -1016,6 +1033,76 @@ static int usbmisc_imx6sx_power_lost_check(struct imx_usbmisc_data *data)
> > > return 0;
> > > }
> > >
> > > +static u32 usbmisc_blkctl_wakeup_setting(struct imx_usbmisc_data *data)
> > > +{
> > > + u32 wakeup_setting = BLKCTL_WAKEUP_SOURCE;
> > > +
> > > + if (data->ext_id || data->available_role != USB_DR_MODE_OTG)
> > > + wakeup_setting &= ~BLKCTL_OTG_ID_WAKEUP_EN;
> > > +
> > > + if (data->ext_vbus || data->available_role == USB_DR_MODE_HOST)
> > > + wakeup_setting &= ~BLKCTL_OTG_VBUS_WAKEUP_EN;
> > > +
> > > + /* Select session valid as VBUS wakeup source */
> > > + wakeup_setting |= BLKCTL_OTG_VBUS_SESSVALID;
> > > +
> > > + return wakeup_setting;
> > > +}
> > > +
> > > +static int usbmisc_imx95_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
> > > +{
> > > + struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> > > + unsigned long flags;
> > > + u32 val;
> > > +
> > > + spin_lock_irqsave(&usbmisc->lock, flags);
> > > + val = readl(usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
> > > + val &= ~BLKCTL_WAKEUP_SOURCE;
> > > +
> > > + if (enabled)
> > > + val |= usbmisc_blkctl_wakeup_setting(data);
> > > +
> > > + writel(val, usbmisc->blkctl + BLKCTL_USB_WAKEUP_CTRL);
> > > + spin_unlock_irqrestore(&usbmisc->lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int usbmisc_imx95_init(struct imx_usbmisc_data *data)
> > > +{
> > > + struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> > > + unsigned long flags;
> > > + u32 reg;
> > > +
> > > + if (data->index >= 1)
> > > + return -EINVAL;
> > > +
> > > + spin_lock_irqsave(&usbmisc->lock, flags);
> > > + reg = readl(usbmisc->base);
> > > +
> > > + if (data->disable_oc) {
> > > + reg |= MX6_BM_OVER_CUR_DIS;
> > > + } else {
> > > + reg &= ~MX6_BM_OVER_CUR_DIS;
> > > +
> > > + if (data->oc_pol_configured && data->oc_pol_active_low)
> > > + reg |= MX6_BM_OVER_CUR_POLARITY;
> > > + else if (data->oc_pol_configured)
> > > + reg &= ~MX6_BM_OVER_CUR_POLARITY;
> > > + }
> > > +
> > > + if (data->pwr_pol == 1)
> > > + reg |= MX6_BM_PWR_POLARITY;
> > > +
> > > + writel(reg, usbmisc->base);
> > > + spin_unlock_irqrestore(&usbmisc->lock, flags);
> > > +
> > > + /* use HSIO blkctl wakeup as source, disable usbmisc setting*/
> > > + usbmisc_imx7d_set_wakeup(data, false);
> > > +
> > > + return 0;
> > > +}
> >
> > Above code has duplicated with some imx7d and imx7ulp init code,
> > Is it possible abstract some common code for all these three platforms?
>
> Sure. Thanks for your suggestion. I'll do it.

I just double-checked imx95 is totally compatible with imx7d. Therefore,
usbmisc_imx95_init() is not needed anymore. I'll use usbmisc_imx7d_init()
for imx95 too.

For duplicated code on some platform's init() function, I may improve them
later.

Thanks,
Xu Yang