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!
> Force 32-bit only reads on affected platforms.
>
Please change the commit title prefix to: "mtd: rawnand: omap2:" in
patch 2, 3, 4.
> 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.
> };
>
> 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.
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
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.
> 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,
Miquèl
