From: Peng Fan <[email protected]>
devm_platform_ioremap_resource() wraps platform_get_resource() and
devm_ioremap_resource(), we could use this API to simplify the code.
Signed-off-by: Peng Fan <[email protected]>
---
drivers/clk/imx/clk-imx8qxp-lpcg.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index c0aff7ca6374..3f2c2b068c73 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -164,7 +164,6 @@ static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev)
struct clk_hw_onecell_data *clk_data;
const struct imx8qxp_ss_lpcg *ss_lpcg;
const struct imx8qxp_lpcg_data *lpcg;
- struct resource *res;
struct clk_hw **clks;
void __iomem *base;
int i;
@@ -173,12 +172,9 @@ static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev)
if (!ss_lpcg)
return -ENODEV;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res)
- return -EINVAL;
- base = devm_ioremap(dev, res->start, resource_size(res));
+ base = devm_platform_ioremap_resource(pdev, 0);
if (!base)
- return -ENOMEM;
+ return PTR_ERR(base);
clk_data = devm_kzalloc(&pdev->dev, struct_size(clk_data, hws,
ss_lpcg->num_max), GFP_KERNEL);
--
2.16.4
On 2019-12-04 12:14 PM, Peng Fan wrote:
> From: Peng Fan <[email protected]>
>
> devm_platform_ioremap_resource() wraps platform_get_resource() and
> devm_ioremap_resource(), we could use this API to simplify the code.
>
> Signed-off-by: Peng Fan <[email protected]>
This patch has been posted before and it breaks uart on imx8qxp-mek and
possibly other things.
The old and new paths are not equivalent: devm_platform_ioremap_resource
calls devm_ioremap_resource differs from devm_ioremap by also calling
devm_request_mem_region.
This prevents other mappings in the area; this is not an issue for most
drivers but imx8qxp-lpcg maps whole subsystems. For example:
adma_lpcg: clock-controller@59000000 {
compatible = "fsl,imx8qxp-lpcg-adma";
reg = <0x59000000 0x2000000>;
#clock-cells = <1>;
};
adma_lpuart0: serial@5a060000 {
reg = <0x5a060000 0x1000>;
...
};
Previously: https://patchwork.kernel.org/patch/10908807/
> ---
> drivers/clk/imx/clk-imx8qxp-lpcg.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index c0aff7ca6374..3f2c2b068c73 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -164,7 +164,6 @@ static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev)
> struct clk_hw_onecell_data *clk_data;
> const struct imx8qxp_ss_lpcg *ss_lpcg;
> const struct imx8qxp_lpcg_data *lpcg;
> - struct resource *res;
> struct clk_hw **clks;
> void __iomem *base;
> int i;
> @@ -173,12 +172,9 @@ static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev)
> if (!ss_lpcg)
> return -ENODEV;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res)
> - return -EINVAL;
> - base = devm_ioremap(dev, res->start, resource_size(res));
> + base = devm_platform_ioremap_resource(pdev, 0);
> if (!base)
> - return -ENOMEM;
> + return PTR_ERR(base);
>
> clk_data = devm_kzalloc(&pdev->dev, struct_size(clk_data, hws,
> ss_lpcg->num_max), GFP_KERNEL);
>
> Subject: Re: [PATCH] clk: imx: imx8qxp-lpcg: use
> devm_platform_ioremap_resource
>
> On 2019-12-04 12:14 PM, Peng Fan wrote:
> > From: Peng Fan <[email protected]>
> >
> > devm_platform_ioremap_resource() wraps platform_get_resource() and
> > devm_ioremap_resource(), we could use this API to simplify the code.
> >
> > Signed-off-by: Peng Fan <[email protected]>
>
> This patch has been posted before and it breaks uart on imx8qxp-mek and
> possibly other things.
>
> The old and new paths are not equivalent: devm_platform_ioremap_resource
> calls devm_ioremap_resource differs from devm_ioremap by also calling
> devm_request_mem_region.
>
> This prevents other mappings in the area; this is not an issue for most drivers
> but imx8qxp-lpcg maps whole subsystems. For example:
>
> adma_lpcg: clock-controller@59000000 {
> compatible = "fsl,imx8qxp-lpcg-adma";
> reg = <0x59000000 0x2000000>;
> #clock-cells = <1>;
> };
>
> adma_lpuart0: serial@5a060000 {
> reg = <0x5a060000 0x1000>;
> ...
> };
>
> Previously: https://patchwork.kernel.org/patch/10908807/
Thanks. I think at least need to provide some comments in code.
Thanks,
Peng.
>
> > ---
> > drivers/clk/imx/clk-imx8qxp-lpcg.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index c0aff7ca6374..3f2c2b068c73 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -164,7 +164,6 @@ static int imx8qxp_lpcg_clk_probe(struct
> platform_device *pdev)
> > struct clk_hw_onecell_data *clk_data;
> > const struct imx8qxp_ss_lpcg *ss_lpcg;
> > const struct imx8qxp_lpcg_data *lpcg;
> > - struct resource *res;
> > struct clk_hw **clks;
> > void __iomem *base;
> > int i;
> > @@ -173,12 +172,9 @@ static int imx8qxp_lpcg_clk_probe(struct
> platform_device *pdev)
> > if (!ss_lpcg)
> > return -ENODEV;
> >
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - if (!res)
> > - return -EINVAL;
> > - base = devm_ioremap(dev, res->start, resource_size(res));
> > + base = devm_platform_ioremap_resource(pdev, 0);
> > if (!base)
> > - return -ENOMEM;
> > + return PTR_ERR(base);
> >
> > clk_data = devm_kzalloc(&pdev->dev, struct_size(clk_data, hws,
> > ss_lpcg->num_max), GFP_KERNEL);
> >
On 2019-12-05 3:38 AM, Peng Fan wrote:
>> Subject: Re: [PATCH] clk: imx: imx8qxp-lpcg: use
>> devm_platform_ioremap_resource
>>
>> On 2019-12-04 12:14 PM, Peng Fan wrote:
>>> From: Peng Fan <[email protected]>
>>>
>>> devm_platform_ioremap_resource() wraps platform_get_resource() and
>>> devm_ioremap_resource(), we could use this API to simplify the code.
>>>
>>> Signed-off-by: Peng Fan <[email protected]>
>>
>> This patch has been posted before and it breaks uart on imx8qxp-mek and
>> possibly other things.
>>
>> The old and new paths are not equivalent: devm_platform_ioremap_resource
>> calls devm_ioremap_resource differs from devm_ioremap by also calling
>> devm_request_mem_region.
>>
>> This prevents other mappings in the area; this is not an issue for most drivers
>> but imx8qxp-lpcg maps whole subsystems. For example:
>>
>> adma_lpcg: clock-controller@59000000 {
>> compatible = "fsl,imx8qxp-lpcg-adma";
>> reg = <0x59000000 0x2000000>;
>> #clock-cells = <1>;
>> };
>>
>> adma_lpuart0: serial@5a060000 {
>> reg = <0x5a060000 0x1000>;
>> ...
>> };
>>
>> Previously: https://patchwork.kernel.org/patch/10908807/
>
> Thanks. I think at least need to provide some comments in code.
Yes, comments would help. I think it's actually the 3rd time this
incorrect cleanup was posted.
But mapping entire subsystems (32mb at a time) for LPCG is deeply
flawed: the LPCG areas are each 64k and they're interspersed among the
peripherals. The correct solution is to have many small clock providers.
This is done by a series of patches from Aisheng, I think this is the
latest one:
https://patchwork.kernel.org/patch/11248235/
If some aspects of that series are dubious perhaps they could be
discussed and maybe the series could be split into smaller chunks?
That series does brings many essential improvements to imx8 clk support.
--
Regards,
Leonard
> Subject: Re: [PATCH] clk: imx: imx8qxp-lpcg: use
> devm_platform_ioremap_resource
>
> On 2019-12-05 3:38 AM, Peng Fan wrote:
> >> Subject: Re: [PATCH] clk: imx: imx8qxp-lpcg: use
> >> devm_platform_ioremap_resource
> >>
> >> On 2019-12-04 12:14 PM, Peng Fan wrote:
> >>> From: Peng Fan <[email protected]>
> >>>
> >>> devm_platform_ioremap_resource() wraps platform_get_resource() and
> >>> devm_ioremap_resource(), we could use this API to simplify the code.
> >>>
> >>> Signed-off-by: Peng Fan <[email protected]>
> >>
> >> This patch has been posted before and it breaks uart on imx8qxp-mek
> >> and possibly other things.
> >>
> >> The old and new paths are not equivalent:
> >> devm_platform_ioremap_resource calls devm_ioremap_resource differs
> >> from devm_ioremap by also calling devm_request_mem_region.
> >>
> >> This prevents other mappings in the area; this is not an issue for
> >> most drivers but imx8qxp-lpcg maps whole subsystems. For example:
> >>
> >> adma_lpcg: clock-controller@59000000 {
> >> compatible = "fsl,imx8qxp-lpcg-adma";
> >> reg = <0x59000000 0x2000000>;
> >> #clock-cells = <1>;
> >> };
> >>
> >> adma_lpuart0: serial@5a060000 {
> >> reg = <0x5a060000 0x1000>;
> >> ...
> >> };
> >>
> >> Previously: https://patchwork.kernel.org/patch/10908807/
> >
> > Thanks. I think at least need to provide some comments in code.
>
> Yes, comments would help. I think it's actually the 3rd time this incorrect
> cleanup was posted.
>
> But mapping entire subsystems (32mb at a time) for LPCG is deeply
> flawed: the LPCG areas are each 64k and they're interspersed among the
> peripherals. The correct solution is to have many small clock providers.
>
> This is done by a series of patches from Aisheng, I think this is the latest one:
>
> https://patchwork.kernel.org/patch/11248235/
>
> If some aspects of that series are dubious perhaps they could be discussed
> and maybe the series could be split into smaller chunks?
That would be lots of lpcg nodes in device tree.
>
> That series does brings many essential improvements to imx8 clk support.
Seems that pending for long time? What is the blocking point?
Regards,
Peng.
>
> --
> Regards,
> Leonard