| Message ID | 20211123103609.14063-1-rogerq@kernel.org |
|---|---|
| Headers | show |
| Series | mtd: nand: omap2: Switch to exec_ops, support AM64 SoC | expand |
Hi Roger, rogerq@kernel.org 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@kernel.org> > --- > 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
Hi Miquel, On 24/11/2021 14:15, Miquel Raynal wrote: > Hi Roger, > > rogerq@kernel.org 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@kernel.org> >> --- >> 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
Hi Roger, rogerq@kernel.org wrote on Thu, 25 Nov 2021 16:12:01 +0200: > Hi Miquel, > > On 24/11/2021 14:15, Miquel Raynal wrote: > > Hi Roger, > > > > rogerq@kernel.org 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@kernel.org> > >> --- > >> 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. Only access the lowest byte on the bus. This is only needed for meta-data reads (like status reads or ids) where the upper byte would be a duplicate. > 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? If on a 16-bit bus, you would receive 2c 2c d3 d3 d0 d0 a6 a6 etc and of course that's not what you want. > > 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. Ok great, in this case you should configure the bus width depending on the actual used width (8 or 16 bits). When an 8-bit access is requested with force_8bit, you should ensure the buswidth is changed to 8 and then use ioread8/16/32 as you wish and then return the bus back into its default state. > > > > > 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. This is precisely what compatibles are for, I believe we should declare the necessary additional compatibles and fix the device trees that are wrong. > >> 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 Thanks, Miquèl
+Rob and DT list Miques & Nishanth, On 26/11/2021 11:42, Miquel Raynal wrote: > Hi Roger, > > rogerq@kernel.org wrote on Thu, 25 Nov 2021 16:12:01 +0200: > >> Hi Miquel, >> >> On 24/11/2021 14:15, Miquel Raynal wrote: >>> Hi Roger, >>> >>> rogerq@kernel.org 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@kernel.org> >>>> --- >>>> 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. > > Only access the lowest byte on the bus. This is only needed for > meta-data reads (like status reads or ids) where the upper byte would > be a duplicate. > >> 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? > > If on a 16-bit bus, you would receive 2c 2c d3 d3 d0 d0 a6 a6 etc and > of course that's not what you want. > >>> 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. > > Ok great, in this case you should configure the bus width depending > on the actual used width (8 or 16 bits). When an 8-bit access is > requested with force_8bit, you should ensure the buswidth is changed > to 8 and then use ioread8/16/32 as you wish and then return the bus > back into its default state. > OK. I will try this out. Thanks for the tip. >> >>> >>> 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. > > This is precisely what compatibles are for, I believe we should declare > the necessary additional compatibles and fix the device trees that are > wrong. AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC. My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some hardware fixes and some quirks may not be needed anymore. Nishanth, Could you please chime in on why SoC revisions can't use different compatibles? > >>>> 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 :) >> <snip> cheers, -roger
On 13:10-20211126, Roger Quadros wrote: [...] > >>>> + /* 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. > > > > This is precisely what compatibles are for, I believe we should declare > > the necessary additional compatibles and fix the device trees that are > > wrong. > > AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC. > My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some > hardware fixes and some quirks may not be needed anymore. > > Nishanth, > > Could you please chime in on why SoC revisions can't use different compatibles? > The permutations of boards (with add-on cards) and SRs become un-manageable esp when Silicon Revisions(SRs) dont actually get into production. Instead, what we do suggest are one of two things: a) The dts in k.org always reflect the latest SR for the chip that is going into production. Older SR revisions are supported as overlays on top of the dtb. b) Where possible, use the chip-id framework[1] to dynamically detect the variations. This might be easier with newer K3 generation SoCs. In this instance, an overlay corresponding to older SoC might be feasible. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/soc/ti/k3-socinfo.yaml
Hi Nishanth, On 29/11/2021 06:36, Nishanth Menon wrote: > On 13:10-20211126, Roger Quadros wrote: > [...] > >>>>>> + /* 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. >>> >>> This is precisely what compatibles are for, I believe we should declare >>> the necessary additional compatibles and fix the device trees that are >>> wrong. >> >> AFAIK TI SoCs don't have different compatibles for different revisions of the same SoC. >> My understanding is that the SoC is the same so compatible shouldn't change. Just that there were some >> hardware fixes and some quirks may not be needed anymore. >> >> Nishanth, >> >> Could you please chime in on why SoC revisions can't use different compatibles? >> > > The permutations of boards (with add-on cards) and SRs become > un-manageable esp when Silicon Revisions(SRs) dont actually get into > production. Instead, what we do suggest are one of two things: > a) The dts in k.org always reflect the latest SR for the chip that is > going into production. Older SR revisions are supported as overlays on top > of the dtb. > b) Where possible, use the chip-id framework[1] to dynamically detect > the variations. This might be easier with newer K3 generation SoCs. > > > In this instance, an overlay corresponding to older SoC might be > feasible. > Did I understand correctly that we can use a different compatible for older SoC in the overlay? e.g. ti,am642-es1.0 ? If so then I can get rid of soc_device_match and use compatibles matching only in this patch. > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/soc/ti/k3-socinfo.yaml > cheers, -roger
On 16:45-20211208, Roger Quadros wrote: [..] > Did I understand correctly that we can use a different compatible for older SoC > in the overlay? e.g. ti,am642-es1.0 ? If that is what we would desire. There are a few SR1.0 (not ES1.0) examples for previous devices in [1] NOTE: the dts in k.org will always point to the latest production SRx.y device that has been released to market (aka products going to market). previous pre-production SR support tends to be with overlays for various quirks. > > If so then I can get rid of soc_device_match and use compatibles matching only in this patch. As appropriate for the subsystem. [1] https://git.ti.com/cgit/ti-linux-kernel/ti-upstream-tools/tree/arch/arm64/boot/dts/ti