| Message ID | 20210506010440.7016-1-peng.fan@oss.nxp.com |
|---|---|
| Headers | show |
| Series | soc: imx: gpcv2: support i.MX8MM | expand |
On 06.05.21 03:04, Peng Fan (OSS) wrote: > From: Lucas Stach <l.stach@pengutronix.de> > > Use clk_bulk API to simplify the code a bit. Also add some error > checking to the clk_prepare_enable calls. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/soc/imx/gpcv2.c | 60 +++++++++-------------------------------- > 1 file changed, 12 insertions(+), 48 deletions(-) > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > index 552d3e6bee52..4222b6e87e7c 100644 > --- a/drivers/soc/imx/gpcv2.c > +++ b/drivers/soc/imx/gpcv2.c > @@ -100,13 +100,11 @@ > > #define GPC_PGC_CTRL_PCR BIT(0) > > -#define GPC_CLK_MAX 6 > - > struct imx_pgc_domain { > struct generic_pm_domain genpd; > struct regmap *regmap; > struct regulator *regulator; > - struct clk *clk[GPC_CLK_MAX]; > + struct clk_bulk_data *clks; > int num_clks; > > unsigned int pgc; > @@ -149,8 +147,12 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > } > > /* Enable reset clocks for all devices in the domain */ > - for (i = 0; i < domain->num_clks; i++) > - clk_prepare_enable(domain->clk[i]); > + ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks); > + if (ret) { > + dev_err(domain->dev, "failed to enable reset clocks\n"); > + regulator_disable(domain->regulator); > + return ret; > + } > > if (enable_power_control) > regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc), > @@ -187,8 +189,7 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > GPC_PGC_CTRL_PCR, 0); > > /* Disable reset clocks for all devices in the domain */ > - for (i = 0; i < domain->num_clks; i++) > - clk_disable_unprepare(domain->clk[i]); > + clk_bulk_disable_unprepare(domain->num_clks, domain->clks); > > if (has_regulator && !on) { > int err; > @@ -438,41 +439,6 @@ static const struct imx_pgc_domain_data imx8m_pgc_domain_data = { > .reg_access_table = &imx8m_access_table, > }; > > -static int imx_pgc_get_clocks(struct imx_pgc_domain *domain) > -{ > - int i, ret; > - > - for (i = 0; ; i++) { > - struct clk *clk = of_clk_get(domain->dev->of_node, i); > - if (IS_ERR(clk)) > - break; > - if (i >= GPC_CLK_MAX) { > - dev_err(domain->dev, "more than %d clocks\n", > - GPC_CLK_MAX); > - ret = -EINVAL; > - goto clk_err; > - } > - domain->clk[i] = clk; > - } > - domain->num_clks = i; > - > - return 0; > - > -clk_err: > - while (i--) > - clk_put(domain->clk[i]); > - > - return ret; > -} > - > -static void imx_pgc_put_clocks(struct imx_pgc_domain *domain) > -{ > - int i; > - > - for (i = domain->num_clks - 1; i >= 0; i--) > - clk_put(domain->clk[i]); > -} > - > static int imx_pgc_domain_probe(struct platform_device *pdev) > { > struct imx_pgc_domain *domain = pdev->dev.platform_data; > @@ -490,9 +456,10 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) > domain->voltage, domain->voltage); > } > > - ret = imx_pgc_get_clocks(domain); > - if (ret) > - return dev_err_probe(domain->dev, ret, "Failed to get domain's clocks\n"); > + domain->num_clks = devm_clk_bulk_get_all(domain->dev, &domain->clks); > + if (domain->num_clks < 0) > + return dev_err_probe(domain->dev, domain->num_clks, > + "Failed to get domain's clocks\n"); > > regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > domain->bits.map, domain->bits.map); > @@ -517,7 +484,6 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) > out_domain_unmap: > regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > domain->bits.map, 0); > - imx_pgc_put_clocks(domain); > > return ret; > } > @@ -532,8 +498,6 @@ static int imx_pgc_domain_remove(struct platform_device *pdev) > regmap_update_bits(domain->regmap, GPC_PGC_CPU_MAPPING, > domain->bits.map, 0); > > - imx_pgc_put_clocks(domain); > - > return 0; > } > >
On 06.05.21 03:04, Peng Fan (OSS) wrote: > From: Lucas Stach <l.stach@pengutronix.de> > > The current mixed function to control both power up and power down > sequences is very hard to follow and already contains some sequence > errors like triggering the ADB400 handshake at the wrong time due to > this. Split the function into two, which results in slightly more > code, but is way easier to get right. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/soc/imx/gpcv2.c | 141 ++++++++++++++++++++++++---------------- > 1 file changed, 86 insertions(+), 55 deletions(-) > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > index 4222b6e87e7c..bcf1f338b0bf 100644 > --- a/drivers/soc/imx/gpcv2.c > +++ b/drivers/soc/imx/gpcv2.c > @@ -125,20 +125,19 @@ struct imx_pgc_domain_data { > const struct regmap_access_table *reg_access_table; > }; > > -static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > - bool on) > +static inline struct imx_pgc_domain * > +to_imx_pgc_domain(struct generic_pm_domain *genpd) > { > - struct imx_pgc_domain *domain = container_of(genpd, > - struct imx_pgc_domain, > - genpd); > - unsigned int offset = on ? > - GPC_PU_PGC_SW_PUP_REQ : GPC_PU_PGC_SW_PDN_REQ; > - const bool enable_power_control = !on; > - const bool has_regulator = !IS_ERR(domain->regulator); > - int i, ret = 0; > - u32 pxx_req; > - > - if (has_regulator && on) { > + return container_of(genpd, struct imx_pgc_domain, genpd); > +} > + > +static int imx_pgc_power_up(struct generic_pm_domain *genpd) > +{ > + struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd); > + u32 reg_val; > + int ret; > + > + if (!IS_ERR(domain->regulator)) { > ret = regulator_enable(domain->regulator); > if (ret) { > dev_err(domain->dev, "failed to enable regulator\n"); > @@ -150,69 +149,101 @@ static int imx_gpc_pu_pgc_sw_pxx_req(struct generic_pm_domain *genpd, > ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks); > if (ret) { > dev_err(domain->dev, "failed to enable reset clocks\n"); > - regulator_disable(domain->regulator); > - return ret; > + goto out_regulator_disable; > } > > - if (enable_power_control) > - regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc), > - GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR); > - > - if (domain->bits.hsk) > - regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, > - domain->bits.hsk, on ? domain->bits.hsk : 0); > - > - regmap_update_bits(domain->regmap, offset, > + /* request the domain to power up */ > + regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PUP_REQ, > domain->bits.pxx, domain->bits.pxx); > - > /* > * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait > * for PUP_REQ/PDN_REQ bit to be cleared > */ > - ret = regmap_read_poll_timeout(domain->regmap, offset, pxx_req, > - !(pxx_req & domain->bits.pxx), > + ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PUP_REQ, > + reg_val, !(reg_val & domain->bits.pxx), > 0, USEC_PER_MSEC); > if (ret) { > dev_err(domain->dev, "failed to command PGC\n"); > - /* > - * If we were in a process of enabling a > - * domain and failed we might as well disable > - * the regulator we just enabled. And if it > - * was the opposite situation and we failed to > - * power down -- keep the regulator on > - */ > - on = !on; > + goto out_clk_disable; > } > > - if (enable_power_control) > - regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc), > - GPC_PGC_CTRL_PCR, 0); > + /* disable power control */ > + regmap_clear_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc), > + GPC_PGC_CTRL_PCR); > + > + /* request the ADB400 to power up */ > + if (domain->bits.hsk) > + regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, > + domain->bits.hsk, domain->bits.hsk); > > /* Disable reset clocks for all devices in the domain */ > clk_bulk_disable_unprepare(domain->num_clks, domain->clks); > > - if (has_regulator && !on) { > - int err; > + return 0; > > - err = regulator_disable(domain->regulator); > - if (err) > - dev_err(domain->dev, > - "failed to disable regulator: %d\n", err); > - /* Preserve earlier error code */ > - ret = ret ?: err; > - } > +out_clk_disable: > + clk_bulk_disable_unprepare(domain->num_clks, domain->clks); > +out_regulator_disable: > + if (!IS_ERR(domain->regulator)) > + regulator_disable(domain->regulator); > > return ret; > } > > -static int imx_gpc_pu_pgc_sw_pup_req(struct generic_pm_domain *genpd) > +static int imx_pgc_power_down(struct generic_pm_domain *genpd) > { > - return imx_gpc_pu_pgc_sw_pxx_req(genpd, true); > -} > + struct imx_pgc_domain *domain = to_imx_pgc_domain(genpd); > + u32 reg_val; > + int ret; > > -static int imx_gpc_pu_pgc_sw_pdn_req(struct generic_pm_domain *genpd) > -{ > - return imx_gpc_pu_pgc_sw_pxx_req(genpd, false); > + /* Enable reset clocks for all devices in the domain */ > + ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks); > + if (ret) { > + dev_err(domain->dev, "failed to enable reset clocks\n"); > + return ret; > + } > + > + /* request the ADB400 to power down */ > + if (domain->bits.hsk) > + regmap_clear_bits(domain->regmap, GPC_PU_PWRHSK, > + domain->bits.hsk); > + > + /* enable power control */ > + regmap_update_bits(domain->regmap, GPC_PGC_CTRL(domain->pgc), > + GPC_PGC_CTRL_PCR, GPC_PGC_CTRL_PCR); > + > + /* request the domain to power down */ > + regmap_update_bits(domain->regmap, GPC_PU_PGC_SW_PDN_REQ, > + domain->bits.pxx, domain->bits.pxx); > + /* > + * As per "5.5.9.4 Example Code 4" in IMX7DRM.pdf wait > + * for PUP_REQ/PDN_REQ bit to be cleared > + */ > + ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PGC_SW_PDN_REQ, > + reg_val, !(reg_val & domain->bits.pxx), > + 0, USEC_PER_MSEC); > + if (ret) { > + dev_err(domain->dev, "failed to command PGC\n"); > + goto out_clk_disable; > + } > + > + /* Disable reset clocks for all devices in the domain */ > + clk_bulk_disable_unprepare(domain->num_clks, domain->clks); > + > + if (!IS_ERR(domain->regulator)) { > + ret = regulator_disable(domain->regulator); > + if (ret) { > + dev_err(domain->dev, "failed to disable regulator\n"); > + return ret; > + } > + } > + > + return 0; > + > +out_clk_disable: > + clk_bulk_disable_unprepare(domain->num_clks, domain->clks); > + > + return ret; > } > > static const struct imx_pgc_domain imx7_pgc_domains[] = { > @@ -590,8 +621,8 @@ static int imx_gpcv2_probe(struct platform_device *pdev) > > domain = pd_pdev->dev.platform_data; > domain->regmap = regmap; > - domain->genpd.power_on = imx_gpc_pu_pgc_sw_pup_req; > - domain->genpd.power_off = imx_gpc_pu_pgc_sw_pdn_req; > + domain->genpd.power_on = imx_pgc_power_up; > + domain->genpd.power_off = imx_pgc_power_down; > > pd_pdev->dev.parent = dev; > pd_pdev->dev.of_node = np; >
On 06.05.21 03:04, Peng Fan (OSS) wrote: > From: Lucas Stach <l.stach@pengutronix.de> > > For some domains the resets of the devices in the domain are not > automatically triggered. Add an optional resets property to allow > the GPC driver to trigger those resets explicitly. > > The resets belong to devices located inside the power domain, > which need to be held in reset across the power-up sequence. So we > have no means to specify what each reset is in a generic power-domain > binding. Same situation as with the clocks in this binding actually. My understanding was that Rob wanted this explanation to be contained in the binding docs itself and not only in the commit message, but I might be wrong. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > index a96e6dbf1858..4330c73a2c30 100644 > --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > @@ -66,6 +66,13 @@ properties: > > power-supply: true > > + resets: > + description: | > + A number of phandles to resets that need to be asserted during > + power-up sequencing of the domain. > + minItems: 1 > + maxItems: 4 > + > required: > - '#power-domain-cells' > - reg >
On 06.05.21 03:04, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > > V2: > - Add R-b/A-b tag > - Merge V1 patch 13 to V2 patch 6 > - Drop V1 patch 15 > - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain > details > - Add explaination in patch 8 for "why the resets are not defined" > > This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several > minor changes from me to make it could work with i.MX BLK-CTL driver. > > Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting > all the patches, Jacky Bai on help debug issues. I tested this series together with the BLK CTL patches by using the GPU and the display stack. Everything looks good to me. Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > Lucas Stach (12): > soc: imx: gpcv2: move to more ideomatic error handling in probe > soc: imx: gpcv2: move domain mapping to domain driver probe > soc: imx: gpcv2: switch to clk_bulk_* API > soc: imx: gpcv2: split power up and power down sequence control > soc: imx: gpcv2: wait for ADB400 handshake > soc: imx: gpcv2: add runtime PM support for power-domains > soc: imx: gpcv2: allow domains without power-sequence control > dt-bindings: imx: gpcv2: add support for optional resets > soc: imx: gpcv2: add support for optional resets > dt-bindings: power: add defines for i.MX8MM power domains > soc: imx: gpcv2: add support for i.MX8MM power domains > soc: imx: gpcv2: Add support for missing i.MX8MM VPU/DISPMIX power > domains > > Peng Fan (1): > soc: imx: gpcv2: move reset assert after requesting domain power up > > .../bindings/power/fsl,imx-gpcv2.yaml | 9 + > drivers/soc/imx/gpcv2.c | 542 ++++++++++++++---- > include/dt-bindings/power/imx8mm-power.h | 22 + > 3 files changed, 458 insertions(+), 115 deletions(-) > create mode 100644 include/dt-bindings/power/imx8mm-power.h >
On Thu, May 06, 2021 at 08:43:17AM +0200, Frieder Schrempf wrote: > On 06.05.21 03:04, Peng Fan (OSS) wrote: > > From: Lucas Stach <l.stach@pengutronix.de> > > > > For some domains the resets of the devices in the domain are not > > automatically triggered. Add an optional resets property to allow > > the GPC driver to trigger those resets explicitly. > > > > The resets belong to devices located inside the power domain, > > which need to be held in reset across the power-up sequence. So we > > have no means to specify what each reset is in a generic power-domain > > binding. Same situation as with the clocks in this binding actually. > > My understanding was that Rob wanted this explanation to be contained in the binding docs itself and not only in the commit message, but I might be wrong. Yes, that would be better. > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > index a96e6dbf1858..4330c73a2c30 100644 > > --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > @@ -66,6 +66,13 @@ properties: > > > > power-supply: true > > > > + resets: > > + description: | > > + A number of phandles to resets that need to be asserted during > > + power-up sequencing of the domain. > > + minItems: 1 > > + maxItems: 4 > > + > > required: > > - '#power-domain-cells' > > - reg > >
> Subject: Re: [PATCH V2 08/13] dt-bindings: imx: gpcv2: add support for > optional resets > > On Thu, May 06, 2021 at 08:43:17AM +0200, Frieder Schrempf wrote: > > On 06.05.21 03:04, Peng Fan (OSS) wrote: > > > From: Lucas Stach <l.stach@pengutronix.de> > > > > > > For some domains the resets of the devices in the domain are not > > > automatically triggered. Add an optional resets property to allow > > > the GPC driver to trigger those resets explicitly. > > > > > > The resets belong to devices located inside the power domain, which > > > need to be held in reset across the power-up sequence. So we have no > > > means to specify what each reset is in a generic power-domain > > > binding. Same situation as with the clocks in this binding actually. > > > > My understanding was that Rob wanted this explanation to be contained in > the binding docs itself and not only in the commit message, but I might be > wrong. > > Yes, that would be better. Sure, I will include that in V3. Thanks, Peng. > > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml | 7 > > > +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > > b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > > index a96e6dbf1858..4330c73a2c30 100644 > > > --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > > > @@ -66,6 +66,13 @@ properties: > > > > > > power-supply: true > > > > > > + resets: > > > + description: | > > > + A number of phandles to resets that need to be > asserted during > > > + power-up sequencing of the domain. > > > + minItems: 1 > > > + maxItems: 4 > > > + > > > required: > > > - '#power-domain-cells' > > > - reg > > >
On 06.05.21 10:32, Frieder Schrempf wrote: > On 06.05.21 03:04, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> >> V2: >> - Add R-b/A-b tag >> - Merge V1 patch 13 to V2 patch 6 >> - Drop V1 patch 15 >> - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain >> details >> - Add explaination in patch 8 for "why the resets are not defined" >> >> This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several >> minor changes from me to make it could work with i.MX BLK-CTL driver. >> >> Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting >> all the patches, Jacky Bai on help debug issues. > > I tested this series together with the BLK CTL patches by using the GPU and the display stack. Everything looks good to me. > > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> So after some more testing on different hardware I stumbled upon the problem that USB autosuspend doesn't work properly anymore. I have an onboard LTE module that is connected to OTG2 on the i.MX8MM. When using the mainline TF-A (that enables USB power-domains by default) and removing the power-domain control from the kernel, the device comes up after a few seconds and is enumerated on the bus. Now, when I let the kernel control the power-domains, the device comes up at boot, but isn't enumerated on the USB bus. As soon as I disable autosuspend for the port, it comes up. Is this something that needs to be fixed on the USB driver side or is something to be considered for the GPCv2 driver?
On 19.05.21 18:09, Frieder Schrempf wrote: > On 06.05.21 10:32, Frieder Schrempf wrote: >> On 06.05.21 03:04, Peng Fan (OSS) wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> >>> V2: >>> - Add R-b/A-b tag >>> - Merge V1 patch 13 to V2 patch 6 >>> - Drop V1 patch 15 >>> - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain >>> details >>> - Add explaination in patch 8 for "why the resets are not defined" >>> >>> This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several >>> minor changes from me to make it could work with i.MX BLK-CTL driver. >>> >>> Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting >>> all the patches, Jacky Bai on help debug issues. >> >> I tested this series together with the BLK CTL patches by using the GPU and the display stack. Everything looks good to me. >> >> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > So after some more testing on different hardware I stumbled upon the problem that USB autosuspend doesn't work properly anymore. > > I have an onboard LTE module that is connected to OTG2 on the i.MX8MM. When using the mainline TF-A (that enables USB power-domains by default) and removing the power-domain control from the kernel, the device comes up after a few seconds and is enumerated on the bus. > > Now, when I let the kernel control the power-domains, the device comes up at boot, but isn't enumerated on the USB bus. As soon as I disable autosuspend for the port, it comes up. > > Is this something that needs to be fixed on the USB driver side or is something to be considered for the GPCv2 driver? So I think this is something that needs to be covered on the USB driver side. I would expect that a device appearing on the bus should resume the autosuspended bus, but I don't really know much about USB so there might be other things I miss. For now I will disable autosuspend in this case. A different, probably more severe problem is that I was still able to reliably run into lockups with suspend/resume and the GPU. I thought I had tested this before as it was one of the things that already failed with the previous implementation, but I must have missed something as it still fails with kernel v5.12.1 + v2 of the GPC patches. This is how I run into the lockup: echo mem > /sys/power/state # Sleep # Wake up again glmark2-es2-drm # Use the GPU # Device locks up Peng, is this something you can reproduce?
Hi Frieder, Am Donnerstag, dem 20.05.2021 um 17:16 +0200 schrieb Frieder Schrempf: > On 19.05.21 18:09, Frieder Schrempf wrote: > > On 06.05.21 10:32, Frieder Schrempf wrote: > > > On 06.05.21 03:04, Peng Fan (OSS) wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > > > V2: > > > > - Add R-b/A-b tag > > > > - Merge V1 patch 13 to V2 patch 6 > > > > - Drop V1 patch 15 > > > > - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 > > > > to explain > > > > details > > > > - Add explaination in patch 8 for "why the resets are not > > > > defined" > > > > > > > > This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and > > > > several > > > > minor changes from me to make it could work with i.MX BLK-CTL > > > > driver. > > > > > > > > Thanks for Lucas's work and suggestion, Frieder Schrempf for > > > > collecting > > > > all the patches, Jacky Bai on help debug issues. > > > > > > I tested this series together with the BLK CTL patches by using > > > the GPU and the display stack. Everything looks good to me. > > > > > > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> > > > > So after some more testing on different hardware I stumbled upon > > the problem that USB autosuspend doesn't work properly anymore. > > > > I have an onboard LTE module that is connected to OTG2 on the > > i.MX8MM. When using the mainline TF-A (that enables USB power- > > domains by default) and removing the power-domain control from the > > kernel, the device comes up after a few seconds and is enumerated > > on the bus. > > > > Now, when I let the kernel control the power-domains, the device > > comes up at boot, but isn't enumerated on the USB bus. As soon as I > > disable autosuspend for the port, it comes up. > > > > Is this something that needs to be fixed on the USB driver side or > > is something to be considered for the GPCv2 driver? > > So I think this is something that needs to be covered on the USB > driver side. I would expect that a device appearing on the bus should > resume the autosuspended bus, but I don't really know much about USB > so there might be other things I miss. For now I will disable > autosuspend in this case. > > A different, probably more severe problem is that I was still able to > reliably run into lockups with suspend/resume and the GPU. I thought > I had tested this before as it was one of the things that already > failed with the previous implementation, but I must have missed > something as it still fails with kernel v5.12.1 + v2 of the GPC > patches. > > This is how I run into the lockup: > > echo mem > /sys/power/state # Sleep > # Wake up again > glmark2-es2-drm # Use the GPU > # Device locks up > > Peng, is this something you can reproduce? I could reproduce this issue on my last GPC+BLK_CTRL series. This was caused by a bad interaction between our slightly unusual way to control the nested power domains via runtime PM and the system suspend/resume code, which lead to some of the power domains not properly coming up again in the resume path. v2 of my series fixes this issue and the above sequence works as expected. Regards, Lucas
Hi Lucas, On 21.07.21 22:51, Lucas Stach wrote: > Hi Frieder, > > Am Donnerstag, dem 20.05.2021 um 17:16 +0200 schrieb Frieder Schrempf: >> On 19.05.21 18:09, Frieder Schrempf wrote: >>> On 06.05.21 10:32, Frieder Schrempf wrote: >>>> On 06.05.21 03:04, Peng Fan (OSS) wrote: >>>>> From: Peng Fan <peng.fan@nxp.com> >>>>> >>>>> >>>>> V2: >>>>> - Add R-b/A-b tag >>>>> - Merge V1 patch 13 to V2 patch 6 >>>>> - Drop V1 patch 15 >>>>> - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 >>>>> to explain >>>>> details >>>>> - Add explaination in patch 8 for "why the resets are not >>>>> defined" >>>>> >>>>> This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and >>>>> several >>>>> minor changes from me to make it could work with i.MX BLK-CTL >>>>> driver. >>>>> >>>>> Thanks for Lucas's work and suggestion, Frieder Schrempf for >>>>> collecting >>>>> all the patches, Jacky Bai on help debug issues. >>>> >>>> I tested this series together with the BLK CTL patches by using >>>> the GPU and the display stack. Everything looks good to me. >>>> >>>> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> >>> >>> So after some more testing on different hardware I stumbled upon >>> the problem that USB autosuspend doesn't work properly anymore. >>> >>> I have an onboard LTE module that is connected to OTG2 on the >>> i.MX8MM. When using the mainline TF-A (that enables USB power- >>> domains by default) and removing the power-domain control from the >>> kernel, the device comes up after a few seconds and is enumerated >>> on the bus. >>> >>> Now, when I let the kernel control the power-domains, the device >>> comes up at boot, but isn't enumerated on the USB bus. As soon as I >>> disable autosuspend for the port, it comes up. >>> >>> Is this something that needs to be fixed on the USB driver side or >>> is something to be considered for the GPCv2 driver? >> >> So I think this is something that needs to be covered on the USB >> driver side. I would expect that a device appearing on the bus should >> resume the autosuspended bus, but I don't really know much about USB >> so there might be other things I miss. For now I will disable >> autosuspend in this case. >> >> A different, probably more severe problem is that I was still able to >> reliably run into lockups with suspend/resume and the GPU. I thought >> I had tested this before as it was one of the things that already >> failed with the previous implementation, but I must have missed >> something as it still fails with kernel v5.12.1 + v2 of the GPC >> patches. >> >> This is how I run into the lockup: >> >> echo mem > /sys/power/state # Sleep >> # Wake up again >> glmark2-es2-drm # Use the GPU >> # Device locks up >> >> Peng, is this something you can reproduce? > > I could reproduce this issue on my last GPC+BLK_CTRL series. This was > caused by a bad interaction between our slightly unusual way to control > the nested power domains via runtime PM and the system suspend/resume > code, which lead to some of the power domains not properly coming up > again in the resume path. v2 of my series fixes this issue and the > above sequence works as expected. Glad you could reproduce and fix this issue. Thanks for the effort. I will try to do some tests myself soon. Thanks Frieder
Hi Peng, Lucas, On Wed, 5 May 2021 at 21:32, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > V2: > - Add R-b/A-b tag > - Merge V1 patch 13 to V2 patch 6 > - Drop V1 patch 15 > - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain > details > - Add explaination in patch 8 for "why the resets are not defined" > > This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several > minor changes from me to make it could work with i.MX BLK-CTL driver. > > Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting > all the patches, Jacky Bai on help debug issues. > > Lucas Stach (12): > soc: imx: gpcv2: move to more ideomatic error handling in probe > soc: imx: gpcv2: move domain mapping to domain driver probe > soc: imx: gpcv2: switch to clk_bulk_* API > soc: imx: gpcv2: split power up and power down sequence control > soc: imx: gpcv2: wait for ADB400 handshake > soc: imx: gpcv2: add runtime PM support for power-domains > soc: imx: gpcv2: allow domains without power-sequence control > dt-bindings: imx: gpcv2: add support for optional resets > soc: imx: gpcv2: add support for optional resets > dt-bindings: power: add defines for i.MX8MM power domains > soc: imx: gpcv2: add support for i.MX8MM power domains > soc: imx: gpcv2: Add support for missing i.MX8MM VPU/DISPMIX power > domains > It's nice to see this finally moving forward! As you know, Hantro G2 support for i.MX8MQ (and i.MX8MP, i.MX8MM) is currently blocked, as you have requested: https://lore.kernel.org/driverdev-devel/5aa5700b862234895a7a6eb251ca3c80fdc1a6d3.camel@collabora.com/ So, I think we really need to include i.MX8MP and i.MX8MQ on this series. It's been quite a while and we really need to have that. To be honest, I fear that if we merge this series as-is, MX8MP and MX8MP support will fall in the oblivion, and Hantro G2 VPU will remain unusable. We are planning to submit Hantro G2 VP9 support soon, and we've been testing on various platforms, but it will also be blocked by lack of power-domains. In the future, please Cc the linux-media mailing list, as well as Benjamin, Andrzej and myself, so we can follow this. Thanks a lot for the hard work! Ezequiel
Hi Ezequiel, Am Mittwoch, dem 04.08.2021 um 11:30 -0300 schrieb Ezequiel Garcia: > Hi Peng, Lucas, > > On Wed, 5 May 2021 at 21:32, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > V2: > > - Add R-b/A-b tag > > - Merge V1 patch 13 to V2 patch 6 > > - Drop V1 patch 15 > > - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain > > details > > - Add explaination in patch 8 for "why the resets are not defined" > > > > This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several > > minor changes from me to make it could work with i.MX BLK-CTL driver. > > > > Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting > > all the patches, Jacky Bai on help debug issues. > > > > Lucas Stach (12): > > soc: imx: gpcv2: move to more ideomatic error handling in probe > > soc: imx: gpcv2: move domain mapping to domain driver probe > > soc: imx: gpcv2: switch to clk_bulk_* API > > soc: imx: gpcv2: split power up and power down sequence control > > soc: imx: gpcv2: wait for ADB400 handshake > > soc: imx: gpcv2: add runtime PM support for power-domains > > soc: imx: gpcv2: allow domains without power-sequence control > > dt-bindings: imx: gpcv2: add support for optional resets > > soc: imx: gpcv2: add support for optional resets > > dt-bindings: power: add defines for i.MX8MM power domains > > soc: imx: gpcv2: add support for i.MX8MM power domains > > soc: imx: gpcv2: Add support for missing i.MX8MM VPU/DISPMIX power > > domains > > > > It's nice to see this finally moving forward! > > As you know, Hantro G2 support for i.MX8MQ (and i.MX8MP, i.MX8MM) is currently > blocked, as you have requested: > > https://lore.kernel.org/driverdev-devel/5aa5700b862234895a7a6eb251ca3c80fdc1a6d3.camel@collabora.com/ > > So, I think we really need to include i.MX8MP and i.MX8MQ on this series. > It's been quite a while and we really need to have that. To be honest, > I fear that > if we merge this series as-is, MX8MP and MX8MP support will fall in > the oblivion, > and Hantro G2 VPU will remain unusable. > > We are planning to submit Hantro G2 VP9 support soon, and we've been testing > on various platforms, but it will also be blocked by lack of power-domains. > > In the future, please Cc the linux-media mailing list, as well as > Benjamin, Andrzej and myself, so we can follow this. Please take a look at [1], which is the current state of this work. I intend to add both i.MX8MQ and i.MX8MP support to the series now, as it seems that there have been no big objections to my approach over the last 2 weeks, where I was on vacation. ;) Regards, Lucas [1] https://lore.kernel.org/linux-arm-kernel/20210716232916.3572966-14-l.stach@pengutronix.de/T/#m43cbf6b8615b2a37ff2abb0346e7e7f6594976d1
Le 09/08/2021 à 10:15, Lucas Stach a écrit : > Hi Ezequiel, > > Am Mittwoch, dem 04.08.2021 um 11:30 -0300 schrieb Ezequiel Garcia: >> Hi Peng, Lucas, >> >> On Wed, 5 May 2021 at 21:32, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> >>> V2: >>> - Add R-b/A-b tag >>> - Merge V1 patch 13 to V2 patch 6 >>> - Drop V1 patch 15 >>> - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain >>> details >>> - Add explaination in patch 8 for "why the resets are not defined" >>> >>> This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several >>> minor changes from me to make it could work with i.MX BLK-CTL driver. >>> >>> Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting >>> all the patches, Jacky Bai on help debug issues. >>> >>> Lucas Stach (12): >>> soc: imx: gpcv2: move to more ideomatic error handling in probe >>> soc: imx: gpcv2: move domain mapping to domain driver probe >>> soc: imx: gpcv2: switch to clk_bulk_* API >>> soc: imx: gpcv2: split power up and power down sequence control >>> soc: imx: gpcv2: wait for ADB400 handshake >>> soc: imx: gpcv2: add runtime PM support for power-domains >>> soc: imx: gpcv2: allow domains without power-sequence control >>> dt-bindings: imx: gpcv2: add support for optional resets >>> soc: imx: gpcv2: add support for optional resets >>> dt-bindings: power: add defines for i.MX8MM power domains >>> soc: imx: gpcv2: add support for i.MX8MM power domains >>> soc: imx: gpcv2: Add support for missing i.MX8MM VPU/DISPMIX power >>> domains >>> >> It's nice to see this finally moving forward! >> >> As you know, Hantro G2 support for i.MX8MQ (and i.MX8MP, i.MX8MM) is currently >> blocked, as you have requested: >> >> https://lore.kernel.org/driverdev-devel/5aa5700b862234895a7a6eb251ca3c80fdc1a6d3.camel@collabora.com/ >> >> So, I think we really need to include i.MX8MP and i.MX8MQ on this series. >> It's been quite a while and we really need to have that. To be honest, >> I fear that >> if we merge this series as-is, MX8MP and MX8MP support will fall in >> the oblivion, >> and Hantro G2 VPU will remain unusable. >> >> We are planning to submit Hantro G2 VP9 support soon, and we've been testing >> on various platforms, but it will also be blocked by lack of power-domains. >> >> In the future, please Cc the linux-media mailing list, as well as >> Benjamin, Andrzej and myself, so we can follow this. > Please take a look at [1], which is the current state of this work. I > intend to add both i.MX8MQ and i.MX8MP support to the series now, as it > seems that there have been no big objections to my approach over the > last 2 weeks, where I was on vacation. ;) Hi Lucas, I have tried to implement the block control driver for imx8mq. I didn't manage to get it working. My implementation is here: https://gitlab.collabora.com/benjamin.gaignard/for-upstream/-/tree/IMX8MQ_BLK_CTRL While you have the same in IMX8MM do you have also made changes in Hantro driver ? If it is that can you share these changes ? I have include mine in the above branch. Regards, Benjamin > > Regards, > Lucas > > [1] > https://lore.kernel.org/linux-arm-kernel/20210716232916.3572966-14-l.stach@pengutronix.de/T/#m43cbf6b8615b2a37ff2abb0346e7e7f6594976d1 > >
From: Peng Fan <peng.fan@nxp.com> V2: - Add R-b/A-b tag - Merge V1 patch 13 to V2 patch 6 - Drop V1 patch 15 - Merge V1 patch 16 to V2 patch 5 and add comments in patch 5 to explain details - Add explaination in patch 8 for "why the resets are not defined" This patchset is a pick up Lucas's gpcv2 work for i.MX8MM and several minor changes from me to make it could work with i.MX BLK-CTL driver. Thanks for Lucas's work and suggestion, Frieder Schrempf for collecting all the patches, Jacky Bai on help debug issues. Lucas Stach (12): soc: imx: gpcv2: move to more ideomatic error handling in probe soc: imx: gpcv2: move domain mapping to domain driver probe soc: imx: gpcv2: switch to clk_bulk_* API soc: imx: gpcv2: split power up and power down sequence control soc: imx: gpcv2: wait for ADB400 handshake soc: imx: gpcv2: add runtime PM support for power-domains soc: imx: gpcv2: allow domains without power-sequence control dt-bindings: imx: gpcv2: add support for optional resets soc: imx: gpcv2: add support for optional resets dt-bindings: power: add defines for i.MX8MM power domains soc: imx: gpcv2: add support for i.MX8MM power domains soc: imx: gpcv2: Add support for missing i.MX8MM VPU/DISPMIX power domains Peng Fan (1): soc: imx: gpcv2: move reset assert after requesting domain power up .../bindings/power/fsl,imx-gpcv2.yaml | 9 + drivers/soc/imx/gpcv2.c | 542 ++++++++++++++---- include/dt-bindings/power/imx8mm-power.h | 22 + 3 files changed, 458 insertions(+), 115 deletions(-) create mode 100644 include/dt-bindings/power/imx8mm-power.h -- 2.30.0