On 20-08-27 16:20:15, Felipe Balbi wrote:
> Peter Chen <peter.chen@xxxxxxx> writes:
>
> > Add imx glue layer runtime pm implementation, and the runtime
> > pm is default off.
> >
> > Reviewed-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> > ---
> > drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
> > 1 file changed, 192 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
> > index aba988e71958..a413df26e948 100644
> > --- a/drivers/usb/cdns3/cdns3-imx.c
> > +++ b/drivers/usb/cdns3/cdns3-imx.c
> > @@ -15,6 +15,8 @@
> > #include <linux/io.h>
> > #include <linux/of_platform.h>
> > #include <linux/iopoll.h>
> > +#include <linux/pm_runtime.h>
> > +#include "core.h"
> >
> > #define USB3_CORE_CTRL1 0x00
> > #define USB3_CORE_CTRL2 0x04
> > @@ -32,7 +34,7 @@
> > /* Register bits definition */
> >
> > /* USB3_CORE_CTRL1 */
> > -#define SW_RESET_MASK (0x3f << 26)
> > +#define SW_RESET_MASK GENMASK(31, 26)
>
> why is this part of adding imx runtime pm?
Sorry, will delete this improvement
>
> > @@ -44,17 +46,17 @@
> > #define OC_DISABLE BIT(9)
> > #define MDCTRL_CLK_SEL BIT(7)
> > #define MODE_STRAP_MASK (0x7)
> > -#define DEV_MODE (1 << 2)
> > -#define HOST_MODE (1 << 1)
> > -#define OTG_MODE (1 << 0)
> > +#define DEV_MODE BIT(2)
> > +#define HOST_MODE BIT(1)
> > +#define OTG_MODE BIT(0)
>
> and these?
Sorry, will delete this improvement
>
> >
> > /* USB3_INT_REG */
> > #define CLK_125_REQ BIT(29)
> > #define LPM_CLK_REQ BIT(28)
> > #define DEVU3_WAEKUP_EN BIT(14)
> > #define OTG_WAKEUP_EN BIT(12)
> > -#define DEV_INT_EN (3 << 8) /* DEV INT b9:8 */
> > -#define HOST_INT1_EN (1 << 0) /* HOST INT b7:0 */
> > +#define DEV_INT_EN GENMASK(9, 8) /* DEV INT b9:8 */
> > +#define HOST_INT1_EN BIT(0) /* HOST INT b7:0 */
>
Sorry, will delete this improvement
>
> > @@ -62,15 +64,34 @@
> > #define HOST_POWER_ON_READY BIT(12)
> >
> > /* USB3_SSPHY_STATUS */
> > -#define CLK_VALID_MASK (0x3f << 26)
> > -#define CLK_VALID_COMPARE_BITS (0xf << 28)
> > -#define PHY_REFCLK_REQ (1 << 0)
> > +#define CLK_VALID_MASK GENMASK(31, 26)
> > +#define CLK_VALID_COMPARE_BITS GENMASK(31, 28)
> > +#define PHY_REFCLK_REQ BIT(0)
>
Sorry, will delete this improvement
>
> > +/* OTG registers definition */
> > +#define OTGSTS 0x4
> > +/* OTGSTS */
> > +#define OTG_NRDY BIT(11)
> > +
> > +/* xHCI registers definition */
> > +#define XECP_PM_PMCSR 0x8018
> > +#define XECP_AUX_CTRL_REG1 0x8120
> > +
> > +/* Register bits definition */
> > +/* XECP_AUX_CTRL_REG1 */
> > +#define CFG_RXDET_P3_EN BIT(15)
> > +
> > +/* XECP_PM_PMCSR */
> > +#define PS_MASK GENMASK(1, 0)
> > +#define PS_D0 0
> > +#define PS_D1 1
>
> I think only these are part of $subject
Yes, you are right.
>
> > struct cdns_imx {
> > struct device *dev;
> > void __iomem *noncore;
> > struct clk_bulk_data *clks;
> > int num_clks;
> > + struct platform_device *cdns3_pdev;
> > };
> >
> > static inline u32 cdns_imx_readl(struct cdns_imx *data, u32 offset)
> > @@ -126,6 +147,20 @@ static int cdns_imx_noncore_init(struct cdns_imx *data)
> > return ret;
> > }
> >
> > +static int cdns_imx_platform_suspend(struct device *dev,
> > + bool suspend, bool wakeup);
> > +static struct cdns3_platform_data cdns_imx_pdata = {
>
> make it const?
Will change
>
> > + .platform_suspend = cdns_imx_platform_suspend,
> > +};
> > +
> > +static struct of_dev_auxdata cdns_imx_auxdata[] = {
>
> const?
Will change
>
> > + {
> > + .compatible = "cdns,usb3",
> > + .platform_data = &cdns_imx_pdata,
> > + },
>
> bad indentation
Will change
>
> > @@ -194,6 +233,147 @@ static int cdns_imx_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM
> > +static void cdns3_set_wakeup(struct cdns_imx *data, bool enable)
> > +{
> > + u32 value;
> > +
> > + value = cdns_imx_readl(data, USB3_INT_REG);
> > + if (enable)
> > + value |= OTG_WAKEUP_EN | DEVU3_WAEKUP_EN;
> > + else
> > + value &= ~(OTG_WAKEUP_EN | DEVU3_WAEKUP_EN);
> > +
> > + cdns_imx_writel(data, USB3_INT_REG, value);
> > +}
> > +
> > +static int cdns_imx_platform_suspend(struct device *dev,
> > + bool suspend, bool wakeup)
> > +{
> > + struct cdns3 *cdns = dev_get_drvdata(dev);
> > + struct device *parent = dev->parent;
> > + struct cdns_imx *data = dev_get_drvdata(parent);
> > + void __iomem *otg_regs = (void __iomem *)(cdns->otg_regs);
>
> why the cast?
You mean __iomem?
The kernel test robot reports warning:
https://www.spinics.net/lists/linux-usb/msg199770.html
> > +
> > +static int cdns_imx_resume(struct device *dev)
> > +{
> > + struct cdns_imx *data = dev_get_drvdata(dev);
> > +
> > + return clk_bulk_prepare_enable(data->num_clks, data->clks);
>
> do you need to prepare and unprepare for every suspend/resume? Isn't
> enable/disable enough?
Since runtime suspend/resume is not often, only at below situations,
we want the lowest power consumption:
- CDNS3 device is connected from the host
- There is no device on the CDNS3 host
- The device which supports runtime-suspend is suspended at
CDNS3 host.
>
> > +static const struct dev_pm_ops cdns_imx_pm_ops = {
> > + SET_RUNTIME_PM_OPS(cdns_imx_suspend, cdns_imx_resume, NULL)
>
> could the same be used for system sleep?
The suspend is the same, but resume is not since the controller may lost
its power during system sleep, it needs to judge that power status and
restore the controller. I will submit these system PM supports later.
--
Thanks,
Peter Chen
