linux-kernel - Re: [PATCH v1 1/2] arm64: dts: nuvoton: npcm845: Add peripheral 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: <CAP6Zq1jDCfhOWj4JwORy22TDZRBr0fnuy5-=G4WO9DFRv7pTdQ@mail.gmail.com>
Date: Mon, 28 Jul 2025 14:30:39 +0300
From: Tomer Maimon <tmaimon77@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	avifishman70@...il.com, tali.perry1@...il.com, joel@....id.au, 
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com, 
	openbmc@...ts.ozlabs.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] arm64: dts: nuvoton: npcm845: Add peripheral nodes

Hi Krzysztof

Thanks for your comments

On Thu, 17 Jul 2025 at 17:05, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 17/07/2025 15:53, Tomer Maimon wrote:
> > Enable peripheral support for the Nuvoton NPCM845 SoC by adding device
> > nodes for Ethernet controllers, MMC controller, SPI controllers, USB
> > device controllers, random number generator, ADC, PWM-FAN controller,
> > and I2C controllers. Include pinmux configurations for relevant
> > peripherals to support hardware operation. Add an OP-TEE firmware node
> > for secure services.
> > This patch enhances functionality for NPCM845-based platforms.
>
> Drop this sentence, redundant and not in style (see submitting patches).
> >
> > Depends-on: ARM: dts: nuvoton: npcm845: Add pinctrl groups
Maybe it's an issue with our work mail server,
https://patchwork.ozlabs.org/project/openbmc/patch/20250706153551.2180052-1-tmaimon77@gmail.com/
I believe you didn't receive the patches below as well, since I didn't
see any comments. Am I correct?
https://patchwork.ozlabs.org/project/openbmc/patch/20250706134207.2168184-2-tmaimon77@gmail.com/
https://patchwork.ozlabs.org/project/openbmc/patch/20250706134207.2168184-3-tmaimon77@gmail.com/

>
> There is no such tag.
>
> Use changelog for this purpose or b4 dependencies.
>
>
> > Signed-off-by: Tomer Maimon <tmaimon77@...il.com>
> > ---
> >  .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi   | 695 ++++++++++++++++++
> >  .../boot/dts/nuvoton/nuvoton-npcm845.dtsi     |   7 +
> >  2 files changed, 702 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > index 5cd877e6c20a..3564e0e30791 100644
> > --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> > @@ -4,6 +4,7 @@
> >  #include <dt-bindings/clock/nuvoton,npcm845-clk.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/gpio/gpio.h>
> >
> >  / {
> >       #address-cells = <2>;
> > @@ -40,6 +41,11 @@ ppi_cluster0: interrupt-partition-0 {
> >               };
> >       };
> >
> > +     udc0_phy: usb-phy {
> > +             #phy-cells = <0>;
> > +             compatible = "usb-nop-xceiv";
>
> Please follow DTS coding style.
Do you mean the property order?
>
> > +     };
> > +
> >       ahb {
> >               #address-cells = <2>;
> >               #size-cells = <2>;
> > @@ -56,6 +62,259 @@ clk: rstc: reset-controller@...01000 {
> >                       #clock-cells = <1>;
> >               };
> >
> > +             gmac1: eth@...04000 {
>
> Please follow established naming for node names. I'll fix whatever old
> stuff you took it from.
>
>
> > +                     device_type = "network";
> > +                     compatible = "snps,dwmac";
>
> I don't think we want the generic one, even if it is allowed by bindings.
Do you mean to use "snps,dwmac-3.72a" and not "snps,dwmac"?
>
> Also... You CC-ed an address, which suggests you do not work on mainline
> kernel or you do not use get_maintainers.pl/b4/patman. Please rebase and
> always work on mainline or start using mentioned tools, so correct
> addresses will be used.
>
>
> > +                     reg = <0x0 0xf0804000 0x0 0x2000>;
>
>
> > +                     interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> > +                     interrupt-names = "macirq";
> > +                     clocks  = <&clk NPCM8XX_CLK_AHB>;
> > +                     clock-names = "stmmaceth";
> > +                     pinctrl-names = "default";
> > +                     pinctrl-0 = <&rg2_pins
> > +                                     &rg2mdio_pins>;
> > +                     status = "disabled";
> > +             };
>
>
> ...
>
> > +
> > +             mc: memory-controller@...24000 {
> > +                     compatible = "nuvoton,npcm845-memory-controller";
> > +                     reg = <0x0 0xf0824000 0x0 0x1000>;
> > +                     interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> > +             };
> > +
> > +             udc0:usb@...30000 {
>
> DTS coding style...
>
> > +                     compatible = "nuvoton,npcm845-udc";
> > +                     reg = <0x0 0xf0830000 0x0 0x1000
> > +                            0x0 0xfffeb000 0x0 0x800>;
> > +                     interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&clk NPCM8XX_CLK_SU>;
> > +                     clock-names = "clk_usb_bridge";
> > +
> > +                     phys = <&udc0_phy>;
> > +                     phy_type = "utmi_wide";
> > +                     dr_mode = "peripheral";
> > +                     status = "disabled";
> > +             };
> > +
>
> ...
>
>
> Best regards,
> Krzysztof

Thanks,

Tomer