| Message ID | 20200608160649.3717152-1-noltari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mtd: parsers: bcm63xx: simplify CFE detection | expand |
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));
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.
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 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!
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
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
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.
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
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.
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));