linux-kernel - Re: [PATCH] arm64: dts: exynos: gs101: define all PERIC USI nodes

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]

Message-ID: <CADrjBPojf92=Vvq9rVt9frC-61BQbKeX+5fRa-q8fqr9iGMryA@mail.gmail.com>
Date: Mon, 11 Mar 2024 10:47:06 +0000
From: Peter Griffin <peter.griffin@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	alim.akhtar@...sung.com, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, andre.draszik@...aro.org, 
	willmcvicker@...gle.com, kernel-team@...roid.com
Subject: Re: [PATCH] arm64: dts: exynos: gs101: define all PERIC USI nodes

Hi Krzysztof & Tudor,

On Fri, 8 Mar 2024 at 16:45, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 07/03/2024 14:59, Tudor Ambarus wrote:
> > Universal Serial Interface (USI) supports three types of serial
> > interface such as UART, SPI and I2C. Each protocol works independently.
> > USI can be configured to work as one of these protocols. Define all the
> > USI nodes from the PERIC blocks (USI0-14), in all their possible
> > configurations. These blocks have the TX/RX FIFO depth of 64 bytes.
> >
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@...aro.org>

With Krzystof's comments addressed:

Reviewed-by: Peter Griffin <peter.griffin@...aro.org>

> > ---
> > Please note that:
> > - google,gs101-spi compatible was queued through the SPI tree:
> >   https://lore.kernel.org/linux-arm-kernel/170742731537.2266792.3851016361229293846.b4-ty@kernel.org/
> > - SPI dma properties were marked as not requiered. Queued through the
> >   SPI tree:
> >   https://lore.kernel.org/linux-spi/170967132774.228925.1759895846287455970.b4-ty@kernel.org/
> >
> >  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 782 +++++++++++++++++++
> >  1 file changed, 782 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > index ee65ed9d2cfc..d7ecfbc7e440 100644
> > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > @@ -373,6 +373,398 @@ pinctrl_peric0: pinctrl@...40000 {
> >                       interrupts = <GIC_SPI 625 IRQ_TYPE_LEVEL_HIGH 0>;
> >               };
> >
> > +             usi1: usi@...000c0 {
> > +                     compatible = "google,gs101-usi",
> > +                                  "samsung,exynos850-usi";
>
> This should fit within 81 characters, which is pretty close to 80, so if
> there is going to be any resend/new version, please join the lines.
>
> > +                     reg = <0x109000c0 0x20>;
> > +                     ranges;
> > +                     #address-cells = <1>;
> > +                     #size-cells = <1>;
> > +                     clocks = <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0>,
> > +                              <&cmu_peric0 CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0>;
> > +                     clock-names = "pclk", "ipclk";
> > +                     samsung,sysreg = <&sysreg_peric0 0x1000>;
> > +                     status = "disabled";
> > +
> > +                     hsi2c_1: i2c@...00000 {
> > +                             compatible = "google,gs101-hsi2c",
> > +                                          "samsung,exynosautov9-hsi2c";
> > +                             reg = <0x10900000 0xc0>;
> > +                             interrupts = <GIC_SPI 635 IRQ_TYPE_LEVEL_HIGH 0>;
>
> I wonder why we use four cells in GIC...

Due to the ppi-partitions defined in the gic for the pmu-(0-2) devices.

regards,

Peter.



>
> > +                             #address-cells = <1>;
> > +                             #size-cells = <0>;
> > +                             pinctrl-names = "default";
> > +                             pinctrl-0 = <&hsi2c1_bus>;
>
> Please reverse two lines, first pinctrl-0 then pinctrl-names. I know we
> did not follow this convention till now, but at least new code can be
> correct. Also clocks should be before pinctrl, so we keep some sort of
> alphabetical order.
>
> It is anyway too late in the cycle for me to pick it up, so no need to
> hurry with resend.
>
> Best regards,
> Krzysztof
>