linux-kernel - Re: [PATCH 1/4] spi: Add parameter for clock to rx delay

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231027144646.577210ff@xps-13>
Date:   Fri, 27 Oct 2023 14:46:46 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     "Stoll, Eberhard" <eberhard.stoll@...tron.de>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Eberhard Stoll <estl@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        "Schrempf, Frieder" <frieder.schrempf@...tron.de>,
        Amit Kumar Mahapatra <amit.kumar-mahapatra@....com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Krishna Yarlagadda <kyarlagadda@...dia.com>,
        Leonard Göhrs <l.goehrs@...gutronix.de>,
        Yang Yingliang <yangyingliang@...wei.com>
Subject: Re: [PATCH 1/4] spi: Add parameter for clock to rx delay

Hi Eberhard,

eberhard.stoll@...tron.de wrote on Fri, 27 Oct 2023 12:41:23 +0000:

> Hello,
> 
> > > > Can you be more specific? I am wondering how big the need is.  
> > >
> > > In our case it's a QSPI NAND chip (Winbond W25N02KV). This device
> > > can operate at 104MHz SPI clock. But it also has a tCLQV value of 7ns.
> > > The tCLQV value limits the SPI clock speed for this device to 2x7ns
> > > (if it is not adjustable in the SPI controller) which is approximately
> > > 70MHz.
> > >
> > > Without the ability to set the tCLQV value, the SPI clock has to be
> > > limited to 70MHz in device tree for this bus.
> > >
> > > In our case the Winbond W25N02KV chip is a replacement of an
> > > older chip. The older chip can operate at 104MHz and does not
> > > have the tCLQV restrictions as this new one.
> > > The new chip is mostly is better than the data sheet and meet the
> > > timing requirements for 104MHz. But on higher temperatures
> > > devices fail.
> > >
> > > In device tree QSPI NAND chips are configured by a compatible
> > > property of 'spi-nand'. The mtd layer detects the real device
> > > and fetches the properties of this device from the appropriate
> > > driver.
> > >
> > > So for our case (boards containing the old and new chip) we well
> > > have to reduce the SPI clock for the entire QSPI bus to 70MHz, even
> > > for the elder chips which can operate well also with 104MHz.  
> > 
> > So, to me sounds like device tree source issue. I.e. you need to provide
> > different DT(b)s depending on the platform (and how it should be).
> > The cleanest solution (as I see not the first time people I trying quirks like
> > this to be part of the subsystems / drivers) is to make DT core (OF) to have
> > conditionals or boot-time modifications allowed.  
> 
> We don't talk about device tree modifications on boot time!
> 
> Currently the SPI NAND chips are not fully configured in the device
> tree data. Today a QSPI NAND is represented by an abstract 'compatible' entry
> of 'spi-nand' in device tree. Which can be seen as something like a 'spi-nand'
> bus with autodetection of the connected chips.
> 
> Therefore there is no way to reference a QSPI NAND chip directly, it's
> auto-detected by the framework. There is also currently no possibility to
> set the tCLQV parameter for a single SPI CS line.
> 
> Some parameters for the SPI NAND chips are already provided only by
> the fitting chip driver (e.g. flash layout, banks, variants of the command
> set of the device, ...). With this patchset it's now possible to provide also
> the tCLQV data for this chip.
> 
> IMHO a autodetect system does not make much sense if you have to provide
> parts of the chip configuration also in device tree. The framework should
> detect the chip and fetch the operation parameters either from the chip
> itself or from a chip driver which provides the required configuration settings.

Yes, if the information is discoverable, we should propagate it to the
spi layer so that the relevant action is taken, from the most desirable
to the less desirable:
- adapting the sampling point
- lowering the bus frequency
- refusing the probe if none of these solutions are possible

Can you update your patchset with this in mind?

Thanks,
Miquèl