lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 7 Feb 2024 03:09:27 +0000
From: Yuklin Soo <yuklin.soo@...rfivetech.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Yuklin Soo
	<yuklin.soo@...rfivetech.com>, Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng
	<hal.feng@...rfivetech.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>,
	Jianlong Huang <jianlong.huang@...rfivetech.com>, Emil Renner Berthing
	<kernel@...il.dk>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Drew
 Fustini <drew@...gleboard.org>
CC: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>, Paul
 Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>
Subject: RE: [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device
 tree nodes



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Thursday, December 21, 2023 11:53 PM
> To: Yuklin Soo <yuklin.soo@...rfivetech.com>; Linus Walleij
> <linus.walleij@...aro.org>; Bartosz Golaszewski
> <bartosz.golaszewski@...aro.org>; Hal Feng <hal.feng@...rfivetech.com>;
> Leyfoon Tan <leyfoon.tan@...rfivetech.com>; Jianlong Huang
> <jianlong.huang@...rfivetech.com>; Emil Renner Berthing
> <kernel@...il.dk>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley <conor+dt@...nel.org>;
> Drew Fustini <drew@...gleboard.org>
> Cc: linux-gpio@...r.kernel.org; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org; linux-riscv@...ts.infradead.org; Paul Walmsley
> <paul.walmsley@...ive.com>; Palmer Dabbelt <palmer@...belt.com>;
> Albert Ou <aou@...s.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@...rfivetech.com>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@...rfivetech.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@...00000 {
> >  &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@...rfivetech.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ