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.
--
Best regards,
Marek Vasut
--
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
