linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: "Marty E. Plummer" <hanetzer@startmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: arnd@arndb.de, cai.huoqing@linux.dev, christian.koenig@amd.com,
	devicetree@vger.kernel.org, gengdongjiu@huawei.com,
	krzysztof.kozlowski+dt@linaro.org,
	linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
	linux@armlinux.org.uk, michael@walle.cc,
	miquel.raynal@bootlin.com, mturquette@baylibre.com,
	novikov@ispras.ru, olof@lixom.net, p.yadav@ti.com,
	rdunlap@infradead.org, richard@nod.at, robh+dt@kernel.org,
	sboyd@kernel.org, soc@kernel.org, sumit.semwal@linaro.org,
	tudor.ambarus@microchip.com, vigneshr@ti.com,
	xuwei5@hisilicon.com
Subject: Re: [RFC v2 2/2] arm: hisi: enable Hi3521a soc
Date: Tue, 3 May 2022 10:51:41 -0500	[thread overview]
Message-ID: <20220503155141.ekbysx6fjom5el2h@proprietary-killer> (raw)
In-Reply-To: <75a48dfa-6fc9-aed9-b00e-d928bd9f33af@linaro.org>

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@startmail.com>
> >>> ---
> >>>  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@startmail.com>
> >>> + */
> >>> +
> >>> +#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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply	other threads:[~2022-05-03 15:52 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-01  5:44 [PATCH 0/2] Hi3521a support Marty E. Plummer
     [not found] ` <20220501173423.2473093-1-hanetzer@startmail.com>
2022-05-03 11:37   ` [RFC v2 " Krzysztof Kozlowski
     [not found]   ` <20220501173423.2473093-2-hanetzer@startmail.com>
2022-05-03 11:37     ` [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC Krzysztof Kozlowski
2022-06-01 10:58       ` Marty E. Plummer
2022-06-01 11:00         ` Krzysztof Kozlowski
2022-06-01 11:06           ` Marty E. Plummer
2022-06-01 11:09             ` Krzysztof Kozlowski
2022-06-01 18:24               ` Marty E. Plummer
2022-06-02  6:37                 ` Krzysztof Kozlowski
2022-06-03 11:22                   ` Marty E. Plummer
2022-06-05 14:54                     ` Krzysztof Kozlowski
2022-06-06  7:29                       ` Krzysztof Kozlowski
2022-06-06 11:34                         ` Marty E. Plummer
     [not found]   ` <20220501173423.2473093-3-hanetzer@startmail.com>
2022-05-03 11:47     ` [RFC v2 2/2] arm: hisi: enable Hi3521a soc Krzysztof Kozlowski
2022-05-03 13:44       ` Marty E. Plummer
2022-05-03 14:55         ` Krzysztof Kozlowski
2022-05-03 15:51           ` Marty E. Plummer [this message]
2022-05-03 15:57             ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220503155141.ekbysx6fjom5el2h@proprietary-killer \
    --to=hanetzer@startmail.com \
    --cc=arnd@arndb.de \
    --cc=cai.huoqing@linux.dev \
    --cc=christian.koenig@amd.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gengdongjiu@huawei.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=krzysztof.kozlowski@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux@armlinux.org.uk \
    --cc=michael@walle.cc \
    --cc=miquel.raynal@bootlin.com \
    --cc=mturquette@baylibre.com \
    --cc=novikov@ispras.ru \
    --cc=olof@lixom.net \
    --cc=p.yadav@ti.com \
    --cc=rdunlap@infradead.org \
    --cc=richard@nod.at \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=soc@kernel.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tudor.ambarus@microchip.com \
    --cc=vigneshr@ti.com \
    --cc=xuwei5@hisilicon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).