Re: [PATCH v2 2/3] spi: wpcm-fiu: Add driver for Nuvoton WPCM450 Flash Interface Unit (FIU)

From: Mark Brown <broonie@kernel.org>
To: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
Cc: linux-spi@vger.kernel.org, openbmc@lists.ozlabs.org,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] spi: wpcm-fiu: Add driver for Nuvoton WPCM450 Flash Interface Unit (FIU)
Date: Fri, 25 Nov 2022 13:04:54 +0000	[thread overview]
Message-ID: <Y4C9druicCm0m3wi@sirena.org.uk> (raw)
In-Reply-To: <20221124191400.287918-3-j.neuschaefer@gmx.net>

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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.

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

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

> +/*
> + * 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]