Re: [PATCH 2/2] arm64: dts: cix: add FCH(S0)/S5 GPIO controllers for sky1

[Zichar Zhang]

On Sat, Mar 07, 2026 at 04:22:39PM +0100, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On Fri, Mar 06, 2026 at 05:32:38PM +0800, Zichar Zhang wrote:
> > From: "Zichar.Zhang" <zichar.zhang@xxxxxxxxxxx>
> >
> > Add Cadence GPIO controller nodes for Sky1 FCH(S0) and S5 domains in
> > sky1.dtsi, and enable those controllers on sky1-orion-o6.
> >
> > Signed-off-by: Zichar Zhang <zichar.zhang@xxxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/cix/sky1-orion-o6.dts |  28 +++++
> >  arch/arm64/boot/dts/cix/sky1.dtsi         | 123 ++++++++++++++++++++++
> >  2 files changed, 151 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> > index 4dee8cd0b86d..4dc76e0135ee 100644
> > --- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> > +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> > @@ -89,3 +89,31 @@ &pcie_x1_1_rc {
> >  &uart2 {
> >       status = "okay";
> >  };
> > +
> > +&s5_gpio0 {
> 
> I already asked cixtech contributors to read DTS coding style. More than
> once. Does it mean I need to ask EACH contributor that? Maybe create
> internal guideline to avoid trivial mistakes?

Thanks, I’m working on the next revision of this patch.
I have reordered the s5_gpioX and fch_gpioX nodes alphabetically
in the next version of the patch.

> 
> > +     status = "okay";
> > +};
> > +
> > +&s5_gpio1 {
> > +     status = "okay";
> > +};
> > +
> > +&s5_gpio2 {
> > +     status = "okay";
> > +};
> > +
> > +&fch_gpio0 {
> > +     status = "okay";
> > +};
> > +
> > +&fch_gpio1 {
> > +     status = "okay";
> > +};
> > +
> > +&fch_gpio2 {
> > +     status = "okay";
> > +};
> > +
> > +&fch_gpio3 {
> > +     status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> > index 72f3b195a927..9ceaf8f68e83 100644
> > --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> > +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> > @@ -185,6 +185,13 @@ psci {
> >               method = "smc";
> >       };
> >
> > +     s5_gpio_apb_clk: s5-gpio-apb-clk {
> 
> Please use name for all fixed clocks which matches current format
> recommendation: 'clock-<freq>' (see also the pattern in the binding for
> any other options).
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml

I have changed node name as "clock-100000000":
s5_gpio_apb_clk: clock-100000000 {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                clock-frequency = <100000000>;
                clock-output-names = "s5_gpio_apb_clk";
}

> 
> > +             compatible = "fixed-clock";
> > +             #clock-cells = <0>;
> > +             clock-frequency = <100000000>;
> > +             clock-output-names = "s5_gpio_apb_clk";
> > +     };
> > +
> >       soc@0 {
> >               compatible = "simple-bus";
> >               ranges = <0 0 0 0 0x20 0>;
> > @@ -354,6 +361,74 @@ syscon: syscon@4160000 {
> >                       #reset-cells = <1>;
> >               };
> >
> > +             fch_gpio0: gpio-controller@4120000 {
> 
> Again, read DTS coding style.
> 
> > +                     compatible = "cdns,gpio-r1p02";
> > +                     reg = <0x0 0x4120000 0x0 0x1000>;
> > +                     clocks = <&scmi_clk CLK_TREE_FCH_GPIO_APB>;
> > +                     clock-names = "fch_gpio_apb_clk";
> 
> This is pointless name. GPIO block does not take some "fch" input. You
> just called the input clock based on clock output which is completely
> misunderstanding of the DTS.

Thanks, I will remove the property "clock-names" as well as the changes
in yaml file.

fch is a hardware "module" in the Sky1 chip. This module is powered down
when the system enters the S3 and S5 states. It contains several GPIO
controllers, so we refer to them as "fch_gpio". In contrast, "s5_gpio"
remains powered in the S5 state.

Based on the hierarchy, the clock input for fch_gpio on the APB bus is
named fch_gpio_apb_clk. By the same logic, the clock ID macro is named
CLK_TREE_FCH_GPIO_APB.

Best regards,
Zichar Zhang