[PATCH linux dev-4.7 07/12] mtd: spi-nor: aspeed: make locking more explicit

[Cédric Le Goater]

On 10/17/2016 07:10 AM, Joel Stanley wrote:
> On Fri, Oct 14, 2016 at 11:07 PM, Cédric Le Goater <clg at kaod.org> wrote:
>> The routine aspeed_smc_stop_user() and aspeed_smc_start_user() hide
>> the fact that they lock an unlock the controller. Change that to make
>> it obvious in the code.
>>
>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index a47eecfdfda2..b22e07d3ef32 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -276,8 +276,6 @@ static void aspeed_smc_start_user(struct spi_nor *nor)
>>         struct aspeed_smc_per_chip *chip = nor->priv;
>>         u32 ctl = chip->ctl_val[smc_base];
>>
>> -       mutex_lock(&chip->controller->mutex);
>> -
> 
> This removes all locking from start_user and stop_user. There are some
> accesses and register writes in both of these functions, is that okay?

ah. Indeed, I did half of the job in this patch. Probably when I tried 
to split the changes :/

Currently, the driver only support User mode, in which the parameters of 
an operation on the chip are passed by writing to the address where the 
flash is mapped. So start_user() is always called to start a read or a 
write transaction, if it is to access a flash register or if it is to 
access its data. 

It is completed by calling stop_user(). The flash then goes back to 
a Command mode where operations are driven differently. 

The current flow is not correct from the spi-nor API perspective. I think 
we should be using the prepare() and unprepare() ops instead. I might give 
that a try before mainline. I am pretty sure it will be spotted.

> What is the lock (supposed to be) protecting?

read/write ops which change the controller configuration. The nor has
its own locking so we might not need this lock if we use the prepare() 
and unprepare() ops.
 
> (I note that you effectively add it back in patch 9 where you
> introduce DMA, by putting locks in aspeed_smc_read_user and write_user
> that then calls start_user.)

I made one big patch with all the changes for mainline. We should be 
fine now.

Thanks,

C. 


> 
>>         ctl |= CONTROL_SPI_COMMAND_MODE_USER |
>>                 CONTROL_SPI_CE_STOP_ACTIVE_CONTROL;
>>         writel(ctl, chip->ctl);
>> @@ -296,19 +294,21 @@ static void aspeed_smc_stop_user(struct spi_nor *nor)
>>
>>         writel(ctl2, chip->ctl);        /* stop user CE control */
>>         writel(ctl, chip->ctl);         /* default to fread or read */
>> -
>> -       mutex_unlock(&chip->controller->mutex);
>>  }
>>
>>  static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>  {
>>         struct aspeed_smc_per_chip *chip = nor->priv;
>>
>> +       mutex_lock(&chip->controller->mutex);
>> +
>>         aspeed_smc_start_user(nor);
>>         aspeed_smc_to_fifo(chip->base, &opcode, 1);
>>         aspeed_smc_from_fifo(buf, chip->base, len);
>>         aspeed_smc_stop_user(nor);
>>
>> +       mutex_unlock(&chip->controller->mutex);
>> +
>>         return 0;
>>  }
>>
>> @@ -317,11 +317,15 @@ static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
>>  {
>>         struct aspeed_smc_per_chip *chip = nor->priv;
>>
>> +       mutex_lock(&chip->controller->mutex);
>> +
>>         aspeed_smc_start_user(nor);
>>         aspeed_smc_to_fifo(chip->base, &opcode, 1);
>>         aspeed_smc_to_fifo(chip->base, buf, len);
>>         aspeed_smc_stop_user(nor);
>>
>> +       mutex_unlock(&chip->controller->mutex);
>> +
>>         return 0;
>>  }
>>
>> --
>> 2.7.4
>>