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@xxxxxxxxxx>
>>>
>>> Signed-off-by: Cédric Le Goater <clg@xxxxxxxx>
>>> ---
>> [...]
>>> +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.
Anyway, sorry for the noise!
Best regards,
Cyrille
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
