Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Tue, May 19, 2020 at 12:17:27AM +0300, Serge Semin wrote: > Here is what we need to do to perform the EEPROM-read operation: > 1) Enable EEPROM-read mode. > 2) Initialize a corresponding registers with a number of SPI transfer words >(with bits-per-word taken into account) to read. > 3) Push opcode + address + dummy bytes into the Tx FIFO. When it's done and >the Tx FIFO is empty, the controller will proceed with read operations by >pushing zeros (or ones, don't remember what level it's by default) to MOSI >and pulling data from MISO into the RX FIFO. > 4) Keep up with getting data from the Rx FIFO so one wouldn't get overflown. > Regarding programming write each time. Well, it's up to the driver > implementation. > If opcode, address, dummy bytes and number of words to read are the same as > before, > then re-programming isn't required. Ah, nice. This should be useful for far more than just flash - most register reads will also be able to take advantage of this, they follow a similar write then read pattern. signature.asc Description: PGP signature
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Mon, May 18, 2020 at 04:19:47PM +0100, Mark Brown wrote: > On Mon, May 18, 2020 at 03:05:42AM +0300, Serge Semin wrote: > > On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote: > > > > 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. > > > Ok. New SPI flag it is then. It will be something like this: > > + #define SPI_CONTROLLER_FLASH_SS BIT(6) > > I'd rather use CS than SS (it's more common in the code). Ok. > > > So, what do you think? > > Should be fine, controllers that have an issue implementing just > shouldn't set the flag. Yes, exactly what I intended. > > > > > > 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. > > > No, hardware can't detect whether the flash is attached. This must be > > defined by > > the platform, like based on the DT sub-nodes. > > This isn't about autodetection, it's about the abstraction level the > hardware is operating on - some hardware is able to generate flash > operations by itself (possibly with some help programming the opcodes > that are needed by a given flash), some hardware just works at the > bytestream level. > > > > 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. > > > > Well, yeah, they do provide some acceleration. EEPROM-read provides > > automatic > > write-cmd-dummy-data-then-read operations. But in this case the only thing > > we > > have to push into the SPI Tx FIFO is command and dummy bytes. The read > > operation > > So it's a write then read but you have to program the write each time? Here is what we need to do to perform the EEPROM-read operation: 1) Enable EEPROM-read mode. 2) Initialize a corresponding registers with a number of SPI transfer words (with bits-per-word taken into account) to read. 3) Push opcode + address + dummy bytes into the Tx FIFO. When it's done and the Tx FIFO is empty, the controller will proceed with read operations by pushing zeros (or ones, don't remember what level it's by default) to MOSI and pulling data from MISO into the RX FIFO. 4) Keep up with getting data from the Rx FIFO so one wouldn't get overflown. Regarding programming write each time. Well, it's up to the driver implementation. If opcode, address, dummy bytes and number of words to read are the same as before, then re-programming isn't required. -Sergey
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Mon, May 18, 2020 at 03:05:42AM +0300, Serge Semin wrote: > On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote: > > 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. > Ok. New SPI flag it is then. It will be something like this: > + #define SPI_CONTROLLER_FLASH_SS BIT(6) I'd rather use CS than SS (it's more common in the code). > So, what do you think? Should be fine, controllers that have an issue implementing just shouldn't set the flag. > > > > 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. > No, hardware can't detect whether the flash is attached. This must be defined > by > the platform, like based on the DT sub-nodes. This isn't about autodetection, it's about the abstraction level the hardware is operating on - some hardware is able to generate flash operations by itself (possibly with some help programming the opcodes that are needed by a given flash), some hardware just works at the bytestream level. > > 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. > > Well, yeah, they do provide some acceleration. EEPROM-read provides automatic > write-cmd-dummy-data-then-read operations. But in this case the only thing we > have to push into the SPI Tx FIFO is command and dummy bytes. The read > operation So it's a write then read but you have to program the write each time? signature.asc Description: PGP signature
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
Mark,
I give up.) I'll try to integrate what I've done here in the generic DW APB SSI
driver framework. Then add some hooks so to support our specific DW APB SSI
controller. The I create a glue-layer for it. My answers to you comments are
below.
On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote:
> 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.
Ok. New SPI flag it is then. It will be something like this:
+ #define SPI_CONTROLLER_FLASH_SS BIT(6)
Then we have to convert spi_set_cs() method to be non-static in the spi.c
to be used in the spi-mem.c to properly select slaves
Then the spi_mem_access_start() and spi_mem_access_stop() of spi-mem.c should be
updated in the following way:
--- spi_mem_access_start()
+++ spi_mem_access_start()
mutex_lock(>bus_lock_mutex);
mutex_lock(>io_mutex);
+
+ if (ctlr->flags & SPI_CONTROLLER_FLASH_SS)
+ spi_set_cs(mem->spi, true);
return 0;
}
--- spi_mem_access_end()
+++ spi_mem_access_end()
static void spi_mem_access_end(struct spi_mem *mem)
{
struct spi_controller *ctlr = mem->spi->controller;
+
+ if (ctlr->flags & SPI_CONTROLLER_FLASH_SS)
+ spi_set_cs(mem->spi, false);
mutex_unlock(>io_mutex);
mutex_unlock(>bus_lock_mutex);
---
Method spi_mem_exec_op() shall also make sure that even if GPIO-driven CS is
utilized the normal mem-op is executed. Something like this should do this:
--- spi_mem_exec_op()
+++ spi_mem_exec_op()
if (!spi_mem_internal_supports_op(mem, op))
return -ENOTSUPP;
- if (ctlr->mem_ops && !mem->spi->cs_gpiod) {
+ if (ctlr->mem_ops &&
+ (!mem->spi->cs_gpiod || (ctlr->flags & SPI_CONTROLLER_FLASH_SS))) {
ret = spi_mem_access_start(mem);
if (ret)
return ret;
---
So, what do you think?
>
> > 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
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
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
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On 10/05/20 12:20 pm, Serge Semin wrote:
> On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:
>>> + writel(BIT(req->cs), bs->regs + BC_SPI_SER);
>>> + if (req->cs_gpiod) {
>>> + gpiod_set_value_cansleep(req->cs_gpiod,
>>> +!!(bs->cfg.mode & SPI_CS_HIGH));
>> If you have a GPIO chip select you should just let the core manage it
>> through cs_gpiod rather than open coding.
> Of course I know this, and normally I would have omitted the GPIO manual
> assertion (hopefully soon my hands get to merging the AX99100 driver I've
> developed some time ago). The thing is that this Baikal-T1 System SSI device
> driver has been initially written before commit 05766050d5bd ("spi: spi-mem:
> fallback to using transfers when CS gpios are used"). So asserting GPIO CS had
> been required to initiate the SPI memory communications seeing the generic
> spi_mem_exec_op() doesn't do this. Manual GPIO manipulation is indeed
> redundant
> for the current SPI-mem op execution procedure.
>
> 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.
On this specific bit. My use-case for 05766050d5bd was a SPI controller
that supported direct mem accesses but a hardware design that required a
GPIO CS. So yes I probably should have qualified it as _some_ devices.
> 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...
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 12:37:51PM +0100, Mark Brown wrote:
> On Fri, May 08, 2020 at 12:36:21PM +0300, Serge Semin 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.
>
> > This SPI-controller is a part of the Baikal-T1 System Controller and
> > is based on the DW APB SSI IP-core, but with very limited resources:
> > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> > FIFO available. In order to provide a transparent initial boot code
> > execution this controller is also utilized by an vendor-specific block,
> > which provides an CS0 SPI flash direct mapping interface. Since both
> > direct mapping and SPI controller normal utilization are mutual exclusive
> > only a one of these interfaces can be used to access an external SPI
> > slave device. Taking into account the peculiarities of the controller
> > registers and physically mapped SPI flash access, very limited resources
> > and seeing the normal usecase of the controller is to access an external
> > SPI-nor flash, we decided to create a dedicated SPI driver for it.
>
> My overall impression reading this is that there could be a lot more
> reliance on both generic functionality and as Andy suggested the spi-dw
> driver - the headers seem easy for example. As far as I can tell the
> main things this is adding compared to spi-dw are the dirmap code and
> the IRQ disabling around the PIO on the FIFO both of which seem like
> relatively small additions which it should be possible to accomodate
> within spi-dw. For example the driver could have multiple transfer
> functions and pick a different one with the interrupt disabling when
> running on this hardware.
dirmap and IRQs disabling isn't all they have different. Please see my answer to
you in the cover-letter thread and the comments below in this email.
>
> > --- /dev/null
> > +++ b/drivers/spi/spi-bt1-sys.c
> > @@ -0,0 +1,873 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
>
> Please make the entire comment a C++ one so things look a bit cleaner
> and more intentional.
This has been unexpected. It's first time I see such a requirement.
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.
>
> > + writel(BIT(req->cs), bs->regs + BC_SPI_SER);
> > + if (req->cs_gpiod) {
> > + gpiod_set_value_cansleep(req->cs_gpiod,
> > +!!(bs->cfg.mode & SPI_CS_HIGH));
>
> If you have a GPIO chip select you should just let the core manage it
> through cs_gpiod rather than open coding.
Of course I know this, and normally I would have omitted the GPIO manual
assertion (hopefully soon my hands get to merging the AX99100 driver I've
developed some time ago). The thing is that this Baikal-T1 System SSI device
driver has been initially written before commit 05766050d5bd ("spi: spi-mem:
fallback to using transfers when CS gpios are used"). So asserting GPIO CS had
been required to initiate the SPI memory communications seeing the generic
spi_mem_exec_op() doesn't do this. Manual GPIO manipulation is indeed redundant
for the current SPI-mem op execution procedure.
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...
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
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 06:42:10PM +0300, Serge Semin wrote: > On Fri, May 08, 2020 at 11:22:10AM +0100, Mark Brown wrote: > > Can you be more specific about the issues? From what you wrote it > > sounded like the main thing was chip select handling. > I thought it would be obvious from the patch itself. I've thoroughly > described all > the issues there. Here in cover-letter it's a summary of the main ones. Bear in mind that the patch is a stand alone thing, there's not a copy of the existing driver sitting with it and the stylistic changes make comparisons even less obvious. > 1) Registers mapping. The DW SSI registers are shifted by 0x100 with > respect to the MMIO region start. The lowest 0x100 registers are > responsible for the Baikal-T1 Boot Controller settings. There aren't much > of them there though. Our code is interested only in a flag, which switches > an accessibility of the DW APB SSI registers and direct SPI flash mapping. > And this switchability is a reason of another peculiarity (see the next > item for details). That seems fairly easy to address, for example with a subdevice or indirecting through ops for I/O that could add on an offset (what a subdevice would end up accomplishing really). > 2) SPI flash direct mapping. SPI flash direct mapping and DW APB SSI registers > are mutual exclusive, so only one of them can be enabled at a time. In > order to use the dirmap we have to switch the RDA bit off in the Boot > Controller setup register. If DW APB SSI registers need to be accessed the > RDA bit should be set. For this reason we have to make sure that dirmap > operations, SPI operations and SPI-mem-ops operations are exclusive, since > some of them need to interact with the DW APB SSI registers, while another This exclusivity requirement is pretty standard for these flash memory map controllers, the framework should ensure you don't get any overlap between memory mapped and regular interactions. > the directly mapped SPI flash MMIO (currently ctlr->io_mutex is responsible > for this). It only seemed to be referenced in the debugfs code? > 3) A specific access to MMIO (concerning directly mapped SPI flash MMIO). > The SoC interconnect is designed in a way so we can't use any instruction to > read/write from/to the MMIO space. It has to be done by lw/sw with > dword-aligned address passed. Though in this driver we only use a read > operation from the directly mapped SPI flash memory. That's a custom IP block for your systems so that'd be a separate operation no matter what. > 4) No direct handling of the CS. Though this is an issue of all DW SSI > controllers, here with very small FIFO and no DMA/IRQ supported it mandates to > workaround any preemption/interruption during a non-GPIO-CS-based transfer. > For the same reason the driver doesn't support normal spi-messages based As I said when reviewing the driver I think all you need here is support in the core for linearizing messages into a single transfer and then what you're left with is what should be a fairly small quirk for running with interrupts disabled if there's no DMA or interrupts. I'd expect both bits of that to benefit some other users too, there's definitely other controllers that have problems with automated chip select handling but happen to get away with it a lot of the time. > interface if no GPIO-CS supplied. In addition since FIFO is too small and most > of our platforms don't have GPIO-CS support we had to create the SPI-mem-ops > instead of generic SPI-callback. As I also said in reviewing the driver that's just not a good idea anyway, there is no way a driver should be open coding things like that and there are much better ways to support this. This is only there because the driver isn't able to cope with normal messages, it's better to solve that problem and use the generic flash code than to emulate the generic flash code here. In both these cases it looks like the majority of the reason the driver is different is that you're trying to solve problems in the driver without changing the core, some things are a lot easier to handle further up the stack. > 5) MMIO access race condition. As I described in the in-code comment it's a > very tricky race happening during concurrent access from different cores to > the > APB bus. Due to this if SPI interface is working high frequency like This looks like it should be a fairly small quirk? > I am pretty sure I have forgotten something. Anyway it has been much easier > to create a new driver instead of integrating all of these into a generic > one. Integrating something like this in the current DW APB SSI driver would > mean to have it completely overwritten (refactored if you want) which would > bring us to a new driver anyway. I don't think it would be good to > complicate the generic driver with so many peculiar things scattered around > the code with various hooks or ifdef, especially seeing the current code has > already become a bit messy. It
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 01:26:52PM +0300, Andy Shevchenko wrote: > On Fri, May 8, 2020 at 1:15 PM Serge Semin > wrote: > > > > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote: > > > On Fri, May 8, 2020 at 12:37 PM Serge Semin > > > wrote: > > > > > > > > This SPI-controller is a part of the Baikal-T1 System Controller and > > > > is based on the DW APB SSI IP-core, but with very limited resources: > > > > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx > > > > FIFO available. In order to provide a transparent initial boot code > > > > execution this controller is also utilized by an vendor-specific block, > > > > which provides an CS0 SPI flash direct mapping interface. Since both > > > > direct mapping and SPI controller normal utilization are mutual > > > > exclusive > > > > only a one of these interfaces can be used to access an external SPI > > > > slave device. Taking into account the peculiarities of the controller > > > > registers and physically mapped SPI flash access, very limited resources > > > > and seeing the normal usecase of the controller is to access an external > > > > SPI-nor flash, we decided to create a dedicated SPI driver for it. > > > > > > It seems a lot of code. > > > Why can't you use spi-dw-mmio.c et al.? > > > > I said above why. Even though the registers set is similar It's too specific > > to be integrated into the generic DW SSI driver. > > At least you may do at the beginning is to reuse header spi-dw.h and > put your stuff under > spi-dw-baikal.c or so. Then, look at the spi-dw.c and check what can > be reused (I think a lot). Nah, It's not a lot, barely a few things. What is useful is the registers map (though it has to be shifted and most of the registers are unused), IO operations (two small read-write methods with questionable design) and possibly a few lines of config setting procedure. You think I didn't had in mind to integrate this controller support into the generic driver or resuse something from there? If it was worth a try I would have done it. -Sergey > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 11:22:10AM +0100, Mark Brown wrote: > On Fri, May 08, 2020 at 01:15:41PM +0300, Serge Semin wrote: > > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote: > > > > > slave device. Taking into account the peculiarities of the controller > > > > registers and physically mapped SPI flash access, very limited resources > > > > and seeing the normal usecase of the controller is to access an external > > > > SPI-nor flash, we decided to create a dedicated SPI driver for it. > > > > It seems a lot of code. > > > Why can't you use spi-dw-mmio.c et al.? > > > I said above why. Even though the registers set is similar It's too specific > > to be integrated into the generic DW SSI driver. > > Can you be more specific about the issues? From what you wrote it > sounded like the main thing was chip select handling. I thought it would be obvious from the patch itself. I've thoroughly described all the issues there. Here in cover-letter it's a summary of the main ones. Anyway here are all collected at once: 1) Registers mapping. The DW SSI registers are shifted by 0x100 with respect to the MMIO region start. The lowest 0x100 registers are responsible for the Baikal-T1 Boot Controller settings. There aren't much of them there though. Our code is interested only in a flag, which switches an accessibility of the DW APB SSI registers and direct SPI flash mapping. And this switchability is a reason of another peculiarity (see the next item for details). 2) SPI flash direct mapping. SPI flash direct mapping and DW APB SSI registers are mutual exclusive, so only one of them can be enabled at a time. In order to use the dirmap we have to switch the RDA bit off in the Boot Controller setup register. If DW APB SSI registers need to be accessed the RDA bit should be set. For this reason we have to make sure that dirmap operations, SPI operations and SPI-mem-ops operations are exclusive, since some of them need to interact with the DW APB SSI registers, while another the directly mapped SPI flash MMIO (currently ctlr->io_mutex is responsible for this). 3) A specific access to MMIO (concerning directly mapped SPI flash MMIO). The SoC interconnect is designed in a way so we can't use any instruction to read/write from/to the MMIO space. It has to be done by lw/sw with dword-aligned address passed. Though in this driver we only use a read operation from the directly mapped SPI flash memory. 4) No direct handling of the CS. Though this is an issue of all DW SSI controllers, here with very small FIFO and no DMA/IRQ supported it mandates to workaround any preemption/interruption during a non-GPIO-CS-based transfer. For the same reason the driver doesn't support normal spi-messages based interface if no GPIO-CS supplied. In addition since FIFO is too small and most of our platforms don't have GPIO-CS support we had to create the SPI-mem-ops instead of generic SPI-callback. 5) MMIO access race condition. As I described in the in-code comment it's a very tricky race happening during concurrent access from different cores to the APB bus. Due to this if SPI interface is working high frequency like 12.5 - 25 MHz and there is some another code working with APB bus on another core, the SPI data pushing function might not keep up with filling small FIFO (8 bytes) on time, since the APB bus has got just 50 MHz frequency. Due to this I had artificially limit the SPI bus frequency if there is more than one CPU in the system. 6) A single CS. It's normally connected to an external SPI flash so the driver is equipped with mem-ops and dirmap out of the box. BTW normal SPI-operations are in fact unneeded on all the platforms currently equipped with Baikal-T1 because all of them have got SPI flash attached to this interface, and most likely it will be like in new platforms too. 7) No interrupts. Yes, this is another peculiarity. Since this DW APB core is a part of the boot controller it just don't need IRQs. Boot controller is responsible for the code loading from SPI flash. It has got a dedicated RTL which provide a transparent access to the flash just by reading from a corresponding MMIO range (see the SPI flash direct mapping described above). 8) No DMA. Yeah, and DMA isn't supported for the same reason. I am pretty sure I have forgotten something. Anyway it has been much easier to create a new driver instead of integrating all of these into a generic one. Integrating something like this in the current DW APB SSI driver would mean to have it completely overwritten (refactored if you want) which would bring us to a new driver anyway. I don't think it would be good to complicate the generic driver with so many peculiar things scattered around the code with various hooks or ifdef, especially seeing the current code has already become a bit messy. > > > > > The driver provides callbacks for native messages-based SPI interface, > > > > SPI-memory and direct mapping read operations. Due to not having any >
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 12:36:21PM +0300, Serge Semin 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.
> This SPI-controller is a part of the Baikal-T1 System Controller and
> is based on the DW APB SSI IP-core, but with very limited resources:
> no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx
> FIFO available. In order to provide a transparent initial boot code
> execution this controller is also utilized by an vendor-specific block,
> which provides an CS0 SPI flash direct mapping interface. Since both
> direct mapping and SPI controller normal utilization are mutual exclusive
> only a one of these interfaces can be used to access an external SPI
> slave device. Taking into account the peculiarities of the controller
> registers and physically mapped SPI flash access, very limited resources
> and seeing the normal usecase of the controller is to access an external
> SPI-nor flash, we decided to create a dedicated SPI driver for it.
My overall impression reading this is that there could be a lot more
reliance on both generic functionality and as Andy suggested the spi-dw
driver - the headers seem easy for example. As far as I can tell the
main things this is adding compared to spi-dw are the dirmap code and
the IRQ disabling around the PIO on the FIFO both of which seem like
relatively small additions which it should be possible to accomodate
within spi-dw. For example the driver could have multiple transfer
functions and pick a different one with the interrupt disabling when
running on this hardware.
> --- /dev/null
> +++ b/drivers/spi/spi-bt1-sys.c
> @@ -0,0 +1,873 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
Please make the entire comment a C++ one so things look a bit cleaner
and more intentional.
> + writel(BIT(req->cs), bs->regs + BC_SPI_SER);
> + if (req->cs_gpiod) {
> + gpiod_set_value_cansleep(req->cs_gpiod,
> + !!(bs->cfg.mode & SPI_CS_HIGH));
If you have a GPIO chip select you should just let the core manage it
through cs_gpiod rather than open coding.
> + } else if (!req->dirmap) {
> + local_irq_save(bs->cfg.flags);
> + preempt_disable();
> + }
This is obviously not great, especially for larger transfers. It would
be a lot easier to review and manage this if the IRQ disabling was done
around the transfers in a single function rather than separately, that
way everything is next to each other and it's clearer that we're doing
this for the minimum time and didn't miss anything. Right now it's hard
to tell if everything is joined up.
> +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.
> +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.
> + /*
> + * Spin around the BUSY-state bit waiting for the controller to finish
> + * all the requested IO-operations.
> + */
> + do {
> + data = readl(bs->regs + BC_SPI_SR);
> + } while ((data & BC_SPI_SR_BUSY) || !(data & BC_SPI_SR_TFE));
We could use a timeout or something here in case something goes wrong,
as things stand we could end up spinning for ever.
It's also not clear to me why we only check for over/underrun before we
do the busy wait, especially a RX overrun could happen and cause us to
loose data while things are finishing up.
> + while (txlen || rxlen) {
> + cnt = BC_SPI_FIFO_LVL - __raw_readl(bs->regs + BC_SPI_TXFLR);
> + cnt = min(cnt, txlen);
> + txlen -= cnt;
> + while (cnt--) {
> + data = src ? *src++ : 0xFF;
> + __raw_writel(data, bs->regs + BC_SPI_DR);
> + }
It's more typical to write zeros when there's no data.
> +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?
> + 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
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 8, 2020 at 1:15 PM Serge Semin wrote: > > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote: > > On Fri, May 8, 2020 at 12:37 PM Serge Semin > > wrote: > > > > > > This SPI-controller is a part of the Baikal-T1 System Controller and > > > is based on the DW APB SSI IP-core, but with very limited resources: > > > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx > > > FIFO available. In order to provide a transparent initial boot code > > > execution this controller is also utilized by an vendor-specific block, > > > which provides an CS0 SPI flash direct mapping interface. Since both > > > direct mapping and SPI controller normal utilization are mutual exclusive > > > only a one of these interfaces can be used to access an external SPI > > > slave device. Taking into account the peculiarities of the controller > > > registers and physically mapped SPI flash access, very limited resources > > > and seeing the normal usecase of the controller is to access an external > > > SPI-nor flash, we decided to create a dedicated SPI driver for it. > > > > It seems a lot of code. > > Why can't you use spi-dw-mmio.c et al.? > > I said above why. Even though the registers set is similar It's too specific > to be integrated into the generic DW SSI driver. At least you may do at the beginning is to reuse header spi-dw.h and put your stuff under spi-dw-baikal.c or so. Then, look at the spi-dw.c and check what can be reused (I think a lot). -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 01:15:41PM +0300, Serge Semin wrote: > On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote: > > > slave device. Taking into account the peculiarities of the controller > > > registers and physically mapped SPI flash access, very limited resources > > > and seeing the normal usecase of the controller is to access an external > > > SPI-nor flash, we decided to create a dedicated SPI driver for it. > > It seems a lot of code. > > Why can't you use spi-dw-mmio.c et al.? > I said above why. Even though the registers set is similar It's too specific > to be integrated into the generic DW SSI driver. Can you be more specific about the issues? From what you wrote it sounded like the main thing was chip select handling. > > > The driver provides callbacks for native messages-based SPI interface, > > > SPI-memory and direct mapping read operations. Due to not having any > > > asynchronous signaling interface provided by the core we have no choice What do you mean by "asynchronous signaling interface provided by the core" here? signature.asc Description: PGP signature
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 08, 2020 at 01:03:11PM +0300, Andy Shevchenko wrote: > On Fri, May 8, 2020 at 12:37 PM Serge Semin > wrote: > > > > This SPI-controller is a part of the Baikal-T1 System Controller and > > is based on the DW APB SSI IP-core, but with very limited resources: > > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx > > FIFO available. In order to provide a transparent initial boot code > > execution this controller is also utilized by an vendor-specific block, > > which provides an CS0 SPI flash direct mapping interface. Since both > > direct mapping and SPI controller normal utilization are mutual exclusive > > only a one of these interfaces can be used to access an external SPI > > slave device. Taking into account the peculiarities of the controller > > registers and physically mapped SPI flash access, very limited resources > > and seeing the normal usecase of the controller is to access an external > > SPI-nor flash, we decided to create a dedicated SPI driver for it. > > It seems a lot of code. > Why can't you use spi-dw-mmio.c et al.? I said above why. Even though the registers set is similar It's too specific to be integrated into the generic DW SSI driver. -Sergey > > > The driver provides callbacks for native messages-based SPI interface, > > SPI-memory and direct mapping read operations. Due to not having any > > asynchronous signaling interface provided by the core we have no choice > > but to implement a polling-based data transmission/reception algorithm. > > In addition to that in order to bypass the automatic native chip-select > > toggle the driver disables the local interrupts during the memory-based > > transfers if no complementary GPIO-based chip-select detected in the > > platform. > > > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
On Fri, May 8, 2020 at 12:37 PM Serge Semin wrote: > > This SPI-controller is a part of the Baikal-T1 System Controller and > is based on the DW APB SSI IP-core, but with very limited resources: > no IRQ, no DMA, only a single native chip-select and just 8 bytes Tx/Rx > FIFO available. In order to provide a transparent initial boot code > execution this controller is also utilized by an vendor-specific block, > which provides an CS0 SPI flash direct mapping interface. Since both > direct mapping and SPI controller normal utilization are mutual exclusive > only a one of these interfaces can be used to access an external SPI > slave device. Taking into account the peculiarities of the controller > registers and physically mapped SPI flash access, very limited resources > and seeing the normal usecase of the controller is to access an external > SPI-nor flash, we decided to create a dedicated SPI driver for it. It seems a lot of code. Why can't you use spi-dw-mmio.c et al.? > The driver provides callbacks for native messages-based SPI interface, > SPI-memory and direct mapping read operations. Due to not having any > asynchronous signaling interface provided by the core we have no choice > but to implement a polling-based data transmission/reception algorithm. > In addition to that in order to bypass the automatic native chip-select > toggle the driver disables the local interrupts during the memory-based > transfers if no complementary GPIO-based chip-select detected in the > platform. -- With Best Regards, Andy Shevchenko
