Re: [PATCH 2/4] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

["Cédric Le Goater" <clg@kaod.org>]

On 06/21/2017 09:32 AM, Cédric Le Goater wrote:
> On 06/21/2017 12:50 AM, Cyrille Pitchen wrote:
>> Hi Cédric,
>>
>>
>> Le 20/04/2017 à 13:56, Cédric Le Goater a écrit :
>>> From: Robert Lippert <roblip@gmail.com>
>>>
>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>> controllers which uses both MISO and MOSI lines for data during a read
>>> to double the read bandwidth.
>>>
>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>> [clg: adapted to mainline driver ]
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index 2940f2098420..183d950e621b 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>  	struct aspeed_smc_chip *chip = nor->priv;
>>>  	int i;
>>>  	u8 dummy = 0xFF;
>>> +	u32 ctl;
>>>  
>>>  	aspeed_smc_start_user(nor);
>>>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>  	for (i = 0; i < chip->nor.read_dummy / 8; i++)
>>>  		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
>>>  
>>> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>> +		/* Switch to dual I/O mode for data cycle */
>>> +		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
>>> +		ctl |= CONTROL_IO_DUAL_DATA;
>>> +		writel(ctl, chip->ctl);
>>> +	}
>>> +
>>
>> As expected this patch doesn't apply as is to the spi-nor/next branch of
>> l2-mtd because flash_read and SPI_NOR_{DUAL|QUAD} enums no longer exists.
>>
>> Besides it looks like the Aspeed controller can support SPI 1-2-2 as
>> well as SPI 1-1-2.
>>
>> I see:
>> #define CONTROL_IO_DUAL_DATA		BIT(29)
>> #define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))
>>
>> Now spi_nor_scan() makes the difference between these 2 SPI protocols.
>>
>> Besides, you may also want to use the compatible DT string to make the
>> difference between Aspeed controllers also supporting SPI 1-1-4 and SPI
>> 1-4-4 and other controllers supporting only Dual SPI protocols so you
>> could fill the hwcaps parameter of spi_nor_scan() as needed.
>>
>> So I think we should spend a little more time to rework this patch.
> 
> OK. I will take a look.
> 
> Please consider taking :
> 
>   [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

Forget it. I am having issues with this patch on newer kernels. I will 
cook a patchset after the merge window.

Thanks,

C. 
   
 
> while I reconcile my patchset with the spi-nor tree.
> 
> Thanks,
> 
> C.
> 
>>
>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>  	aspeed_smc_stop_user(nor);
>>>  	return len;
>>> @@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>  	case SPI_NOR_FAST:
>>>  		cmd = CONTROL_COMMAND_MODE_FREAD;
>>>  		break;
>>> +	case SPI_NOR_DUAL:
>>> +		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
>>> +		break;
>>>  	default:
>>>  		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>>>  		return -EINVAL;
>>> @@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>  	chip->ctl_val[smc_read] |= cmd |
>>>  		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>>>  
>>> -	dev_dbg(controller->dev, "base control register: %08x\n",
>>> +	dev_dbg(controller->dev, "read control register: %08x\n",
>>>  		chip->ctl_val[smc_read]);
>>>  	return 0;
>>>  }
>>> @@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>>  			break;
>>>  
>>>  		/*
>>> -		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>>> -		 * attach when board support is present as determined
>>> -		 * by of property.
>>> +		 * Aspeed SoCs only support Dual mode
>>>  		 */
>>> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
>>>  		if (ret)
>>>  			break;
>>>  
>>>
>>
>