Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver

[Mark Brown]

On Sun, May 10, 2020 at 03:20:39AM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:

> > Your CC list on this series is *very* wide - are you sure that this is
> > relevant to everyone you're sending things to?  Kernel developers often
> > get a lot of mail meaning that extra mail can become a bit overwhelming
> > and cause things to get lost in the noise.

> MIPS folks are here since Baikal-T1 is a MIPS-based chip and this patchset
> is a part of the campaign of integrating the SoC support into the kernel.
> With Lee and Miquel we discussed the dirmap support in the framework of another
> patchset. Rob and devicetree-list are CC'ed due to having the bindings tree
> updated. Then a series of folks who recently submitted the biggest updates into
> the spi subsystems so might have valuable comments for this driver as well. So
> yes, I am sure.

I think you've got too many people who've been contributing to the
subsystem here at least - it looks like you picked up some people who
recently wrote drivers but haven't been doing a lot of review for
example for example.

> Anyway ok. I'll fix it. Is it ok to have the C-style comments in the header
> file?
> * It isn't included by any assembly so from this point of view C++ style
> * shall also work there.

The SPDX stuff requires C style comments in headers so yes.

> Secondly the message of that commit states "Devices with chip selects driven
> via GPIO are not compatible with the spi-mem operations." I find this statement
> questionable, because for instance this device supports memory operations with
> GPIO-driven CS. Though in current implementation the driver fallback to using normal
> push-pull IO mode if GPIO CS is utilized as safer one. But even in this case
> it's better than splitting the memory operations up into the transfers, which is
> developed in the spi_mem_exec_op() method.

> So in this matter my question is: how to modify the SPI-mem interface so the
> SPI-memory operations would also work with GPIO driven CS? Some additional flag
> might work...

Yes, some flags should work here - the issue was that at least some
controllers may end up trying to do multiple SPI operations for one
spi-mem thing which will break if the chip select doesn't get changed to
correspond with what's going on.

> Thirdly what about dirmap operations? If we got a GPIO driven CS then due to
> lacking any CS manipulation in spi_mem_dirmap_read() method we wouldn't have
> been able to make the direct mapping working without manual setting the GPIO.
> So the same question here. How to work this around and justify your requirement?
> Until the question is answered and we come up with reasonable solution in order
> to have the SPI-mem/dirmap interface working together with GPIO CS support I have
> to leave the manual GPIO manipulation.

If the issue with ensuring that chip select is managed appropriately for
transfers can be resolved then push that stuff up to the framework.  If
not then it's not clear that the open coded version can work well
with GPIO chip selects either.

> > > +static void bs_update_cfg(struct bt1_spi *bs, struct spi_transfer *xfer)
> > > +{

> > This has exactly one caller, perhaps it could just be inlined there?  I
> > think this applies to some of the other internal functions.

> I don't want to inline methods just because they are used in a single place. This
> might worsen readability and in this case it will. Currently the transfer_one()
> method is consistent with self-explanatory methods: update config, push-pull
> bytes/words, check status. This also applies to the rest of the code. I can
> consider some improvements/optimizations in this matter though, but embedding
> the functions isn't one of them. Moreover the compiler does the static
> methods inlining automatically as soon as it sees they are called just once.

One of the things that creating internal APIs like this does is that it
adds another layer of structure to the driver, making it a bit harder to
follow that it's following the usual structures of a Linux driver
(especially noticable here given the open coding).  It creates the
impression of the platform portability layers that people sometimes try
to build.

> > > +static int bs_check_status(struct bt1_spi *bs)
> > > +{

> > It's not obvious from the name that this function will busy wait rather
> > than just reading some status registers.

> What do you suggest then? Renaming? Splitting up? If renaming, then what name do
> you prefer? Something like bs_check_completion()?

wait_for_completion() or something else that mentions that it will wait
for completion rather than just looking to see if the operation has
completed.

> > > +static int bs_exec_mem_op(struct spi_mem *mem,
> > > +			  const struct spi_mem_op *op)
> > > +{

> > It's not clear to me that this hardware actually supports spi_mem in
> > hardware?

> SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> modes of the controller.

Sure, but those seem like normal SPI-level things rather than cases
where the hardware understands that it has a flash attached and is doing
flash specific things.  A very common case for this stuff is that
controllers have acceleration blocks for read and fall back on normal
SPI for writes and erases, that sounds like what's going on here.

> > > +	if (!bs->cfg.cs_gpiod) {
> > > +		bs_push_bytes(bs, cmd, len);
> > > +
> > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > +			ret = bs_pull_bytes(bs, op->data.buf.in,
> > > +					    op->data.nbytes);
> > > +	} else {
> > > +		bs_pp_bytes(bs, cmd, NULL, len);
> > > +
> > > +		if (op->data.dir == SPI_MEM_DATA_IN)
> > > +			bs_pp_bytes(bs, NULL, op->data.buf.in,
> > > +				    op->data.nbytes);
> > > +	}
> > 

> > The actual transfers here are done using the same PIO functions as
> > everything else as far as I can see which makes me wonder what this
> > spi_mem implementation is adding over the standard SPI flash support.

> No, if no GPIO CS is supplied the memory read operation is performed by means of
> the EEPROM-read mode of the DW SSI controller (Write command is written to the
> Tx FIFO and read data length - to the CTRLR1 register). The memory write

So EEPROM read mode is a combined write/read - is that somehow less
performant or something than separate opeations so there's a reason not
to use it except in this special circumstance?

> operation utilizes the Tx-only mode (just push data to the Tx FIFO, controller
> discards all incoming data). See bs_set_cfg() for details.

That's not super illuminating on this issue?

> If GPIO CS is utilized then we fallback to the normal push-pull methods because
> first it's safer this way, second it's still better than handling multiple SPI
> transfers.

> BTW The EEPROM/Tx-only modes are required to be utilized for the controller on
> our SoC in order to support the SPI bus operations with highest frequencies.
> Due to relatively slow APB bus performance we have to minimize MMIO operation
> per a single byte read/write. This is another reason why we had to create a
> dedicated driver for this controller.

If the controller has support for TX only mode that seems like a good
thing to have in the generic driver - for TX only transfers that's less
bus I/O.  The EEPROM mode potentially also, I'm less clear on what
exactly it is but it sounds potentially useful for common cases with
register operations.  Perhaps other systems might have similarly slow
buses and get noticable performance benefits from using these modes.

> > > +static void bs_copy_from_map(void *to, void __iomem *from, size_t len)
> > > +{
> > > +	size_t shift, chunk;
> > > +	u32 data;

> > This feels like something that is likely to be suitable for a generic
> > implementation?  Indeed I'd kind of expect that the architecture
> > memcpy_to/fromio() would try to copy aligned blocks since it's usually
> > going to perform better (which I assume is why this function is doing
> > it).

> MIPS memcpy_to/fromio don't do this. They expect the MMIO space can be read in
> a same way as normal memory space. It's our controller "feature", that MMIO can
> be only accessible by lw/sw instructions. So I had no choice but to implement the
> aligned read/write operations by myself here. AFAICS this won't be suitable for
> the generic implementation due to the alignment restriction and the controller
> setups, which are supposed to be performed before accessing the dirmap MMIO.

You should document this in the driver so nobody comes along and does
the refactoring as a cleanup, it looks like duplicated or redundant code.

> > This seems a bit much - we can validate that the message doesn't contain
> > any delays (which is going to be the overwhelming majority of devices)
> > or multiple transfers and reject just those transfers we can't support.
> > Multiple transfers are an issue with this hardware but you could
> > implement support in the core for coalescing them into single transfers,
> > this isn't the only hardware with automatic chip select handling that
> > can't cope with scatter/gather so it would be a useful thing to have.

> As I said in the commit message to the binding this controller is nearly always
> connected to an external SPI flash (I'd say always, because Baikal-T1 is fully
> functional only when initially booted from an SPI flash). At least I don't know
> any hardware using this interface differently (I've worked with all devices with
> Baikal-T1 SoC on board). In current driver implementation the SPI-mem operations
> covers the native CS case while the prepare_message()/transfer_one()/unprepare_message()
> are used when GPIO-driven CS is available (due to the commit 05766050d5bd I had
> to implement it). So I didn't want to over-complicate the driver just to support
> some virtual hardware before it's really necessary.

Aside from the issue of tempting fate in board designs this circles back
to the open coding of the bits of the spi-mem stuff that aren't really
offloaded by the hardware - the driver would look a lot more standard
(and be closer to spi-dw I guess) if it only implemented the bits that
are offloaded and this interface was available.  Having the normal
SPI message interface would not only support non-flash hardware, it'd
also enable the use of more of the generic SPI flash code like other
similar controllers.

> > > +static int bs_transfer_one(struct spi_controller *ctrl, struct spi_device *spi,
> > > +			   struct spi_transfer *xfer)
> > > +{
> > > +	struct bt1_spi *bs = spi_controller_get_devdata(ctrl);
> > > +
> > > +	bs_update_cfg(bs, xfer);
> > > +
> > > +	if (bs->cfg.dfs <= 8)
> > > +		bs_pp_bytes(bs, xfer->tx_buf, xfer->rx_buf, xfer->len);
> > > +	else
> > > +		bs_pp_words(bs, xfer->tx_buf, xfer->rx_buf, xfer->len / 2);
> > 
> > This will have issues with transfers with an odd number of bytes won't
> > it?

> No, it won't. Transfers with bits per word greater than 8 should have even
> number of bytes seeing the len field is supposed to have the buffers length in
> bytes:

So dfs is bits per word?  This is one of those things where the internal
APIs make things harder to follow.

> > Perhaps consider regmap_mmio?

> For multiple reasons no. First of all I need a direct access to the DR register
> and I need to do this as fast as possible due to relatively slow APB
> bus. The same concern is regarding the controller configs setting. Secondly
> everywhere in the driver I use the normal read/write methods and there is no
> need in even a basic regmap-update function. So an additional abstraction like
> regmap would be unnecessary complication. Thirdly the access to the registers

That comment was more due to writing debug infrastructure than stuff
like update_bits - the main reason regmap-mmio exists is for the debug
and cache features.  If the performance is too marginal to allow any
overhead then fine.  There's nothing stopping you combining regmap and
non-regmap on the same device if you exclude the more sensitive
registers from regmap with the access operations but it's not exactly
nice.

> > > +	if (!ret) {
> > > +		mutex_lock(&bs->ctrl->io_mutex);

> > Holding a mutex over the entire time that a file is open is not good,
> > it's also not clear to me what this mutex is supposed to be protecting
> > given that it's only referenced in these debugfs functions.

> Hm, it's io_mutex protecting the SPI IO operation to be thread-safe. See:
> spi_mem_access_start()/spi_mem_access_end();
> __spi_pump_messages();

> By locking the mutex here I make sure the DebugFS registers dump operation won't
> interfere the IO operations like dirmap, which make the registers unavailable
> for normal MMIO-based access.

Oh, it's the SPI level mutex...  that is a bit icky, and it does let
userspace code block the bus fairly easily which could be problematic
but I guess it's debugfs so meh.

> > > +		writel(BC_CSR_SPI_RDA, bs->regs + BC_CSR);

> > Whatever this write is doing we never seem to undo it?

> Here I switch on the DW APB SSI registers to be available for normal
> readl/writel accesses. It's not a problem to leave the mode on, since
> bs_set_cfg() will do a proper config anyway.

A comment would help here, the fact that it's not undone looks like a
bug.

Attachment: signature.asc
Description: PGP signature