[PATCH v3 0/3] pwm: imx: add 32k clock for 8qm/qxp

[PATCH v3 0/3] pwm: imx: add 32k clock for 8qm/qxp

[Frank Li]

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v3:
- Using clk bulk API
- Remove PWM errata workaround patch, which sent as seperated patches.
- Link to v2: https://lore.kernel.org/r/20240715-pwm-v2-0-ff3eece83cbb@nxp.com

Changes in v2:
- See each patch
- Link to v1: https://lore.kernel.org/r/20240711-pwm-v1-0-4d5766f99b8b@nxp.com

---
Frank Li (2):
      dt-bindings: pwm: imx: Add optional clock '32k'
      pwm: imx27: Use clk_bulk_*() API to simplify clock handling

Liu Ying (1):
      pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

 Documentation/devicetree/bindings/pwm/imx-pwm.yaml |  4 ++
 drivers/pwm/pwm-imx27.c                            | 74 ++++++++++------------
 2 files changed, 37 insertions(+), 41 deletions(-)
---
base-commit: aef199c66580d309697820e0846d0ba110c81454
change-id: 20240708-pwm-5993e602c9b2

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Frank Li]

The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
increase maxItems for clock and clock-names.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- remove compatible string fsl,imx8qxp-mipi-pwm
---
 Documentation/devicetree/bindings/pwm/imx-pwm.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
index 04148198e34d0..bc6991bd466e1 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.yaml
@@ -51,11 +51,15 @@ properties:
     items:
       - description: SoC PWM ipg clock
       - description: SoC PWM per clock
+      - description: 32k clock
+    minItems: 2
 
   clock-names:
     items:
       - const: ipg
       - const: per
+      - const: 32k
+    minItems: 2
 
   interrupts:
     maxItems: 1

-- 
2.34.1

Re: [PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Marek Vasut]

On 9/10/24 9:07 PM, Frank Li wrote:
> The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
> increase maxItems for clock and clock-names.

This mentions MIPI subsystem, but the IP in question here is PWM.

Are you sure the clock are assigned to the correct IP ?

Shouldn't the clock be assigned to some MIPI IP instead ?

Could you please clarify this in the commit message ?

Re: [PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Frank Li]

On Wed, Sep 11, 2024 at 10:28:52PM +0200, Marek Vasut wrote:
> On 9/10/24 9:07 PM, Frank Li wrote:
> > The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
> > increase maxItems for clock and clock-names.
>
> This mentions MIPI subsystem, but the IP in question here is PWM.

Here, mipi just name of subsystem, not related MIPI IP at all.

There are many IP in i.MX8QXP mipi subsystem, such as i2c, PWM, MIPI PHY,
MIPI controller, PLL, clock-gate, ...

>
> Are you sure the clock are assigned to the correct IP ?
>
> Shouldn't the clock be assigned to some MIPI IP instead ?
>

Are both question still validate if treat 'mipi' just name of subsystem.

> Could you please clarify this in the commit message ?

'mipi' just name of subsystem because the major ip is for MIPI. is word
'mipi-subsystem' better?


Frank

Re: [PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Marek Vasut]

On 9/11/24 11:10 PM, Frank Li wrote:
> On Wed, Sep 11, 2024 at 10:28:52PM +0200, Marek Vasut wrote:
>> On 9/10/24 9:07 PM, Frank Li wrote:
>>> The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
>>> increase maxItems for clock and clock-names.
>>
>> This mentions MIPI subsystem, but the IP in question here is PWM.
> 
> Here, mipi just name of subsystem, not related MIPI IP at all.
> 
> There are many IP in i.MX8QXP mipi subsystem, such as i2c, PWM, MIPI PHY,
> MIPI controller, PLL, clock-gate, ...
> 
>>
>> Are you sure the clock are assigned to the correct IP ?
>>
>> Shouldn't the clock be assigned to some MIPI IP instead ?
>>
> 
> Are both question still validate if treat 'mipi' just name of subsystem.
> 
>> Could you please clarify this in the commit message ?
> 
> 'mipi' just name of subsystem because the major ip is for MIPI. is word
> 'mipi-subsystem' better?
Let's find out.

What is the 32kHz clock used for in the PWM block ?

Re: [PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Frank Li]

On Wed, Sep 11, 2024 at 11:14:15PM +0200, Marek Vasut wrote:
> On 9/11/24 11:10 PM, Frank Li wrote:
> > On Wed, Sep 11, 2024 at 10:28:52PM +0200, Marek Vasut wrote:
> > > On 9/10/24 9:07 PM, Frank Li wrote:
> > > > The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
> > > > increase maxItems for clock and clock-names.
> > >
> > > This mentions MIPI subsystem, but the IP in question here is PWM.
> >
> > Here, mipi just name of subsystem, not related MIPI IP at all.
> >
> > There are many IP in i.MX8QXP mipi subsystem, such as i2c, PWM, MIPI PHY,
> > MIPI controller, PLL, clock-gate, ...
> >
> > >
> > > Are you sure the clock are assigned to the correct IP ?
> > >
> > > Shouldn't the clock be assigned to some MIPI IP instead ?
> > >
> >
> > Are both question still validate if treat 'mipi' just name of subsystem.
> >
> > > Could you please clarify this in the commit message ?
> >
> > 'mipi' just name of subsystem because the major ip is for MIPI. is word
> > 'mipi-subsystem' better?
> Let's find out.
>
> What is the 32kHz clock used for in the PWM block ?

After read document again, it is one option of input, CLKSRC in PWMCR.

Frank

Re: [PATCH v3 1/3] dt-bindings: pwm: imx: Add optional clock '32k'

[Marek Vasut]

On 9/12/24 12:19 AM, Frank Li wrote:
> On Wed, Sep 11, 2024 at 11:14:15PM +0200, Marek Vasut wrote:
>> On 9/11/24 11:10 PM, Frank Li wrote:
>>> On Wed, Sep 11, 2024 at 10:28:52PM +0200, Marek Vasut wrote:
>>>> On 9/10/24 9:07 PM, Frank Li wrote:
>>>>> The pwm in imx8qxp mipi subsystem require one extra '32k' clock. So
>>>>> increase maxItems for clock and clock-names.
>>>>
>>>> This mentions MIPI subsystem, but the IP in question here is PWM.
>>>
>>> Here, mipi just name of subsystem, not related MIPI IP at all.
>>>
>>> There are many IP in i.MX8QXP mipi subsystem, such as i2c, PWM, MIPI PHY,
>>> MIPI controller, PLL, clock-gate, ...
>>>
>>>>
>>>> Are you sure the clock are assigned to the correct IP ?
>>>>
>>>> Shouldn't the clock be assigned to some MIPI IP instead ?
>>>>
>>>
>>> Are both question still validate if treat 'mipi' just name of subsystem.
>>>
>>>> Could you please clarify this in the commit message ?
>>>
>>> 'mipi' just name of subsystem because the major ip is for MIPI. is word
>>> 'mipi-subsystem' better?
>> Let's find out.
>>
>> What is the 32kHz clock used for in the PWM block ?
> 
> After read document again, it is one option of input, CLKSRC in PWMCR.
Thank you for checking.

It seems PWMCR CLKSRC is currently hard-coded to IPG_HIGH in this PWM 
driver, so the 32kHz clock are currently not used ?

The question is, does it make sense to add them ? And if so, what would 
be the use case compared to current IPG_HIGH ?

[PATCH v3 2/3] pwm: imx27: Use clk_bulk_*() API to simplify clock handling

[Frank Li]

Simplify the clock handling logic by using the clk_bulk_*() API.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pwm/pwm-imx27.c | 63 +++++++++++++++++--------------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 14706c3bb96cc..ce9208540f1b8 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -80,9 +80,12 @@
 /* PWMPR register value of 0xffff has the same effect as 0xfffe */
 #define MX3_PWMPR_MAX			0xfffe
 
+static const char * const pwm_imx27_clks[] = {"ipg", "per"};
+#define PWM_IMX27_PER			1
+
 struct pwm_imx27_chip {
-	struct clk	*clk_ipg;
-	struct clk	*clk_per;
+	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
+	int clks_cnt;
 	void __iomem	*mmio_base;
 
 	/*
@@ -98,29 +101,6 @@ static inline struct pwm_imx27_chip *to_pwm_imx27_chip(struct pwm_chip *chip)
 	return pwmchip_get_drvdata(chip);
 }
 
-static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
-{
-	int ret;
-
-	ret = clk_prepare_enable(imx->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(imx->clk_per);
-	if (ret) {
-		clk_disable_unprepare(imx->clk_ipg);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
-{
-	clk_disable_unprepare(imx->clk_per);
-	clk_disable_unprepare(imx->clk_ipg);
-}
-
 static int pwm_imx27_get_state(struct pwm_chip *chip,
 			       struct pwm_device *pwm, struct pwm_state *state)
 {
@@ -129,7 +109,7 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	u64 tmp;
 	int ret;
 
-	ret = pwm_imx27_clk_prepare_enable(imx);
+	ret = clk_bulk_prepare_enable(imx->clks_cnt, imx->clks);
 	if (ret < 0)
 		return ret;
 
@@ -152,7 +132,7 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	}
 
 	prescaler = MX3_PWMCR_PRESCALER_GET(val);
-	pwm_clk = clk_get_rate(imx->clk_per);
+	pwm_clk = clk_get_rate(imx->clks[PWM_IMX27_PER].clk);
 	val = readl(imx->mmio_base + MX3_PWMPR);
 	period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
 
@@ -172,7 +152,7 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	tmp = NSEC_PER_SEC * (u64)(val) * prescaler;
 	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);
 
-	pwm_imx27_clk_disable_unprepare(imx);
+	clk_bulk_disable_unprepare(imx->clks_cnt, imx->clks);
 
 	return 0;
 }
@@ -229,7 +209,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	int ret;
 	u32 cr;
 
-	clkrate = clk_get_rate(imx->clk_per);
+	clkrate = clk_get_rate(imx->clks[PWM_IMX27_PER].clk);
 	c = clkrate * state->period;
 
 	do_div(c, NSEC_PER_SEC);
@@ -259,7 +239,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (pwm->state.enabled) {
 		pwm_imx27_wait_fifo_slot(chip, pwm);
 	} else {
-		ret = pwm_imx27_clk_prepare_enable(imx);
+		ret = clk_bulk_prepare_enable(imx->clks_cnt, imx->clks);
 		if (ret)
 			return ret;
 
@@ -352,7 +332,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(cr, imx->mmio_base + MX3_PWMCR);
 
 	if (!state->enabled)
-		pwm_imx27_clk_disable_unprepare(imx);
+		clk_bulk_disable_unprepare(imx->clks_cnt, imx->clks);
 
 	return 0;
 }
@@ -374,21 +354,22 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	struct pwm_imx27_chip *imx;
 	int ret;
 	u32 pwmcr;
+	int i;
 
 	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*imx));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 	imx = to_pwm_imx27_chip(chip);
 
-	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(imx->clk_ipg))
-		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_ipg),
-				     "getting ipg clock failed\n");
+	imx->clks_cnt = ARRAY_SIZE(pwm_imx27_clks);
+	for (i = 0; i < imx->clks_cnt; ++i)
+		imx->clks[i].id = pwm_imx27_clks[i];
 
-	imx->clk_per = devm_clk_get(&pdev->dev, "per");
-	if (IS_ERR(imx->clk_per))
-		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
-				     "failed to get peripheral clock\n");
+	ret = devm_clk_bulk_get(&pdev->dev, imx->clks_cnt, imx->clks);
+
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "getting clocks failed\n");
 
 	chip->ops = &pwm_imx27_ops;
 
@@ -396,14 +377,14 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	ret = pwm_imx27_clk_prepare_enable(imx);
+	ret = clk_bulk_prepare_enable(imx->clks_cnt, imx->clks);
 	if (ret)
 		return ret;
 
 	/* keep clks on if pwm is running */
 	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
 	if (!(pwmcr & MX3_PWMCR_EN))
-		pwm_imx27_clk_disable_unprepare(imx);
+		clk_bulk_disable_unprepare(imx->clks_cnt, imx->clks);
 
 	return devm_pwmchip_add(&pdev->dev, chip);
 }

-- 
2.34.1

Re: [PATCH v3 2/3] pwm: imx27: Use clk_bulk_*() API to simplify clock handling

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

Hello,

On Tue, Sep 10, 2024 at 03:07:19PM -0400, Frank Li wrote:
> @@ -229,7 +209,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int ret;
>  	u32 cr;
>  
> -	clkrate = clk_get_rate(imx->clk_per);
> +	clkrate = clk_get_rate(imx->clks[PWM_IMX27_PER].clk);
>  	c = clkrate * state->period;

Unrelated to this patch: clk_get_rate() should only be called on enabled
clocks. Given that further down in that function (see next hunk)
pwm_imx27_clk_prepare_enable() (or clk_bulk_prepare_enable()
respectively) is called, that clk might be off?!

>  	do_div(c, NSEC_PER_SEC);
> @@ -259,7 +239,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (pwm->state.enabled) {
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	} else {
> -		ret = pwm_imx27_clk_prepare_enable(imx);
> +		ret = clk_bulk_prepare_enable(imx->clks_cnt, imx->clks);
>  		if (ret)
>  			return ret;
>  

I applied just this patch to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
. The others are still under discussion, right?

I see you signed your patch (which is fine!), but I couldn't find your
key, neither on hkps://keys.openpgp.org/ nor on
hkps://keyserver.ubuntu.com nor in the kernel keyring. At least the
first two should be easy to fix.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] pwm: imx27: Use clk_bulk_*() API to simplify clock handling

[Frank Li]

On Tue, Oct 22, 2024 at 08:53:40AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Sep 10, 2024 at 03:07:19PM -0400, Frank Li wrote:
> > @@ -229,7 +209,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	int ret;
> >  	u32 cr;
> >
> > -	clkrate = clk_get_rate(imx->clk_per);
> > +	clkrate = clk_get_rate(imx->clks[PWM_IMX27_PER].clk);
> >  	c = clkrate * state->period;
>
> Unrelated to this patch: clk_get_rate() should only be called on enabled
> clocks. Given that further down in that function (see next hunk)
> pwm_imx27_clk_prepare_enable() (or clk_bulk_prepare_enable()
> respectively) is called, that clk might be off?!
>
> >  	do_div(c, NSEC_PER_SEC);
> > @@ -259,7 +239,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (pwm->state.enabled) {
> >  		pwm_imx27_wait_fifo_slot(chip, pwm);
> >  	} else {
> > -		ret = pwm_imx27_clk_prepare_enable(imx);
> > +		ret = clk_bulk_prepare_enable(imx->clks_cnt, imx->clks);
> >  		if (ret)
> >  			return ret;
> >
>
> I applied just this patch to
> https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
> . The others are still under discussion, right?

Yes, thanks. I think 32k is not necessary and need more research.

>
> I see you signed your patch (which is fine!), but I couldn't find your
> key, neither on hkps://keys.openpgp.org/ nor on

Thanks. Fixed.

> hkps://keyserver.ubuntu.com nor in the kernel keyring. At least the
> first two should be easy to fix.
>
> Best regards
> Uwe

Re: [PATCH v3 2/3] pwm: imx27: Use clk_bulk_*() API to simplify clock handling

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

Hello,

On Tue, Oct 22, 2024 at 12:01:42PM -0400, Frank Li wrote:
> On Tue, Oct 22, 2024 at 08:53:40AM +0200, Uwe Kleine-König wrote:
> > I applied just this patch to
> > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
> > . The others are still under discussion, right?
> 
> Yes, thanks. I think 32k is not necessary and need more research.

OK, thanks for confirming, I discard these from patchwork.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frank Li]

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

[Marek Vasut]

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

[Frank Li]

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

[Marek Vasut]

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 ?

Re: [PATCH v3 0/3] pwm: imx: add 32k clock for 8qm/qxp

[Marek Vasut]

On 9/10/24 9:07 PM, Frank Li wrote:

Cover letter should contain some high-level description of the series.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
[...]