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

[Marco Felsch <m.felsch@pengutronix.de>]

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 <han.xu@nxp.com>
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > > >
> > > ---
> > >  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 |