Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[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: f.fainelli, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, jonas.gorski,
	linux-mtd, linux-arm-kernel

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));

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Florian Fainelli, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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.


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Florian Fainelli, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel



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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, vigneshr, richard,
	linus.walleij, linux-mips, linux-kernel, tsbogend,
	bcm-kernel-feedback-list, Jonas Gorski, linux-mtd,
	linux-arm-kernel


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!

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: tsbogend, vigneshr, richard, linus.walleij, Miquel Raynal,
	linux-mips, linux-kernel, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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.



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, Jonas Gorski,
	linux-mtd, linux-arm-kernel

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.


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[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: f.fainelli, vigneshr, richard, linus.walleij, linux-mips,
	linux-kernel, tsbogend, bcm-kernel-feedback-list, jonas.gorski,
	linux-mtd, linux-arm-kernel

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: f.fainelli, vigneshr, richard, linus.walleij, jonas.gorski,
	linux-mips, linux-kernel, tsbogend, bcm-kernel-feedback-list,
	miquel.raynal, linux-mtd, linux-arm-kernel

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));

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Florian Fainelli, Vignesh Raghavendra, Richard Weinberger,
	Linus Walleij, jonas.gorski, open list, lkft-triage, tsbogend,
	bcm-kernel-feedback-list, Guenter Roeck, Miquel Raynal,
	linux-mtd, Linux ARM

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));

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Florian Fainelli, Vignesh Raghavendra, Richard Weinberger,
	Linus Walleij, jonas.gorski, open list, lkft-triage, tsbogend,
	bcm-kernel-feedback-list, Miquel Raynal, linux-mtd, Linux ARM

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));


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Florian Fainelli, Vignesh Raghavendra, Richard Weinberger,
	Naresh Kamboju, linux-mips, lkft-triage, open list,
	Álvaro Fernández Rojas, bcm-kernel-feedback-list,
	jonas.gorski, linux-mtd, tsbogend, Linus Walleij, Linux ARM

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Álvaro Fernández Rojas, Vignesh Raghavendra,
	Richard Weinberger, Naresh Kamboju, linux-mips, lkft-triage,
	open list, tsbogend, bcm-kernel-feedback-list, jonas.gorski,
	linux-mtd, Linus Walleij, Linux ARM



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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, Vignesh Raghavendra, Richard Weinberger,
	Miquel Raynal, open list, Álvaro Fernández Rojas,
	Jonas Gorski

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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
  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: Álvaro Fernández Rojas, Vignesh Raghavendra,
	Richard Weinberger, Miquel Raynal, open list, 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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: Álvaro Fernández Rojas, Vignesh Raghavendra,
	Richard Weinberger, Miquel Raynal, open list, linux-mtd,
	Jonas Gorski

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
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, Vignesh Raghavendra, Richard Weinberger,
	open list, Álvaro Fernández Rojas, linux-mtd,
	Jonas Gorski

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)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ 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, Vignesh Raghavendra, Richard Weinberger,
	open list, Álvaro Fernández Rojas, linux-mtd,
	Jonas Gorski

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, back to index

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

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git