[v2,0/3] Nuvoton WPCM450 FIU SPI flash controller

This patchset adds DT bindings and a driver for the Flash Interface Unit
(FIU), the SPI flash controller in the Nuvoton WPCM450 BMC SoC. It
supports four chip selects, and direct (memory-mapped) access to 16 MiB
per chip. Larger flash chips can be accessed by software-defined SPI
transfers.

The existing NPCM7xx FIU driver is sufficitently incompatible with the
WPCM450 FIU that I decided to write a new driver.

Changelog for v2:

- Dropped the patches which have been applied in the meantime, leaving
  only three out of eight
- Changed the binding to require both items in the reg property, because
  there is no need to keep the second item optional, suggested by
  Krzysztof Kozlowski
- Various other cleanups suggested by Krzysztof Kozlowski and the kernel
  test robot


Jonathan


Jonathan Neuschäfer (3):
  dt-bindings: spi: Add Nuvoton WPCM450 Flash Interface Unit (FIU)
  spi: wpcm-fiu: Add driver for Nuvoton WPCM450 Flash Interface Unit
    (FIU)
  spi: wpcm-fiu: Add direct map support

 .../bindings/spi/nuvoton,wpcm450-fiu.yaml     |  66 +++
 drivers/spi/Kconfig                           |  11 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-wpcm-fiu.c                    | 508 ++++++++++++++++++
 4 files changed, 586 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
 create mode 100644 drivers/spi/spi-wpcm-fiu.c

--
2.35.1

On Thu, Nov 24, 2022 at 08:13:58PM +0100, Jonathan Neuschäfer wrote:
> The Flash Interface Unit (FIU) is the SPI flash controller in the
> Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> accessed by software-defined SPI transfers.

You didn't send me the cover letter for this series.  As documented in
submitting-patches.rst please send things to the maintainers for the
code you would like to change.  The normal kernel workflow is that
people apply patches from their inboxes, if they aren't copied they are
likely to not see the patch at all and it is much more difficult to
apply patches.

Hello,

On Fri, Nov 25, 2022 at 01:04:54PM +0000, Mark Brown wrote:
> On Thu, Nov 24, 2022 at 08:13:59PM +0100, Jonathan Neuschäfer wrote:
> 
> > The Flash Interface Unit (FIU) is the SPI flash controller in the
> > Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> > (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> > accessed by software-defined SPI transfers.
> 
> Those software defined SPI transfers seem to be most of the way to
> supporting normal SPI controller operations, they just need wiring up.
> That would both support people hooking other SPI chips up to the board
> and might help support future flash stuff without needing custom code in
> the driver like you've got now.

I'm not so sure. The hardware mechanism allowing "software defined" SPI
transfers is strongly oriented towards SPI flash, and it already felt
like a stretch to implement all the features that Linux expects of a
SPI MEM controller.

As to connecting non-memory chips: There is also a second, completely
different SPI controller in this SoC, which is used on some boards (in
factory configuration) to drive a little status LCD. I think it would be
easiest to use that one for custom hardware extensions.


> 
> > +static int wpcm_fiu_do_uma(struct wpcm_fiu_spi *fiu, unsigned int cs,
> > +			   bool use_addr, bool write, int data_bytes)
> > +{
> 
> This appears to only support half duplex access but the driver doesn't
> flag itself as SPI_CONTROLLER_HALF_DUPLEX.  

Ok, I'll add it.

> 
> > +	cts |= FIU_UMA_CTS_D_SIZE(data_bytes);
> 
> I'm guessing there's a limit on data_bytes that should be enforced.  The
> driver should probably also flag a max transfer size, though that might
> cause issues if the limit is different between spi-mem and regular
> transfers.

For the existing spi-mem case, the transfer size is limited through
wpcm_fiu_adjust_op_size. I *think* this is enough, but please correct me
if I'm wrong.


> > +/*
> > + * RDID (Read Identification) needs special handling because Linux expects to
> > + * be able to read 6 ID bytes and FIU can only read up to 4 at once.
> > + *
> > + * We're lucky in this case, because executing the RDID instruction twice will
> > + * result in the same result.
> > + *
> > + * What we do is as follows (C: write command/opcode byte, D: read data byte,
> > + * A: write address byte):
> > + *
> > + *  1. C D D D
> > + *  2. C A A A D D D
> > + */
> 
> If the driver were implementing regular SPI operations and advertising
> a maximum transfer length this should just work without having to jump
> through hoops.  The core can split transfers up into sections that fit
> within the controller limits transparently.

As far as I'm aware, the controller is not capable of performing a pure
read transfer, because the command byte (a byte that is written, in
half-duplex) is always included at the start. I think this limitation
would break your idea.

IOW, the hoops aren't nice, but I think they're necessary.


Thanks for your review,
Jonathan

On Thu, 24 Nov 2022 20:13:58 +0100, Jonathan Neuschäfer wrote:
> The Flash Interface Unit (FIU) is the SPI flash controller in the
> Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> accessed by software-defined SPI transfers.
> 
> The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v2:
> - A few cleanups suggested by Krzysztof Kozlowski
> - Simplify binding by making second reg item mandatory
> 
> v1:
> - https://lore.kernel.org/lkml/20221105185911.1547847-4-j.neuschaefer@gmx.net/
> ---
>  .../bindings/spi/nuvoton,wpcm450-fiu.yaml     | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dts:18:18: fatal error: dt-bindings/clock/nuvoton,wpcm450-clk.h: No such file or directory
   18 |         #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221124191400.287918-2-j.neuschaefer@gmx.net

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command.

On Mon, Nov 28, 2022 at 11:05:31AM +0000, Conor Dooley wrote:
> On Sat, Nov 26, 2022 at 04:25:36PM -0600, Rob Herring wrote:
[...]
> > dtschema/dtc warnings/errors:
> > Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dts:18:18: fatal error: dt-bindings/clock/nuvoton,wpcm450-clk.h: No such file or directory
> >    18 |         #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
> >       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > compilation terminated.
> > make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dtb] Error 1
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [Makefile:1492: dt_binding_check] Error 2
> 
> FWIW this seems to now be in linux-next as dd71cd4dd6c9 ("spi: Add Nuvoton
> WPCM450 Flash Interface Unit (FIU) bindings") & is breaking
> dt_binding_check.

Ah, sorry about that. It should resolve itself once nuvoton,wpcm450-clk
binding gets merged, but I don't see a definite timeframe for that, yet.

Alternatively, I can send a patch to simplify the example in the FIU
binding.

Jonathan

On Mon, Nov 28, 2022 at 02:09:37PM +0000, Conor Dooley wrote:

> Without being a Responsible Adult^TM for either SPI or DT, my preference
> would be for simplifying the binding so that if your clk stuff doesn't
> land for 6.2 the binding checks still work.

Yes, please simplify the example.