1. Patchew
  2. linux
  3. [PATCH 0/2] pinctrl: spacemit: add clock/reset support
  4. View patch

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

Yixun Lan posted 2 patches 10 months ago
There is a newer version of this series
[PATCH 2/2] pinctrl: spacemit: add clock support for K1 SoC
Posted by Yixun Lan 10 months ago 
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
Posted by Rob Herring 10 months ago 
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
Posted by Yixun Lan 10 months ago 
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
Posted by Rob Herring 9 months, 4 weeks ago 
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
Posted by Yixun Lan 9 months, 4 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.