[RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree nodes
Yuklin Soo
yuklin.soo at starfivetech.com
Tue Feb 6 19:09:27 PST 2024
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> Sent: Thursday, December 21, 2023 11:53 PM
> To: Yuklin Soo <yuklin.soo at starfivetech.com>; Linus Walleij
> <linus.walleij at linaro.org>; Bartosz Golaszewski
> <bartosz.golaszewski at linaro.org>; Hal Feng <hal.feng at starfivetech.com>;
> Leyfoon Tan <leyfoon.tan at starfivetech.com>; Jianlong Huang
> <jianlong.huang at starfivetech.com>; Emil Renner Berthing
> <kernel at esmil.dk>; Rob Herring <robh at kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt at linaro.org>; Conor Dooley <conor+dt at kernel.org>;
> Drew Fustini <drew at beagleboard.org>
> Cc: linux-gpio at vger.kernel.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; linux-riscv at lists.infradead.org; Paul Walmsley
> <paul.walmsley at sifive.com>; Palmer Dabbelt <palmer at dabbelt.com>;
> Albert Ou <aou at eecs.berkeley.edu>
> Subject: Re: [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree
> nodes
>
> On 21/12/2023 09:36, Alex Soo wrote:
> > Add pinctrl_east/pinctrl_west/pinctrl_gmac/pinctrl_aon device tree
> > nodes for JH8100 SoC.
> >
> > Signed-off-by: Alex Soo <yuklin.soo at starfivetech.com>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan at starfivetech.com>
>
> I have some doubts about it...
>
> > ---
> > 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 at 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?
>
> > + 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.
>
> > +};
> > 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 at 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".
>
> > +#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
More information about the linux-riscv
mailing list