| Message ID | 20250227095348.837223-5-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add USB2.0 support for i.MX95-19x19 EVK board | expand |
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. > + > + 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? Best regards Alexander > data->ops = of_device_get_match_data(&pdev->dev); > platform_set_drvdata(pdev, data); > >
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. Some errors will be printed for user. Also dt-bindings has restriction for imx95. Thanks, Xu Yang
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. Best regards Alexander
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? Thanks, Xu Yang
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
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); + + 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); + } + data->ops = of_device_get_match_data(&pdev->dev); platform_set_drvdata(pdev, data);