[PATCH 0/2] pinctrl: spacemit: add clock/reset support

[PATCH 0/2] pinctrl: spacemit: add clock/reset support

[Yixun Lan]

SpacemiT K1 pinctrl requires two clocks in order to be functional,
also one reset line from hardware persepective.

In this series, adding clock property in dt-binding, and activate
them in the driver. But for reset, making it optional for now.

Notice that, devm_clk_get_optional_enabled() API has be choosed,
which will make old dtb blob still functioinal with new kernel.

For DT part patch, I plan to submit after clock driver merged.
This may result dtb warnings in this version due to the mising
clock property in pinctrl dt node.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Yixun Lan (2):
      dt-bindings: pinctrl: spacemit: add clock and reset property
      pinctrl: spacemit: add clock support for K1 SoC

 .../bindings/pinctrl/spacemit,k1-pinctrl.yaml          | 18 ++++++++++++++++++
 drivers/pinctrl/spacemit/pinctrl-k1.c                  | 10 ++++++++++
 2 files changed, 28 insertions(+)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250412-02-k1-pinctrl-clk-9649d6ad22c4

Best regards,
-- 
Yixun Lan

[PATCH 1/2] dt-bindings: pinctrl: spacemit: add clock and reset property

[Yixun Lan]

SpacemiT K1 SoC's pinctrl controller requires two clocks in order
to work propertly, also has one reset line from hardware persepective.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 .../bindings/pinctrl/spacemit,k1-pinctrl.yaml          | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
index b01ecd83b71b5e9f47256adde23bc8b183bef05e..d80e88aa07b45f4a3d90e8217caf93d32655927d 100644
--- a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
@@ -17,6 +17,19 @@ properties:
     items:
       - description: pinctrl io memory base
 
+  clocks:
+    items:
+      - description: Functional Clock
+      - description: Bus Clock
+
+  clock-names:
+    items:
+      - const: func
+      - const: bus
+
+  resets:
+    maxItems: 1
+
 patternProperties:
   '-cfg$':
     type: object
@@ -94,6 +107,8 @@ patternProperties:
 required:
   - compatible
   - reg
+  - clocks
+  - clock-names
 
 additionalProperties: false
 
@@ -108,6 +123,9 @@ examples:
         pinctrl@d401e000 {
             compatible = "spacemit,k1-pinctrl";
             reg = <0x0 0xd401e000 0x0 0x400>;
+            clocks = <&syscon_apbc 42>,
+                     <&syscon_apbc 94>;
+            clock-names = "func", "bus";
 
             uart0_2_cfg: uart0-2-cfg {
                 uart0-2-pins {

-- 
2.49.0

Re: [PATCH 1/2] dt-bindings: pinctrl: spacemit: add clock and reset property

[ALOK TIWARI]

On 12-04-2025 12:28, Yixun Lan wrote:
> SpacemiT K1 SoC's pinctrl controller requires two clocks in order
> to work propertly, also has one reset line from hardware persepective.

typo in commit message
propertly -> properly
persepective -> perspective

> 
> Signed-off-by: Yixun Lan<dlan@gentoo.org>
> ---
>   .../bindings/pinctrl/spacemit,k1-pinctrl.yaml


Thanks,
Alok

Re: [PATCH 1/2] dt-bindings: pinctrl: spacemit: add clock and reset property

[Yixun Lan]

Hi ALOK,

On 17:01 Sat 12 Apr     , ALOK TIWARI wrote:
> 
> 
> On 12-04-2025 12:28, Yixun Lan wrote:
> > SpacemiT K1 SoC's pinctrl controller requires two clocks in order
> > to work propertly, also has one reset line from hardware persepective.
> 
> typo in commit message
> propertly -> properly
> persepective -> perspective
> 
thanks for catching this..
will wait for more comments, and fix in next version

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

[PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC

[Yixun Lan]

For SpacemiT K1 SoC's pinctrl, explicitly acquiring clocks in
the driver instead of relying on bootloader or default hardware
settings to enable it.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/pinctrl/spacemit/pinctrl-k1.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
index 67e867b04a02ea1887d93aedfdea5bda037f88b1..3805fb09c1bc3b8cf2ccfc22dd25367292b397b9 100644
--- a/drivers/pinctrl/spacemit/pinctrl-k1.c
+++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> */
 
 #include <linux/bits.h>
+#include <linux/clk.h>
 #include <linux/cleanup.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -721,6 +722,7 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct spacemit_pinctrl *pctrl;
+	struct clk *func_clk, *bus_clk;
 	const struct spacemit_pinctrl_data *pctrl_data;
 	int ret;
 
@@ -739,6 +741,14 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
 	if (IS_ERR(pctrl->regs))
 		return PTR_ERR(pctrl->regs);
 
+	func_clk = devm_clk_get_optional_enabled(dev, "func");
+	if (IS_ERR(func_clk))
+		return dev_err_probe(dev, PTR_ERR(func_clk), "failed to get func clock\n");
+
+	bus_clk = devm_clk_get_optional_enabled(dev, "bus");
+	if (IS_ERR(bus_clk))
+		return dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
+
 	pctrl->pdesc.name = dev_name(dev);
 	pctrl->pdesc.pins = pctrl_data->pins;
 	pctrl->pdesc.npins = pctrl_data->npins;

-- 
2.49.0

Re: [PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC

[Rob Herring]

On Sat, Apr 12, 2025 at 02:58:11PM +0800, Yixun Lan wrote:
> For SpacemiT K1 SoC's pinctrl, explicitly acquiring clocks in
> the driver instead of relying on bootloader or default hardware
> settings to enable it.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  drivers/pinctrl/spacemit/pinctrl-k1.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> index 67e867b04a02ea1887d93aedfdea5bda037f88b1..3805fb09c1bc3b8cf2ccfc22dd25367292b397b9 100644
> --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> */
>  
>  #include <linux/bits.h>
> +#include <linux/clk.h>
>  #include <linux/cleanup.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> @@ -721,6 +722,7 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct spacemit_pinctrl *pctrl;
> +	struct clk *func_clk, *bus_clk;
>  	const struct spacemit_pinctrl_data *pctrl_data;
>  	int ret;
>  
> @@ -739,6 +741,14 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
>  	if (IS_ERR(pctrl->regs))
>  		return PTR_ERR(pctrl->regs);
>  
> +	func_clk = devm_clk_get_optional_enabled(dev, "func");
> +	if (IS_ERR(func_clk))
> +		return dev_err_probe(dev, PTR_ERR(func_clk), "failed to get func clock\n");
> +
> +	bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> +	if (IS_ERR(bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");

Do you really need these to be optional? Yes, it maintains 
compatibility, but if this platform isn't stable, then do you really 
need that?

Rob

Re: [PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC

[Yixun Lan]

Hi Rob,

On 13:27 Sat 12 Apr     , Rob Herring wrote:
> On Sat, Apr 12, 2025 at 02:58:11PM +0800, Yixun Lan wrote:
> > For SpacemiT K1 SoC's pinctrl, explicitly acquiring clocks in
> > the driver instead of relying on bootloader or default hardware
> > settings to enable it.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  drivers/pinctrl/spacemit/pinctrl-k1.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > index 67e867b04a02ea1887d93aedfdea5bda037f88b1..3805fb09c1bc3b8cf2ccfc22dd25367292b397b9 100644
> > --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> > +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > @@ -2,6 +2,7 @@
> >  /* Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> */
> >  
> >  #include <linux/bits.h>
> > +#include <linux/clk.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/io.h>
> >  #include <linux/of.h>
> > @@ -721,6 +722,7 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct spacemit_pinctrl *pctrl;
> > +	struct clk *func_clk, *bus_clk;
> >  	const struct spacemit_pinctrl_data *pctrl_data;
> >  	int ret;
> >  
> > @@ -739,6 +741,14 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pctrl->regs))
> >  		return PTR_ERR(pctrl->regs);
> >  
> > +	func_clk = devm_clk_get_optional_enabled(dev, "func");
> > +	if (IS_ERR(func_clk))
> > +		return dev_err_probe(dev, PTR_ERR(func_clk), "failed to get func clock\n");
> > +
> > +	bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > +	if (IS_ERR(bus_clk))
> > +		return dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> 
> Do you really need these to be optional? Yes, it maintains 
>
Yes, the motivation here to make it optional is maintaining the 
compatibility, to not break the case of using old dtb with new kernel,
since the serial console device (uart0) is activated now [1]

IIUC, from the DT perspective, it's mandatory to keep this compatibility?

In dt-binding of patch [1/2], the clocks/clock-names has been described as 
required property, so it's sort of mandatory from DT's view.

One lesson I learned is that the pinctrl dt node shouldn't be activated
untill all prerequisite dependencies meet.. it's ok to push the driver,
but should postpone the DT part..

> compatibility, but if this platform isn't stable, then do you really 
> need that?
> 
I don't get what's your meaning of "isn't stable" here, the fact won't
change for K1 SoC: the pinctrl controller requires two clocks

[1] https://github.com/torvalds/linux/blob/master/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts#L22

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC

[Rob Herring]

On Sat, Apr 12, 2025 at 10:34:31PM +0000, Yixun Lan wrote:
> Hi Rob,
> 
> On 13:27 Sat 12 Apr     , Rob Herring wrote:
> > On Sat, Apr 12, 2025 at 02:58:11PM +0800, Yixun Lan wrote:
> > > For SpacemiT K1 SoC's pinctrl, explicitly acquiring clocks in
> > > the driver instead of relying on bootloader or default hardware
> > > settings to enable it.
> > > 
> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > ---
> > >  drivers/pinctrl/spacemit/pinctrl-k1.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > index 67e867b04a02ea1887d93aedfdea5bda037f88b1..3805fb09c1bc3b8cf2ccfc22dd25367292b397b9 100644
> > > --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > @@ -2,6 +2,7 @@
> > >  /* Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> */
> > >  
> > >  #include <linux/bits.h>
> > > +#include <linux/clk.h>
> > >  #include <linux/cleanup.h>
> > >  #include <linux/io.h>
> > >  #include <linux/of.h>
> > > @@ -721,6 +722,7 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > >  	struct spacemit_pinctrl *pctrl;
> > > +	struct clk *func_clk, *bus_clk;
> > >  	const struct spacemit_pinctrl_data *pctrl_data;
> > >  	int ret;
> > >  
> > > @@ -739,6 +741,14 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> > >  	if (IS_ERR(pctrl->regs))
> > >  		return PTR_ERR(pctrl->regs);
> > >  
> > > +	func_clk = devm_clk_get_optional_enabled(dev, "func");
> > > +	if (IS_ERR(func_clk))
> > > +		return dev_err_probe(dev, PTR_ERR(func_clk), "failed to get func clock\n");
> > > +
> > > +	bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > > +	if (IS_ERR(bus_clk))
> > > +		return dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> > 
> > Do you really need these to be optional? Yes, it maintains 
> >
> Yes, the motivation here to make it optional is maintaining the 
> compatibility, to not break the case of using old dtb with new kernel,
> since the serial console device (uart0) is activated now [1]
> 
> IIUC, from the DT perspective, it's mandatory to keep this compatibility?

It's up to the platform. It's only mandatory to understand you are 
breaking compatibility when you do.

> 
> In dt-binding of patch [1/2], the clocks/clock-names has been described as 
> required property, so it's sort of mandatory from DT's view.
> 
> One lesson I learned is that the pinctrl dt node shouldn't be activated
> untill all prerequisite dependencies meet.. it's ok to push the driver,
> but should postpone the DT part..
> 
> > compatibility, but if this platform isn't stable, then do you really 
> > need that?
> > 
> I don't get what's your meaning of "isn't stable" here, the fact won't
> change for K1 SoC: the pinctrl controller requires two clocks

You required 1 clock and now you require 2. That's not stable. 
Generally, early on with platforms, their DT is not complete enough to 
maintain compatibility. There's also likely not many users or h/w 
availability for compatibility to be an issue. So requiring the DT to be 
in-sync with the kernel is not a problem.

Rob

Re: [PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC

[Yixun Lan]

Hi Rob,

On 14:38 Tue 15 Apr     , Rob Herring wrote:
> On Sat, Apr 12, 2025 at 10:34:31PM +0000, Yixun Lan wrote:
> > Hi Rob,
> > 
> > On 13:27 Sat 12 Apr     , Rob Herring wrote:
> > > On Sat, Apr 12, 2025 at 02:58:11PM +0800, Yixun Lan wrote:
> > > > For SpacemiT K1 SoC's pinctrl, explicitly acquiring clocks in
> > > > the driver instead of relying on bootloader or default hardware
> > > > settings to enable it.
> > > > 
> > > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > > ---
> > > >  drivers/pinctrl/spacemit/pinctrl-k1.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > > index 67e867b04a02ea1887d93aedfdea5bda037f88b1..3805fb09c1bc3b8cf2ccfc22dd25367292b397b9 100644
> > > > --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > > +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > > > @@ -2,6 +2,7 @@
> > > >  /* Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> */
> > > >  
> > > >  #include <linux/bits.h>
> > > > +#include <linux/clk.h>
> > > >  #include <linux/cleanup.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/of.h>
> > > > @@ -721,6 +722,7 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct device *dev = &pdev->dev;
> > > >  	struct spacemit_pinctrl *pctrl;
> > > > +	struct clk *func_clk, *bus_clk;
> > > >  	const struct spacemit_pinctrl_data *pctrl_data;
> > > >  	int ret;
> > > >  
> > > > @@ -739,6 +741,14 @@ static int spacemit_pinctrl_probe(struct platform_device *pdev)
> > > >  	if (IS_ERR(pctrl->regs))
> > > >  		return PTR_ERR(pctrl->regs);
> > > >  
> > > > +	func_clk = devm_clk_get_optional_enabled(dev, "func");
> > > > +	if (IS_ERR(func_clk))
> > > > +		return dev_err_probe(dev, PTR_ERR(func_clk), "failed to get func clock\n");
> > > > +
> > > > +	bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > > > +	if (IS_ERR(bus_clk))
> > > > +		return dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> > > 
> > > Do you really need these to be optional? Yes, it maintains 
> > >
> > Yes, the motivation here to make it optional is maintaining the 
> > compatibility, to not break the case of using old dtb with new kernel,
> > since the serial console device (uart0) is activated now [1]
> > 
> > IIUC, from the DT perspective, it's mandatory to keep this compatibility?
> 
> It's up to the platform. It's only mandatory to understand you are 
> breaking compatibility when you do.
> 
> > 
> > In dt-binding of patch [1/2], the clocks/clock-names has been described as 
> > required property, so it's sort of mandatory from DT's view.
> > 
> > One lesson I learned is that the pinctrl dt node shouldn't be activated
> > untill all prerequisite dependencies meet.. it's ok to push the driver,
> > but should postpone the DT part..
> > 
> > > compatibility, but if this platform isn't stable, then do you really 
> > > need that?
> > > 
> > I don't get what's your meaning of "isn't stable" here, the fact won't
> > change for K1 SoC: the pinctrl controller requires two clocks
> 
> You required 1 clock and now you require 2. That's not stable. 
previously pinctrl requires no clock (no clock property populated)

> Generally, early on with platforms, their DT is not complete enough to 
> maintain compatibility. There's also likely not many users or h/w 
> availability for compatibility to be an issue. So requiring the DT to be 
> in-sync with the kernel is not a problem.
> 
Ok, then in this case, I will simply drop this _optional_ API

thanks

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55