| 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
| ||
|
Message-ID: <12338e4a-62ca-3cac-6cd9-d0a5f7576a18@kontron.de>
Date: Mon, 6 Jul 2020 09:46:48 +0200
From: Frieder Schrempf <frieder.schrempf@...tron.de>
To: Marcel Ziswiler <marcel.ziswiler@...adex.com>,
"Anson.Huang@....com" <Anson.Huang@....com>,
"leoyang.li@....com" <leoyang.li@....com>,
"andreas@...nade.info" <andreas@...nade.info>,
"festevam@...il.com" <festevam@...il.com>,
"linux-imx@....com" <linux-imx@....com>,
"rjones@...eworks.com" <rjones@...eworks.com>,
"robh@...nel.org" <robh@...nel.org>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"michael@...le.cc" <michael@...le.cc>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"sebastien.szymanski@...adeus.com" <sebastien.szymanski@...adeus.com>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
Cc: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>
Subject: Re: [PATCH 1/2] arm64: dts: Add the Kontron i.MX8M-Mini SoMs and
baseboards
Hi Marcel,
On 03.07.20 09:56, Marcel Ziswiler wrote:
> Hi Frieder
>
> Nice to see some more i.MX 8M Mini action. Much appreciated!
>
> On Thu, 2020-07-02 at 16:33 +0200, Schrempf Frieder wrote:
>> From: Frieder Schrempf <frieder.schrempf@...tron.de>
>>
>> Kontron Electronics GmbH offers small and powerful SoMs based on the
>> i.MX8MM
>
> To avoid much confusion in NXP's nomenclature I would recommend writing this always as i.MX 8M Mini. Of course,
> in code, using imx8mm is fine.
Ok, sounds reasonable.
>
>> including PMIC, LPDDR4-RAM, eMMC and SPI NOR.
>>
>> The matching baseboards have the same form factor and similar interfaces
>> as the other boards from the Kontron "Board-Line" family, including
>> SD card, 1G Ethernet, 100M Ethernet, USB Host/OTG, digital IOs, RS232,
>> RS485, CAN, LVDS or HDMI, RTC and much more.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>> ---
>> .../dts/freescale/imx8mm-kontron-n8010-s.dts | 15 +
>> .../freescale/imx8mm-kontron-n8010-som.dtsi | 16 +
>> .../dts/freescale/imx8mm-kontron-n8011-s.dts | 15 +
>> .../freescale/imx8mm-kontron-n8011-som.dtsi | 16 +
>> .../dts/freescale/imx8mm-kontron-n801x-s.dtsi | 326 ++++++++++++++++++
>> .../freescale/imx8mm-kontron-n801x-som.dtsi | 281 +++++++++++++++
>> 6 files changed, 669 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-s.dts
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-som.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-s.dts
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-som.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-som.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-
>> kontron-n8010-s.dts
>> new file mode 100644
>> index 000000000000..0911f2d0555b
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-s.dts
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> Don't you want to use the more common GPL-2.0+ OR MIT variant which allows for more freedom? At least NXP's
> imx8mm.dtsi also uses that.
That actually sounds like a good idea. I will change that.
>
>> +/*
>> + * Copyright (C) 2019 Kontron Electronics GmbH
>
> I know that there is much about 2020 to rather be ignored.
Indeed, but I also once learned that you don't change the original date
in a copyright notice as it is used to express when the content has been
published or created first (and the original version has been online on
our server publically since 2019).
>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "imx8mm-kontron-n8010-som.dtsi"
>> +#include "imx8mm-kontron-n801x-s.dtsi"
>> +
>> +/ {
>> + model = "Kontron i.MX8MM N8010 S";
>> + compatible = "kontron,imx8mm-n8010-s", "kontron,imx8mm-n8010-som",
>> + "fsl,imx8mm";
>
> I believe now with Linux having dropped the strict 80-column line length coding style limit we are allowed to
> go up to 100 (;-p).
Right, I remember seeing some discussion about extending the column
limit. I will need to update my text editor's config.
>
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-som.dtsi
>> b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-som.dtsi
>> new file mode 100644
>> index 000000000000..5b178ce4ce1b
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8010-som.dtsi
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 Kontron Electronics GmbH
>> + */
>> +
>> +#include "imx8mm-kontron-n801x-som.dtsi"
>> +
>> +/ {
>> + model = "Kontron i.MX8MM N8010 SoM";
>> + compatible = "kontron,imx8mm-n8010-som", "fsl,imx8mm";
>> +
>> + memory@...00000 {
>> + device_type = "memory";
>> + reg = <0x0 0x40000000 0 0x80000000>;
>> + };
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-
>> kontron-n8011-s.dts
>> new file mode 100644
>> index 000000000000..5c44bd77ed32
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-s.dts
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 Kontron Electronics GmbH
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "imx8mm-kontron-n8011-som.dtsi"
>> +#include "imx8mm-kontron-n801x-s.dtsi"
>> +
>> +/ {
>> + model = "Kontron i.MX8MM N8011 S";
>> + compatible = "kontron,imx8mm-n8011-s", "kontron,imx8mm-n8011-som",
>> + "fsl,imx8mm";
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-som.dtsi
>> b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-som.dtsi
>> new file mode 100644
>> index 000000000000..303594867b8f
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n8011-som.dtsi
>> @@ -0,0 +1,16 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 Kontron Electronics GmbH
>> + */
>> +
>> +#include "imx8mm-kontron-n801x-som.dtsi"
>> +
>> +/ {
>> + model = "Kontron i.MX8MM N8011 SoM";
>> + compatible = "kontron,imx8mm-n8011-som", "fsl,imx8mm";
>> +
>> + memory@...00000 {
>> + device_type = "memory";
>> + reg = <0x0 0x40000000 0 0xC0000000>;
>> + };
>
> Isn't the boot loader supposed to filling that in?
Hm, probably yes. I thought it would be good to have the upstream DT
fully describe the modules, including the memory size. But if it is more
common or useful, I will fill in the memory size for the smallest
configuration (1GB) only and let U-Boot overwrite the value if it
detects modules with larger DDR.
>
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dtsi
>> b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dtsi
>> new file mode 100644
>> index 000000000000..d825e52e0beb
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-n801x-s.dtsi
>> @@ -0,0 +1,326 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 Kontron Electronics GmbH
>> + */
>> +
>> +/ {
>> + aliases {
>> + ethernet1 = &usbnet;
>> + };
>> +
>> + /* fixed crystal dedicated to mcp2515 */
>> + clk16m: clk16m {
>
> I believe a more human readable variant e.g. as follows is preferred:
>
> osc_16m: clock-osc-16m
Ok.
>
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <16000000>;
>
> Also the use of the optional property clock-output-names is recommended.
Ok.
>
>> + };
>> +
>> + leds {
>> + compatible = "gpio-leds";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_gpio_led>;
>> +
>> + led1 {
>> + label = "led1";
>> + gpios = <&gpio4 17 GPIO_ACTIVE_LOW>;
>> + linux,default-trigger = "heartbeat";
>> + };
>> +
>> + led2 {
>> + label = "led2";
>> + gpios = <&gpio4 19 GPIO_ACTIVE_LOW>;
>> + };
>> +
>> + led3 {
>> + label = "led3";
>> + gpios = <&gpio4 18 GPIO_ACTIVE_LOW>;
>> + };
>> +
>> + led4 {
>> + label = "led4";
>> + gpios = <&gpio4 8 GPIO_ACTIVE_LOW>;
>> + };
>> +
>> + led5 {
>> + label = "led5";
>> + gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
>> + };
>> +
>> + led6 {
>> + label = "led6";
>> + gpios = <&gpio4 7 GPIO_ACTIVE_LOW>;
>> + };
>> + };
>> +
>> + pwm-beeper {
>> + compatible = "pwm-beeper";
>> + pwms = <&pwm2 0 5000 0>;
>> + };
>> +
>> + reg_rst_eth2: regulator-rst-eth2 {
>> + compatible = "regulator-fixed";
>> + regulator-name = "rst-usb-eth2";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_usb_eth2>;
>> + gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
>> + status = "okay";
>> + };
>> +
>> + vdd_5v: regulator-5v {
>
> I would stick to consistent reg_ pre-fixing.
Ok.
>
>> + compatible = "regulator-fixed";
>> + regulator-name = "vdd_5v";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + regulator-boot-on;
>> + regulator-always-on;
>> + status = "okay";
>> + };
>> +};
>> +
>> +&ecspi2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_ecspi2>;
>> + cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
>> + status = "okay";
>> +
>> + can0: can@0 {
>> + compatible = "microchip,mcp2515";
>> + reg = <0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_can>;
>> + clocks = <&clk16m>;
>> + interrupt-parent = <&gpio4>;
>> + interrupts = <28 IRQ_TYPE_EDGE_FALLING>;
>> + spi-max-frequency = <100000>;
>> + vdd-supply = <&vdd_3v3>;
>> + xceiver-supply = <&vdd_5v>;
>> + status = "okay";
>
> I find the property ordering a little confusing but that might just be me.
I think it's "compatible", "reg" and "pinctrl" first, then the other
properties in alphabetical order and "status" last. So I'm not really
sure what to improve here.
>
>> + };
>> +};
>> +
>> +&ecspi3 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_ecspi3>;
>> + cs-gpios = <&gpio5 25 GPIO_ACTIVE_LOW>;
>> + status = "okay";
>> +
>> + userspi: userspi@0 {
>> + compatible = "kontron,user-spi";
>> + reg = <0>;
>> + spi-max-frequency = <100000000>;
>> + status = "okay";
>> + };
>
> I thought from earlier discussions you intended to drop that and either specify it from user-space directly or
> use an overlay instead.
Yes, indeed, but when I learned about the possibility to override the
device's driver from userspace, I already had sent out this patch. I
will remove the userspi node in the next version.
Thanks so far for reviewing!
Frieder
Powered by blists - more mailing lists