Re: [RFC v2 0/2] Hi3521a support.

Re: [RFC v2 0/2] Hi3521a support.

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Resend RFC.
> 
> Changes in v2:
> - Actually include the dts files.
> - DT Bindings still missing, as the the driver is not quite complete
>   (need to add the reset controller bindings, have't quite figured that
>   out yet.)
> 
> Marty E. Plummer (2):
>   clk: hisilicon: add CRG driver Hi3521a SoC
>   arm: hisi: enable Hi3521a soc

Still no bindings for boards/SoC.



Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/05/2022 19:34, Marty E. Plummer wrote:
> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> generates clock and reset signals used by other module blocks on SoC.
> 
> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> ---
>  drivers/clk/hisilicon/Kconfig             |   8 ++
>  drivers/clk/hisilicon/Makefile            |   1 +
>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++

Bindings go to separate patch. Your patchset is unmerge'able.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> On 01/05/2022 19:34, Marty E. Plummer wrote:
> > Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> > generates clock and reset signals used by other module blocks on SoC.
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> >  drivers/clk/hisilicon/Kconfig             |   8 ++
> >  drivers/clk/hisilicon/Makefile            |   1 +
> >  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> 
> Bindings go to separate patch. Your patchset is unmerge'able.
> 
So, assuming I have the following patches:
1: +include/dt-bindings/clock/hi3521a-clock.h
2: +drivers/clk/hisilicon/crg-hi3521a.c
3: +Documentation/devicetree/bindings/whatever

In what order should they be applied?
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 12:58, Marty E. Plummer wrote:
> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>> generates clock and reset signals used by other module blocks on SoC.
>>>
>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>> ---
>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>
>> Bindings go to separate patch. Your patchset is unmerge'able.
>>
> So, assuming I have the following patches:
> 1: +include/dt-bindings/clock/hi3521a-clock.h
> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> 3: +Documentation/devicetree/bindings/whatever
> 
> In what order should they be applied?

Applied or sent? The maintainer will apply them in proper order, this is
bisectable.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 12:58, Marty E. Plummer wrote:
> > On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>> generates clock and reset signals used by other module blocks on SoC.
> >>>
> >>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>> ---
> >>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>
> >> Bindings go to separate patch. Your patchset is unmerge'able.
> >>
> > So, assuming I have the following patches:
> > 1: +include/dt-bindings/clock/hi3521a-clock.h
> > 2: +drivers/clk/hisilicon/crg-hi3521a.c
> > 3: +Documentation/devicetree/bindings/whatever
> > 
> > In what order should they be applied?
> 
> Applied or sent? The maintainer will apply them in proper order, this is
> bisectable.
> 
> 
Either or. Whatever makes the workload easier is what I'm looking for.
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 13:06, Marty E. Plummer wrote:
> On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 12:58, Marty E. Plummer wrote:
>>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
>>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
>>>>> generates clock and reset signals used by other module blocks on SoC.
>>>>>
>>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
>>>>> ---
>>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
>>>>>  drivers/clk/hisilicon/Makefile            |   1 +
>>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
>>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
>>>>
>>>> Bindings go to separate patch. Your patchset is unmerge'able.
>>>>
>>> So, assuming I have the following patches:
>>> 1: +include/dt-bindings/clock/hi3521a-clock.h
>>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
>>> 3: +Documentation/devicetree/bindings/whatever
>>>
>>> In what order should they be applied?
>>
>> Applied or sent? The maintainer will apply them in proper order, this is
>> bisectable.
>>
>>
> Either or. Whatever makes the workload easier is what I'm looking for.

Sorry, you need to be more specific. Apply is not a job for you, for the
patch submitter.

Then you miss here important piece - which is the first patch. DTS goes
always via separate branch (or even tree) from driver changes. That's
why bindings are always separate first patches.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Wed, Jun 01, 2022 at 01:09:28PM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 13:06, Marty E. Plummer wrote:
> > On Wed, Jun 01, 2022 at 01:00:38PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/06/2022 12:58, Marty E. Plummer wrote:
> >>> On Tue, May 03, 2022 at 01:37:42PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/05/2022 19:34, Marty E. Plummer wrote:
> >>>>> Add CRG driver for Hi3521A SoC. CRG (Clock and Reset Generator) module
> >>>>> generates clock and reset signals used by other module blocks on SoC.
> >>>>>
> >>>>> Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> >>>>> ---
> >>>>>  drivers/clk/hisilicon/Kconfig             |   8 ++
> >>>>>  drivers/clk/hisilicon/Makefile            |   1 +
> >>>>>  drivers/clk/hisilicon/crg-hi3521a.c       | 141 ++++++++++++++++++++++
> >>>>>  include/dt-bindings/clock/hi3521a-clock.h |  34 ++++++
> >>>>
> >>>> Bindings go to separate patch. Your patchset is unmerge'able.
> >>>>
> >>> So, assuming I have the following patches:
> >>> 1: +include/dt-bindings/clock/hi3521a-clock.h
> >>> 2: +drivers/clk/hisilicon/crg-hi3521a.c
> >>> 3: +Documentation/devicetree/bindings/whatever
> >>>
> >>> In what order should they be applied?
> >>
> >> Applied or sent? The maintainer will apply them in proper order, this is
> >> bisectable.
> >>
> >>
> > Either or. Whatever makes the workload easier is what I'm looking for.
> 
> Sorry, you need to be more specific. Apply is not a job for you, for the
> patch submitter.
> 
> Then you miss here important piece - which is the first patch. DTS goes
> always via separate branch (or even tree) from driver changes. That's
> why bindings are always separate first patches.
> 
So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
to the above list, or should those be the same patch as well?

> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 01/06/2022 20:24, Marty E. Plummer wrote:

>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>
>> Sorry, you need to be more specific. Apply is not a job for you, for the
>> patch submitter.
>>
>> Then you miss here important piece - which is the first patch. DTS goes
>> always via separate branch (or even tree) from driver changes. That's
>> why bindings are always separate first patches.
>>
> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> to the above list, or should those be the same patch as well?

For me does not matter, sub architecture maintainer might have preference.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2022 20:24, Marty E. Plummer wrote:
> 
> >>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>
> >> Sorry, you need to be more specific. Apply is not a job for you, for the
> >> patch submitter.
> >>
> >> Then you miss here important piece - which is the first patch. DTS goes
> >> always via separate branch (or even tree) from driver changes. That's
> >> why bindings are always separate first patches.
> >>
> > So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> > to the above list, or should those be the same patch as well?
> 
> For me does not matter, sub architecture maintainer might have preference.
> 
Fair enough. That being said, for the dt-bindings patch, is it
permissible to include #define CLOCK_FOO 1337 and so on for clocks which
haven't been wired up in the driver yet? As in, you know they're there,
and are important enough to model, but you haven't gotten to that point
yet?
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 03/06/2022 13:22, Marty E. Plummer wrote:
> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>
>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>
>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>> patch submitter.
>>>>
>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>> always via separate branch (or even tree) from driver changes. That's
>>>> why bindings are always separate first patches.
>>>>
>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>> to the above list, or should those be the same patch as well?
>>
>> For me does not matter, sub architecture maintainer might have preference.
>>
> Fair enough. That being said, for the dt-bindings patch, is it
> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> haven't been wired up in the driver yet? As in, you know they're there,
> and are important enough to model, but you haven't gotten to that point
> yet?

What would be the benefit to include them now? I imagine that if you
plan to add such clocks to the driver in next week or something, and you
need to use them in DTS, then it's fine. If that's not the case,
probably there is little sense in defining them upfront...


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Krzysztof Kozlowski]

On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> On 03/06/2022 13:22, Marty E. Plummer wrote:
>> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
>>> On 01/06/2022 20:24, Marty E. Plummer wrote:
>>>
>>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
>>>>>
>>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
>>>>> patch submitter.
>>>>>
>>>>> Then you miss here important piece - which is the first patch. DTS goes
>>>>> always via separate branch (or even tree) from driver changes. That's
>>>>> why bindings are always separate first patches.
>>>>>
>>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
>>>> to the above list, or should those be the same patch as well?
>>>
>>> For me does not matter, sub architecture maintainer might have preference.
>>>
>> Fair enough. That being said, for the dt-bindings patch, is it
>> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
>> haven't been wired up in the driver yet? As in, you know they're there,
>> and are important enough to model, but you haven't gotten to that point
>> yet?
> 
> What would be the benefit to include them now? I imagine that if you
> plan to add such clocks to the driver in next week or something, and you
> need to use them in DTS, then it's fine. If that's not the case,
> probably there is little sense in defining them upfront...

Actually I see one more benefit - since IDs should be incremented by
one, you can define all of them upfront thus having some
logical/alphabetical order/grouping. If you extend the bindings header
with new IDs later, they must go to the end of the list, thus maybe
ordering will not be that nice.

If you want, go ahead with all IDs. Just remeber that these must be IDs,
not register values or some programming offsets.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC v2 1/2] clk: hisilicon: add CRG driver Hi3521a SoC

[Marty E. Plummer]

On Mon, Jun 06, 2022 at 09:29:59AM +0200, Krzysztof Kozlowski wrote:
> On 05/06/2022 16:54, Krzysztof Kozlowski wrote:
> > On 03/06/2022 13:22, Marty E. Plummer wrote:
> >> On Thu, Jun 02, 2022 at 08:37:43AM +0200, Krzysztof Kozlowski wrote:
> >>> On 01/06/2022 20:24, Marty E. Plummer wrote:
> >>>
> >>>>>> Either or. Whatever makes the workload easier is what I'm looking for.
> >>>>>
> >>>>> Sorry, you need to be more specific. Apply is not a job for you, for the
> >>>>> patch submitter.
> >>>>>
> >>>>> Then you miss here important piece - which is the first patch. DTS goes
> >>>>> always via separate branch (or even tree) from driver changes. That's
> >>>>> why bindings are always separate first patches.
> >>>>>
> >>>> So, add a 4: arch/arm/boot/dts/soc.dtsi and 5: arch/arm/boot/dts/board.dts
> >>>> to the above list, or should those be the same patch as well?
> >>>
> >>> For me does not matter, sub architecture maintainer might have preference.
> >>>
> >> Fair enough. That being said, for the dt-bindings patch, is it
> >> permissible to include #define CLOCK_FOO 1337 and so on for clocks which
> >> haven't been wired up in the driver yet? As in, you know they're there,
> >> and are important enough to model, but you haven't gotten to that point
> >> yet?
> > 
> > What would be the benefit to include them now? I imagine that if you
> > plan to add such clocks to the driver in next week or something, and you
> > need to use them in DTS, then it's fine. If that's not the case,
> > probably there is little sense in defining them upfront...
> 
> Actually I see one more benefit - since IDs should be incremented by
> one, you can define all of them upfront thus having some
> logical/alphabetical order/grouping. If you extend the bindings header
> with new IDs later, they must go to the end of the list, thus maybe
> ordering will not be that nice.
> 
> If you want, go ahead with all IDs. Just remeber that these must be IDs,
> not register values or some programming offsets.
> 
Yeah, this was my intent. There are a number of non-standard,
proprietary IP blocks on this soc who's 'organized clock number' come
'in between' the more standard bits, depending on how you decide to
organize them (based on their parent clocks, based on the order they
appear in the crg register block, whatever). I *do* intend on hopefully
putting together drivers for these as well, though that's a long-term
stretch goal.
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

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.

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

> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory@800000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0xf00000>;
> +	};
> +};
> +
> +&hi_sfc {
> +	status = "okay";
> +	spi-nor@0 {

Looks like wrong node name. You need to run "dtbs_check" and fix
warnings related to your DTS.

> +		// compatible = "jedec,spi-nor";

No dead code in Linux kernel. Commented out stuff sometimes is okay if
it is explained with comments.

> +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <150000000>;
> +		// spi-rx-bus-width = <1>;
> +		// spi-tx-bus-width = <1>;
> +		// m25p,default-addr-width = <4>;
> +		// spi-max-frequency = <160000000>;

No dead code.

> +		m25p,fast-read;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x50000>;
> +				read-only;
> +			};
> +			u-boot-env@50000 {
> +				label = "u-boot-env";
> +				reg = <0x50000 0x20000>;
> +			};
> +			kernel@70000 {
> +				label = "kernel";
> +				reg = <0x70000 0x700000>;
> +			};
> +			rootfs@770000 {
> +				label = "rootfs";
> +				reg = <0x800000 0x300000>;
> +				read-only;
> +			};
> +			extra@b00000 {
> +				label = "extra";
> +				reg = <0xb00000 0x1500000>;
> +			};
> +		};
> +	};
> +};
> +
> +&dual_timer0 {
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> +	status = "okay";
> +};
> +
> +&pmx0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_pmx_func>;
> +	uart0_pmx_func: uart0_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};
> +
> +	sfc_pmx_func: sfc_pmx_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x1
> +			0x00c8 0x1
> +			0x00cc 0x1
> +			0x00d0 0x1
> +			0x00d4 0x1
> +		>;
> +	};
> +};
> +
> +&pmx1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sfc_cfg_func>;
> +	uart0_cfg_func: uart0_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00e8 0x0
> +			0x00ec 0x0
> +		>;
> +	};

Blank line

> +	sfc_cfg_func: sfc_cfg_func {
> +		pinctrl-single,pins = <
> +			0x00c4 0x58
> +			0x00c8 0x28
> +			0x00cc 0x38
> +			0x00d0 0x38
> +			0x00d4 0x38
> +		>;
> +	};
> +};
> +
> +/* &gmac0 { */
> +/* 	#address-cells = <1>; */
> +/* 	#size-cells = <0>; */
> +/* 	phy-handle = <&phy3>; */
> +/* 	phy-mode = "rgmii"; */
> +/* 	mac-address = [00 00 00 00 00 00]; */
> +/* 	status = "okay"; */
> +/**/
> +/* 	phy3: ethernet-phy@3 { */
> +/* 		reg = <3>; */
> +/* 	}; */
> +/* }; */


? What's this?

> diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> new file mode 100644
> index 000000000000..53993a32fd5d
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3521a.dtsi
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> + */
> +
> +#include <dt-bindings/clock/hi3521a-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +/dts-v1/;

Blank line.

> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0>;
> +			clock-frequency = <1100000000>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 13 0xf08>,
> +			     <GIC_PPI 14 0xf08>,
> +			     <GIC_PPI 11 0xf08>,
> +			     <GIC_PPI 10 0xf08>;

Use macros for these interrupts.

> +		clock-frequency = <24000000>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	gic: interrupt-controller@10301000 {
> +		compatible = "arm,pl390";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;

Put reg just after compatible. This applies to each other node as well
(if it comes with reg).

> +	};
> +
> +	xtal24m: xtal24m {

Generic node names, so one of: "clock-0" "clock-xtal24m"

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	clk_3m: clk_3m {

No underscores in node names, generic node name (see above).

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <3000000>;

This does not look like property of the SoC, so should be defined by boards.

> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "simple-bus";
> +		interrupt-parent = <&gic>;
> +		ranges;
> +
> +		hi_sfc: spi@10000000 {
> +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> +			reg-names = "control", "memory";
> +			clocks = <&crg HI3521A_FMC_CLK>;
> +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		/* usb0: usb@10030000 { */
> +		/* 		compatible = "generic-ohci"; */
> +		/* 		reg = <0x10030000 0x1000>; */
> +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> +		/* 		clocks = <&crg > */
> +		/* 	}; */

No dead code please.

> +		dmac: dma-controller@10060000 {
> +			compatible = "arm,pl080", "arm,primecell";
> +			arm,primecell-periphid = <0x00041080>;
> +			reg = <0x10060000 0x1000>;
> +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_DMAC_CLK>;
> +			clock-names = "apb_pclk";
> +			lli-bus-interface-ahb1;
> +			lli-bus-interface-ahb2;
> +			mem-bus-interface-ahb1;
> +			mem-bus-interface-ahb2;
> +			memcpy-burst-size = <256>;
> +			memcpy-bus-width = <32>;
> +			#dma-cells = <2>;
> +			status = "okay";

No need for status okay, it's by default.

> +		};
> +
> +		gmac0: ethernet@100a0000 {
> +			compatible = "hisilicon,hisi-gmac-v1";
> +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> +			clock-names = "mac_core", "mac_ifc";
> +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */

No dead code.

> +			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?

> +			reg = <0x12000000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> +				 <&sysctrl HI3521A_TIMER1_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer1: timer@12010000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12010000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> +				 <&sysctrl HI3521A_TIMER3_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer2: timer@12020000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12020000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> +				 <&sysctrl HI3521A_TIMER5_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		dual_timer3: timer@12030000 {
> +			compatible = "arm,sp804", "arm,primecell";
> +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x12030000 0x1000>;
> +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> +				 <&sysctrl HI3521A_TIMER7_CLK>,
> +				 <&crg HI3521A_APB_CLK>;
> +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> +			status = "disabled";
> +		};
> +
> +		crg: clock-reset-controller@12040000 {
> +			compatible = "hisilicon,hi3521a-crg";

Undocumented compatible.  Didn't checkpatch warn about it?

> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +			reg = <0x12040000 0x1000>;
> +		};
> +
> +		sysctrl: system-controller@12050000 {
> +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";

Same.

> +			reg = <0x12050000 0x1000>;
> +			#clock-cells = <1>;
> +			#reset-cells = <2>;
> +		};
> +
> +		reboot {
> +			compatible = "syscon-reboot";
> +			regmap = <&sysctrl>;
> +			offset = <0x4>;
> +			mask = <0xdeadbeef>;
> +		};
> +
> +		wdt0: watchdog@12070000 {
> +			compatible = "arm,sp805", "arm,primecell";
> +			arm,primecell-periphid = <0x00141805>;
> +			reg = <0x12070000 0x1000>;
> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_FIXED_3M>;
> +			clock-names = "apb_pclk";
> +		};
> +
> +		uart0: serial@12080000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12080000 0x1000>;
> +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> +			clock-names = "uartclk", "apb_pclk";
> +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart1: serial@12090000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x12090000 0x1000>;
> +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART1_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		uart2: serial@120a0000 {
> +			compatible = "arm,pl011", "arm,primecell";
> +			reg = <0x120a0000 0x1000>;
> +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_UART2_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> +			dma-names = "rx", "tx";
> +			status = "disabled";
> +		};
> +
> +		spi_bus0: spi@120d0000 {
> +			compatible = "arm,pl022", "arm,primecell";
> +			reg = <0x120d0000 0x1000>;
> +			arm,primecell-periphid = <0x00041022>;
> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&crg HI3521A_SPI0_CLK>;
> +			clock-names = "apb_pclk";
> +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> +			dma-names = "rx", "tx";
> +			num-cs = <2>;
> +			status = "disabled";
> +		};
> +
> +		pmx0: pinmux@120f0000 {
> +			compatible = "pinctrl-single";
> +			reg = <0x120f0000 0x188>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			#gpio-range-cells = <3>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +			pinctrl-single,function-mask = <7>;
> +			/* pin base, nr pins & gpio function */
> +			pinctrl-single,gpio-range = <
> +				&range 58 4 0
> +			>;
> +
> +			range: gpio-range {
> +				#pinctrl-single,gpio-range-cells = <3>;
> +			};
> +		};
> +
> +		pmx1: pinmux@120f0800 {
> +			compatible = "pinconf-single";
> +			reg = <0x120f0800 0x1d4>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#pinctrl-cells = <1>;
> +			ranges;
> +
> +			pinctrl-single,register-width = <32>;
> +		};
> +
> +		gpio0: gpio@12150000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12150000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio1: gpio@12160000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12160000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio2: gpio@12170000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12170000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio3: gpio@12180000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12180000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio4: gpio@12190000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x12190000 0x1000>;
> +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio5: gpio@121a0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121a0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio6: gpio@121b0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121b0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;

Same interrupts as for other nodes? Is it correct?

> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +
> +		gpio7: gpio@121c0000 {
> +			compatible = "arm,pl061", "arm,primecell";
> +			reg = <0x121c0000 0x1000>;
> +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			status = "disabled";
> +		};
> +_MULTI_V7


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marty E. Plummer]

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?
> 
> >  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.
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@800000000 {
> > +		device_type = "memory";
> > +		reg = <0x80000000 0xf00000>;
> > +	};
> > +};
> > +
> > +&hi_sfc {
> > +	status = "okay";
> > +	spi-nor@0 {
> 
> Looks like wrong node name. You need to run "dtbs_check" and fix
> warnings related to your DTS.
> 
Ah, that's a good thing to know. dt-doc-validate isn't found, doesn't
seem packaged for gentoo either. I'll see about that asap.
> > +		// compatible = "jedec,spi-nor";
> 
> No dead code in Linux kernel. Commented out stuff sometimes is okay if
> it is explained with comments.
> 
As mentioned in the title, just an RFC, not final.
> > +		compatible = "macronix,mx25l25635e", "jedec,spi-nor";
> > +		reg = <0>;
> > +		spi-max-frequency = <150000000>;
> > +		// spi-rx-bus-width = <1>;
> > +		// spi-tx-bus-width = <1>;
> > +		// m25p,default-addr-width = <4>;
> > +		// spi-max-frequency = <160000000>;
> 
> No dead code.
> 
> > +		m25p,fast-read;
> > +
> > +		partitions {
> > +			compatible = "fixed-partitions";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			
> > +			u-boot@0 {
> > +				label = "u-boot";
> > +				reg = <0x0 0x50000>;
> > +				read-only;
> > +			};
> > +			u-boot-env@50000 {
> > +				label = "u-boot-env";
> > +				reg = <0x50000 0x20000>;
> > +			};
> > +			kernel@70000 {
> > +				label = "kernel";
> > +				reg = <0x70000 0x700000>;
> > +			};
> > +			rootfs@770000 {
> > +				label = "rootfs";
> > +				reg = <0x800000 0x300000>;
> > +				read-only;
> > +			};
> > +			extra@b00000 {
> > +				label = "extra";
> > +				reg = <0xb00000 0x1500000>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&dual_timer0 {
> > +	status = "okay";
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart0_pmx_func &uart0_cfg_func>;
> > +	status = "okay";
> > +};
> > +
> > +&pmx0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_pmx_func>;
> > +	uart0_pmx_func: uart0_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> > +
> > +	sfc_pmx_func: sfc_pmx_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x1
> > +			0x00c8 0x1
> > +			0x00cc 0x1
> > +			0x00d0 0x1
> > +			0x00d4 0x1
> > +		>;
> > +	};
> > +};
> > +
> > +&pmx1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&sfc_cfg_func>;
> > +	uart0_cfg_func: uart0_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00e8 0x0
> > +			0x00ec 0x0
> > +		>;
> > +	};
> 
> Blank line
> 
> > +	sfc_cfg_func: sfc_cfg_func {
> > +		pinctrl-single,pins = <
> > +			0x00c4 0x58
> > +			0x00c8 0x28
> > +			0x00cc 0x38
> > +			0x00d0 0x38
> > +			0x00d4 0x38
> > +		>;
> > +	};
> > +};
> > +
> > +/* &gmac0 { */
> > +/* 	#address-cells = <1>; */
> > +/* 	#size-cells = <0>; */
> > +/* 	phy-handle = <&phy3>; */
> > +/* 	phy-mode = "rgmii"; */
> > +/* 	mac-address = [00 00 00 00 00 00]; */
> > +/* 	status = "okay"; */
> > +/**/
> > +/* 	phy3: ethernet-phy@3 { */
> > +/* 		reg = <3>; */
> > +/* 	}; */
> > +/* }; */
> 
> 
> ? What's this?
> 
ethernet port. its giving me a bit of trouble at the moment, but it does
work and has a phy at 3.
> > diff --git a/arch/arm/boot/dts/hi3521a.dtsi b/arch/arm/boot/dts/hi3521a.dtsi
> > new file mode 100644
> > index 000000000000..53993a32fd5d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/hi3521a.dtsi
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2017-2022 Marty Plummer <hanetzer@startmail.com>
> > + */
> > +
> > +#include <dt-bindings/clock/hi3521a-clock.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +/dts-v1/;
> 
> Blank line.
> 
> > +/ {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu0: cpu@0 {
> > +			device_type = "cpu";
> > +			compatible = "arm,cortex-a7";
> > +			reg = <0>;
> > +			clock-frequency = <1100000000>;
> > +		};
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv7-timer";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_PPI 13 0xf08>,
> > +			     <GIC_PPI 14 0xf08>,
> > +			     <GIC_PPI 11 0xf08>,
> > +			     <GIC_PPI 10 0xf08>;
> 
> Use macros for these interrupts.
> 
moot point, turns out enabling this timer is causing the system to lock
up. Something's weird there.
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> > +	pmu {
> > +		compatible = "arm,cortex-a7-pmu";
> > +		interrupt-parent = <&gic>;
> > +		interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> > +	};
> > +
> > +	gic: interrupt-controller@10301000 {
> > +		compatible = "arm,pl390";
> > +		#interrupt-cells = <3>;
> > +		interrupt-controller;
> > +		reg = <0x10301000 0x1000>, <0x10302000 0x1000>;
> 
> Put reg just after compatible. This applies to each other node as well
> (if it comes with reg).
> 
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.
> > +	};
> > +
> > +	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.
> 
> > +	};
> > +
> > +	soc {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "simple-bus";
> > +		interrupt-parent = <&gic>;
> > +		ranges;
> > +
> > +		hi_sfc: spi@10000000 {
> > +			compatible = "hisilicon,hi3521a-spi-nor", "hisilicon,fmc-spi-nor";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x10000000 0x1000>, <0x14000000 0x10000>;
> > +			reg-names = "control", "memory";
> > +			clocks = <&crg HI3521A_FMC_CLK>;
> > +			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > +			status = "disabled";
> > +		};
> > +
> > +		/* usb0: usb@10030000 { */
> > +		/* 		compatible = "generic-ohci"; */
> > +		/* 		reg = <0x10030000 0x1000>; */
> > +		/* 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; */
> > +		/* 		clocks = <&crg > */
> > +		/* 	}; */
> 
> No dead code please.
> 
> > +		dmac: dma-controller@10060000 {
> > +			compatible = "arm,pl080", "arm,primecell";
> > +			arm,primecell-periphid = <0x00041080>;
> > +			reg = <0x10060000 0x1000>;
> > +			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_DMAC_CLK>;
> > +			clock-names = "apb_pclk";
> > +			lli-bus-interface-ahb1;
> > +			lli-bus-interface-ahb2;
> > +			mem-bus-interface-ahb1;
> > +			mem-bus-interface-ahb2;
> > +			memcpy-burst-size = <256>;
> > +			memcpy-bus-width = <32>;
> > +			#dma-cells = <2>;
> > +			status = "okay";
> 
> No need for status okay, it's by default.
> 
ah, good to know.
> > +		};
> > +
> > +		gmac0: ethernet@100a0000 {
> > +			compatible = "hisilicon,hisi-gmac-v1";
> > +			reg = <0x100a0000 0x1000>, <0x1204008c 0x4>;
> > +			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_ETH_CLK>, <&crg HI3521A_ETH_MACIF_CLK>;
> > +			clock-names = "mac_core", "mac_ifc";
> > +			/* resets = <&crg 0x78 0>, <&crg 0x78 2>, <&crg 0x78 5>; */
> > +			/* reset-names = "mac_core", "mac_ifc", "phy"; */
> > +			/* hisilicon,phy-reset-delays-us = <10000 10000 30000>; */
> 
> No dead code.
> 
> > +			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.
> > +			reg = <0x12000000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER0_CLK>,
> > +				 <&sysctrl HI3521A_TIMER1_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer1: timer@12010000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12010000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER2_CLK>,
> > +				 <&sysctrl HI3521A_TIMER3_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer2: timer@12020000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12020000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER4_CLK>,
> > +				 <&sysctrl HI3521A_TIMER5_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		dual_timer3: timer@12030000 {
> > +			compatible = "arm,sp804", "arm,primecell";
> > +			interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> > +			reg = <0x12030000 0x1000>;
> > +			clocks = <&sysctrl HI3521A_TIMER6_CLK>,
> > +				 <&sysctrl HI3521A_TIMER7_CLK>,
> > +				 <&crg HI3521A_APB_CLK>;
> > +			clock-names = "timer0clk", "timer1clk", "apb_pclk";
> > +			status = "disabled";
> > +		};
> > +
> > +		crg: clock-reset-controller@12040000 {
> > +			compatible = "hisilicon,hi3521a-crg";
> 
> Undocumented compatible.  Didn't checkpatch warn about it?
> 
As mentioned, RFC. I'm more concerned with getting it to work consistently
at the moment.
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +			reg = <0x12040000 0x1000>;
> > +		};
> > +
> > +		sysctrl: system-controller@12050000 {
> > +			compatible = "hisilicon,hi3521a-sysctrl", "syscon";
> 
> Same.
> 
> > +			reg = <0x12050000 0x1000>;
> > +			#clock-cells = <1>;
> > +			#reset-cells = <2>;
> > +		};
> > +
> > +		reboot {
> > +			compatible = "syscon-reboot";
> > +			regmap = <&sysctrl>;
> > +			offset = <0x4>;
> > +			mask = <0xdeadbeef>;
> > +		};
> > +
> > +		wdt0: watchdog@12070000 {
> > +			compatible = "arm,sp805", "arm,primecell";
> > +			arm,primecell-periphid = <0x00141805>;
> > +			reg = <0x12070000 0x1000>;
> > +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_FIXED_3M>;
> > +			clock-names = "apb_pclk";
> > +		};
> > +
> > +		uart0: serial@12080000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12080000 0x1000>;
> > +			interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART0_CLK>, <&crg HI3521A_UART0_CLK>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +			dmas = <&dmac 0 1>, <&dmac 1 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart1: serial@12090000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x12090000 0x1000>;
> > +			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART1_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 2 1>, <&dmac 3 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		uart2: serial@120a0000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x120a0000 0x1000>;
> > +			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_UART2_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 4 1>, <&dmac 5 2>;
> > +			dma-names = "rx", "tx";
> > +			status = "disabled";
> > +		};
> > +
> > +		spi_bus0: spi@120d0000 {
> > +			compatible = "arm,pl022", "arm,primecell";
> > +			reg = <0x120d0000 0x1000>;
> > +			arm,primecell-periphid = <0x00041022>;
> > +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&crg HI3521A_SPI0_CLK>;
> > +			clock-names = "apb_pclk";
> > +			dmas = <&dmac 6 1>, <&dmac 7 2>;
> > +			dma-names = "rx", "tx";
> > +			num-cs = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		pmx0: pinmux@120f0000 {
> > +			compatible = "pinctrl-single";
> > +			reg = <0x120f0000 0x188>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			#gpio-range-cells = <3>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +			pinctrl-single,function-mask = <7>;
> > +			/* pin base, nr pins & gpio function */
> > +			pinctrl-single,gpio-range = <
> > +				&range 58 4 0
> > +			>;
> > +
> > +			range: gpio-range {
> > +				#pinctrl-single,gpio-range-cells = <3>;
> > +			};
> > +		};
> > +
> > +		pmx1: pinmux@120f0800 {
> > +			compatible = "pinconf-single";
> > +			reg = <0x120f0800 0x1d4>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			#pinctrl-cells = <1>;
> > +			ranges;
> > +
> > +			pinctrl-single,register-width = <32>;
> > +		};
> > +
> > +		gpio0: gpio@12150000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12150000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio1: gpio@12160000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12160000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio2: gpio@12170000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12170000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio3: gpio@12180000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12180000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio4: gpio@12190000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x12190000 0x1000>;
> > +			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio5: gpio@121a0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121a0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio6: gpio@121b0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121b0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> 
> Same interrupts as for other nodes? Is it correct?
> 
Yep, there are three interrupts for the gpios and they're shared among a
few each.
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +
> > +		gpio7: gpio@121c0000 {
> > +			compatible = "arm,pl061", "arm,primecell";
> > +			reg = <0x121c0000 0x1000>;
> > +			interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			interrupt-controller;
> > +			#interrupt-cells = <2>;
> > +			status = "disabled";
> > +		};
> > +_MULTI_V7
> 
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On 03/05/2022 17:51, Marty E. Plummer wrote:
>>>> 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.

So it looks like a combined interrupt, doesn't it?


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel