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: <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

Powered by Openwall GNU/*/Linux Powered by OpenVZ