* [PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection
@ 2020-06-08 9:40 Álvaro Fernández Rojas
2020-06-08 9:40 ` [PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE Álvaro Fernández Rojas
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Álvaro Fernández Rojas @ 2020-06-08 9:40 UTC (permalink / raw)
To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal,
richard, vigneshr, jonas.gorski, linus.walleij, linux-mips,
linux-arm-kernel, linux-kernel, linux-mtd
Cc: Álvaro Fernández Rojas
Instead of trying to parse CFE version string, which is customized by some
vendors, let's just check that "CFE1" was passed on argument 3.
Álvaro Fernández Rojas (2):
MIPS: BCM63xx: add helper function to detect CFE
mtd: parsers: bcm63xx: simplify CFE detection
.../include/asm/mach-bcm63xx/bcm63xx_board.h | 6 ++++
drivers/mtd/parsers/bcm63xxpart.c | 28 ++-----------------
2 files changed, 9 insertions(+), 25 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE 2020-06-08 9:40 [PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas @ 2020-06-08 9:40 ` Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas 2020-06-08 16:06 ` [PATCH v2] " Álvaro Fernández Rojas 2 siblings, 0 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-08 9:40 UTC (permalink / raw) To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Cc: Álvaro Fernández Rojas CFE passes argument 3 as "CFE1". Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- arch/mips/include/asm/mach-bcm63xx/bcm63xx_board.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/arch/mips/include/asm/mach-bcm63xx/bcm63xx_board.h b/arch/mips/include/asm/mach-bcm63xx/bcm63xx_board.h index 1d19a726f86c..5af07796e8c7 100644 --- a/arch/mips/include/asm/mach-bcm63xx/bcm63xx_board.h +++ b/arch/mips/include/asm/mach-bcm63xx/bcm63xx_board.h @@ -2,6 +2,8 @@ #ifndef BCM63XX_BOARD_H_ #define BCM63XX_BOARD_H_ +#include <asm/bootinfo.h> + const char *board_get_name(void); void board_prom_init(void); @@ -10,4 +12,8 @@ void board_setup(void); int board_register_devices(void); +static inline bool bcm63xx_is_cfe_present(void) { + return fw_arg3 == 0x43464531; +} + #endif /* ! BCM63XX_BOARD_H_ */ -- 2.26.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-08 9:40 [PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE Álvaro Fernández Rojas @ 2020-06-08 9:40 ` Álvaro Fernández Rojas 2020-06-08 16:06 ` [PATCH v2] " Álvaro Fernández Rojas 2 siblings, 0 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-08 9:40 UTC (permalink / raw) To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Cc: Álvaro Fernández Rojas Instead of trying to parse CFE version string, which is customized by some vendors, let's just check that "CFE1" was passed on argument 3. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- drivers/mtd/parsers/bcm63xxpart.c | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c index 78f90c6c18fd..06b69e905a42 100644 --- a/drivers/mtd/parsers/bcm63xxpart.c +++ b/drivers/mtd/parsers/bcm63xxpart.c @@ -22,6 +22,8 @@ #include <linux/mtd/partitions.h> #include <linux/of.h> +#include <asm/mach-bcm63xx/bcm63xx_board.h> + #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 @@ -32,30 +34,6 @@ #define STR_NULL_TERMINATE(x) \ do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) -static int bcm63xx_detect_cfe(struct mtd_info *master) -{ - char buf[9]; - int ret; - size_t retlen; - - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, - (void *)buf); - buf[retlen] = 0; - - if (ret) - return ret; - - if (strncmp("cfe-v", buf, 5) == 0) - return 0; - - /* very old CFE's do not have the cfe-v string, so check for magic */ - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, - (void *)buf); - buf[retlen] = 0; - - return strncmp("CFE1CFE1", buf, 8); -} - static int bcm63xx_read_nvram(struct mtd_info *master, struct bcm963xx_nvram *nvram) { @@ -138,7 +116,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, struct bcm963xx_nvram *nvram = NULL; int ret; - if (bcm63xx_detect_cfe(master)) + if (!bcm63xx_is_cfe_present()) return -EINVAL; nvram = vzalloc(sizeof(*nvram)); -- 2.26.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-08 9:40 [PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas @ 2020-06-08 16:06 ` Álvaro Fernández Rojas 2020-06-11 7:55 ` Miquel Raynal 2020-06-12 7:35 ` [PATCH v3] " Álvaro Fernández Rojas 2 siblings, 2 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-08 16:06 UTC (permalink / raw) To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Cc: Álvaro Fernández Rojas Instead of trying to parse CFE version string, which is customized by some vendors, let's just check that "CFE1" was passed on argument 3. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- v2: use CFE_EPTSEAL definition and avoid using an additional funtion. drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c index 78f90c6c18fd..493a75b2f266 100644 --- a/drivers/mtd/parsers/bcm63xxpart.c +++ b/drivers/mtd/parsers/bcm63xxpart.c @@ -22,6 +22,9 @@ #include <linux/mtd/partitions.h> #include <linux/of.h> +#include <asm/bootinfo.h> +#include <asm/fw/cfe/cfe_api.h> + #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 @@ -32,30 +35,6 @@ #define STR_NULL_TERMINATE(x) \ do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) -static int bcm63xx_detect_cfe(struct mtd_info *master) -{ - char buf[9]; - int ret; - size_t retlen; - - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, - (void *)buf); - buf[retlen] = 0; - - if (ret) - return ret; - - if (strncmp("cfe-v", buf, 5) == 0) - return 0; - - /* very old CFE's do not have the cfe-v string, so check for magic */ - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, - (void *)buf); - buf[retlen] = 0; - - return strncmp("CFE1CFE1", buf, 8); -} - static int bcm63xx_read_nvram(struct mtd_info *master, struct bcm963xx_nvram *nvram) { @@ -138,7 +117,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, struct bcm963xx_nvram *nvram = NULL; int ret; - if (bcm63xx_detect_cfe(master)) + if (fw_arg3 != CFE_EPTSEAL) return -EINVAL; nvram = vzalloc(sizeof(*nvram)); -- 2.26.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-08 16:06 ` [PATCH v2] " Álvaro Fernández Rojas @ 2020-06-11 7:55 ` Miquel Raynal 2020-06-11 15:16 ` Álvaro Fernández Rojas 2020-06-12 7:35 ` [PATCH v3] " Álvaro Fernández Rojas 1 sibling, 1 reply; 28+ messages in thread From: Miquel Raynal @ 2020-06-11 7:55 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: tsbogend, f.fainelli, bcm-kernel-feedback-list, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Álvaro, Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 18:06:49 +0200: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > --- > v2: use CFE_EPTSEAL definition and avoid using an additional funtion. > > drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- > 1 file changed, 4 insertions(+), 25 deletions(-) > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > index 78f90c6c18fd..493a75b2f266 100644 > --- a/drivers/mtd/parsers/bcm63xxpart.c > +++ b/drivers/mtd/parsers/bcm63xxpart.c > @@ -22,6 +22,9 @@ > #include <linux/mtd/partitions.h> > #include <linux/of.h> > > +#include <asm/bootinfo.h> > +#include <asm/fw/cfe/cfe_api.h> Are you sure both includes are needed? I don't think it is a good habit to include asm/ headers, are you sure there is not another header doing it just fine? > + > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > @@ -32,30 +35,6 @@ > #define STR_NULL_TERMINATE(x) \ > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > -{ > - char buf[9]; > - int ret; > - size_t retlen; > - > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - if (ret) > - return ret; > - > - if (strncmp("cfe-v", buf, 5) == 0) > - return 0; > - > - /* very old CFE's do not have the cfe-v string, so check for magic */ > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - return strncmp("CFE1CFE1", buf, 8); > -} > - > static int bcm63xx_read_nvram(struct mtd_info *master, > struct bcm963xx_nvram *nvram) > { > @@ -138,7 +117,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > struct bcm963xx_nvram *nvram = NULL; > int ret; > > - if (bcm63xx_detect_cfe(master)) > + if (fw_arg3 != CFE_EPTSEAL) > return -EINVAL; > > nvram = vzalloc(sizeof(*nvram)); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-11 7:55 ` Miquel Raynal @ 2020-06-11 15:16 ` Álvaro Fernández Rojas 2020-06-11 15:42 ` Florian Fainelli 0 siblings, 1 reply; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-11 15:16 UTC (permalink / raw) To: Miquel Raynal Cc: tsbogend, Florian Fainelli, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Miquel, > El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > > Hi Álvaro, > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 > 18:06:49 +0200: > >> Instead of trying to parse CFE version string, which is customized by some >> vendors, let's just check that "CFE1" was passed on argument 3. >> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >> --- >> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. >> >> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- >> 1 file changed, 4 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >> index 78f90c6c18fd..493a75b2f266 100644 >> --- a/drivers/mtd/parsers/bcm63xxpart.c >> +++ b/drivers/mtd/parsers/bcm63xxpart.c >> @@ -22,6 +22,9 @@ >> #include <linux/mtd/partitions.h> >> #include <linux/of.h> >> >> +#include <asm/bootinfo.h> >> +#include <asm/fw/cfe/cfe_api.h> > > Are you sure both includes are needed? asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. > > I don't think it is a good habit to include asm/ headers, are you sure > there is not another header doing it just fine? Both are needed unless you want to add another definition of CFE_EPTSEAL value. There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 > >> + >> #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ >> >> #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 >> @@ -32,30 +35,6 @@ >> #define STR_NULL_TERMINATE(x) \ >> do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) >> >> -static int bcm63xx_detect_cfe(struct mtd_info *master) >> -{ >> - char buf[9]; >> - int ret; >> - size_t retlen; >> - >> - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, >> - (void *)buf); >> - buf[retlen] = 0; >> - >> - if (ret) >> - return ret; >> - >> - if (strncmp("cfe-v", buf, 5) == 0) >> - return 0; >> - >> - /* very old CFE's do not have the cfe-v string, so check for magic */ >> - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, >> - (void *)buf); >> - buf[retlen] = 0; >> - >> - return strncmp("CFE1CFE1", buf, 8); >> -} >> - >> static int bcm63xx_read_nvram(struct mtd_info *master, >> struct bcm963xx_nvram *nvram) >> { >> @@ -138,7 +117,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, >> struct bcm963xx_nvram *nvram = NULL; >> int ret; >> >> - if (bcm63xx_detect_cfe(master)) >> + if (fw_arg3 != CFE_EPTSEAL) >> return -EINVAL; >> >> nvram = vzalloc(sizeof(*nvram)); Best regards, Álvaro. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-11 15:16 ` Álvaro Fernández Rojas @ 2020-06-11 15:42 ` Florian Fainelli 2020-06-11 15:46 ` Miquel Raynal 2020-06-11 16:14 ` Álvaro Fernández Rojas 0 siblings, 2 replies; 28+ messages in thread From: Florian Fainelli @ 2020-06-11 15:42 UTC (permalink / raw) To: Álvaro Fernández Rojas, Miquel Raynal Cc: tsbogend, Florian Fainelli, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: > Hi Miquel, > >> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: >> >> Hi Álvaro, >> >> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 >> 18:06:49 +0200: >> >>> Instead of trying to parse CFE version string, which is customized by some >>> vendors, let's just check that "CFE1" was passed on argument 3. >>> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>> --- >>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. >>> >>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- >>> 1 file changed, 4 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>> index 78f90c6c18fd..493a75b2f266 100644 >>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>> @@ -22,6 +22,9 @@ >>> #include <linux/mtd/partitions.h> >>> #include <linux/of.h> >>> >>> +#include <asm/bootinfo.h> >>> +#include <asm/fw/cfe/cfe_api.h> >> >> Are you sure both includes are needed? > > asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. > >> >> I don't think it is a good habit to include asm/ headers, are you sure >> there is not another header doing it just fine? > > Both are needed unless you want to add another definition of CFE_EPTSEAL value. > There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: > https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 > https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 The caveat with that approach is that this reduces the compilation surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit small. If we could move the CFE definitions to a shared header, and consolidate the value used by bcm47xxpart.c as well, that would allow us to build the bcm63xxpart.c file with COMPILE_TEST on other architectures. This does not really have functional value, but for maintainers like Miquel, it allows them to quickly test their entire drivers/mtd/ directory. -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-11 15:42 ` Florian Fainelli @ 2020-06-11 15:46 ` Miquel Raynal 2020-06-11 16:14 ` Álvaro Fernández Rojas 1 sibling, 0 replies; 28+ messages in thread From: Miquel Raynal @ 2020-06-11 15:46 UTC (permalink / raw) To: Florian Fainelli Cc: Álvaro Fernández Rojas, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Florian Fainelli <f.fainelli@gmail.com> wrote on Thu, 11 Jun 2020 08:42:42 -0700: > On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: > > Hi Miquel, > > > >> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > >> > >> Hi Álvaro, > >> > >> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 > >> 18:06:49 +0200: > >> > >>> Instead of trying to parse CFE version string, which is customized by some > >>> vendors, let's just check that "CFE1" was passed on argument 3. > >>> > >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >>> --- > >>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. > >>> > >>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- > >>> 1 file changed, 4 insertions(+), 25 deletions(-) > >>> > >>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > >>> index 78f90c6c18fd..493a75b2f266 100644 > >>> --- a/drivers/mtd/parsers/bcm63xxpart.c > >>> +++ b/drivers/mtd/parsers/bcm63xxpart.c > >>> @@ -22,6 +22,9 @@ > >>> #include <linux/mtd/partitions.h> > >>> #include <linux/of.h> > >>> > >>> +#include <asm/bootinfo.h> > >>> +#include <asm/fw/cfe/cfe_api.h> > >> > >> Are you sure both includes are needed? > > > > asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. > > > >> > >> I don't think it is a good habit to include asm/ headers, are you sure > >> there is not another header doing it just fine? > > > > Both are needed unless you want to add another definition of CFE_EPTSEAL value. > > There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: > > https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 > > https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 > > The caveat with that approach is that this reduces the compilation > surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit > small. If we could move the CFE definitions to a shared header, and > consolidate the value used by bcm47xxpart.c as well, that would allow us > to build the bcm63xxpart.c file with COMPILE_TEST on other > architectures. This does not really have functional value, but for > maintainers like Miquel, it allows them to quickly test their entire > drivers/mtd/ directory. Absolutely! ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-11 15:42 ` Florian Fainelli 2020-06-11 15:46 ` Miquel Raynal @ 2020-06-11 16:14 ` Álvaro Fernández Rojas 2020-06-12 7:02 ` Miquel Raynal 1 sibling, 1 reply; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-11 16:14 UTC (permalink / raw) To: Florian Fainelli Cc: Miquel Raynal, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Florian, > El 11 jun 2020, a las 17:42, Florian Fainelli <f.fainelli@gmail.com> escribió: > > > > On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: >> Hi Miquel, >> >>> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: >>> >>> Hi Álvaro, >>> >>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 >>> 18:06:49 +0200: >>> >>>> Instead of trying to parse CFE version string, which is customized by some >>>> vendors, let's just check that "CFE1" was passed on argument 3. >>>> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>>> --- >>>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. >>>> >>>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- >>>> 1 file changed, 4 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>>> index 78f90c6c18fd..493a75b2f266 100644 >>>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>>> @@ -22,6 +22,9 @@ >>>> #include <linux/mtd/partitions.h> >>>> #include <linux/of.h> >>>> >>>> +#include <asm/bootinfo.h> >>>> +#include <asm/fw/cfe/cfe_api.h> >>> >>> Are you sure both includes are needed? >> >> asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. >> >>> >>> I don't think it is a good habit to include asm/ headers, are you sure >>> there is not another header doing it just fine? >> >> Both are needed unless you want to add another definition of CFE_EPTSEAL value. >> There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: >> https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 >> https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 > > The caveat with that approach is that this reduces the compilation > surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit > small. If we could move the CFE definitions to a shared header, and > consolidate the value used by bcm47xxpart.c as well, that would allow us > to build the bcm63xxpart.c file with COMPILE_TEST on other > architectures. This does not really have functional value, but for > maintainers like Miquel, it allows them to quickly test their entire > drivers/mtd/ directory. I don’t think fw_arg3 available on non mips archs, is it? I’m happy to move it to a shared header (which would be a good location for this?), but if I’m right it would still be restricted to MIPS. > -- > Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-11 16:14 ` Álvaro Fernández Rojas @ 2020-06-12 7:02 ` Miquel Raynal 2020-06-12 7:30 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 28+ messages in thread From: Miquel Raynal @ 2020-06-12 7:02 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: Florian Fainelli, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Álvaro, Álvaro Fernández Rojas <noltari@gmail.com> wrote on Thu, 11 Jun 2020 18:14:20 +0200: > Hi Florian, > > > El 11 jun 2020, a las 17:42, Florian Fainelli <f.fainelli@gmail.com> escribió: > > > > > > > > On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: > >> Hi Miquel, > >> > >>> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > >>> > >>> Hi Álvaro, > >>> > >>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 > >>> 18:06:49 +0200: > >>> > >>>> Instead of trying to parse CFE version string, which is customized by some > >>>> vendors, let's just check that "CFE1" was passed on argument 3. > >>>> > >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >>>> --- > >>>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. > >>>> > >>>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- > >>>> 1 file changed, 4 insertions(+), 25 deletions(-) > >>>> > >>>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > >>>> index 78f90c6c18fd..493a75b2f266 100644 > >>>> --- a/drivers/mtd/parsers/bcm63xxpart.c > >>>> +++ b/drivers/mtd/parsers/bcm63xxpart.c > >>>> @@ -22,6 +22,9 @@ > >>>> #include <linux/mtd/partitions.h> > >>>> #include <linux/of.h> > >>>> > >>>> +#include <asm/bootinfo.h> > >>>> +#include <asm/fw/cfe/cfe_api.h> > >>> > >>> Are you sure both includes are needed? > >> > >> asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. > >> > >>> > >>> I don't think it is a good habit to include asm/ headers, are you sure > >>> there is not another header doing it just fine? > >> > >> Both are needed unless you want to add another definition of CFE_EPTSEAL value. > >> There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: > >> https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 > >> https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 > > > > The caveat with that approach is that this reduces the compilation > > surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit > > small. If we could move the CFE definitions to a shared header, and > > consolidate the value used by bcm47xxpart.c as well, that would allow us > > to build the bcm63xxpart.c file with COMPILE_TEST on other > > architectures. This does not really have functional value, but for > > maintainers like Miquel, it allows them to quickly test their entire > > drivers/mtd/ directory. > > I don’t think fw_arg3 available on non mips archs, is it? > I’m happy to move it to a shared header (which would be a good location for this?), but if I’m right it would still be restricted to MIPS. Restricting a definition to MIPS, even if it makes sense for you is very limiting for me. I need to be able to build as much drivers as I can from my laptop and verify they at least compile correctly. If I need a MIPS toolchain, an ARC toolchain, a PowerPC, an ARM, an ARM64 and whatever other funky toolchain to do just that in X steps: it's very painful. We have been adding COMPILE_TEST dependencies on as much drivers as we could and we want to continue moving forward. Using such include would need to drop the COMPILE_TEST condition from Kconfig and this is not something I am willing to do. Thanks for your understanding :) Miquèl ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-12 7:02 ` Miquel Raynal @ 2020-06-12 7:30 ` Álvaro Fernández Rojas 2020-06-12 7:33 ` Miquel Raynal 0 siblings, 1 reply; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-12 7:30 UTC (permalink / raw) To: Miquel Raynal Cc: Florian Fainelli, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Miquèl, > El 12 jun 2020, a las 9:02, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > > Hi Álvaro, > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Thu, 11 Jun 2020 > 18:14:20 +0200: > >> Hi Florian, >> >>> El 11 jun 2020, a las 17:42, Florian Fainelli <f.fainelli@gmail.com> escribió: >>> >>> >>> >>> On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: >>>> Hi Miquel, >>>> >>>>> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: >>>>> >>>>> Hi Álvaro, >>>>> >>>>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 >>>>> 18:06:49 +0200: >>>>> >>>>>> Instead of trying to parse CFE version string, which is customized by some >>>>>> vendors, let's just check that "CFE1" was passed on argument 3. >>>>>> >>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>>>>> --- >>>>>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. >>>>>> >>>>>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- >>>>>> 1 file changed, 4 insertions(+), 25 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>>>>> index 78f90c6c18fd..493a75b2f266 100644 >>>>>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>>>>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>>>>> @@ -22,6 +22,9 @@ >>>>>> #include <linux/mtd/partitions.h> >>>>>> #include <linux/of.h> >>>>>> >>>>>> +#include <asm/bootinfo.h> >>>>>> +#include <asm/fw/cfe/cfe_api.h> >>>>> >>>>> Are you sure both includes are needed? >>>> >>>> asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. >>>> >>>>> >>>>> I don't think it is a good habit to include asm/ headers, are you sure >>>>> there is not another header doing it just fine? >>>> >>>> Both are needed unless you want to add another definition of CFE_EPTSEAL value. >>>> There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: >>>> https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 >>>> https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 >>> >>> The caveat with that approach is that this reduces the compilation >>> surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit >>> small. If we could move the CFE definitions to a shared header, and >>> consolidate the value used by bcm47xxpart.c as well, that would allow us >>> to build the bcm63xxpart.c file with COMPILE_TEST on other >>> architectures. This does not really have functional value, but for >>> maintainers like Miquel, it allows them to quickly test their entire >>> drivers/mtd/ directory. >> >> I don’t think fw_arg3 available on non mips archs, is it? >> I’m happy to move it to a shared header (which would be a good location for this?), but if I’m right it would still be restricted to MIPS. > > Restricting a definition to MIPS, even if it makes sense for you is > very limiting for me. I need to be able to build as much drivers as I > can from my laptop and verify they at least compile correctly. If I need > a MIPS toolchain, an ARC toolchain, a PowerPC, an ARM, an ARM64 and > whatever other funky toolchain to do just that in X steps: it's very > painful. We have been adding COMPILE_TEST dependencies on as much > drivers as we could and we want to continue moving forward. Using such > include would need to drop the COMPILE_TEST condition from Kconfig and > this is not something I am willing to do. I totally understand and agree with your point, but I still think that there could be a solution which would be valid for both of us. > > Thanks for your understanding :) The current way of detecting CFE isn’t the proper one. Thank you for understanding that too. > Miquèl Best regards, Álvaro. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-12 7:30 ` Álvaro Fernández Rojas @ 2020-06-12 7:33 ` Miquel Raynal 2020-06-12 7:37 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 28+ messages in thread From: Miquel Raynal @ 2020-06-12 7:33 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: Florian Fainelli, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Álvaro, Álvaro Fernández Rojas <noltari@gmail.com> wrote on Fri, 12 Jun 2020 09:30:27 +0200: > Hi Miquèl, > > > El 12 jun 2020, a las 9:02, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > > > > Hi Álvaro, > > > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Thu, 11 Jun 2020 > > 18:14:20 +0200: > > > >> Hi Florian, > >> > >>> El 11 jun 2020, a las 17:42, Florian Fainelli <f.fainelli@gmail.com> escribió: > >>> > >>> > >>> > >>> On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: > >>>> Hi Miquel, > >>>> > >>>>> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > >>>>> > >>>>> Hi Álvaro, > >>>>> > >>>>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 > >>>>> 18:06:49 +0200: > >>>>> > >>>>>> Instead of trying to parse CFE version string, which is customized by some > >>>>>> vendors, let's just check that "CFE1" was passed on argument 3. > >>>>>> > >>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >>>>>> --- > >>>>>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. > >>>>>> > >>>>>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- > >>>>>> 1 file changed, 4 insertions(+), 25 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > >>>>>> index 78f90c6c18fd..493a75b2f266 100644 > >>>>>> --- a/drivers/mtd/parsers/bcm63xxpart.c > >>>>>> +++ b/drivers/mtd/parsers/bcm63xxpart.c > >>>>>> @@ -22,6 +22,9 @@ > >>>>>> #include <linux/mtd/partitions.h> > >>>>>> #include <linux/of.h> > >>>>>> > >>>>>> +#include <asm/bootinfo.h> > >>>>>> +#include <asm/fw/cfe/cfe_api.h> > >>>>> > >>>>> Are you sure both includes are needed? > >>>> > >>>> asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. > >>>> > >>>>> > >>>>> I don't think it is a good habit to include asm/ headers, are you sure > >>>>> there is not another header doing it just fine? > >>>> > >>>> Both are needed unless you want to add another definition of CFE_EPTSEAL value. > >>>> There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: > >>>> https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 > >>>> https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 > >>> > >>> The caveat with that approach is that this reduces the compilation > >>> surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit > >>> small. If we could move the CFE definitions to a shared header, and > >>> consolidate the value used by bcm47xxpart.c as well, that would allow us > >>> to build the bcm63xxpart.c file with COMPILE_TEST on other > >>> architectures. This does not really have functional value, but for > >>> maintainers like Miquel, it allows them to quickly test their entire > >>> drivers/mtd/ directory. > >> > >> I don’t think fw_arg3 available on non mips archs, is it? > >> I’m happy to move it to a shared header (which would be a good location for this?), but if I’m right it would still be restricted to MIPS. > > > > Restricting a definition to MIPS, even if it makes sense for you is > > very limiting for me. I need to be able to build as much drivers as I > > can from my laptop and verify they at least compile correctly. If I need > > a MIPS toolchain, an ARC toolchain, a PowerPC, an ARM, an ARM64 and > > whatever other funky toolchain to do just that in X steps: it's very > > painful. We have been adding COMPILE_TEST dependencies on as much > > drivers as we could and we want to continue moving forward. Using such > > include would need to drop the COMPILE_TEST condition from Kconfig and > > this is not something I am willing to do. > > I totally understand and agree with your point, but I still think that there could be a solution which would be valid for both of us. What do you suggest? > > > > > Thanks for your understanding :) > > The current way of detecting CFE isn’t the proper one. > Thank you for understanding that too. Of course, I'm not saying I don't want this change, I'm just saying we should find another way to handle it, the below idea is totally fine by me. Thanks, Miquèl ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-12 7:33 ` Miquel Raynal @ 2020-06-12 7:37 ` Álvaro Fernández Rojas 0 siblings, 0 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-12 7:37 UTC (permalink / raw) To: Miquel Raynal Cc: Florian Fainelli, tsbogend, bcm-kernel-feedback-list, richard, vigneshr, Jonas Gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Miquèl, > El 12 jun 2020, a las 9:33, Miquel Raynal <miquel.raynal@bootlin.com> escribió: > > Hi Álvaro, > > Álvaro Fernández Rojas <noltari@gmail.com> wrote on Fri, 12 Jun 2020 > 09:30:27 +0200: > >> Hi Miquèl, >> >>> El 12 jun 2020, a las 9:02, Miquel Raynal <miquel.raynal@bootlin.com> escribió: >>> >>> Hi Álvaro, >>> >>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Thu, 11 Jun 2020 >>> 18:14:20 +0200: >>> >>>> Hi Florian, >>>> >>>>> El 11 jun 2020, a las 17:42, Florian Fainelli <f.fainelli@gmail.com> escribió: >>>>> >>>>> >>>>> >>>>> On 6/11/2020 8:16 AM, Álvaro Fernández Rojas wrote: >>>>>> Hi Miquel, >>>>>> >>>>>>> El 11 jun 2020, a las 9:55, Miquel Raynal <miquel.raynal@bootlin.com> escribió: >>>>>>> >>>>>>> Hi Álvaro, >>>>>>> >>>>>>> Álvaro Fernández Rojas <noltari@gmail.com> wrote on Mon, 8 Jun 2020 >>>>>>> 18:06:49 +0200: >>>>>>> >>>>>>>> Instead of trying to parse CFE version string, which is customized by some >>>>>>>> vendors, let's just check that "CFE1" was passed on argument 3. >>>>>>>> >>>>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>>>>>>> --- >>>>>>>> v2: use CFE_EPTSEAL definition and avoid using an additional funtion. >>>>>>>> >>>>>>>> drivers/mtd/parsers/bcm63xxpart.c | 29 ++++------------------------- >>>>>>>> 1 file changed, 4 insertions(+), 25 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>>>>>>> index 78f90c6c18fd..493a75b2f266 100644 >>>>>>>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>>>>>>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>>>>>>> @@ -22,6 +22,9 @@ >>>>>>>> #include <linux/mtd/partitions.h> >>>>>>>> #include <linux/of.h> >>>>>>>> >>>>>>>> +#include <asm/bootinfo.h> >>>>>>>> +#include <asm/fw/cfe/cfe_api.h> >>>>>>> >>>>>>> Are you sure both includes are needed? >>>>>> >>>>>> asm/bootinfo.h is needed for fw_arg3 and asm/fw/cfe/cfe_api.h is needed for CFE_EPTSEAL. >>>>>> >>>>>>> >>>>>>> I don't think it is a good habit to include asm/ headers, are you sure >>>>>>> there is not another header doing it just fine? >>>>>> >>>>>> Both are needed unless you want to add another definition of CFE_EPTSEAL value. >>>>>> There are currently two CFE magic definitions, the one in asm/fw/cfe/cfe_api.h and another one in bcm47xxpart.c: >>>>>> https://github.com/torvalds/linux/blob/master/arch/mips/include/asm/fw/cfe/cfe_api.h#L28 >>>>>> https://github.com/torvalds/linux/blob/master/drivers/mtd/parsers/bcm47xxpart.c#L33 >>>>> >>>>> The caveat with that approach is that this reduces the compilation >>>>> surface to MIPS and BMIPS_GENERIC and BCM63XX only, which is a bit >>>>> small. If we could move the CFE definitions to a shared header, and >>>>> consolidate the value used by bcm47xxpart.c as well, that would allow us >>>>> to build the bcm63xxpart.c file with COMPILE_TEST on other >>>>> architectures. This does not really have functional value, but for >>>>> maintainers like Miquel, it allows them to quickly test their entire >>>>> drivers/mtd/ directory. >>>> >>>> I don’t think fw_arg3 available on non mips archs, is it? >>>> I’m happy to move it to a shared header (which would be a good location for this?), but if I’m right it would still be restricted to MIPS. >>> >>> Restricting a definition to MIPS, even if it makes sense for you is >>> very limiting for me. I need to be able to build as much drivers as I >>> can from my laptop and verify they at least compile correctly. If I need >>> a MIPS toolchain, an ARC toolchain, a PowerPC, an ARM, an ARM64 and >>> whatever other funky toolchain to do just that in X steps: it's very >>> painful. We have been adding COMPILE_TEST dependencies on as much >>> drivers as we could and we want to continue moving forward. Using such >>> include would need to drop the COMPILE_TEST condition from Kconfig and >>> this is not something I am willing to do. >> >> I totally understand and agree with your point, but I still think that there could be a solution which would be valid for both of us. > > What do you suggest? I’ve just sent v3 with my suggestion. If this isn’t valid then I’m out of ideas... > >> >>> >>> Thanks for your understanding :) >> >> The current way of detecting CFE isn’t the proper one. >> Thank you for understanding that too. > > Of course, I'm not saying I don't want this change, I'm just saying we > should find another way to handle it, the below idea is totally fine by > me. > > > Thanks, > Miquèl Regards, Álvaro. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-08 16:06 ` [PATCH v2] " Álvaro Fernández Rojas 2020-06-11 7:55 ` Miquel Raynal @ 2020-06-12 7:35 ` Álvaro Fernández Rojas 2020-06-15 8:54 ` Miquel Raynal 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas 1 sibling, 2 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-12 7:35 UTC (permalink / raw) To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Cc: Álvaro Fernández Rojas Instead of trying to parse CFE version string, which is customized by some vendors, let's just check that "CFE1" was passed on argument 3. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- v3: keep COMPILE_TEST compatibility by adding a new function that only checks fw_arg3 when CONFIG_MIPS is defined. v2: use CFE_EPTSEAL definition and avoid using an additional function. drivers/mtd/parsers/bcm63xxpart.c | 34 +++++++++++-------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c index 78f90c6c18fd..c514c04789af 100644 --- a/drivers/mtd/parsers/bcm63xxpart.c +++ b/drivers/mtd/parsers/bcm63xxpart.c @@ -22,6 +22,11 @@ #include <linux/mtd/partitions.h> #include <linux/of.h> +#ifdef CONFIG_MIPS +#include <asm/bootinfo.h> +#include <asm/fw/cfe/cfe_api.h> +#endif /* CONFIG_MIPS */ + #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 @@ -32,28 +37,13 @@ #define STR_NULL_TERMINATE(x) \ do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) -static int bcm63xx_detect_cfe(struct mtd_info *master) +static inline int bcm63xx_detect_cfe(void) { - char buf[9]; - int ret; - size_t retlen; - - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, - (void *)buf); - buf[retlen] = 0; - - if (ret) - return ret; - - if (strncmp("cfe-v", buf, 5) == 0) - return 0; - - /* very old CFE's do not have the cfe-v string, so check for magic */ - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, - (void *)buf); - buf[retlen] = 0; - - return strncmp("CFE1CFE1", buf, 8); +#ifdef CONFIG_MIPS + return (fw_arg3 == CFE_EPTSEAL); +#else + return 0; +#endif /* CONFIG_MIPS */ } static int bcm63xx_read_nvram(struct mtd_info *master, @@ -138,7 +128,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, struct bcm963xx_nvram *nvram = NULL; int ret; - if (bcm63xx_detect_cfe(master)) + if (!bcm63xx_detect_cfe()) return -EINVAL; nvram = vzalloc(sizeof(*nvram)); -- 2.26.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-12 7:35 ` [PATCH v3] " Álvaro Fernández Rojas @ 2020-06-15 8:54 ` Miquel Raynal 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas 1 sibling, 0 replies; 28+ messages in thread From: Miquel Raynal @ 2020-06-15 8:54 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: tsbogend, f.fainelli, bcm-kernel-feedback-list, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Hi Álvaro, Álvaro Fernández Rojas <noltari@gmail.com> wrote on Fri, 12 Jun 2020 09:35:49 +0200: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > --- > v3: keep COMPILE_TEST compatibility by adding a new function that only checks > fw_arg3 when CONFIG_MIPS is defined. > v2: use CFE_EPTSEAL definition and avoid using an additional function. > > drivers/mtd/parsers/bcm63xxpart.c | 34 +++++++++++-------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > index 78f90c6c18fd..c514c04789af 100644 > --- a/drivers/mtd/parsers/bcm63xxpart.c > +++ b/drivers/mtd/parsers/bcm63xxpart.c > @@ -22,6 +22,11 @@ > #include <linux/mtd/partitions.h> > #include <linux/of.h> > > +#ifdef CONFIG_MIPS > +#include <asm/bootinfo.h> > +#include <asm/fw/cfe/cfe_api.h> > +#endif /* CONFIG_MIPS */ > + > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > @@ -32,28 +37,13 @@ > #define STR_NULL_TERMINATE(x) \ > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > +static inline int bcm63xx_detect_cfe(void) > { > - char buf[9]; > - int ret; > - size_t retlen; > - > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - if (ret) > - return ret; > - > - if (strncmp("cfe-v", buf, 5) == 0) > - return 0; > - > - /* very old CFE's do not have the cfe-v string, so check for magic */ > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - return strncmp("CFE1CFE1", buf, 8); > +#ifdef CONFIG_MIPS > + return (fw_arg3 == CFE_EPTSEAL); > +#else > + return 0; > +#endif /* CONFIG_MIPS */ What about: ret = 0; #ifdef CONFIG_MIPS ret = (fw_arg3 == CFE_EPTSEAL) #endif return ret; This is for shortening the conditional part. > } > > static int bcm63xx_read_nvram(struct mtd_info *master, > @@ -138,7 +128,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > struct bcm963xx_nvram *nvram = NULL; > int ret; > > - if (bcm63xx_detect_cfe(master)) > + if (!bcm63xx_detect_cfe()) > return -EINVAL; > > nvram = vzalloc(sizeof(*nvram)); Thanks, Miquèl ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-12 7:35 ` [PATCH v3] " Álvaro Fernández Rojas 2020-06-15 8:54 ` Miquel Raynal @ 2020-06-15 9:17 ` Álvaro Fernández Rojas 2020-06-15 16:30 ` Florian Fainelli ` (2 more replies) 1 sibling, 3 replies; 28+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-15 9:17 UTC (permalink / raw) To: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd Cc: Álvaro Fernández Rojas Instead of trying to parse CFE version string, which is customized by some vendors, let's just check that "CFE1" was passed on argument 3. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- v4: shorten conditional compilation part as suggested by Miquèl. v3: keep COMPILE_TEST compatibility by adding a new function that only checks fw_arg3 when CONFIG_MIPS is defined. v2: use CFE_EPTSEAL definition and avoid using an additional function. drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c index 78f90c6c18fd..b15bdadaedb5 100644 --- a/drivers/mtd/parsers/bcm63xxpart.c +++ b/drivers/mtd/parsers/bcm63xxpart.c @@ -22,6 +22,11 @@ #include <linux/mtd/partitions.h> #include <linux/of.h> +#ifdef CONFIG_MIPS +#include <asm/bootinfo.h> +#include <asm/fw/cfe/cfe_api.h> +#endif /* CONFIG_MIPS */ + #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 @@ -32,28 +37,15 @@ #define STR_NULL_TERMINATE(x) \ do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) -static int bcm63xx_detect_cfe(struct mtd_info *master) +static inline int bcm63xx_detect_cfe(void) { - char buf[9]; - int ret; - size_t retlen; + int ret = 0; - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, - (void *)buf); - buf[retlen] = 0; +#ifdef CONFIG_MIPS + ret = (fw_arg3 == CFE_EPTSEAL); +#endif /* CONFIG_MIPS */ - if (ret) - return ret; - - if (strncmp("cfe-v", buf, 5) == 0) - return 0; - - /* very old CFE's do not have the cfe-v string, so check for magic */ - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, - (void *)buf); - buf[retlen] = 0; - - return strncmp("CFE1CFE1", buf, 8); + return ret; } static int bcm63xx_read_nvram(struct mtd_info *master, @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, struct bcm963xx_nvram *nvram = NULL; int ret; - if (bcm63xx_detect_cfe(master)) + if (!bcm63xx_detect_cfe()) return -EINVAL; nvram = vzalloc(sizeof(*nvram)); -- 2.27.0 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas @ 2020-06-15 16:30 ` Florian Fainelli 2020-06-15 17:38 ` Miquel Raynal 2020-08-14 8:56 ` Guenter Roeck 2 siblings, 0 replies; 28+ messages in thread From: Florian Fainelli @ 2020-06-15 16:30 UTC (permalink / raw) To: Álvaro Fernández Rojas, tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd On 6/15/2020 2:17 AM, Álvaro Fernández Rojas wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas 2020-06-15 16:30 ` Florian Fainelli @ 2020-06-15 17:38 ` Miquel Raynal 2020-08-14 8:56 ` Guenter Roeck 2 siblings, 0 replies; 28+ messages in thread From: Miquel Raynal @ 2020-06-15 17:38 UTC (permalink / raw) To: Álvaro Fernández Rojas, tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd On Mon, 2020-06-15 at 09:17:40 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas 2020-06-15 16:30 ` Florian Fainelli 2020-06-15 17:38 ` Miquel Raynal @ 2020-08-14 8:56 ` Guenter Roeck 2020-09-22 3:18 ` Naresh Kamboju 2 siblings, 1 reply; 28+ messages in thread From: Guenter Roeck @ 2020-08-14 8:56 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: tsbogend, f.fainelli, bcm-kernel-feedback-list, miquel.raynal, richard, vigneshr, jonas.gorski, linus.walleij, linux-mips, linux-arm-kernel, linux-kernel, linux-mtd On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > Instead of trying to parse CFE version string, which is customized by some > vendors, let's just check that "CFE1" was passed on argument 3. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> mips:allmodconfig: ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! This is not surprising, since fw_arg3 is not exported. Guenter > --- > v4: shorten conditional compilation part as suggested by Miquèl. > v3: keep COMPILE_TEST compatibility by adding a new function that only checks > fw_arg3 when CONFIG_MIPS is defined. > v2: use CFE_EPTSEAL definition and avoid using an additional function. > > drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- > 1 file changed, 12 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > index 78f90c6c18fd..b15bdadaedb5 100644 > --- a/drivers/mtd/parsers/bcm63xxpart.c > +++ b/drivers/mtd/parsers/bcm63xxpart.c > @@ -22,6 +22,11 @@ > #include <linux/mtd/partitions.h> > #include <linux/of.h> > > +#ifdef CONFIG_MIPS > +#include <asm/bootinfo.h> > +#include <asm/fw/cfe/cfe_api.h> > +#endif /* CONFIG_MIPS */ > + > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > @@ -32,28 +37,15 @@ > #define STR_NULL_TERMINATE(x) \ > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > +static inline int bcm63xx_detect_cfe(void) > { > - char buf[9]; > - int ret; > - size_t retlen; > + int ret = 0; > > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > - (void *)buf); > - buf[retlen] = 0; > +#ifdef CONFIG_MIPS > + ret = (fw_arg3 == CFE_EPTSEAL); > +#endif /* CONFIG_MIPS */ > > - if (ret) > - return ret; > - > - if (strncmp("cfe-v", buf, 5) == 0) > - return 0; > - > - /* very old CFE's do not have the cfe-v string, so check for magic */ > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > - (void *)buf); > - buf[retlen] = 0; > - > - return strncmp("CFE1CFE1", buf, 8); > + return ret; > } > > static int bcm63xx_read_nvram(struct mtd_info *master, > @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > struct bcm963xx_nvram *nvram = NULL; > int ret; > > - if (bcm63xx_detect_cfe(master)) > + if (!bcm63xx_detect_cfe()) > return -EINVAL; > > nvram = vzalloc(sizeof(*nvram)); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-08-14 8:56 ` Guenter Roeck @ 2020-09-22 3:18 ` Naresh Kamboju 2020-09-22 3:26 ` Guenter Roeck 0 siblings, 1 reply; 28+ messages in thread From: Naresh Kamboju @ 2020-09-22 3:18 UTC (permalink / raw) To: Álvaro Fernández Rojas, linux-mips Cc: tsbogend, Florian Fainelli, bcm-kernel-feedback-list, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, jonas.gorski, Linus Walleij, Linux ARM, open list, linux-mtd, lkft-triage, Guenter Roeck On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > > Instead of trying to parse CFE version string, which is customized by some > > vendors, let's just check that "CFE1" was passed on argument 3. > > > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > We still see mips: allmodconfig build failure on Linus tree make -sk KBUILD_BUILD_USER=TuxBuild -C/linux ARCH=mips CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache mips-linux-gnu-gcc" O=build allmodconfig make -sk KBUILD_BUILD_USER=TuxBuild -C/linux -j16 ARCH=mips CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache mips-linux-gnu-gcc" O=build > mips:allmodconfig: > > ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Full build link, https://builds.tuxbuild.com/Sm59_9tjFMpIvT27qf5kNA/build.log > > This is not surprising, since fw_arg3 is not exported. > > Guenter > > > --- > > v4: shorten conditional compilation part as suggested by Miquèl. > > v3: keep COMPILE_TEST compatibility by adding a new function that only checks > > fw_arg3 when CONFIG_MIPS is defined. > > v2: use CFE_EPTSEAL definition and avoid using an additional function. > > > > drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- > > 1 file changed, 12 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c > > index 78f90c6c18fd..b15bdadaedb5 100644 > > --- a/drivers/mtd/parsers/bcm63xxpart.c > > +++ b/drivers/mtd/parsers/bcm63xxpart.c > > @@ -22,6 +22,11 @@ > > #include <linux/mtd/partitions.h> > > #include <linux/of.h> > > > > +#ifdef CONFIG_MIPS > > +#include <asm/bootinfo.h> > > +#include <asm/fw/cfe/cfe_api.h> > > +#endif /* CONFIG_MIPS */ > > + > > #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ > > > > #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 > > @@ -32,28 +37,15 @@ > > #define STR_NULL_TERMINATE(x) \ > > do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) > > > > -static int bcm63xx_detect_cfe(struct mtd_info *master) > > +static inline int bcm63xx_detect_cfe(void) > > { > > - char buf[9]; > > - int ret; > > - size_t retlen; > > + int ret = 0; > > > > - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, > > - (void *)buf); > > - buf[retlen] = 0; > > +#ifdef CONFIG_MIPS > > + ret = (fw_arg3 == CFE_EPTSEAL); > > +#endif /* CONFIG_MIPS */ > > > > - if (ret) > > - return ret; > > - > > - if (strncmp("cfe-v", buf, 5) == 0) > > - return 0; > > - > > - /* very old CFE's do not have the cfe-v string, so check for magic */ > > - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, > > - (void *)buf); > > - buf[retlen] = 0; > > - > > - return strncmp("CFE1CFE1", buf, 8); > > + return ret; > > } > > > > static int bcm63xx_read_nvram(struct mtd_info *master, > > @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, > > struct bcm963xx_nvram *nvram = NULL; > > int ret; > > > > - if (bcm63xx_detect_cfe(master)) > > + if (!bcm63xx_detect_cfe()) > > return -EINVAL; > > > > nvram = vzalloc(sizeof(*nvram)); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-09-22 3:18 ` Naresh Kamboju @ 2020-09-22 3:26 ` Guenter Roeck 2020-09-28 14:16 ` Miquel Raynal 0 siblings, 1 reply; 28+ messages in thread From: Guenter Roeck @ 2020-09-22 3:26 UTC (permalink / raw) To: Naresh Kamboju, Álvaro Fernández Rojas, linux-mips Cc: tsbogend, Florian Fainelli, bcm-kernel-feedback-list, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, jonas.gorski, Linus Walleij, Linux ARM, open list, linux-mtd, lkft-triage On 9/21/20 8:18 PM, Naresh Kamboju wrote: > On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: >>> Instead of trying to parse CFE version string, which is customized by some >>> vendors, let's just check that "CFE1" was passed on argument 3. >>> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> > > We still see mips: allmodconfig build failure on Linus tree > Yes, same here. Guenter > make -sk KBUILD_BUILD_USER=TuxBuild -C/linux ARCH=mips > CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache > mips-linux-gnu-gcc" O=build allmodconfig > > make -sk KBUILD_BUILD_USER=TuxBuild -C/linux -j16 ARCH=mips > CROSS_COMPILE=mips-linux-gnu- HOSTCC=gcc CC="sccache > mips-linux-gnu-gcc" O=build > > >> mips:allmodconfig: >> >> ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! > > ERROR: modpost: "fw_arg3" [drivers/mtd/parsers/bcm63xxpart.ko] undefined! > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > > Full build link, > https://builds.tuxbuild.com/Sm59_9tjFMpIvT27qf5kNA/build.log > >> >> This is not surprising, since fw_arg3 is not exported. >> >> Guenter >> >>> --- >>> v4: shorten conditional compilation part as suggested by Miquèl. >>> v3: keep COMPILE_TEST compatibility by adding a new function that only checks >>> fw_arg3 when CONFIG_MIPS is defined. >>> v2: use CFE_EPTSEAL definition and avoid using an additional function. >>> >>> drivers/mtd/parsers/bcm63xxpart.c | 32 ++++++++++++------------------- >>> 1 file changed, 12 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/mtd/parsers/bcm63xxpart.c b/drivers/mtd/parsers/bcm63xxpart.c >>> index 78f90c6c18fd..b15bdadaedb5 100644 >>> --- a/drivers/mtd/parsers/bcm63xxpart.c >>> +++ b/drivers/mtd/parsers/bcm63xxpart.c >>> @@ -22,6 +22,11 @@ >>> #include <linux/mtd/partitions.h> >>> #include <linux/of.h> >>> >>> +#ifdef CONFIG_MIPS >>> +#include <asm/bootinfo.h> >>> +#include <asm/fw/cfe/cfe_api.h> >>> +#endif /* CONFIG_MIPS */ >>> + >>> #define BCM963XX_CFE_BLOCK_SIZE SZ_64K /* always at least 64KiB */ >>> >>> #define BCM963XX_CFE_MAGIC_OFFSET 0x4e0 >>> @@ -32,28 +37,15 @@ >>> #define STR_NULL_TERMINATE(x) \ >>> do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0) >>> >>> -static int bcm63xx_detect_cfe(struct mtd_info *master) >>> +static inline int bcm63xx_detect_cfe(void) >>> { >>> - char buf[9]; >>> - int ret; >>> - size_t retlen; >>> + int ret = 0; >>> >>> - ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen, >>> - (void *)buf); >>> - buf[retlen] = 0; >>> +#ifdef CONFIG_MIPS >>> + ret = (fw_arg3 == CFE_EPTSEAL); >>> +#endif /* CONFIG_MIPS */ >>> >>> - if (ret) >>> - return ret; >>> - >>> - if (strncmp("cfe-v", buf, 5) == 0) >>> - return 0; >>> - >>> - /* very old CFE's do not have the cfe-v string, so check for magic */ >>> - ret = mtd_read(master, BCM963XX_CFE_MAGIC_OFFSET, 8, &retlen, >>> - (void *)buf); >>> - buf[retlen] = 0; >>> - >>> - return strncmp("CFE1CFE1", buf, 8); >>> + return ret; >>> } >>> >>> static int bcm63xx_read_nvram(struct mtd_info *master, >>> @@ -138,7 +130,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master, >>> struct bcm963xx_nvram *nvram = NULL; >>> int ret; >>> >>> - if (bcm63xx_detect_cfe(master)) >>> + if (!bcm63xx_detect_cfe()) >>> return -EINVAL; >>> >>> nvram = vzalloc(sizeof(*nvram)); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-09-22 3:26 ` Guenter Roeck @ 2020-09-28 14:16 ` Miquel Raynal 2020-09-28 19:35 ` Florian Fainelli 2020-09-29 17:27 ` [PATCH] mtd: parsers: bcm63xx: Do not make it modular Florian Fainelli 0 siblings, 2 replies; 28+ messages in thread From: Miquel Raynal @ 2020-09-28 14:16 UTC (permalink / raw) To: Guenter Roeck Cc: Naresh Kamboju, Álvaro Fernández Rojas, linux-mips, tsbogend, Florian Fainelli, bcm-kernel-feedback-list, Richard Weinberger, Vignesh Raghavendra, jonas.gorski, Linus Walleij, Linux ARM, open list, linux-mtd, lkft-triage Hello, Guenter Roeck <linux@roeck-us.net> wrote on Mon, 21 Sep 2020 20:26:19 -0700: > On 9/21/20 8:18 PM, Naresh Kamboju wrote: > > On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: > >>> Instead of trying to parse CFE version string, which is customized by some > >>> vendors, let's just check that "CFE1" was passed on argument 3. > >>> > >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > >> > > > > We still see mips: allmodconfig build failure on Linus tree > > > > Yes, same here. Perhaps this check should be done by an exported helper so that we do not blindly export the variable? Alvaro, Jonas, can one of you try to address this issue please? Thanks, Miquèl ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection 2020-09-28 14:16 ` Miquel Raynal @ 2020-09-28 19:35 ` Florian Fainelli 2020-09-29 17:27 ` [PATCH] mtd: parsers: bcm63xx: Do not make it modular Florian Fainelli 1 sibling, 0 replies; 28+ messages in thread From: Florian Fainelli @ 2020-09-28 19:35 UTC (permalink / raw) To: Miquel Raynal, Guenter Roeck Cc: Naresh Kamboju, Álvaro Fernández Rojas, linux-mips, tsbogend, bcm-kernel-feedback-list, Richard Weinberger, Vignesh Raghavendra, jonas.gorski, Linus Walleij, Linux ARM, open list, linux-mtd, lkft-triage On 9/28/2020 7:16 AM, Miquel Raynal wrote: > Hello, > > Guenter Roeck <linux@roeck-us.net> wrote on Mon, 21 Sep 2020 20:26:19 > -0700: > >> On 9/21/20 8:18 PM, Naresh Kamboju wrote: >>> On Fri, 14 Aug 2020 at 14:26, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On Mon, Jun 15, 2020 at 11:17:40AM +0200, Álvaro Fernández Rojas wrote: >>>>> Instead of trying to parse CFE version string, which is customized by some >>>>> vendors, let's just check that "CFE1" was passed on argument 3. >>>>> >>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >>>>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >>>> >>> >>> We still see mips: allmodconfig build failure on Linus tree >>> >> >> Yes, same here. > > Perhaps this check should be done by an exported helper so that we do > not blindly export the variable? > > Alvaro, Jonas, can one of you try to address this issue please? We should probably just make the parser 'bool' there is no much point building this as a module, it will not improve compile time coverage, and it will most likely not help with the kernel image size on the platforms that require the parser to be there anyway. -- Florian ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] mtd: parsers: bcm63xx: Do not make it modular 2020-09-28 14:16 ` Miquel Raynal 2020-09-28 19:35 ` Florian Fainelli @ 2020-09-29 17:27 ` Florian Fainelli 2020-10-02 7:15 ` Miquel Raynal 2020-10-11 14:14 ` Guenter Roeck 1 sibling, 2 replies; 28+ messages in thread From: Florian Fainelli @ 2020-09-29 17:27 UTC (permalink / raw) To: linux-mtd Cc: Florian Fainelli, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Álvaro Fernández Rojas, Jonas Gorski, open list With commit 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection"), we generate a reference to fw_arg3 which is the fourth firmware/command line argument on MIPS platforms. That symbol is not exported and would cause a linking failure. The parser is typically necessary to boot a BCM63xx-based system anyway so having it be part of the kernel image makes sense, therefore make it 'bool' instead of 'tristate'. Fixes: 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/mtd/parsers/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig index f98363c9b363..e72354322f62 100644 --- a/drivers/mtd/parsers/Kconfig +++ b/drivers/mtd/parsers/Kconfig @@ -12,7 +12,7 @@ config MTD_BCM47XX_PARTS boards. config MTD_BCM63XX_PARTS - tristate "BCM63XX CFE partitioning parser" + bool "BCM63XX CFE partitioning parser" depends on BCM63XX || BMIPS_GENERIC || COMPILE_TEST select CRC32 select MTD_PARSER_IMAGETAG -- 2.25.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular 2020-09-29 17:27 ` [PATCH] mtd: parsers: bcm63xx: Do not make it modular Florian Fainelli @ 2020-10-02 7:15 ` Miquel Raynal 2020-10-11 14:14 ` Guenter Roeck 1 sibling, 0 replies; 28+ messages in thread From: Miquel Raynal @ 2020-10-02 7:15 UTC (permalink / raw) To: Florian Fainelli, linux-mtd Cc: Miquel Raynal, Vignesh Raghavendra, Richard Weinberger, open list, Álvaro Fernández Rojas, Jonas Gorski On Tue, 2020-09-29 at 17:27:21 UTC, Florian Fainelli wrote: > With commit 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE > detection"), we generate a reference to fw_arg3 which is the fourth > firmware/command line argument on MIPS platforms. That symbol is not > exported and would cause a linking failure. > > The parser is typically necessary to boot a BCM63xx-based system anyway > so having it be part of the kernel image makes sense, therefore make it > 'bool' instead of 'tristate'. > > Fixes: 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular 2020-09-29 17:27 ` [PATCH] mtd: parsers: bcm63xx: Do not make it modular Florian Fainelli 2020-10-02 7:15 ` Miquel Raynal @ 2020-10-11 14:14 ` Guenter Roeck 2020-10-12 7:04 ` Miquel Raynal 1 sibling, 1 reply; 28+ messages in thread From: Guenter Roeck @ 2020-10-11 14:14 UTC (permalink / raw) To: Florian Fainelli Cc: linux-mtd, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Álvaro Fernández Rojas, Jonas Gorski, open list On Tue, Sep 29, 2020 at 10:27:21AM -0700, Florian Fainelli wrote: > With commit 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE > detection"), we generate a reference to fw_arg3 which is the fourth > firmware/command line argument on MIPS platforms. That symbol is not > exported and would cause a linking failure. > > The parser is typically necessary to boot a BCM63xx-based system anyway > so having it be part of the kernel image makes sense, therefore make it > 'bool' instead of 'tristate'. > > Fixes: 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> What happened with this patch ? The build failure is still seen in mainline and in next-20201009. Guenter > --- > drivers/mtd/parsers/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig > index f98363c9b363..e72354322f62 100644 > --- a/drivers/mtd/parsers/Kconfig > +++ b/drivers/mtd/parsers/Kconfig > @@ -12,7 +12,7 @@ config MTD_BCM47XX_PARTS > boards. > > config MTD_BCM63XX_PARTS > - tristate "BCM63XX CFE partitioning parser" > + bool "BCM63XX CFE partitioning parser" > depends on BCM63XX || BMIPS_GENERIC || COMPILE_TEST > select CRC32 > select MTD_PARSER_IMAGETAG > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular 2020-10-11 14:14 ` Guenter Roeck @ 2020-10-12 7:04 ` Miquel Raynal 2020-10-12 13:24 ` Guenter Roeck 0 siblings, 1 reply; 28+ messages in thread From: Miquel Raynal @ 2020-10-12 7:04 UTC (permalink / raw) To: Guenter Roeck Cc: Florian Fainelli, linux-mtd, Richard Weinberger, Vignesh Raghavendra, Álvaro Fernández Rojas, Jonas Gorski, open list Hi Guenter, Guenter Roeck <linux@roeck-us.net> wrote on Sun, 11 Oct 2020 07:14:47 -0700: > On Tue, Sep 29, 2020 at 10:27:21AM -0700, Florian Fainelli wrote: > > With commit 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE > > detection"), we generate a reference to fw_arg3 which is the fourth > > firmware/command line argument on MIPS platforms. That symbol is not > > exported and would cause a linking failure. > > > > The parser is typically necessary to boot a BCM63xx-based system anyway > > so having it be part of the kernel image makes sense, therefore make it > > 'bool' instead of 'tristate'. > > > > Fixes: 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection") > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > What happened with this patch ? The build failure is still seen in mainline > and in next-20201009. It has been applied on mtd/next: https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd/next (I don't remember when though) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular 2020-10-12 7:04 ` Miquel Raynal @ 2020-10-12 13:24 ` Guenter Roeck 0 siblings, 0 replies; 28+ messages in thread From: Guenter Roeck @ 2020-10-12 13:24 UTC (permalink / raw) To: Miquel Raynal Cc: Florian Fainelli, linux-mtd, Richard Weinberger, Vignesh Raghavendra, Álvaro Fernández Rojas, Jonas Gorski, open list On Mon, Oct 12, 2020 at 09:04:20AM +0200, Miquel Raynal wrote: > Hi Guenter, > > Guenter Roeck <linux@roeck-us.net> wrote on Sun, 11 Oct 2020 07:14:47 > -0700: > > > On Tue, Sep 29, 2020 at 10:27:21AM -0700, Florian Fainelli wrote: > > > With commit 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE > > > detection"), we generate a reference to fw_arg3 which is the fourth > > > firmware/command line argument on MIPS platforms. That symbol is not > > > exported and would cause a linking failure. > > > > > > The parser is typically necessary to boot a BCM63xx-based system anyway > > > so having it be part of the kernel image makes sense, therefore make it > > > 'bool' instead of 'tristate'. > > > > > > Fixes: 91e81150d388 ("mtd: parsers: bcm63xx: simplify CFE detection") > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > > What happened with this patch ? The build failure is still seen in mainline > > and in next-20201009. > > It has been applied on mtd/next: > https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd/next > (I don't remember when though) Hmm, lets see ... Ah, I see: mips:allmodconfig now fails to build in -next for a different reason. Error log: In file included from <command-line>: arch/mips/mm/init.c: In function 'mem_init': include/linux/compiler_types.h:319:38: error: call to '__compiletime_assert_331' declared with attribute error: BUILD_BUG_ON failed: IS_ENABLED(CONFIG_32BIT) && (_PFN_SHIFT > PAGE_SHIFT) I'll send a separate report on that (if it wasn't reported before). Anyway, any reason for not applying this fix to mainline, ie why the mips:allmodconfig build failure was not fixed in v5.9 ? Guenter ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-10-12 13:24 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-08 9:40 [PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE Álvaro Fernández Rojas 2020-06-08 9:40 ` [PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection Álvaro Fernández Rojas 2020-06-08 16:06 ` [PATCH v2] " Álvaro Fernández Rojas 2020-06-11 7:55 ` Miquel Raynal 2020-06-11 15:16 ` Álvaro Fernández Rojas 2020-06-11 15:42 ` Florian Fainelli 2020-06-11 15:46 ` Miquel Raynal 2020-06-11 16:14 ` Álvaro Fernández Rojas 2020-06-12 7:02 ` Miquel Raynal 2020-06-12 7:30 ` Álvaro Fernández Rojas 2020-06-12 7:33 ` Miquel Raynal 2020-06-12 7:37 ` Álvaro Fernández Rojas 2020-06-12 7:35 ` [PATCH v3] " Álvaro Fernández Rojas 2020-06-15 8:54 ` Miquel Raynal 2020-06-15 9:17 ` [PATCH v4] " Álvaro Fernández Rojas 2020-06-15 16:30 ` Florian Fainelli 2020-06-15 17:38 ` Miquel Raynal 2020-08-14 8:56 ` Guenter Roeck 2020-09-22 3:18 ` Naresh Kamboju 2020-09-22 3:26 ` Guenter Roeck 2020-09-28 14:16 ` Miquel Raynal 2020-09-28 19:35 ` Florian Fainelli 2020-09-29 17:27 ` [PATCH] mtd: parsers: bcm63xx: Do not make it modular Florian Fainelli 2020-10-02 7:15 ` Miquel Raynal 2020-10-11 14:14 ` Guenter Roeck 2020-10-12 7:04 ` Miquel Raynal 2020-10-12 13:24 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).