* [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource
@ 2019-12-04 10:14 Peng Fan
2019-12-04 14:24 ` Leonard Crestez
0 siblings, 1 reply; 5+ messages in thread
From: Peng Fan @ 2019-12-04 10:14 UTC (permalink / raw)
To: sboyd, shawnguo, s.hauer, festevam, Abel Vesa, Aisheng Dong
Cc: kernel, dl-linux-imx, linux-clk, linux-arm-kernel, linux-kernel,
Leonard Crestez, Alice Guo, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
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 <peng.fan@nxp.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource
2019-12-04 10:14 [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource Peng Fan
@ 2019-12-04 14:24 ` Leonard Crestez
2019-12-05 1:37 ` Peng Fan
0 siblings, 1 reply; 5+ messages in thread
From: Leonard Crestez @ 2019-12-04 14:24 UTC (permalink / raw)
To: Peng Fan, shawnguo, Aisheng Dong
Cc: sboyd, s.hauer, festevam, Abel Vesa, kernel, dl-linux-imx,
linux-clk, linux-arm-kernel, linux-kernel, Alice Guo
On 2019-12-04 12:14 PM, Peng Fan wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> 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 <peng.fan@nxp.com>
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource
2019-12-04 14:24 ` Leonard Crestez
@ 2019-12-05 1:37 ` Peng Fan
2019-12-05 8:59 ` Leonard Crestez
0 siblings, 1 reply; 5+ messages in thread
From: Peng Fan @ 2019-12-05 1:37 UTC (permalink / raw)
To: Leonard Crestez, shawnguo, Aisheng Dong
Cc: sboyd, s.hauer, festevam, Abel Vesa, kernel, dl-linux-imx,
linux-clk, linux-arm-kernel, linux-kernel, Alice Guo
> 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 <peng.fan@nxp.com>
> >
> > 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 <peng.fan@nxp.com>
>
> 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);
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource
2019-12-05 1:37 ` Peng Fan
@ 2019-12-05 8:59 ` Leonard Crestez
2019-12-05 9:03 ` Peng Fan
0 siblings, 1 reply; 5+ messages in thread
From: Leonard Crestez @ 2019-12-05 8:59 UTC (permalink / raw)
To: Peng Fan, shawnguo, Aisheng Dong, sboyd
Cc: s.hauer, festevam, Abel Vesa, kernel, dl-linux-imx, linux-clk,
linux-arm-kernel, linux-kernel, Alice Guo
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 <peng.fan@nxp.com>
>>>
>>> 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 <peng.fan@nxp.com>
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource
2019-12-05 8:59 ` Leonard Crestez
@ 2019-12-05 9:03 ` Peng Fan
0 siblings, 0 replies; 5+ messages in thread
From: Peng Fan @ 2019-12-05 9:03 UTC (permalink / raw)
To: Leonard Crestez, shawnguo, Aisheng Dong, sboyd
Cc: s.hauer, festevam, Abel Vesa, kernel, dl-linux-imx, linux-clk,
linux-arm-kernel, linux-kernel, Alice Guo
> 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 <peng.fan@nxp.com>
> >>>
> >>> 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 <peng.fan@nxp.com>
> >>
> >> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-05 9:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 10:14 [PATCH] clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource Peng Fan
2019-12-04 14:24 ` Leonard Crestez
2019-12-05 1:37 ` Peng Fan
2019-12-05 8:59 ` Leonard Crestez
2019-12-05 9:03 ` Peng Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).