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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Am Freitag, 28. Februar 2025, 10:16:49 CET schrieb Xu Yang:
> On Fri, Feb 28, 2025 at 08:22:19AM +0100, Alexander Stein wrote:
> > Hi,
> > 
> > Am Freitag, 28. Februar 2025, 03:33:29 CET schrieb Xu Yang:
> > > On Thu, Feb 27, 2025 at 04:12:54PM +0100, Alexander Stein wrote:
> > > > Hi,
> > > > 
> > > > Am Donnerstag, 27. Februar 2025, 10:53:46 CET schrieb Xu Yang:
> > > > > 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 v3:
> > > > >  - remove usbmisc_imx95_init(), use usbmisc_imx7d_init()
> > > > > Changes in v2:
> > > > >  - add Rb tag
> > > > > ---
> > > > >  drivers/usb/chipidea/usbmisc_imx.c | 72 ++++++++++++++++++++++++++++++
> > > > >  1 file changed, 72 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> > > > > index 1394881fde5f..8c30908c11ed 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,41 @@ 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);
> > > > 
> > > > usbmisc->blkctl is NULL if DT does not provide a 2nd IORESOURCE_MEM.
> > > 
> > > Good catch. Thanks!
> > > 
> > > It may return an errno if usbmisc->blkctl is NULL.
> > > 
> > > > 
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static const struct usbmisc_ops imx25_usbmisc_ops = {
> > > > >  	.init = usbmisc_imx25_init,
> > > > >  	.post = usbmisc_imx25_post,
> > > > > @@ -1068,6 +1120,14 @@ static const struct usbmisc_ops imx7ulp_usbmisc_ops = {
> > > > >  	.power_lost_check = usbmisc_imx7d_power_lost_check,
> > > > >  };
> > > > >  
> > > > > +static const struct usbmisc_ops imx95_usbmisc_ops = {
> > > > > +	.init = usbmisc_imx7d_init,
> > > > > +	.set_wakeup = usbmisc_imx95_set_wakeup,
> > > > > +	.charger_detection = imx7d_charger_detection,
> > > > > +	.power_lost_check = usbmisc_imx7d_power_lost_check,
> > > > > +	.vbus_comparator_on = usbmisc_imx7d_vbus_comparator_on,
> > > > > +};
> > > > > +
> > > > >  static inline bool is_imx53_usbmisc(struct imx_usbmisc_data *data)
> > > > >  {
> > > > >  	struct imx_usbmisc *usbmisc = dev_get_drvdata(data->dev);
> > > > > @@ -1289,6 +1349,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
> > > > >  		.compatible = "fsl,imx8ulp-usbmisc",
> > > > >  		.data = &imx7ulp_usbmisc_ops,
> > > > >  	},
> > > > > +	{
> > > > > +		.compatible = "fsl,imx95-usbmisc",
> > > > > +		.data = &imx95_usbmisc_ops,
> > > > > +	},
> > > > >  	{ /* sentinel */ }
> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> > > > > @@ -1296,6 +1360,7 @@ MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> > > > >  static int usbmisc_imx_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  	struct imx_usbmisc *data;
> > > > > +	struct resource *res;
> > > > >  
> > > > >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > > > >  	if (!data)
> > > > > @@ -1307,6 +1372,13 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
> > > > >  	if (IS_ERR(data->base))
> > > > >  		return PTR_ERR(data->base);
> > > > >  
> > > > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > > +	if (res) {
> > > > > +		data->blkctl = devm_ioremap_resource(&pdev->dev, res);
> > > > > +		if (IS_ERR(data->blkctl))
> > > > > +			return PTR_ERR(data->blkctl);
> > > > > +	}
> > > > > +
> > > > 
> > > > Any chance to ensure imx95 has actually data->blkctl?
> > > 
> > > I think let usbmisc_imx95_set_wakeup() return an error should be enough.
> > 
> > Better fail early rather than later.
> > 
> > > Some errors will be printed for user. Also dt-bindings has restriction for
> > > imx95.
> > 
> > That's true, but don't assume the DT is always setup correctly.
> 
> Sure, I thought about this question again.
> 
> If 2nd IORESOURCE_MEM is not provided, the USB controller could still function
> normally in most scenarios except when USB wakeup is required. So if the probe()
> return a failure just caused by improper wakeup configurations seems not that
> necessary. In this case, ensuring the kernel does not panic should suffice.
> 
> Therfore, I prefer to skip wakeup setting if blkctl is NULL now.
> Do you have any ideas?

That also seems reasonable. So in case 2nd IORESOURCE_MEM is missing (only)
for imx95 there should be a warning that wakeup is not possible/supported.
But the driver itself should work without issue.

Best regards
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/