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: <717e4a53-c0a8-0b60-4502-a819cf2089a6@linaro.org>
Date: Sat, 24 Jun 2023 10:09:07 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Varshini Rajendran <varshini.rajendran@...rochip.com>,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
 claudiu.beznea@...rochip.com, mturquette@...libre.com, sboyd@...nel.org,
 herbert@...dor.apana.org.au, davem@...emloft.net, vkoul@...nel.org,
 tglx@...utronix.de, maz@...nel.org, lee@...nel.org, ulf.hansson@...aro.org,
 tudor.ambarus@...aro.org, miquel.raynal@...tlin.com, richard@....at,
 vigneshr@...com, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 linus.walleij@...aro.org, p.zabel@...gutronix.de, olivia@...enic.com,
 a.zummo@...ertech.it, radu_nicolae.pirea@....ro, richard.genoud@...il.com,
 gregkh@...uxfoundation.org, lgirdwood@...il.com, broonie@...nel.org,
 wim@...ux-watchdog.org, linux@...ck-us.net, arnd@...db.de, olof@...om.net,
 soc@...nel.org, linux@...linux.org.uk, sre@...nel.org,
 jerry.ray@...rochip.com, horatiu.vultur@...rochip.com,
 durai.manickamkr@...rochip.com, andrew@...n.ch, alain.volmat@...s.st.com,
 neil.armstrong@...aro.org, mihai.sain@...rochip.com,
 eugen.hristev@...labora.com, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-clk@...r.kernel.org, linux-crypto@...r.kernel.org,
 dmaengine@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-mmc@...r.kernel.org, linux-mtd@...ts.infradead.org,
 netdev@...r.kernel.org, linux-gpio@...r.kernel.org,
 linux-rtc@...r.kernel.org, linux-spi@...r.kernel.org,
 linux-serial@...r.kernel.org, alsa-devel@...a-project.org,
 linux-usb@...r.kernel.org, linux-watchdog@...r.kernel.org,
 linux-pm@...r.kernel.org
Cc: Hari.PrasathGE@...rochip.com, cristian.birsan@...rochip.com,
 balamanikandan.gunasundar@...rochip.com, manikandan.m@...rochip.com,
 dharma.b@...rochip.com, nayabbasha.sayed@...rochip.com,
 balakrishnan.s@...rochip.com
Subject: Re: [PATCH v2 43/45] ARM: dts: at91: sam9x7: add device tree for SoC

On 23/06/2023 22:30, Varshini Rajendran wrote:
> Add device tree file for SAM9X7 SoC family.
> 
> Co-developed-by: Nicolas Ferre <nicolas.ferre@...rochip.com>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@...rochip.com>
> Signed-off-by: Varshini Rajendran <varshini.rajendran@...rochip.com>
> ---
>  arch/arm/boot/dts/sam9x7.dtsi | 1237 +++++++++++++++++++++++++++++++++
>  1 file changed, 1237 insertions(+)
>  create mode 100644 arch/arm/boot/dts/sam9x7.dtsi
> 
> diff --git a/arch/arm/boot/dts/sam9x7.dtsi b/arch/arm/boot/dts/sam9x7.dtsi
> new file mode 100644
> index 000000000000..535a55f13dd0
> --- /dev/null
> +++ b/arch/arm/boot/dts/sam9x7.dtsi
> @@ -0,0 +1,1237 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * sam9x7.dtsi - Device Tree Include file for Microchip SAM9X7 SoC family
> + *
> + * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Varshini Rajendran <varshini.rajendran@...rochip.com>
> + */
> +
> +#include <dt-bindings/clock/at91.h>
> +#include <dt-bindings/dma/at91.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <dt-bindings/pinctrl/at91.h>
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	model = "Microchip SAM9X7 SoC";
> +	compatible = "microchip,sam9x7";
> +	interrupt-parent = <&aic>;
> +
> +	aliases {
> +		serial0 = &dbgu;

serial alias is rarely property of SoC. Do you claim that absolutely all
boards must use this serial and they cannot use anything else?

> +		gpio0 = &pioA;
> +		gpio1 = &pioB;
> +		gpio2 = &pioC;
> +		gpio3 = &pioD;

GPIOs are discussible but sometimes we keep them in SoC DTSI.

> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,arm926ej-s";
> +			device_type = "cpu";
> +			reg = <0>;
> +		};
> +	};
> +
> +	clocks {
> +		slow_xtal: clock-slowxtal {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +		};
> +
> +		main_xtal: clock-mainxtal {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +		};
> +	};
> +
> +	sram: sram@...000 {
> +		compatible = "mmio-sram";
> +		reg = <0x300000 0x10000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x300000 0x10000>;
> +	};
> +
> +	ahb {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		usb0: gadget@...000 {
> +			compatible = "microchip,sam9x7-udc", "microchip,sam9x60-udc";
> +			reg = <0x500000 0x100000>,
> +			      <0xf803c000 0x400>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <23 IRQ_TYPE_LEVEL_HIGH 2>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 23>, <&pmc PMC_TYPE_CORE PMC_UTMI>;
> +			clock-names = "pclk", "hclk";
> +			assigned-clocks = <&pmc PMC_TYPE_CORE PMC_UTMI>;
> +			assigned-clock-rates = <480000000>;
> +			status = "disabled";
> +		};
> +
> +		ohci0: usb@...000 {
> +			compatible = "microchip,sam9x7-ohci", "atmel,at91rm9200-ohci", "usb-ohci";
> +			reg = <0x600000 0x100000>;
> +			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 2>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 22>, <&pmc PMC_TYPE_PERIPHERAL 22>, <&pmc PMC_TYPE_SYSTEM 6>;
> +			clock-names = "ohci_clk", "hclk", "uhpck";
> +			status = "disabled";
> +		};
> +
> +		ehci0: usb@...000 {
> +			compatible = "microchip,sam9x7-ehci", "atmel,at91sam9g45-ehci", "usb-ehci";
> +			reg = <0x700000 0x100000>;
> +			interrupts = <22 IRQ_TYPE_LEVEL_HIGH 2>;
> +			clocks = <&pmc PMC_TYPE_CORE PMC_UTMI>, <&pmc PMC_TYPE_PERIPHERAL 22>;
> +			clock-names = "usb_clk", "ehci_clk";
> +			assigned-clocks = <&pmc PMC_TYPE_CORE PMC_UTMI>;
> +			assigned-clock-rates = <480000000>;
> +			status = "disabled";
> +		};
> +
> +		sdmmc0: sdio-host@...00000 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (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).

> +			compatible = "microchip,sam9x7-sdhci", "microchip,sam9x60-sdhci";

So none of your DTS and bindings patches were tested... Limited review
follows as there is little sense to use reviewers time if you can use
machine.

> +			reg = <0x80000000 0x300>;
> +			interrupts = <12 IRQ_TYPE_LEVEL_HIGH 0>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 12>, <&pmc PMC_TYPE_GCK 12>;
> +			clock-names = "hclock", "multclk";
> +			assigned-clocks = <&pmc PMC_TYPE_GCK 12>;
> +			assigned-clock-rates = <100000000>;
> +			status = "disabled";
> +		};
> +
> +		sdmmc1: sdio-host@...00000 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (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).

> +			compatible = "microchip,sam9x7-sdhci", "microchip,sam9x60-sdhci";
> +			reg = <0x90000000 0x300>;
> +			interrupts = <26 IRQ_TYPE_LEVEL_HIGH 0>;
> +			clocks = <&pmc PMC_TYPE_PERIPHERAL 26>, <&pmc PMC_TYPE_GCK 26>;
> +			clock-names = "hclock", "multclk";
> +			assigned-clocks = <&pmc PMC_TYPE_GCK 26>;
> +			assigned-clock-rates = <100000000>;
> +			status = "disabled";
> +		};
> +
> +		apb {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			flx4: flexcom@...00000 {
> +				compatible = "microchip,sam9x7-flexcom", "atmel,sama5d2-flexcom";
> +				reg = <0xf0000000 0x200>;
> +				clocks = <&pmc PMC_TYPE_PERIPHERAL 13>;
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0x0 0xf0000000 0x800>;
> +				status = "disabled";
> +
> +				uart4: serial@200 {
> +					compatible = "microchip,sam9x7-usart", "microchip,sam9x60-usart", "atmel,at91sam9260-usart";

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> +					reg = <0x200 0x200>;
> +					interrupts = <13 IRQ_TYPE_LEVEL_HIGH 7>;
> +					dmas = <&dma0
> +						(AT91_XDMAC_DT_ME

...


> +
> +			dbgu: serial@...ff200 {
> +				compatible = "microchip,sam9x7-dbgu", "microchip,sam9x60-dbgu", "microchip,sam9x60-usart", "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";

I wonder if we can make the line longer...

> +				reg = <0xfffff200 0x200>;
> +				interrupts = <47 IRQ_TYPE_LEVEL_HIGH 7>;
> +				dmas = <&dma0
> +					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) |
> +					 AT91_XDMAC_DT_PERID(28))>,
> +				       <&dma0
> +					(AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) |
> +					 AT91_XDMAC_DT_PERID(29))>;
> +				dma-names = "tx", "rx";
> +				clocks = <&pmc PMC_TYPE_PERIPHERAL 47>;
> +				clock-names = "usart";
> +				status = "disabled";
> +			};
> +
> +			pinctrl: pinctrl@...ff400 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				compatible = "microchip,sam9x7-pinctrl", "microchip,sam9x60-pinctrl", "atmel,at91sam9x5-pinctrl", "simple-mfd";
> +				ranges = <0xfffff400 0xfffff400 0x800>;
> +
> +				/* mux-mask corresponding to sam9x7 SoC in TFBGA228L package */
> +				atmel,mux-mask = <
> +						 /*  A		B	   C	      D	  */
> +						 0xffffffff 0xffffefc0 0xc0ffd000 0x00000000	/* pioA */
> +						 0x07ffffff 0x0805fe7f 0x01ff9f80 0x06078000	/* pioB */
> +						 0xffffffff 0x07dfffff 0xfa3fffff 0x00000000	/* pioC */
> +						 0x00003fff 0x00003fe0 0x0000003f 0x00000000	/* pioD */
> +						 >;
> +
> +				pioA: gpio@...ff400 {
> +					compatible = "microchip,sam9x7-gpio", "microchip,sam9x60-gpio", "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfffff400 0x200>;
> +					interrupts = <2 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pmc PMC_TYPE_PERIPHERAL 2>;
> +				};
> +
> +				pioB: gpio@...ff600 {
> +					compatible = "microchip,sam9x7-gpio", "microchip,sam9x60-gpio", "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfffff600 0x200>;
> +					interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					#gpio-lines = <26>;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pmc PMC_TYPE_PERIPHERAL 3>;
> +				};
> +
> +				pioC: gpio@...ff800 {
> +					compatible = "microchip,sam9x7-gpio", "microchip,sam9x60-gpio", "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfffff800 0x200>;
> +					interrupts = <4 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pmc PMC_TYPE_PERIPHERAL 4>;
> +				};
> +
> +				pioD: gpio@...ffa00 {
> +					compatible = "microchip,sam9x7-gpio", "microchip,sam9x60-gpio", "atmel,at91sam9x5-gpio", "atmel,at91rm9200-gpio";
> +					reg = <0xfffffa00 0x200>;
> +					interrupts = <44 IRQ_TYPE_LEVEL_HIGH 1>;
> +					#gpio-cells = <2>;
> +					gpio-controller;
> +					#gpio-lines = <22>;
> +					interrupt-controller;
> +					#interrupt-cells = <2>;
> +					clocks = <&pmc PMC_TYPE_PERIPHERAL 44>;
> +				};
> +			};
> +
> +			pmc: pmc@...ffc00 {
> +				compatible = "microchip,sam9x7-pmc", "syscon";
> +				reg = <0xfffffc00 0x200>;
> +				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				#clock-cells = <2>;
> +				clocks = <&clk32k 1>, <&clk32k 0>, <&main_xtal>;
> +				clock-names = "td_slck", "md_slck", "main_xtal";
> +			};
> +
> +			reset_controller: rstc@...ffe00 {

reset-controller

> +				compatible = "microchip,sam9x7-rstc", "microchip,sam9x60-rstc";
> +				reg = <0xfffffe00 0x10>;
> +				clocks = <&clk32k 0>;
> +			};
> +
> +			shutdown_controller: shdwc@...ffe10 {

Usually power-management or reset-controller or something like this.



> +				compatible = "microchip,sam9x7-shdwc", "microchip,sam9x60-shdwc";
> +				reg = <0xfffffe10 0x10>;
> +				clocks = <&clk32k 0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				atmel,wakeup-rtc-timer;
> +				atmel,wakeup-rtt-timer;
> +				status = "disabled";


Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ