2020-07-28 11:29:45

by Matthias Schiffer

[permalink] [raw]
Subject: [PATCH] arm: dts: imx7: add QSPI

In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add the
QSPI controller to imx7s.dtsi.

Based-on-patch-by: Han Xu <[email protected]>
Signed-off-by: Matthias Schiffer <[email protected]>
---
arch/arm/boot/dts/imx7s.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 1cfaf410aa43..e45683e61593 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1162,6 +1162,19 @@
status = "disabled";
};

+ qspi1: spi@30bb0000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,imx7d-qspi";
+ reg = <0x30bb0000 0x10000>, <0x60000000 0x10000000>;
+ reg-names = "QuadSPI", "QuadSPI-memory";
+ interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX7D_QSPI_ROOT_CLK>,
+ <&clks IMX7D_QSPI_ROOT_CLK>;
+ clock-names = "qspi_en", "qspi";
+ status = "disabled";
+ };
+
sdma: sdma@30bd0000 {
compatible = "fsl,imx7d-sdma", "fsl,imx35-sdma";
reg = <0x30bd0000 0x10000>;
--
2.17.1


2020-07-28 13:53:39

by Marco Felsch

[permalink] [raw]
Subject: Re: [PATCH] arm: dts: imx7: add QSPI

Hi Matthias,

thanks for the patch.

On 20-07-28 13:28, Matthias Schiffer wrote:
> In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS, add the
> QSPI controller to imx7s.dtsi.
>
> Based-on-patch-by: Han Xu <[email protected]>
> Signed-off-by: Matthias Schiffer <[email protected]>
> ---
> arch/arm/boot/dts/imx7s.dtsi | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 1cfaf410aa43..e45683e61593 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -1162,6 +1162,19 @@
> status = "disabled";
> };
>
> + qspi1: spi@30bb0000 {

Are there more controllers and why not using "qspi@30bb0000" as node
name?

> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "fsl,imx7d-qspi";
> + reg = <0x30bb0000 0x10000>, <0x60000000 0x10000000>;
> + reg-names = "QuadSPI", "QuadSPI-memory";

The node should begin with compatible, reg, reg-names properties.
Pls check the current .dtsi file for examples.

> + interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clks IMX7D_QSPI_ROOT_CLK>,
> + <&clks IMX7D_QSPI_ROOT_CLK>;
> + clock-names = "qspi_en", "qspi";
> + status = "disabled";
> + };
> +
> sdma: sdma@30bd0000 {
> compatible = "fsl,imx7d-sdma", "fsl,imx35-sdma";
> reg = <0x30bd0000 0x10000>;

Regards,
Marco

2020-07-28 14:07:38

by Matthias Schiffer

[permalink] [raw]
Subject: Re: (EXT) Re: [PATCH] arm: dts: imx7: add QSPI

On Tue, 2020-07-28 at 15:51 +0200, Marco Felsch wrote:
> Hi Matthias,
>
> thanks for the patch.
>
> On 20-07-28 13:28, Matthias Schiffer wrote:
> > In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS,
> > add the
> > QSPI controller to imx7s.dtsi.
> >
> > Based-on-patch-by: Han Xu <[email protected]>
> > Signed-off-by: Matthias Schiffer <[email protected]
> > >
> > ---
> > arch/arm/boot/dts/imx7s.dtsi | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/imx7s.dtsi
> > b/arch/arm/boot/dts/imx7s.dtsi
> > index 1cfaf410aa43..e45683e61593 100644
> > --- a/arch/arm/boot/dts/imx7s.dtsi
> > +++ b/arch/arm/boot/dts/imx7s.dtsi
> > @@ -1162,6 +1162,19 @@
> > status = "disabled";
> > };
> >
> > + qspi1: spi@30bb0000 {
>
> Are there more controllers and why not using "qspi@30bb0000" as node
> name?

The vast majority of QSPI controllers use spi@ node names, qspi@ only
appears in a single example in Documentation/devicetree/bindings/, and
in no actual DTS(I) files.

There is only one controller. The label "qspi1" is chosen as this has
been in use in the linux-imx vendor kernels for years; IMHO, switching
to "qspi" would just cause unnecessary churn for dependent device
trees. I have no strong opinions on this though.

>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "fsl,imx7d-qspi";
> > + reg = <0x30bb0000 0x10000>, <0x60000000
> > 0x10000000>;
> > + reg-names = "QuadSPI", "QuadSPI-
> > memory";
>
> The node should begin with compatible, reg, reg-names properties.
> Pls check the current .dtsi file for examples.

Thanks, will fix.

>
> > + interrupts = <GIC_SPI 107
> > IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&clks IMX7D_QSPI_ROOT_CLK>,
> > + <&clks IMX7D_QSPI_ROOT_CLK>;
> > + clock-names = "qspi_en", "qspi";
> > + status = "disabled";
> > + };
> > +
> > sdma: sdma@30bd0000 {
> > compatible = "fsl,imx7d-sdma",
> > "fsl,imx35-sdma";
> > reg = <0x30bd0000 0x10000>;
>
> Regards,
> Marco

2020-07-28 14:32:08

by Marco Felsch

[permalink] [raw]
Subject: Re: (EXT) Re: [PATCH] arm: dts: imx7: add QSPI

On 20-07-28 16:05, Matthias Schiffer wrote:
> On Tue, 2020-07-28 at 15:51 +0200, Marco Felsch wrote:
> > Hi Matthias,
> >
> > thanks for the patch.
> >
> > On 20-07-28 13:28, Matthias Schiffer wrote:
> > > In preparation for an update of the TQ-Systems TQMa7x/MBa7x DTS,
> > > add the
> > > QSPI controller to imx7s.dtsi.
> > >
> > > Based-on-patch-by: Han Xu <[email protected]>
> > > Signed-off-by: Matthias Schiffer <[email protected]
> > > >
> > > ---
> > > arch/arm/boot/dts/imx7s.dtsi | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/imx7s.dtsi
> > > b/arch/arm/boot/dts/imx7s.dtsi
> > > index 1cfaf410aa43..e45683e61593 100644
> > > --- a/arch/arm/boot/dts/imx7s.dtsi
> > > +++ b/arch/arm/boot/dts/imx7s.dtsi
> > > @@ -1162,6 +1162,19 @@
> > > status = "disabled";
> > > };
> > >
> > > + qspi1: spi@30bb0000 {
> >
> > Are there more controllers and why not using "qspi@30bb0000" as node
> > name?
>
> The vast majority of QSPI controllers use spi@ node names, qspi@ only
> appears in a single example in Documentation/devicetree/bindings/, and
> in no actual DTS(I) files.

IMHO using spi as node name is incorrect because this inherits the
assumption to connect 'normal' spi devices to these controllers. But
this is absolutly not the case since in most cases the qspi controllers
are state-machines optimized for qspi memory devices. So all dts(i)
files using spi as node name are incorrect IMHO. But we can't change
that due to the backward compability.

> There is only one controller. The label "qspi1" is chosen as this has
> been in use in the linux-imx vendor kernels for years; IMHO, switching
> to "qspi" would just cause unnecessary churn for dependent device
> trees. I have no strong opinions on this though.

Why? There is no _mainline_ imx7s devicetree using this node currently.
I would drop the number since we have only one instance.

Regards,
Marco


> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + compatible = "fsl,imx7d-qspi";
> > > + reg = <0x30bb0000 0x10000>, <0x60000000
> > > 0x10000000>;
> > > + reg-names = "QuadSPI", "QuadSPI-
> > > memory";
> >
> > The node should begin with compatible, reg, reg-names properties.
> > Pls check the current .dtsi file for examples.
>
> Thanks, will fix.
>
> >
> > > + interrupts = <GIC_SPI 107
> > > IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&clks IMX7D_QSPI_ROOT_CLK>,
> > > + <&clks IMX7D_QSPI_ROOT_CLK>;
> > > + clock-names = "qspi_en", "qspi";
> > > + status = "disabled";
> > > + };
> > > +
> > > sdma: sdma@30bd0000 {
> > > compatible = "fsl,imx7d-sdma",
> > > "fsl,imx35-sdma";
> > > reg = <0x30bd0000 0x10000>;
> >
> > Regards,
> > Marco
>
>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |