[PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection

[PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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

[PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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/

[PATCH 0/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE

[Á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

[PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE

[Á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/

[PATCH 1/2] MIPS: BCM63xx: add helper function to detect CFE

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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

[PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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/

[PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] mtd: parsers: bcm63xx: simplify CFE detection

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

Hi "Álvaro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.7 next-20200605]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/lvaro-Fern-ndez-Rojas/mtd-parsers-bcm63xx-simplify-CFE-detection/20200608-174326
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git af7b4801030c07637840191c69eb666917e4135d
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/mtd/parsers/bcm63xxpart.c:25:10: fatal error: 'asm/mach-bcm63xx/bcm63xx_board.h' file not found
#include <asm/mach-bcm63xx/bcm63xx_board.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

vim +25 drivers/mtd/parsers/bcm63xxpart.c

    24	
  > 25	#include <asm/mach-bcm63xx/bcm63xx_board.h>
    26	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 74578 bytes --]

[PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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

[PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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/

[PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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.

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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!

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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.

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Á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

[PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Á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/

[PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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.

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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/

Re: [PATCH v2] mtd: parsers: bcm63xx: simplify CFE detection

[Álvaro Fernández Rojas]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v3] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Á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

[PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Á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/

[PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Á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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Naresh Kamboju]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Naresh Kamboju]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Naresh Kamboju]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Guenter Roeck]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Miquel Raynal]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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/

Re: [PATCH v4] mtd: parsers: bcm63xx: simplify CFE detection

[Florian Fainelli]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Florian Fainelli]

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

[PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Florian Fainelli]

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/

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Miquel Raynal]

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

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Miquel Raynal]

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/

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Guenter Roeck]

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
> 

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Guenter Roeck]

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/

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Miquel Raynal]

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)

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Miquel Raynal]

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/

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Guenter Roeck]

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

Re: [PATCH] mtd: parsers: bcm63xx: Do not make it modular

[Guenter Roeck]

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/