Re: [v2, 2/4] mtd: spi-nor: Add Octal SPI support to Cadence QSPI driver.

[Marek Vasut <marek.vasut@gmail.com>]

On 03/10/2017 01:00 PM, Artur Jedrysek wrote:
> 
> From: Marek Vasut [mailto:marek.vasut@gmail.com]
> Sent: 10 March 2017 04:37
>> On 03/08/2017 09:02 AM, Artur Jedrysek wrote:
>>> Recent versions of Cadence QSPI controller support Octal SPI transfers
>>> as well. This patch updates existing driver to support such feature.
>>>
>>> It is not possible to determine whether or not octal mode is supported
>>> just by looking at revision register alone. To solve that, an additional
>>> compatible in Device Tree is added to indicate such capability.
>>> Both (revision and compatible) are used to determine, which mode to
>>> pass to spi_nor_scan() call.
>>>
>>> Signed-off-by: Artur Jedrysek <jartur@cadence.com>
>>> ---
>>> Changelog:
>>> v2: Use new compatible in DT, instead of boolean property, to indicate
>>> Octal SPI support.
>>> Extracted Kconfig update to seperate patch.
>>> ---
>>>  drivers/mtd/spi-nor/cadence-quadspi.c | 69 +++++++++++++++++++++++++++++++----
>>>  1 file changed, 61 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
>>> index 9f8102d..a96471d 100644
>>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
>>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
>>> @@ -76,6 +76,7 @@ struct cqspi_st {
>>>  	u32			fifo_depth;
>>>  	u32			fifo_width;
>>>  	u32			trigger_address;
>>> +	u32			max_mode;
>>
>> I think it's time to introduce flags instead,
>> ie. #define CQSPI_FLAG_SUPPORTS_OCTAL BIT(0)
>>
>>>  	struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
>>>  };
>>>
>>> @@ -87,6 +88,10 @@ struct cqspi_st {
>>>  #define CQSPI_INST_TYPE_SINGLE			0
>>>  #define CQSPI_INST_TYPE_DUAL			1
>>>  #define CQSPI_INST_TYPE_QUAD			2
>>> +#define CQSPI_INST_TYPE_OCTAL			3
>>> +
>>> +#define CQSPI_MAX_MODE_QUAD			0
>>> +#define CQSPI_MAX_MODE_OCTAL			1
>>>
>>>  #define CQSPI_DUMMY_CLKS_PER_BYTE		8
>>>  #define CQSPI_DUMMY_BYTES_MAX			4
>>> @@ -204,6 +209,14 @@ struct cqspi_st {
>>>  #define CQSPI_REG_CMDWRITEDATALOWER		0xA8
>>>  #define CQSPI_REG_CMDWRITEDATAUPPER		0xAC
>>>
>>> +#define CQSPI_REG_MODULEID			0xFC
>>> +#define CQSPI_REG_MODULEID_CONF_ID_MASK		0x3
>>> +#define CQSPI_REG_MODULEID_CONF_ID_LSB		0
>>> +#define CQSPI_REG_MODULEID_CONF_ID_OCTAL_PHY	0x0
>>> +#define CQSPI_REG_MODULEID_CONF_ID_OCTAL	0x1
>>> +#define CQSPI_REG_MODULEID_CONF_ID_QUAD_PHY	0x2
>>> +#define CQSPI_REG_MODULEID_CONF_ID_QUAD		0x3
>>> +
>>>  /* Interrupt status bits */
>>>  #define CQSPI_REG_IRQ_MODE_ERR			BIT(0)
>>>  #define CQSPI_REG_IRQ_UNDERFLOW			BIT(1)
>>> @@ -866,6 +879,9 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>>>  		case SPI_NOR_QUAD:
>>>  			f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
>>>  			break;
>>> +		case SPI_NOR_OCTAL:
>>> +			f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
>>> +			break;
>>>  		default:
>>>  			return -EINVAL;
>>>  		}
>>> @@ -977,6 +993,17 @@ static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>>  	return ret;
>>>  }
>>>
>>> +static const u32 cqspi_max_mode_quad = CQSPI_MAX_MODE_QUAD;
>>> +static const u32 cqspi_max_mode_octal = CQSPI_MAX_MODE_OCTAL;
>>> +
>>> +static struct of_device_id const cqspi_dt_ids[] = {
>>> +	{ .compatible = "cdns,qspi-nor", .data = &cqspi_max_mode_quad },
>>> +	{ .compatible = "cdns,ospi-nor", .data = &cqspi_max_mode_octal },
>>
>> .data = (void *)CQSPI_FLAG_SUPPORTS_OCTAL , then you don't need that
>> static const stuff ...
>>
> 
> I had some doubts regarding that approach, as it may be dependent on the
> CPU architecture (32-bit, 64-bit) and endianness. .data needs to be first
> casted to unsigned long for reading, to ensure correct access and to 
> allow bitwise operations on it. That solution works, and if such approach
> is preferred, then it will be used in next version of the patch.

Good

>>> +	{ /* end of table */ }
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, cqspi_dt_ids);
>>> +
>>>  static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
>>>  				    struct cqspi_flash_pdata *f_pdata,
>>>  				    struct device_node *np)
>>> @@ -1018,6 +1045,13 @@ static int cqspi_of_get_pdata(struct platform_device *pdev)
>>>  {
>>>  	struct device_node *np = pdev->dev.of_node;
>>>  	struct cqspi_st *cqspi = platform_get_drvdata(pdev);
>>> +	const struct of_device_id *match;
>>> +
>>> +	cqspi->max_mode = CQSPI_MAX_MODE_QUAD;
>>> +
>>> +	match = of_match_node(cqspi_dt_ids, np);
>>> +	if (match && match->data)
>>> +		cqspi->max_mode = *((u32 *)match->data);
>>
>> Use flags instead, see above.
>>
>>>  	cqspi->is_decoded_cs = of_property_read_bool(np, "cdns,is-decoded-cs");
>>>
>>> @@ -1074,9 +1108,35 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>>  	struct cqspi_flash_pdata *f_pdata;
>>>  	struct spi_nor *nor;
>>>  	struct mtd_info *mtd;
>>> +	enum read_mode mode;
>>> +	enum read_mode dt_mode = SPI_NOR_QUAD;
>>>  	unsigned int cs;
>>> +	unsigned int rev_reg;
>>>  	int i, ret;
>>>
>>> +	/* Determine, whether or not octal transfer MAY be supported */
>>
>> But you already know that from DT, no ?
>>
> 
> Cadence Octal (formerly Quad) SPI Controller is sold as an IP, and is
> configurable. This includes max SPI mode. It is possible to detect, that
> Octal SPI controller is configured (during hardware compilation) to support
> up to Quad mode, using revision register.

So the octal-spi controller always has this config register, but the
quad-spi controller may or may not have this register ?

>>> +	rev_reg = readl(cqspi->iobase + CQSPI_REG_MODULEID);
>>> +	dev_info(dev, "CQSPI Module id %x\n", rev_reg);
>>> +
>>> +	switch (rev_reg & CQSPI_REG_MODULEID_CONF_ID_MASK) {
>>> +	case CQSPI_REG_MODULEID_CONF_ID_OCTAL_PHY:
>>> +	case CQSPI_REG_MODULEID_CONF_ID_OCTAL:
>>> +		mode = SPI_NOR_OCTAL;
>>> +		break;
>>> +	case CQSPI_REG_MODULEID_CONF_ID_QUAD:
>>> +	case CQSPI_REG_MODULEID_CONF_ID_QUAD_PHY:
>>
>> Does this work on all revisions of CQSPI ?
>>
> 
> After having a more thorough look at specification of older IP version
> (quad only) it seems, that revision register format has indeed changed.
> This will be fixed in the next version of the patch.

Can the quad-spi controller be configured only as dual or single ?
What about the octal one ? These cases should probably be handled
somehow too, right ?

>>> +		mode = SPI_NOR_QUAD;
>>> +		break;
>>> +	}
>>> +
>>> +	if (cqspi->max_mode == CQSPI_MAX_MODE_OCTAL)
>>> +		dt_mode = SPI_NOR_OCTAL;
>>> +
>>> +	if (mode == SPI_NOR_QUAD && dt_mode == SPI_NOR_OCTAL)
>>> +		dev_warn(dev, "Requested octal mode is not supported by the device.");
>>> +	else if (mode == SPI_NOR_OCTAL && dt_mode == SPI_NOR_QUAD)
>>> +		mode = SPI_NOR_QUAD;
>>> +
>>>  	/* Get flash device data */
>>>  	for_each_available_child_of_node(dev->of_node, np) {
>>>  		ret = of_property_read_u32(np, "reg", &cs);
>>> @@ -1123,7 +1183,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>>  			goto err;
>>>  		}
>>>
>>> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>>> +		ret = spi_nor_scan(nor, NULL, mode);
>>>  		if (ret)
>>>  			goto err;
>>>
>>> @@ -1277,13 +1337,6 @@ static const struct dev_pm_ops cqspi__dev_pm_ops = {
>>>  #define CQSPI_DEV_PM_OPS	NULL
>>>  #endif
>>>
>>> -static struct of_device_id const cqspi_dt_ids[] = {
>>> -	{.compatible = "cdns,qspi-nor",},
>>> -	{ /* end of table */ }
>>> -};
>>> -
>>> -MODULE_DEVICE_TABLE(of, cqspi_dt_ids);
>>> -
>>>  static struct platform_driver cqspi_platform_driver = {
>>>  	.probe = cqspi_probe,
>>>  	.remove = cqspi_remove,
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut