diff mbox series

clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource

Message ID 1575454349-5762-1-git-send-email-peng.fan@nxp.com (mailing list archive)
State Rejected, archived
Headers show
Series clk: imx: imx8qxp-lpcg: use devm_platform_ioremap_resource | expand

Commit Message

Peng Fan Dec. 4, 2019, 10:14 a.m. UTC 
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(-)

Comments

Leonard Crestez Dec. 4, 2019, 2:24 p.m. UTC | #1 
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);
>
Peng Fan Dec. 5, 2019, 1:37 a.m. UTC | #2 
> 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);
> >
Leonard Crestez Dec. 5, 2019, 8:59 a.m. UTC | #3 
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
Peng Fan Dec. 5, 2019, 9:03 a.m. UTC | #4 
> 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
diff mbox series

Patch

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);