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

[Marty E. Plummer]

On Tue, May 03, 2022 at 04:55:23PM +0200, Krzysztof Kozlowski wrote:
> 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:
> 
ah. Well, I'm not certain what we'll even call the board in the end,
kind of a placeholder for now. What do you even name devices which are
generic consumer hardware that they never intended to be tinkered with
this way? the board is whiteboxed by several different vendors.
> 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.
> 
Ah ok, that makes sense I guess.
> >>> +	};
> >>> +
> >>> +	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...
> 
Unsure. datasheet just says '33 | Timer0/Timer1'. I don't think these
timers are attached to pins, however.
> 
> 
> Best regards,
> Krzysztof