Re: [PATCH 4/4] mtd: nand: omap2: Add support for NAND Controller on AM64 SoC

[Roger Quadros]

Hi Miquel,

On 24/11/2021 14:15, Miquel Raynal wrote:
> Hi Roger,
> 
> rogerq@xxxxxxxxxx wrote on Tue, 23 Nov 2021 12:36:09 +0200:
> 
>> AM64 SoC has an issue which prevents proper 8-bit and 16-bit
>> reads from GPMC. We are limited to do 32-bit reads only.
> 
> First, thanks for this series!

No problem. Just my job :)

> 
>> Force 32-bit only reads on affected platforms.
>>
> 
> Please change the commit title prefix to: "mtd: rawnand: omap2:" in
> patch 2, 3, 4.

OK.

>  
>> Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx>
>> ---
>>  drivers/mtd/nand/raw/omap2.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
>> index f1fc146e09b9..d952de771b35 100644
>> --- a/drivers/mtd/nand/raw/omap2.c
>> +++ b/drivers/mtd/nand/raw/omap2.c
>> @@ -28,6 +28,7 @@
>>  
>>  #include <linux/omap-gpmc.h>
>>  #include <linux/platform_data/mtd-nand-omap2.h>
>> +#include <linux/sys_soc.h>
>>  
>>  #define	DRIVER_NAME	"omap2-nand"
>>  #define	OMAP_NAND_TIMEOUT_MS	5000
>> @@ -181,6 +182,7 @@ struct omap_nand_info {
>>  	void (*data_out)(struct nand_chip *chip,
>>  			 const void *buf, unsigned int len,
>>  			 bool force_8bit);
>> +	bool force_32bit;
> 
> I believe we should have a driver capability instead of something in
> the info structure. You can save the value here as well in the probe if
> you want, but I would like this limitation to be tied to the
> compatible.

I will discuss about this at the end.
> 
>>  };
>>  
>>  static inline struct omap_nand_info *mtd_to_omap(struct mtd_info *mtd)
>> @@ -2070,6 +2072,25 @@ static void omap_nand_data_in(struct nand_chip *chip, void *buf,
>>  	struct omap_nand_info *info = mtd_to_omap(nand_to_mtd(chip));
>>  	u32 alignment = ((uintptr_t)buf | len) & 3;
>>  
>> +	if (info->force_32bit) {
> 
> I am a little bit bothered by this limitation. The force8_bit flag does
> not require the driver to read only 8-bits of the fifo register, it
> actually requires to use only the first 8-bits of the NAND bus (which
> can also be 16-bit wide). The older implementation just limited the
> number of bits reads to be 8 with ioread8, which seems to be a fine
> solution but would require more accesses than using ioread16 (or
> ioread32) when reading more than 1 byte on platforms with only 8-bit
> busses.

I didn't understand the purpose of force8_bit flag. 
How should the driver/controller behave if we get a data_in() call with len 8 and force8_bit flag set?

e.g. if 16-bit NAND ID area contains (little-endian) 2c d3 d0 a6 66 45 67 a3 4f 4e 46 49 ab ef 90 d3
what should data_in(len = 8, force_8_bit = 1) return in buffer?

Based on what you said earlier my guess is it should return 2c d0 66 67 4f 46 ab 90?

> 
> My point here is that:
> 1- the limited controllers cannot be used with a 16-bit bus
> 2- non-limited controllers can use ioread16 if the bus width is 8-bits

Sorry, I did not understand this either. The TI GPMC controller has a configuration setting where we
set the NAND device bus width (8-bit or 16-bit). Then it automatically converts ioread16 or
ioread32 to appropriate number of 8-bit accesses or 16-bit accesses to the NAND chip.

> 
> I guess it's fine not to change the logic to avoid breaking boards so
> we can just ignore [2] but I belive we should check chip->options &
> NAND_BUSWIDTH_16 in ->attach_chip() and refuse probing if this flag is
> set.
> 
>> +		u32 val;
>> +		int left;
>> +		u8 *ptr;
>> +
>> +		ioread32_rep(info->fifo, buf, len >> 2);
>> +		left = len & 0x3;
>> +		if (left) {
>> +			val = ioread32(info->fifo);
>> +			ptr = (u8 *)(buf + (len - left));
>> +			while (left--) {
>> +				*ptr++ = val & 0xff;
>> +				val >>= 8;
>> +			}
>> +		}
>> +
>> +		return;
>> +	}
>> +
>>  	if (force_8bit || (alignment & 1))
>>  		ioread8_rep(info->fifo, buf, len);
>>  	else if (alignment & 3)
>> @@ -2169,8 +2190,15 @@ static const struct nand_controller_ops omap_nand_controller_ops = {
>>  static struct nand_controller omap_gpmc_controller;
>>  static bool omap_gpmc_controller_initialized;
>>  
>> +static const struct of_device_id omap_nand_ids[];
>> +
> 
> I believe this change should be dropped.
> 
>>  static int omap_nand_probe(struct platform_device *pdev)
>>  {
>> +	const struct soc_device_attribute k3_soc_devices[] = {
>> +		{ .family = "AM64X", .revision = "SR1.0" },
>> +		{ /* sentinel */ }
>> +	};
>> +
>>  	struct omap_nand_info		*info;
>>  	struct mtd_info			*mtd;
>>  	struct nand_chip		*nand_chip;
>> @@ -2186,6 +2214,12 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  
>>  	info->pdev = pdev;
>>  
>> +	/* Some SoC's have 32-bit at least, read limitation */
>> +	if (soc_device_match(k3_soc_devices)) {
>> +		dev_info(&pdev->dev, "force 32-bit\n");
>> +		info->force_32bit = true;
>> +	}
>> +
> 
> As suggested above, just adding a capability structure tied to the
> compatible string and retrieved with of_device_get_match_data() should
> be enough and replace this manual tree research.

The trouble comes when TI updates the silicon revision to "SR2.0" and that has the issue fixed
but still uses the same compatible. So compatible string by itself is not sufficient to identify
the troubled devices. soc_device_match() was the easiest way to address this.

> 
>>  	err = omap_get_dt_info(dev, info);
>>  	if (err)
>>  		return err;
>> @@ -2286,6 +2320,7 @@ static int omap_nand_remove(struct platform_device *pdev)
>>  
>>  static const struct of_device_id omap_nand_ids[] = {
>>  	{ .compatible = "ti,omap2-nand", },
>> +	{ .compatible = "ti,am64-nand", },
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, omap_nand_ids);
> 
> The conversion to exec_op looks fine otherwise :)

Thanks :)

> 
> Thanks,
> Miquèl
> 

cheers,
-roger