[PATCH 0/2] i3c: dw: Add APB clk

[PATCH 0/2] i3c: dw: Add APB clk

[Aniket]

The patches add APB clk aka pclk to the
dw i3c driver and the binding doc.

Aniket (2):
  dt-bindings: i3c: dw: Add clock binding
  i3c: dw: Add optional apb clock

 .../bindings/i3c/snps,dw-i3c-master.yaml      | 19 ++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c            | 12 ++++++++++++
 drivers/i3c/master/dw-i3c-master.h            |  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH 1/2] dt-bindings: i3c: dw: Add clock binding

[Aniket]

Add dt binding for optional apb clock.
Core clock is mandatory. Also add
an example.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 .../bindings/i3c/snps,dw-i3c-master.yaml      | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index c0e805e531be..7e3d4f308477 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -20,7 +20,16 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Core interface clock
+      - description: APB interface clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core_clk
+      - const: pclk
 
   interrupts:
     maxItems: 1
@@ -49,4 +58,12 @@ examples:
             pagesize = <0x8>;
         };
     };
+  - |
+    i3c@10400 {
+      compatible = "snps,dw-i3c-master-1.00a";
+      reg = <0x10400 0x1000>;
+      interrupts = <8>;
+      clocks = <&i3c0_cclk>,<&i3c0_pclk>;
+      clock-names = "core_clk", "pclk";
+    };
 ...
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add clock binding

[Conor Dooley]

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

On Tue, Jun 11, 2024 at 05:15:59PM +0000, Aniket wrote:
> Add dt binding for optional apb clock.
> Core clock is mandatory. Also add
> an example.

Please fix your line wrapping here.

> Signed-off-by: Aniket <aniketmaurya@google.com>

Full name please.

> ---
>  .../bindings/i3c/snps,dw-i3c-master.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> index c0e805e531be..7e3d4f308477 100644
> --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> @@ -20,7 +20,16 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Core interface clock

s/interface //.

> +      - description: APB interface clock
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: core_clk
> +      - const: pclk

Why not name them "core" and "apb"? Putting "clk" in both names is
redundant infomration, we already know they're clocks.

>  
>    interrupts:
>      maxItems: 1
> @@ -49,4 +58,12 @@ examples:
>              pagesize = <0x8>;
>          };
>      };
> +  - |
> +    i3c@10400 {

No need for an extra example for this.

Thanks,
Conor.

> +      compatible = "snps,dw-i3c-master-1.00a";
> +      reg = <0x10400 0x1000>;
> +      interrupts = <8>;
> +      clocks = <&i3c0_cclk>,<&i3c0_pclk>;
                              ^^
			missing a space here.

> +      clock-names = "core_clk", "pclk";
> +    };
>  ...
> -- 
> 2.45.2.505.gda0bf45e8d-goog
> 
> 

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

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add clock binding

[Rob Herring (Arm)]

On Tue, 11 Jun 2024 17:15:59 +0000, Aniket wrote:
> Add dt binding for optional apb clock.
> Core clock is mandatory. Also add
> an example.
> 
> Signed-off-by: Aniket <aniketmaurya@google.com>
> ---
>  .../bindings/i3c/snps,dw-i3c-master.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.example.dtb: i3c@10400: '#address-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/snps,dw-i3c-master.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.example.dtb: i3c@10400: '#size-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/snps,dw-i3c-master.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.example.dtb: i3c@10400: '#address-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.example.dtb: i3c@10400: '#size-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240611171600.1105124-2-aniketmaurya@google.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 2/2] i3c: dw: Add optional apb clock

[Aniket]

Besides the core clock, IP also has an apb
interface clock. Add an optional hook for
the same and appropriately enable and disable.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 12 ++++++++++++
 drivers/i3c/master/dw-i3c-master.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 77a2a1c3fd1d..41cdfd6741e3 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1470,12 +1470,20 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_clk))
 		return PTR_ERR(master->core_clk);
 
+	master->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
+	if (IS_ERR(master->pclk))
+		return PTR_ERR(master->pclk);
+
 	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
 								    "core_rst");
 	if (IS_ERR(master->core_rst))
 		return PTR_ERR(master->core_rst);
 
 	ret = clk_prepare_enable(master->core_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(master->pclk);
 	if (ret)
 		goto err_disable_core_clk;
 
@@ -1520,6 +1528,8 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 err_assert_rst:
 	reset_control_assert(master->core_rst);
 
+	clk_disable_unprepare(master->pclk);
+
 err_disable_core_clk:
 	clk_disable_unprepare(master->core_clk);
 
@@ -1533,6 +1543,8 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
 
 	reset_control_assert(master->core_rst);
 
+	clk_disable_unprepare(master->pclk);
+
 	clk_disable_unprepare(master->core_clk);
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 8cb617b8147e..50c38e790c0e 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -36,6 +36,7 @@ struct dw_i3c_master {
 	void __iomem *regs;
 	struct reset_control *core_rst;
 	struct clk *core_clk;
+	struct clk *pclk;
 	char version[5];
 	char type[5];
 	bool ibi_capable;
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH 2/2] i3c: dw: Add optional apb clock

[Alexandre Belloni]

Hello,

On 11/06/2024 17:16:00+0000, Aniket wrote:
> Besides the core clock, IP also has an apb
> interface clock. Add an optional hook for
> the same and appropriately enable and disable.
> 
> Signed-off-by: Aniket <aniketmaurya@google.com>
> ---
>  drivers/i3c/master/dw-i3c-master.c | 12 ++++++++++++
>  drivers/i3c/master/dw-i3c-master.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 77a2a1c3fd1d..41cdfd6741e3 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1470,12 +1470,20 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->core_clk))
>  		return PTR_ERR(master->core_clk);
>  
> +	master->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
> +	if (IS_ERR(master->pclk))
> +		return PTR_ERR(master->pclk);
> +

I guess you need to update the device tree binding documentation too.

>  	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>  								    "core_rst");
>  	if (IS_ERR(master->core_rst))
>  		return PTR_ERR(master->core_rst);
>  
>  	ret = clk_prepare_enable(master->core_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(master->pclk);
>  	if (ret)
>  		goto err_disable_core_clk;
>  
> @@ -1520,6 +1528,8 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  err_assert_rst:
>  	reset_control_assert(master->core_rst);
>  
> +	clk_disable_unprepare(master->pclk);
> +
>  err_disable_core_clk:
>  	clk_disable_unprepare(master->core_clk);
>  
> @@ -1533,6 +1543,8 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
>  
>  	reset_control_assert(master->core_rst);
>  
> +	clk_disable_unprepare(master->pclk);
> +
>  	clk_disable_unprepare(master->core_clk);
>  }
>  EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> index 8cb617b8147e..50c38e790c0e 100644
> --- a/drivers/i3c/master/dw-i3c-master.h
> +++ b/drivers/i3c/master/dw-i3c-master.h
> @@ -36,6 +36,7 @@ struct dw_i3c_master {
>  	void __iomem *regs;
>  	struct reset_control *core_rst;
>  	struct clk *core_clk;
> +	struct clk *pclk;
>  	char version[5];
>  	char type[5];
>  	bool ibi_capable;
> -- 
> 2.45.2.505.gda0bf45e8d-goog
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] i3c: dw: Add optional apb clock

[Alexandre Belloni]

On 18/06/2024 22:06:00+0200, Alexandre Belloni wrote:
> Hello,
> 
> On 11/06/2024 17:16:00+0000, Aniket wrote:
> > Besides the core clock, IP also has an apb
> > interface clock. Add an optional hook for
> > the same and appropriately enable and disable.
> > 
> > Signed-off-by: Aniket <aniketmaurya@google.com>
> > ---
> >  drivers/i3c/master/dw-i3c-master.c | 12 ++++++++++++
> >  drivers/i3c/master/dw-i3c-master.h |  1 +
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> > index 77a2a1c3fd1d..41cdfd6741e3 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
> > @@ -1470,12 +1470,20 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> >  	if (IS_ERR(master->core_clk))
> >  		return PTR_ERR(master->core_clk);
> >  
> > +	master->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
> > +	if (IS_ERR(master->pclk))
> > +		return PTR_ERR(master->pclk);
> > +
> 
> I guess you need to update the device tree binding documentation too.

Sorry, you did and you had changes requested, forget about that.
> 
> >  	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> >  								    "core_rst");
> >  	if (IS_ERR(master->core_rst))
> >  		return PTR_ERR(master->core_rst);
> >  
> >  	ret = clk_prepare_enable(master->core_clk);

It could be worth having a look at devm_clk_get_optional_enabled

> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(master->pclk);
> >  	if (ret)
> >  		goto err_disable_core_clk;
> >  
> > @@ -1520,6 +1528,8 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> >  err_assert_rst:
> >  	reset_control_assert(master->core_rst);
> >  
> > +	clk_disable_unprepare(master->pclk);
> > +
> >  err_disable_core_clk:
> >  	clk_disable_unprepare(master->core_clk);
> >  
> > @@ -1533,6 +1543,8 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
> >  
> >  	reset_control_assert(master->core_rst);
> >  
> > +	clk_disable_unprepare(master->pclk);
> > +
> >  	clk_disable_unprepare(master->core_clk);
> >  }
> >  EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
> > diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> > index 8cb617b8147e..50c38e790c0e 100644
> > --- a/drivers/i3c/master/dw-i3c-master.h
> > +++ b/drivers/i3c/master/dw-i3c-master.h
> > @@ -36,6 +36,7 @@ struct dw_i3c_master {
> >  	void __iomem *regs;
> >  	struct reset_control *core_rst;
> >  	struct clk *core_clk;
> > +	struct clk *pclk;
> >  	char version[5];
> >  	char type[5];
> >  	bool ibi_capable;
> > -- 
> > 2.45.2.505.gda0bf45e8d-goog
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] i3c: dw: Add optional apb clock

[Aniket .]

Hey,
> > >     ret = clk_prepare_enable(master->core_clk);
>
> It could be worth having a look at devm_clk_get_optional_enabled
Do we want to use *_enabled clock apis for both core_clk and pclk?

Thanks,
Aniket

Re: [PATCH 2/2] i3c: dw: Add optional apb clock

[Alexandre Belloni]

On 20/06/2024 11:42:25+0530, Aniket . wrote:
> Hey,
> > > >     ret = clk_prepare_enable(master->core_clk);
> >
> > It could be worth having a look at devm_clk_get_optional_enabled
> Do we want to use *_enabled clock apis for both core_clk and pclk?

Yes

> 
> Thanks,
> Aniket
> 
> -- 
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 0/3] i3c: dw: Add apb clk

[Aniket]

These patches add APB clk aka pclk to the dw i3c driver
and the binding doc. Also move to _enabled clk_get APIs.

Aniket (3):
  dt-bindings: i3c: dw: Add apb clock binding
  i3c: dw: Add optional apb clock
  i3c: dw: Use new *_enabled clk APIs

 .../bindings/i3c/snps,dw-i3c-master.yaml          | 11 ++++++++++-
 drivers/i3c/master/dw-i3c-master.c                | 15 +++++----------
 drivers/i3c/master/dw-i3c-master.h                |  1 +
 3 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.45.2.741.gdbec12cfda-goog

[PATCH v2 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Aniket]

Add dt binding for optional apb clock. Core clock is mandatory.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 .../devicetree/bindings/i3c/snps,dw-i3c-master.yaml   | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index c0e805e531be..4fc13e3c0f75 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -20,7 +20,16 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Core clock
+      - description: APB clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: apb
 
   interrupts:
     maxItems: 1
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH v2 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Krzysztof Kozlowski]

On 24/06/2024 07:28, Aniket wrote:
> Add dt binding for optional apb clock. Core clock is mandatory.
> 
> Signed-off-by: Aniket <aniketmaurya@google.com>

Your email still suggests mismatch with name. Please confirm that above
this is you full name or known identity which you want to consistently
use across all contributions. In case of doubts: please consult
colleagues in Google (or your legal department, dunno).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Aniket .]

> Your email still suggests mismatch with name. Please confirm that above
> this is you full name or known identity which you want to consistently
> use across all contributions. In case of doubts: please consult
> colleagues in Google (or your legal department, dunno).

Hey, my full legal name is "Aniket". Please don't mind the text in the email-id.

Thanks,
Aniket.

[PATCH v2 2/3] i3c: dw: Add optional apb clock

[Aniket]

Besides the core clock, IP also has an apb interface clock.
Add an optional hook for the same and appropriately enable/disable.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 12 ++++++++++++
 drivers/i3c/master/dw-i3c-master.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 77a2a1c3fd1d..41cdfd6741e3 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1470,12 +1470,20 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_clk))
 		return PTR_ERR(master->core_clk);
 
+	master->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
+	if (IS_ERR(master->pclk))
+		return PTR_ERR(master->pclk);
+
 	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
 								    "core_rst");
 	if (IS_ERR(master->core_rst))
 		return PTR_ERR(master->core_rst);
 
 	ret = clk_prepare_enable(master->core_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(master->pclk);
 	if (ret)
 		goto err_disable_core_clk;
 
@@ -1520,6 +1528,8 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 err_assert_rst:
 	reset_control_assert(master->core_rst);
 
+	clk_disable_unprepare(master->pclk);
+
 err_disable_core_clk:
 	clk_disable_unprepare(master->core_clk);
 
@@ -1533,6 +1543,8 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
 
 	reset_control_assert(master->core_rst);
 
+	clk_disable_unprepare(master->pclk);
+
 	clk_disable_unprepare(master->core_clk);
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 8cb617b8147e..50c38e790c0e 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -36,6 +36,7 @@ struct dw_i3c_master {
 	void __iomem *regs;
 	struct reset_control *core_rst;
 	struct clk *core_clk;
+	struct clk *pclk;
 	char version[5];
 	char type[5];
 	bool ibi_capable;
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH v2 3/3] i3c: dw: Use new *_enabled clk APIs

[Aniket]

Move to "enabled" variant of clk_get APIs. It takes care
of enable and disable calls during the probe and remove.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 41cdfd6741e3..37092b8e964f 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1466,11 +1466,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->regs))
 		return PTR_ERR(master->regs);
 
-	master->core_clk = devm_clk_get(&pdev->dev, NULL);
+	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(master->core_clk))
 		return PTR_ERR(master->core_clk);
 
-	master->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
+	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
 	if (IS_ERR(master->pclk))
 		return PTR_ERR(master->pclk);
 
@@ -1479,14 +1479,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_rst))
 		return PTR_ERR(master->core_rst);
 
-	ret = clk_prepare_enable(master->core_clk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(master->pclk);
-	if (ret)
-		goto err_disable_core_clk;
-
 	reset_control_deassert(master->core_rst);
 
 	spin_lock_init(&master->xferqueue.lock);
@@ -1528,11 +1520,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 err_assert_rst:
 	reset_control_assert(master->core_rst);
 
-	clk_disable_unprepare(master->pclk);
-
-err_disable_core_clk:
-	clk_disable_unprepare(master->core_clk);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_probe);
@@ -1542,10 +1529,6 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
 	i3c_master_unregister(&master->base);
 
 	reset_control_assert(master->core_rst);
-
-	clk_disable_unprepare(master->pclk);
-
-	clk_disable_unprepare(master->core_clk);
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
 
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH v2 0/3] i3c: dw: Add apb clk

[Krzysztof Kozlowski]

On 24/06/2024 07:28, Aniket wrote:
> These patches add APB clk aka pclk to the dw i3c driver
> and the binding doc. Also move to _enabled clk_get APIs.

Where is the changelog?

Best regards,
Krzysztof

Re: [PATCH v2 0/3] i3c: dw: Add apb clk

[Aniket .]

> Where is the changelog?
Sorry, I missed the changelog.

Changes from v1 to v2
- extra example removed from yaml file, minor renaming.
- 3rd patch added to migrate to *_enabled clk_get APIs.

Thanks,
Aniket.

Re: [PATCH v2 0/3] i3c: dw: Add apb clk

[Alexandre Belloni]

On 24/06/2024 05:28:48+0000, Aniket wrote:
> These patches add APB clk aka pclk to the dw i3c driver
> and the binding doc. Also move to _enabled clk_get APIs.
> 
> Aniket (3):
>   dt-bindings: i3c: dw: Add apb clock binding
>   i3c: dw: Add optional apb clock
>   i3c: dw: Use new *_enabled clk APIs

You should reorder your patches to have 3/3 before 2/3, else you
introduce code that you immediately remove.

> 
>  .../bindings/i3c/snps,dw-i3c-master.yaml          | 11 ++++++++++-
>  drivers/i3c/master/dw-i3c-master.c                | 15 +++++----------
>  drivers/i3c/master/dw-i3c-master.h                |  1 +
>  3 files changed, 16 insertions(+), 11 deletions(-)
> 
> -- 
> 2.45.2.741.gdbec12cfda-goog
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/3] i3c: dw: Add apb clk

[Aniket .]

> >   dt-bindings: i3c: dw: Add apb clock binding
> >   i3c: dw: Add optional apb clock
> >   i3c: dw: Use new *_enabled clk APIs
>
> You should reorder your patches to have 3/3 before 2/3, else you
> introduce code that you immediately remove.

Ahh! Sorry, let me fix this.

Thanks,
Aniket.

[PATCH v3 0/3] i3c: dw: Add apb clk

[Aniket]

These patches add APB clk aka pclk to the dw i3c driver
and the binding doc. Also move to _enabled clk_get APIs.

Changes from v2 to v3
  - moved to _enabled API first and then added apb clk.

Changes from v1 to v2
 - extra example removed from yaml file, minor renaming.
 - 3rd patch added to migrate to *_enabled clk_get APIs. 

Aniket (3):
  dt-bindings: i3c: dw: Add apb clock binding
  i3c: dw: Use new *_enabled clk API
  i3c: dw: Add optional apb clock

 .../bindings/i3c/snps,dw-i3c-master.yaml          | 11 ++++++++++-
 drivers/i3c/master/dw-i3c-master.c                | 15 +++++----------
 drivers/i3c/master/dw-i3c-master.h                |  1 +
 3 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH v3 0/3] i3c: dw: Add apb clk

[Alexandre Belloni]

On Fri, 28 Jun 2024 15:38:26 +0000, Aniket wrote:
> These patches add APB clk aka pclk to the dw i3c driver
> and the binding doc. Also move to _enabled clk_get APIs.
> 
> Changes from v2 to v3
>   - moved to _enabled API first and then added apb clk.
> 
> Changes from v1 to v2
>  - extra example removed from yaml file, minor renaming.
>  - 3rd patch added to migrate to *_enabled clk_get APIs.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: i3c: dw: Add apb clock binding
      https://git.kernel.org/abelloni/c/a75d62ef7c30
[2/3] i3c: dw: Use new *_enabled clk API
      https://git.kernel.org/abelloni/c/48a74bb38b24
[3/3] i3c: dw: Add optional apb clock
      https://git.kernel.org/abelloni/c/145ca7b61e95

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Aniket]

Add dt binding for optional apb clock. Core clock is mandatory.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 .../devicetree/bindings/i3c/snps,dw-i3c-master.yaml   | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index c0e805e531be..4fc13e3c0f75 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -20,7 +20,16 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Core clock
+      - description: APB clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: apb
 
   interrupts:
     maxItems: 1
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH v3 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Conor Dooley]

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

On Fri, Jun 28, 2024 at 03:45:26PM +0000, Aniket wrote:
> Add dt binding for optional apb clock. Core clock is mandatory.
> 
> Signed-off-by: Aniket <aniketmaurya@google.com>

Missing a review from Krzysztof:
https://lore.kernel.org/all/e3160411-59e0-4806-a00d-b99564384180@linaro.org/

Also, please do not send a new version of a series as a reply to a
previous version. It'll bury it in people's mailboxes depending on how
they sort.

Cheers,
Conor.

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

Re: [PATCH v3 1/3] dt-bindings: i3c: dw: Add apb clock binding

[Aniket .]

> Missing a review from Krzysztof:
> https://lore.kernel.org/all/e3160411-59e0-4806-a00d-b99564384180@linaro.org/
>
> Also, please do not send a new version of a series as a reply to a
> previous version. It'll bury it in people's mailboxes depending on how
> they sort.

Got it, will keep in mind next time.

Thanks,
Aniket

[PATCH v3 2/3] i3c: dw: Use new *_enabled clk API

[Aniket]

Move to "enabled" variant of clk_get API. It takes care
of enable and disable calls during the probe and remove.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 9aae5c8dba8d..4e3335641dcd 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1450,7 +1450,7 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->regs))
 		return PTR_ERR(master->regs);
 
-	master->core_clk = devm_clk_get(&pdev->dev, NULL);
+	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(master->core_clk))
 		return PTR_ERR(master->core_clk);
 
@@ -1459,10 +1459,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_rst))
 		return PTR_ERR(master->core_rst);
 
-	ret = clk_prepare_enable(master->core_clk);
-	if (ret)
-		goto err_disable_core_clk;
-
 	reset_control_deassert(master->core_rst);
 
 	spin_lock_init(&master->xferqueue.lock);
@@ -1501,9 +1497,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 err_assert_rst:
 	reset_control_assert(master->core_rst);
 
-err_disable_core_clk:
-	clk_disable_unprepare(master->core_clk);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_probe);
@@ -1513,8 +1506,6 @@ void dw_i3c_common_remove(struct dw_i3c_master *master)
 	i3c_master_unregister(&master->base);
 
 	reset_control_assert(master->core_rst);
-
-	clk_disable_unprepare(master->core_clk);
 }
 EXPORT_SYMBOL_GPL(dw_i3c_common_remove);
 
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v3 3/3] i3c: dw: Add optional apb clock

[Aniket]

Besides the core clock, IP also has an apb interface clock.
Add an optional hook for the same.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 4 ++++
 drivers/i3c/master/dw-i3c-master.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 4e3335641dcd..0ca41782f3a6 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1454,6 +1454,10 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_clk))
 		return PTR_ERR(master->core_clk);
 
+	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
+	if (IS_ERR(master->pclk))
+		return PTR_ERR(master->pclk);
+
 	master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
 								    "core_rst");
 	if (IS_ERR(master->core_rst))
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index 7e76ca381d9f..fb7121c6c687 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -36,6 +36,7 @@ struct dw_i3c_master {
 	void __iomem *regs;
 	struct reset_control *core_rst;
 	struct clk *core_clk;
+	struct clk *pclk;
 	char version[5];
 	char type[5];
 	u32 sir_rej_mask;
-- 
2.45.2.803.g4e1b14247a-goog