netdev - Re: [PATCH 1/6] arm64: dts: qcom: ipq9574: Add PPE device tree node

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]

Date: Wed, 10 Jan 2024 12:40:48 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Luo Jie <quic_luoj@...cinc.com>, andersson@...nel.org,
 konrad.dybcio@...aro.org, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org
Cc: linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 quic_kkumarcs@...cinc.com, quic_suruchia@...cinc.com, quic_soni@...cinc.com,
 quic_pavir@...cinc.com, quic_souravp@...cinc.com, quic_linchen@...cinc.com,
 quic_leiwei@...cinc.com
Subject: Re: [PATCH 1/6] arm64: dts: qcom: ipq9574: Add PPE device tree node

On 10/01/2024 12:20, Luo Jie wrote:
> The PPE device tree node includes the PPE initialization configurations
> and UNIPHY instance configuration.
> 
> Ther are 3 UNIPHYs(PCS) on the platform ipq9574, which register the
> clock provider to output the clock for PPE port to work on the different
> link speed.
> 
> Signed-off-by: Luo Jie <quic_luoj@...cinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 730 +++++++++++++++++++++++++-
>  1 file changed, 724 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 810cda4a850f..5fa241e27c8b 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -775,16 +775,734 @@ nsscc: nsscc@...00000 {
>  				 <&bias_pll_nss_noc_clk>,
>  				 <&bias_pll_ubi_nc_clk>,
>  				 <&gcc_gpll0_out_aux>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> +				 <&uniphys 0>,
> +				 <&uniphys 1>,
> +				 <&uniphys 2>,
> +				 <&uniphys 3>,
> +				 <&uniphys 4>,
> +				 <&uniphys 5>,
>  				 <&xo_board_clk>;
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  		};
> +
> +		qcom_ppe: qcom-ppe@...00000 {

qcom is definitely not a generic name.

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 = "qcom,ipq9574-ppe";

I don't see this documented. I don't see reference to posted bindings.

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.

Ignoring this warning is a sign you don't really check your patches
before sending.

> +			reg = <0x3a000000 0xb00000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;

Put after reg.

> +			status = "okay";

Drop

All of above comments apply to your entire patchset and all places.

Looking at code further, it does not look like suitable for mainline,
but copy of downstream code. That's not what we expect upstream. Please
go back to your bindings first. Also, I really insist you reaching out
to other folks to help you in this process.

Best regards,
Krzysztof