Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc

[Krzysztof Kozlowski]

On 03/05/2022 15:44, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:47:01PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Enable Hisilicon Hi3521A/Hi3520DCV300 SoC. This SoC series includes
>>> hardware mutlimedia codec cores, commonly used in consumer cctv/dvr
>>> security systems and ipcameras. The arm core is a Cortex A7.
>>>
>>> Add hi3521a.dtsi and hi3521a-rs-dm290e.dts for RaySharp CCTV systems,
>>> marketed under the name Samsung SDR-B74301N.
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@xxxxxxxxxxxxx>
>>> ---
>>>  arch/arm/boot/dts/Makefile              |   2 +
>>>  arch/arm/boot/dts/hi3521a-rs-dm290e.dts | 134 ++++++++
>>>  arch/arm/boot/dts/hi3521a.dtsi          | 423 ++++++++++++++++++++++++
>>
>> DTSes go to separate patches.
> Do you mean dts and dtsi need to be separate patches?

I mean that any changes to "arch/arm/boot/dts/" have to be separate from
other changes. These can be still one patch. See other examples on
mailing lists.

>>
>>>  arch/arm/mach-hisi/Kconfig              |   9 +
>>>  4 files changed, 568 insertions(+)
>>>  create mode 100644 arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>>  create mode 100644 arch/arm/boot/dts/hi3521a.dtsi
>>>
>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>> index 7c16f8a2b738..535cef3b14ab 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -242,6 +242,8 @@ dtb-$(CONFIG_ARCH_GEMINI) += \
>>>  	gemini-ssi1328.dtb \
>>>  	gemini-wbd111.dtb \
>>>  	gemini-wbd222.dtb
>>> +dtb-$(CONFIG_ARCH_HI3521A) += \
>>> +	hi3521a-rs-dm290e.dtb
>>>  dtb-$(CONFIG_ARCH_HI3xxx) += \
>>>  	hi3620-hi4511.dtb
>>>  dtb-$(CONFIG_ARCH_HIGHBANK) += \
>>> diff --git a/arch/arm/boot/dts/hi3521a-rs-dm290e.dts b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>> new file mode 100644
>>> index 000000000000..b24fcf2ca85e
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hi3521a-rs-dm290e.dts
>>> @@ -0,0 +1,134 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@xxxxxxxxxxxxx>
>>> + */
>>> +
>>> +#include "hi3521a.dtsi"
>>> +
>>> +/ {
>>> +	model = "RaySharp RS-DM-290E DVR Board";
>>> +	compatible = "raysharp,rs-dm-290e", "hisilicon,hi3521a";
>>
>> Please run checkpatch and fix the warnings.
>>
> sunova. I could have sworn I had my editor setup right for whitespace
> and such.

It's not about whitespace but:

WARNING: DT compatible string "raysharp,rs-dm-290e" appears
un-documented -- check ./Documentation/devicetree/bindings/


WARNING: DT compatible string vendor "raysharp" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml


(...)

> Ah gotcha.
>>> +	};
>>> +
>>> +	xtal24m: xtal24m {
>>
>> Generic node names, so one of: "clock-0" "clock-xtal24m"
>>
> Will do.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.
>>
> SoC requires a 24Mhz osc (and a 32khz one as well), so it'll always be
> present regardless.

Sure, but DTS/DTSI describes hardware. If the clock is not in the SoC
but on the board, it should be in the board DTSI. Many times such clocks
are put partially in DTSI and only their specific parts - frequency - in
the board DTS, to indicate that implementation is relevant to the board,
not SoC.

>>> +	};
>>> +
>>> +	clk_3m: clk_3m {
>>
>> No underscores in node names, generic node name (see above).
>>
> early debugging clock, will be removed.
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <3000000>;
>>
>> This does not look like property of the SoC, so should be defined by boards.

(...)

>>
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		dual_timer0: timer@12000000 {
>>> +			compatible = "arm,sp804", "arm,primecell";
>>> +			interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>
>> A bit weird interrupts... the same?
>>
> Yes, though I am aware that some sp804 timers do have a separate
> interrupts per pair.

They have also separate interrupts, one combined interrupt or one sole
interrupt. However what you described here is one interrupt line
physically connected to two separate pins on the device yet still not
being somehow shared (shared as "combined interrupt"). I don't think it
is your case...



Best regards,
Krzysztof