RE: [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree nodes

-----Original Message-----
From: Krzysztof Kozlowski <redacted>
Sent: Thursday, December 21, 2023 11:53 PM
To: Yuklin Soo <redacted>; Linus Walleij
[off-list ref]; Bartosz Golaszewski
[off-list ref]; Hal Feng [off-list ref];
Leyfoon Tan [off-list ref]; Jianlong Huang
[off-list ref]; Emil Renner Berthing
[off-list ref]; Rob Herring [off-list ref]; Krzysztof Kozlowski
[off-list ref]; Conor Dooley [off-list ref];
Drew Fustini [off-list ref]
Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; Paul Walmsley
[off-list ref]; Palmer Dabbelt [off-list ref];
Albert Ou [off-list ref]
Subject: Re: [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree
nodes

On 21/12/2023 09:36, Alex Soo wrote:

quoted

Add pinctrl_east/pinctrl_west/pinctrl_gmac/pinctrl_aon device tree
nodes for JH8100 SoC.

Signed-off-by: Alex Soo <redacted>
Reviewed-by: Ley Foon Tan <redacted>

I have some doubts about it...

quoted

---
 arch/riscv/boot/dts/starfive/jh8100-evb.dts   |   5 +
 arch/riscv/boot/dts/starfive/jh8100-pinfunc.h | 418 ++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh8100.dtsi      |  44 ++
 3 files changed, 467 insertions(+)
 create mode 100644 arch/riscv/boot/dts/starfive/jh8100-pinfunc.h

diff --git a/arch/riscv/boot/dts/starfive/jh8100-evb.dts

b/arch/riscv/boot/dts/starfive/jh8100-evb.dts
index c16bc25d8988..8634e41984f8 100644

--- a/arch/riscv/boot/dts/starfive/jh8100-evb.dts
+++ b/arch/riscv/boot/dts/starfive/jh8100-evb.dts

@@ -26,3 +26,8 @@ memory@40000000 {
 &uart0 {
 	status = "okay";
 };
+
+&pinctrl_aon {

Wrong order. Nodes do not go to the end.

Could you please explain what is meant by “Nodes do not go to the end.”? How it causes wrong order?

quoted

+	wakeup-gpios = <&pinctrl_aon PAD_RGPIO2 GPIO_ACTIVE_HIGH>;
+	wakeup-source;

None of these were tested.

It does not look like you tested the DTS against bindings. Please run `make
dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst
or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-
sources-with-the-devicetree-schema/
for instructions).

The “pinctrl_aon” document has been updated:

  wakeup-gpios:
    maxItems: 1
    description: GPIO pin to be used for waking up the system from sleep mode.

  wakeup-source:
    maxItems: 1
    description: to indicate pinctrl has wakeup capability.

Running “make dt_binding_check” and “make dtbs_check” tests are successful.
Will send out in next version.

quoted

+};

diff --git a/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h

b/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
new file mode 100644
index 000000000000..3fb16ef62d90

--- /dev/null
+++ b/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h

@@ -0,0 +1,418 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ * Author: Alex Soo <yuklin.soo@starfivetech.com>
+ *
+ */
+
+#ifndef __JH8100_PINFUNC_H__
+#define __JH8100_PINFUNC_H__
+
+/*
+ * mux bits:
+ *  | 31 - 24 | 23 - 16 | 15 - 10 |  9 - 8   |  7 - 0  |
+ *  |  din    |  dout   |  doen   | function | gpio nr |
+ *
+ * dout:     output signal
+ * doen:     output enable signal
+ * din:      optional input signal, 0xff = none
+ * function:
+ * gpio nr:  gpio number, 0 - 63
+ */
+#define GPIOMUX(n, dout, doen, din) ( \
+		(((din)  & 0xff) << 24) | \
+		(((dout) & 0xff) << 16) | \
+		(((doen) & 0x3f) << 10) | \
+		((n) & 0x3f))
+
+#define PINMUX(n, func) ((1 << 10) | (((func) & 0x3) << 8) | ((n) &
+0xff))
+
+/* sys_iomux_east dout */
+#define GPOUT_LOW				0
+#define GPOUT_HIGH				1

Where are these used?

These macros are used to set the logic level of a pin to 0 or 1 respectively:

        can0_pins: can0-grp {
                can0_rx-pins {
                        pinmux = <GPIOMUX(PAD_GPIO28_E, GPOUT_LOW,
                                GPOEN_SYS_DISABLE, GPI_SYS_CAN0_RXD)>;
                        bias-pull-up;
                        input-enable;
                };

However,  " GPOUT_LOW" and " GPOUT_LOW" will be removed from
"arch/riscv/boot/dts/starfive/jh8100-pinfunc.h", and kept in
" include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h".

quoted

+#define GPOUT_SYS_CAN0_STBY			2
+#define GPOUT_SYS_CAN0_TST_NEXT_BIT		3
+#define GPOUT_SYS_CAN0_TST_SAMPLE_POINT		4
+#define GPOUT_SYS_CAN0_TXD			5
+#define GPOUT_SYS_I2C0_CLK			6
+#define GPOUT_SYS_I2C0_DATA			7
+#define GPOUT_SYS_I2S0_STEREO_RSCKO		8

You add here bunch of constants not used anywhere. No single example of
their usage, not a one.

These constants are indexes of output signals, and it is part of the pinmux value
to be written to an iomux register to configure which output signal will go through
which GPIO_PAD.

For i2c-0,  

#define GPOUT_SYS_I2C0_CLK			6
#define GPOUT_SYS_I2C0_DATA			7

Output signal "GPOUT_SYS_I2C0_CLK" goes through PAD_GPIO9_E.
Output signal "GPOUT_SYS_I2C0_DATA" goes through PAD_GPIO10_E.

        i2c0_pins: i2c0-grp {
                i2c0-scl-pins {
                        pinmux = <GPIOMUX(PAD_GPIO9_E, GPOUT_SYS_I2C0_CLK,
                                  GPOEN_SYS_I2C0_CLK,
                                  GPI_SYS_I2C0_CLK)>;
                        bias-pull-up;
                        input-enable;
                };

                i2c0-sda-pins {
                        pinmux = <GPIOMUX(PAD_GPIO10_E, GPOUT_SYS_I2C0_DATA,
                                  GPOEN_SYS_I2C0_DATA,
                                  GPI_SYS_I2C0_DATA)>;
                        bias-pull-up;
                        input-enable;
                };
        };

Will add the above to device tree for next version.

Best regards,
Krzysztof