[PATCH v2] mtd/spi-nor: Add SPI memory controllers for Aspeed SoCs

[Cédric Le Goater]

Hello Cyrille

On 12/09/2016 03:13 PM, Cyrille Pitchen wrote:
> Le 09/12/2016 à 15:03, Marek Vasut a écrit :
>> On 12/09/2016 02:37 PM, Cyrille Pitchen wrote:
>>> Hi Cedric,
>>>
>>> Le 09/11/2016 à 11:42, Cédric Le Goater a écrit :
>>>> This driver adds mtd support for spi-nor attached to either or both of
>>>> the Firmware Memory Controller or the SPI Flash Controller (AST2400
>>>> only).
>>>>
>>>> The SMC controllers on the Aspeed AST2500 SoC are very similar to the
>>>> ones found on the AST2400. The differences are on the number of
>>>> supported flash modules and their default mappings in the SoC address
>>>> space.
>>>>
>>>> The Aspeed AST2500 has one SPI controller for the BMC firmware and two
>>>> for the host firmware. All controllers have now the same set of
>>>> registers compatible with the AST2400 FMC controller and the legacy
>>>> 'SMC' controller is fully gone.
>>>>
>>>> Each controller has a memory range on which it maps its flash module
>>>> slaves. Each slave is assigned a memory window for its mapping that
>>>> can be changed at bootime with the Segment Address Register.
>>>>
>>>> Each SPI flash slave can then be accessed in two modes: Command and
>>>> User. When in User mode, accesses to the memory segment of the slaves
>>>> are translated in SPI transfers. When in Command mode, the HW
>>>> generates the SPI commands automatically and the memory segment is
>>>> accessed as if doing a MMIO.
>>>>
>>>> Currently, only the User mode is supported. Command mode needs a
>>>> little more work to check that the memory window on the AHB bus fits
>>>> the module size.
>>>>
>>>> Based on previous work from Milton D. Miller II <miltonm at us.ibm.com>
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>>>> ---
>>> [...]
>>>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	mutex_lock(&chip->controller->mutex);
>>>> +
>>>> +	aspeed_smc_start_user(nor);
>>>> +	aspeed_smc_write_to_ahb(chip->base, &opcode, 1);
>>>> +	aspeed_smc_read_from_ahb(buf, chip->base, len);
>>>> +	aspeed_smc_stop_user(nor);
>>>> +
>>>> +	mutex_unlock(&chip->controller->mutex);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
>>>> +				int len)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	mutex_lock(&chip->controller->mutex);
>>>> +
>>>> +	aspeed_smc_start_user(nor);
>>>> +	aspeed_smc_write_to_ahb(chip->base, &opcode, 1);
>>>> +	aspeed_smc_write_to_ahb(chip->base, buf, len);
>>>> +	aspeed_smc_stop_user(nor);
>>>> +
>>>> +	mutex_unlock(&chip->controller->mutex);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>> [...]
>>>> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>> +				    size_t len, u_char *read_buf)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	mutex_lock(&chip->controller->mutex);
>>>> +
>>>> +	aspeed_smc_start_user(nor);
>>>> +	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>> +	aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>> +	aspeed_smc_stop_user(nor);
>>>> +
>>>> +	mutex_unlock(&chip->controller->mutex);
>>>> +
>>>> +	return len;
>>>> +}
>>>> +
>>>> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len,
>>>> +				     const u_char *write_buf)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	mutex_lock(&chip->controller->mutex);
>>>> +
>>>> +	aspeed_smc_start_user(nor);
>>>> +	aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to);
>>>> +	aspeed_smc_write_to_ahb(chip->base, write_buf, len);
>>>> +	aspeed_smc_stop_user(nor);
>>>> +
>>>> +	mutex_unlock(&chip->controller->mutex);
>>>> +
>>>> +	return len;
>>>> +}
>>>
>>> As I've explained in review of the series v1, the chip->controller->mutex
>>> seems to be used only by aspeed_smc_read_reg(), aspeed_smc_write_reg(),
>>> aspeed_smc_read_user() and aspeed_smc_write_user(), the driver
>>> implementations of nor->read_reg(), nor->write_reg(), nor->read() and
>>> nor->write().
>>>
>>> This driver locks the mutex at the very beginning of each of those
>>> functions and unlocks the mutex before returning.
>>>
>>> So my understanding is that you use this mutex to prevent
>>> aspeed_smc_[read|write]_[reg|user]() from being called concurrently.
>>>
>>> If so, the spi-nor framework already takes care of this issue with the
>>> couple of functions: spi_nor_lock_and_prep() / spi_nor_unlock_and_unprep().
>>>
>>> Indeed, spi_nor_lock_and_prep() is called on entering and
>>> spi_nor_unlock_and_unprep() on exiting each of the following functions:
>>> - mtd->_read = spi_nor_read
>>> - mtd->_write = spi_nor_write / sst_write
>>> - mtd->_erase = spi_nor_erase
>>> - mtd->_lock = spi_nor_lock
>>> - mtd->_unlock = spi_nor_unlock
>>> - mtd->_is_lock = spi_nor_is_locked
>>>
>>> Except for spi_nor_scan(), which is called once for all during the probe
>>> and before registering the mtd_info structure, only the above
>>> mtd->_<handlers> call the nor->read_reg, nor->write_reg, nor->read,
>>> nor->erase and nor->write spi-nor handlers.
>>> So your spi-nor / aspeed_smc_<handlers> are always protected from
>>> concurrent access by the mutex locked in spi_nor_lock_and_prep().
>>>
>>> So don't worry about concurrent access issue, the spi-nor framework takes
>>> care of you :)
>>
>> Does it take care of me even if I have multiple flashes ? I recall I had
>> to put mutexes into prepare and unprepare myself in the CQSPI driver to
>> prevent problems when accessing two flashes simultaneously.
>>
>>
> 
> Well indeed you're right, with multiple flashes I guess the driver will
> need to use an additional mutex. Then it can be placed either in each
> read_reg/write_reg/read/write handlers like Cedric did or in
> prepare/unprepare handlers like Marek did in the Cadence Quad SPI drivers.
> Both solutions work and are fine for me.

So I will go for the prepare/unprepare handler solution for now.

Thanks,

C.