pwm: imx: add 32k clock for 8qm/qxp

[PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

Posted by Frank Li 2 months, 2 weeks ago

From: Liu Ying <victor.liu@nxp.com>

PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
provides that.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v2 to v3
- use buck clk API

Change from v1 to v2
- remove if check for clk
- use dev_err_probe
- remove int val
---
 drivers/pwm/pwm-imx27.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index ce9208540f1b8..2a9fba6f9d0a8 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -81,10 +81,11 @@
 #define MX3_PWMPR_MAX			0xfffe
 
 static const char * const pwm_imx27_clks[] = {"ipg", "per"};
+static const char * const pwm_imx27_opt_clks[] = {"32k"};
 #define PWM_IMX27_PER			1
 
 struct pwm_imx27_chip {
-	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
+	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks) + ARRAY_SIZE(pwm_imx27_opt_clks)];
 	int clks_cnt;
 	void __iomem	*mmio_base;
 
@@ -371,6 +372,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, ret,
 				     "getting clocks failed\n");
 
+	for (i = 0; i < ARRAY_SIZE(pwm_imx27_opt_clks); i++)
+		imx->clks[i + imx->clks_cnt].id = pwm_imx27_opt_clks[i];
+
+	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(pwm_imx27_opt_clks),
+					 imx->clks + imx->clks_cnt);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "get optional clocks failed\n");
+
+	imx->clks_cnt += ARRAY_SIZE(pwm_imx27_opt_clks);
+
 	chip->ops = &pwm_imx27_ops;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);

-- 
2.34.1

Re: [PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

Posted by Marek Vasut 2 months, 2 weeks ago

On 9/10/24 9:07 PM, Frank Li wrote:
> From: Liu Ying <victor.liu@nxp.com>
> 
> PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
> provides that.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v2 to v3
> - use buck clk API
> 
> Change from v1 to v2
> - remove if check for clk
> - use dev_err_probe
> - remove int val
> ---
>   drivers/pwm/pwm-imx27.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ce9208540f1b8..2a9fba6f9d0a8 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -81,10 +81,11 @@
>   #define MX3_PWMPR_MAX			0xfffe
>   
>   static const char * const pwm_imx27_clks[] = {"ipg", "per"};
> +static const char * const pwm_imx27_opt_clks[] = {"32k"};
>   #define PWM_IMX27_PER			1
>   
>   struct pwm_imx27_chip {
> -	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
> +	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks) + ARRAY_SIZE(pwm_imx27_opt_clks)];
>   	int clks_cnt;
>   	void __iomem	*mmio_base;
>   
> @@ -371,6 +372,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>   		return dev_err_probe(&pdev->dev, ret,
>   				     "getting clocks failed\n");
>   
> +	for (i = 0; i < ARRAY_SIZE(pwm_imx27_opt_clks); i++)
> +		imx->clks[i + imx->clks_cnt].id = pwm_imx27_opt_clks[i];
> +
> +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(pwm_imx27_opt_clks),
> +					 imx->clks + imx->clks_cnt);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "get optional clocks failed\n");
> +
> +	imx->clks_cnt += ARRAY_SIZE(pwm_imx27_opt_clks);
> +

This will succeed even if the regular PWM clock are invalid or not 
present, wouldn't it? I don't think removing that protection is an 
improvement.

Also, it is not clear whether the 32kHz clock are really supplying the 
PWM, see my comment on 1/3 in this series.

Re: [PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

Posted by Frank Li 2 months, 2 weeks ago

On Wed, Sep 11, 2024 at 10:31:40PM +0200, Marek Vasut wrote:
> On 9/10/24 9:07 PM, Frank Li wrote:
> > From: Liu Ying <victor.liu@nxp.com>
> >
> > PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
> > provides that.
> >
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v2 to v3
> > - use buck clk API
> >
> > Change from v1 to v2
> > - remove if check for clk
> > - use dev_err_probe
> > - remove int val
> > ---
> >   drivers/pwm/pwm-imx27.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index ce9208540f1b8..2a9fba6f9d0a8 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -81,10 +81,11 @@
> >   #define MX3_PWMPR_MAX			0xfffe
> >   static const char * const pwm_imx27_clks[] = {"ipg", "per"};
> > +static const char * const pwm_imx27_opt_clks[] = {"32k"};
> >   #define PWM_IMX27_PER			1
> >   struct pwm_imx27_chip {
> > -	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
> > +	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks) + ARRAY_SIZE(pwm_imx27_opt_clks)];
> >   	int clks_cnt;
> >   	void __iomem	*mmio_base;
> > @@ -371,6 +372,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> >   		return dev_err_probe(&pdev->dev, ret,
> >   				     "getting clocks failed\n");
> > +	for (i = 0; i < ARRAY_SIZE(pwm_imx27_opt_clks); i++)
> > +		imx->clks[i + imx->clks_cnt].id = pwm_imx27_opt_clks[i];
> > +
> > +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(pwm_imx27_opt_clks),
> > +					 imx->clks + imx->clks_cnt);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "get optional clocks failed\n");
> > +
> > +	imx->clks_cnt += ARRAY_SIZE(pwm_imx27_opt_clks);
> > +
>
> This will succeed even if the regular PWM clock are invalid or not present,
> wouldn't it? I don't think removing that protection is an improvement.

I have not touch regular PWM clock's code. Just add more optional clocks.

devm_clk_bulk_get(imx->clks);
devm_clk_bulk_get_optional(imx->clks + required_cnt);

so imx->clks have two part {required_part, optional_part};

require part is the same as the before. If it invalidate or not present,
driver will fail probe.

>
> Also, it is not clear whether the 32kHz clock are really supplying the PWM,
> see my comment on 1/3 in this series.

Yes, it is for pwm.

Re: [PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

Posted by Marek Vasut 2 months, 2 weeks ago

On 9/11/24 11:28 PM, Frank Li wrote:
> On Wed, Sep 11, 2024 at 10:31:40PM +0200, Marek Vasut wrote:
>> On 9/10/24 9:07 PM, Frank Li wrote:
>>> From: Liu Ying <victor.liu@nxp.com>
>>>
>>> PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
>>> provides that.
>>>
>>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>> ---
>>> Change from v2 to v3
>>> - use buck clk API
>>>
>>> Change from v1 to v2
>>> - remove if check for clk
>>> - use dev_err_probe
>>> - remove int val
>>> ---
>>>    drivers/pwm/pwm-imx27.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>>> index ce9208540f1b8..2a9fba6f9d0a8 100644
>>> --- a/drivers/pwm/pwm-imx27.c
>>> +++ b/drivers/pwm/pwm-imx27.c
>>> @@ -81,10 +81,11 @@
>>>    #define MX3_PWMPR_MAX			0xfffe
>>>    static const char * const pwm_imx27_clks[] = {"ipg", "per"};
>>> +static const char * const pwm_imx27_opt_clks[] = {"32k"};
>>>    #define PWM_IMX27_PER			1
>>>    struct pwm_imx27_chip {
>>> -	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
>>> +	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks) + ARRAY_SIZE(pwm_imx27_opt_clks)];
>>>    	int clks_cnt;
>>>    	void __iomem	*mmio_base;
>>> @@ -371,6 +372,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>>>    		return dev_err_probe(&pdev->dev, ret,
>>>    				     "getting clocks failed\n");
>>> +	for (i = 0; i < ARRAY_SIZE(pwm_imx27_opt_clks); i++)
>>> +		imx->clks[i + imx->clks_cnt].id = pwm_imx27_opt_clks[i];
>>> +
>>> +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(pwm_imx27_opt_clks),
>>> +					 imx->clks + imx->clks_cnt);
>>> +	if (ret)
>>> +		return dev_err_probe(&pdev->dev, ret, "get optional clocks failed\n");
>>> +
>>> +	imx->clks_cnt += ARRAY_SIZE(pwm_imx27_opt_clks);
>>> +
>>
>> This will succeed even if the regular PWM clock are invalid or not present,
>> wouldn't it? I don't think removing that protection is an improvement.
> 
> I have not touch regular PWM clock's code. Just add more optional clocks.
> 
> devm_clk_bulk_get(imx->clks);
> devm_clk_bulk_get_optional(imx->clks + required_cnt);
> 
> so imx->clks have two part {required_part, optional_part};
> 
> require part is the same as the before. If it invalidate or not present,
> driver will fail probe.

Ah, understood, thank you for clarifying.

>> Also, it is not clear whether the 32kHz clock are really supplying the PWM,
>> see my comment on 1/3 in this series.
> 
> Yes, it is for pwm.
Do the older SoCs (iMX8M or iMX6 or such) also need 32kHz clock for PWM?

I think what I am asking is, what exactly does consume the 32kHz clock 
in the PWM IP ?