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

[Cyrille Pitchen]

Hi Cédric,

Le 20/10/2016 à 11:17, Cédric Le Goater a écrit :
> Hello Cyrille,
> 
> Thanks for taking a look, some answers and more questions below.
> 
> On 10/19/2016 07:06 PM, Cyrille Pitchen wrote:
>> Hi Cédric,
>>
>> Le 17/10/2016 à 18:57, 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.
>>>
>>> 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 void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
>>> +{
>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>> +	__be32 temp;
>>> +	u32 cmdaddr;
>>> +
>>> +	switch (nor->addr_width) {
>>> +	default:
>>> +		WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n",
>>> +			  nor->addr_width);
>>> +		/* FALLTHROUGH */
>>> +	case 3:
>>> +		cmdaddr = addr & 0xFFFFFF;
>>> +
>>> +		cmdaddr |= (u32)cmd << 24;
>>> +
>>> +		temp = cpu_to_be32(cmdaddr);
>>> +		aspeed_smc_to_fifo(chip->base, &temp, 4);
>>> +		break;
>>> +	case 4:
>>> +		temp = cpu_to_be32(addr);
>>> +		aspeed_smc_to_fifo(chip->base, &cmd, 1);
>>> +		aspeed_smc_to_fifo(chip->base, &temp, 4);
>>> +		break;
>>> +	}
>>> +}
>>> +
>>> +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);
> 
> I think this section of code lock+start_user should be in a prepare handler 
> if I am correct ? and I could get rid of the lock in that case to rely on
> the one of the spi nor.
> 

For the mutex, looking at your driver, you lock it at the beginning then unlock
at the end of the four handlers:
- nor->read()
- nor->write()
- nor->read_reg()
- nor->write_reg()

If this is the only purpose of this mutex, yes, you can remove it and relie on
the internal spi-nor one (nor->lock), which is locked from
spi_nor_lock_and_prep() hence at the beginning of spi_nor_erase(),
spi_nor_read() and spi_nor_write() calls.

However, about moving the call of aspeed_smc_start_user() into a new prepare
handler, I don't know whether it would actually fit your needs: for sure, 
aspeed_smc_start_user() would be no longer called at some points.

2 examples:

A - spi_nor_write() calls:
1 - spi_nor_lock_and_prep() -> nor->prepare()
2 - write_enable() -> nor->write_reg()
3 - nor->write()
4 - spi_nor_wait_till_ready() -> nor->read_reg()
5 - spi_nor_unlock_and_unprep() -> nor->unprepare()

With your current driver, aspeed_smc_start_user()/aspeed_smc_stop_user() are
called 3 times, indeed each time a command is sent on the SPI bus:
WRITE ENABLE, PAGE PROGRAM, READ STATUS

Then if you move then into spi_nor_[un]lock_and_[un]prep(), those two functions
would only be called once each.


B - spi_nor_scan() calls:
1 - spi_nor_read_id() -> nor->read_reg()

but spi_nor_lock_and_prep() is not called before, so if you nead to call
aspeed_smc_start_user() before sending any command on the SPI bus, it won't
work.

I don't know how your SPI controller works but maybe using the nor->prepare()
handler is not what you want!


>>> +	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>
>> I guess the dummy cycles are missing between address cycles and data cycles!
>> Check nor->read_dummy ;)
> 
> ah yes, I meant to and I forgot to add it ... will do in v2. I suppose 
> we are doing fine with the driver now because we only use normal speed.
> 

Indeed, the legacy READ (03h) command has no dummy cycles as opposed to all
other FAST READ commands. Of course, the READ command is far much slower.

>> Or maybe this controller can really only perform READ operations but no FAST
>> READ commands. Hence the "spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL)"
>> below.
> 
> yes it can. I suppose the driver should also call spi_nor_set_flash_node() 
> for module supporting it. that brings another question I would like 
> to discuss for possible followups patches. 
> 

Calling spi_nor_set_flash_node() with the relevant parameter tells
spi_nor_scan() which DT node to parse to tune some settings.
For instance, you could add the "m25p,fast-read" DT property so spi_nor_scan()
would select Fast Read operations rather than Read operations.

> The controller can be fine tuned for a particular chip module. The 
> CE0 control register has a few bits to set the spi clock, the dummy 
> cycles, the CS inactive width and there is also a register to setup 
> read timings delay.
>  
> So you need to loop on these different settings to find a correct one
> and for that, you use a gold image captured at low speed as reference.
> 
> But you need to know which chip module you are dealing with. would it 
> be possible to expose the jedec to the controller for this purpose ? 
> and then do the training to build the timing settings in the driver. 
> This is a similar approach taken by some nand controllers.
> 

I'm not sure to fully understand your needs.
The spi clock is often provided in the device tree, on the controller or memory
node, with the "spi-max-frequency" DT property.
The number of dummy cycles is set in nor->read_dummy by spi_nor_scan()
according to the actual memory model and the selected (Fast) Read op code.
For chip-select timings, those settings aren't currently handled by
spi_nor_scan() but might be supported inside the SPI controller driver through
DT properties if needed.

I don't know why your SPI controller driver needs to know the exact memory
model to work. Aren't the pieces of information provided by spi-nor.c enough?
Actually, for this point I don't quite exactly understand your needs! :)

Best regards,

Cyrille


> Thanks,
> 
> C.
> 
>  
>>> +	aspeed_smc_from_fifo(read_buf, chip->base, len);
>>> +	aspeed_smc_stop_user(nor);
>>> +
>>> +	mutex_unlock(&chip->controller->mutex);
>>> +
>>> +	return len;
>>> +}
>> [...]
>>
>> Best regards,
>>
>> Cyrille
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>