| 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: <8ace15e8-0da1-4384-9042-ab89329c352a@iesy.com>
Date: Tue, 19 Mar 2024 15:17:20 +0000
From: Dominik Poggel <pog@...y.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, "robh+dt@...nel.org"
<robh+dt@...nel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
<conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, Tianling Shen
<cnsztl@...il.com>, Chris Morgan <macromorgan@...mail.com>, Ondrej Jirman
<megi@....cz>, Andy Yan <andyshrk@....com>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-rockchip@...ts.infradead.org"
<linux-rockchip@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] arm64: dts: iesy: add support for iesy PX30 SoM OSM-S
On 19.03.24 13:40, Krzysztof Kozlowski wrote:
> Achtung! Diese Mail kommt von extern. Bitte besonders vorsichtig mit den Inhalten und den Anhängen umgehen.
>
> On 19/03/2024 10:54, Dominik Poggel wrote:
>> This adds support for the iesy SoM px30-iesy-osm-sf and the matching
>> evalboard px30-iesy-eva-mi V2.XX.
>>
>> Signed-off-by: Dominik Poggel <pog@...y.com>
>> ---
>> arch/arm64/boot/dts/rockchip/Makefile | 1 +
>> .../boot/dts/rockchip/px30-iesy-eva-mi-v2.dts | 624 ++++++++++++++++++
>> .../boot/dts/rockchip/px30-iesy-osm-sf.dtsi | 346 ++++++++++
>> 3 files changed, 971 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
>> create mode 100644 arch/arm64/boot/dts/rockchip/px30-iesy-osm-sf.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
>> index f906a868b71a..a46234ccbe15 100644
>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>> @@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2-of10.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-edimm2.2.dtb
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-iesy-eva-mi-v2.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-ringneck-haikou.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-evb.dtb
>> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-roc-cc.dtb
>> diff --git a/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts b/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
>> new file mode 100644
>> index 000000000000..be1d709bbab0
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
>> @@ -0,0 +1,624 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Device Tree for iesy RPX30 EVA-MI V2.xx (Eval Kit)
>> + *
>> + * Copyright (c) 2022 iesy GmbH
>> + */
>> +
>> +/dts-v1/;
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/net/mscc-phy-vsc8531.h>
>> +#include "px30-iesy-osm-sf.dtsi"
>> +
>> +/ {
>> + model = "iesy RPX30 EVA-MI V2.xx (Eval Kit)";
>> + compatible = "iesy,rpx30-eva-mi-v2", "rockchip,px30";
>> +
>> + leds {
>> + compatible = "gpio-leds";
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_leds_bb138>;
>> +
>> + /* BB138a: green user led (LD4) */
>> + led@0 {
> 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).
>
>> + label = "USER_LED_00";
>> + gpios = <&gpio3 RK_PA0 GPIO_ACTIVE_LOW>;
>> + linux,default-trigger = "heartbeat";
>> + };
>> +
>> + /* BB138a: yellow user led (LD9)) */
>> + led@1 {
>> + label = "USER_LED_01";
>> + gpios = <&gpio3 RK_PA1 GPIO_ACTIVE_LOW>;
>> + default-state = "off";
>> + };
>> + };
>> +
>> + /* BB138a: MAX9867ETJ+ audio codec */
>> + max9867-sound {
> sound {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
>
>> + compatible = "simple-audio-card";
>> + simple-audio-card,name = "rockchip,max9867-codec";
>> + simple-audio-card,format = "i2s";
>> +
>> + simple-audio-card,widgets =
>> + "Speaker", "Jack",
>> + "Microphone", "Mic";
>> + simple-audio-card,routing =
>> + "Jack", "LOUT",
>> + "Jack", "ROUT",
>> + "Mic", "DMICL",
>> + "Mic", "DMICR";
>> +
>> + simple-audio-card,frame-master = <&cpudai>;
>> + simple-audio-card,bitclock-master = <&cpudai>;
>> +
>> + status = "okay";
>> +
>> + cpudai: simple-audio-card,cpu {
>> + sound-dai = <&i2s1_2ch>;
>> + dai-tdm-slot-num = <1>;
>> + dai-tdm-slot-width = <16>;
>> + };
>> +
>> + simple-audio-card,codec {
>> + sound-dai = <&max9867>;
>> + clocks = <&cru SCLK_I2S1_OUT>;
>> + };
>> + };
>> +
>> + /* regulator for USB OTG port */
>> + usb_a_vbus_regulator: regulator@1 {
> Not a bus.
>
> 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).
>
>> + compatible = "regulator-fixed";
>> + regulator-name = "usb_a_vbus_regulator";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + gpio = <&gpio3 RK_PC0 GPIO_ACTIVE_HIGH>;
>> + enable-active-high;
>> + };
>> +
>> + /* regulator for USB host port */
>> + usb_b_vbus_regulator: regulator@2 {
> 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).
>
>> + compatible = "regulator-fixed";
>> + regulator-name = "usb_b_vbus_regulator";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + gpio = <&gpio3 RK_PC3 GPIO_ACTIVE_HIGH>;
>> + enable-active-high;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + };
>> +
>> + user-buttons {
>> + compatible = "gpio-keys";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
> 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).
>
> You are repeating the same mistakes we fixed long time ago. This means
> you started work from some downstream or old DTS. Don't. It's a waste of
> time, also mine to tell you that we fixed all this.
>
> Please start from scratch from upstream DTS and test your code before
> sending.
>
> ...
>
> ...
>
>> +/ {
>> + aliases {
>> + mmc0 = &emmc;
>> + mmc1 = &sdmmc;
>> + mmc2 = &sdio;
>> + };
>> +
>> + chosen {
>> + bootargs = "earlycon=uart8250,mmio32,0xff160000 console=ttyFIQ0 rw root=PARTUUID=614e0000-0000 rootwait";
> That's not correct bootargs. earlyocon is debugging, so drop from
> mainline code. console goes to stdout property.
> root/PARTUIID is obviously not correct. My PARTUIID is different.
>
> Drop entire chosen.
>
>> + };
>> +
>> + fiq-debugger {
>> + compatible = "rockchip,fiq-debugger";
> Undocumented. 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.
>
> You MUST run checkpatch on your submissions. All of them.
>
>> + rockchip,serial-id = <2>;
>> + rockchip,wake-irq = <0>;
>> + /* If enable uart uses irq instead of fiq */
>> + rockchip,irq-mode-enable = <1>;
>> + rockchip,baudrate = <115200>; /* Only 115200 and 1500000 */
>> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart2m1_xfer>;
>> + status = "okay";
> Why? Was it disabled anywhere? Anyway, that's not even hardware.
>
> I am not going to review the rest. Please read submitting patches and
> carefully follow it. Run all standard tests on your code before sending
> patches. There is no point in reviewer telling you something which
> automated tools tell.
Thanks for your input, I will use it and resubmit it afterwards.
Unfortunately 'make dtbs_check' isn't working, as it can't resolve
http://devicetree.org/meta-schemas/core.yaml. Probably just a problem on
my end,
I will investigate further.
Best regards,
Dominik
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists