| Message ID | 20250412-02-k1-pinctrl-clk-v1-2-e39734419a2d@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: spacemit: add clock/reset support | expand |
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
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
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;
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(+)