| 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: <1d7ffd09-99b9-43d8-a2f5-6e5455b4e5a1@yandex.com>
Date: Tue, 20 Aug 2024 15:34:58 +0200
From: Johan Jonker <jbx6244@...dex.com>
To: Detlev Casanova <detlev.casanova@...labora.com>,
linux-kernel@...r.kernel.org
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
Andi Shyti <andi.shyti@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>, Lee Jones <lee@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Daniel Lezcano
<daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>,
Chris Morgan <macromorgan@...mail.com>, Jonas Karlman <jonas@...boo.se>,
Tim Lunn <tim@...thertop.org>, Muhammed Efe Cetin <efectn@...tonmail.com>,
Andy Yan <andyshrk@....com>, Jagan Teki <jagan@...eble.ai>,
Dragan Simic <dsimic@...jaro.org>,
Sebastian Reichel <sebastian.reichel@...labora.com>,
Shresth Prasad <shresthprasad7@...il.com>, Ondrej Jirman <megi@....cz>,
Weizhao Ouyang <weizhao.ouyang@....com>, Alexey Charkov <alchark@...il.com>,
Jimmy Hon <honyuenkwun@...il.com>, Finley Xiao <finley.xiao@...k-chips.com>,
Yifeng Zhao <yifeng.zhao@...k-chips.com>,
Elaine Zhang <zhangqing@...k-chips.com>, Liang Chen <cl@...k-chips.com>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-i2c@...r.kernel.org,
linux-iio@...r.kernel.org, linux-serial@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH 09/10] arm64: dts: rockchip: Add rk3576 SoC base DT
On 8/19/24 22:06, Detlev Casanova wrote:
> Hi Johan,
>
> On Thursday, 15 August 2024 05:30:25 EDT Johan Jonker wrote:
>> Some comments below. Whenever useful.
>>
>> On 8/2/24 23:45, Detlev Casanova wrote:
>>> This device tree contains all devices necessary for booting from network
>>> or SD Card.
>>>
>>> It supports CPU, CRU, PM domains, dma, interrupts, timers, UART and
>>> SDHCI (everything necessary to boot Linux on this system on chip) as
>>> well as Ethernet, I2C, SPI and OTP.
>>>
>>> Also add the necessary DT bindings for the SoC.
>>>
>>> Signed-off-by: Liang Chen <cl@...k-chips.com>
>>> Signed-off-by: Finley Xiao <finley.xiao@...k-chips.com>
>>> Signed-off-by: Yifeng Zhao <yifeng.zhao@...k-chips.com>
>>> Signed-off-by: Elaine Zhang <zhangqing@...k-chips.com>
>>> [rebase, squash and reword commit message]
>>> Signed-off-by: Detlev Casanova <detlev.casanova@...labora.com>
>>> ---
>>
>> [..]
>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
>>> b/arch/arm64/boot/dts/rockchip/rk3576.dtsi new file mode 100644
>>> index 0000000000000..00c4d2a153ced
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
>> [..]
>>
>> For uart0..uart11:
>>> +
>>> + uart1: serial@...10000 {
>>> + compatible = "rockchip,rk3576-uart", "snps,dw-apb-
> uart";
>>> + reg = <0x0 0x27310000 0x0 0x100>;
>>>
>>> + interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
>>
>> "interrupts" are sort just like other properties. A mix of sort styles
>> exists, so check all nodes.
>
> Ok, so it should be sorted alphabetically with the following exceptions:
> - 'compatible' and 'reg.*' on top
> - "#.*" at the end, sorted
> - "status" last.
>
> Is that right ?
The dts-coding-style.rst does not say much about things with "#",
so below a property they refer to or at the end looks nicer.
No strict rule, but do it in a consistent style in file.
Original comment by robh for things with "reg":
"It makes more sense to keep reg-io-width together with reg."
https://lore.kernel.org/all/20240131135955.GA966672-robh@kernel.org/
>
>>> + clocks = <&cru SCLK_UART1>, <&cru PCLK_UART1>;
>>> + clock-names = "baudclk", "apb_pclk";
>>>
>>> + reg-shift = <2>;
>>> + reg-io-width = <4>;
>>
>> Move below "reg".
>>
>>> + dmas = <&dmac0 8>, <&dmac0 9>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&uart1m0_xfer>;
>>> + status = "disabled";
>>> + };
>>> +
>>> + pmu: power-management@...80000 {
>
> [...]
>
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + clocks = <&cru ACLK_VOP>,
>>> + <&cru HCLK_VOP>,
>>> + <&cru HCLK_VOP_ROOT>;
>>> + pm_qos = <&qos_vop_m0>,
>>> + <&qos_vop_m1ro>;
>>> +
>>> + power-domain@...576_PD_USB {
>>
>> Since when is USB part of VOP?
>> Recheck?
>
> The TRM doesn't tell me anything, but If I don't put it as a child of VOP, it
> just hangs when the kernel tries to shut it down.
Could the people from Rockchip disclose the USB PD location?
>
> [...]
>
>>> +
>>> + pinctrl: pinctrl {
>>> + compatible = "rockchip,rk3576-pinctrl";
>>> + rockchip,grf = <&ioc_grf>;
>>> + rockchip,sys-grf = <&sys_grf>;
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> + ranges;
>>> +
>>>
>>> + gpio0: gpio@...20000 {
>>
>> The use of gpio nodes as subnode of pinctrl is deprecated.
>>
>> patternProperties:
>> "gpio@[0-9a-f]+$":
>> type: object
>>
>> $ref: /schemas/gpio/rockchip,gpio-bank.yaml#
>> deprecated: true
>>
>> unevaluatedProperties: false
>
> I tried putting the gpio nodes out of the pinctrl node, they should work
> because they already have a gpio-ranges field.
> But unfortunately, that seem to break the pinctrl driver which hangs at some
> point. Maybe some adaptations are needed to support this, or am I missing
> something ?
The aliases that we added to the DT files are a work around to prevent damage when we moved to generic gpio node names.
There just happened to be some code for it in the driver...
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpio/gpio-rockchip.c#n719
Comment by robh:
"GPIO shouldn't really have an alias either IMO."
https://lore.kernel.org/linux-arm-kernel/20230118153236.GA33699-robh@kernel.org/
Mainline Rockchip gpio driver is not so to the standard.
The file gpio-rockchip.c currently does nothing with "gpio-ranges" vs. bank and node relation.
My simple patch was acked, but never applied. There's no public maintainer response of what to improve.
Guess, probably something more complicated idiot prove "gpio-ranges" parsing/bank linking is needed?
https://lore.kernel.org/linux-arm-kernel/890be9a0-8e82-a8f4-bc15-d5d1597343c2@gmail.com/
I leave this subject up to the experts to find out what is needed to improve.
Don't ask me.
Johan
>
>>> + compatible = "rockchip,gpio-bank";
>>
>> When in use as separate node the compatible must be SoC related.
>>
>> Question for the maintainers: Extra entry to rockchip,gpio-bank.yaml ??
>>
>>> + reg = <0x0 0x27320000 0x0 0x200>;
>>> + interrupts = <GIC_SPI 153
> IRQ_TYPE_LEVEL_HIGH>;
>>> + clocks = <&cru PCLK_GPIO0>, <&cru
> DBCLK_GPIO0>;
>>> +
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + gpio-ranges = <&pinctrl 0 0 32>;
>>> + interrupt-controller;
>>> + #interrupt-cells = <2>;
>>> + };
>>> +
>>> + gpio1: gpio@...10000 {
>>> +
>>> + gpio2: gpio@...20000 {
>>> +
>>> + gpio3: gpio@...30000 {
>>> +
>>> + gpio4: gpio@...40000 {
>>> + };
>>> +};
>>> +
>>> +#include "rk3576-pinctrl.dtsi"
>
> Regards,
>
> Detlev
>
>
>
Powered by blists - more mailing lists