| Message ID | 20200121174935.hclxmljpm3nefybp@sapphire.tkos.co.il |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 2020-01-21 10:49, Baruch Siach wrote: > Hi Joel, > > On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote: >> Add a unique entry for ClearFog Base variant, reflected in the board >> name and adjusted SerDes topology. >> >> Signed-off-by: Joel Johnson <mrjoel at lixil.net> >> >> --- >> >> v2 changes: >> - reworked based on Baruch's run-time TLV EEPROM detection series >> v3 changes: >> - rebased on mvebu merged run-time TLV EEPROM detection series >> - minor update to help test regarding runtime detection failures >> >> --- >> arch/arm/mach-mvebu/Kconfig | 2 ++ >> board/solidrun/clearfog/Kconfig | 18 ++++++++++++++++++ >> board/solidrun/clearfog/clearfog.c | 12 ++++++++++++ >> 3 files changed, 32 insertions(+) >> create mode 100644 board/solidrun/clearfog/Kconfig >> >> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig >> index bc5eaa5a76..161dee937f 100644 >> --- a/arch/arm/mach-mvebu/Kconfig >> +++ b/arch/arm/mach-mvebu/Kconfig >> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX >> default 0 >> depends on SECURED_MODE_IMAGE >> >> +source "board/solidrun/clearfog/Kconfig" >> + >> endif >> diff --git a/board/solidrun/clearfog/Kconfig >> b/board/solidrun/clearfog/Kconfig >> new file mode 100644 >> index 0000000000..936d5918f8 >> --- /dev/null >> +++ b/board/solidrun/clearfog/Kconfig >> @@ -0,0 +1,18 @@ >> +menu "ClearFog configuration" >> + depends on TARGET_CLEARFOG >> + >> +config TARGET_CLEARFOG_BASE >> + bool "Use ClearFog Base static configuration" >> + help >> + Use the ClearFog Base as the static configuration instead of the >> + default which uses the ClearFog Pro. >> + >> + Runtime board detection is always attempted and used if available. >> The >> + static configuration is used as a fallback in cases where runtime >> + detection is disabled, is not available in hardware, or otherwise >> fails. >> + >> + Only newer revisions of the ClearFog product line support runtime >> + detection via additional EEPROM hardware. This option enables >> selecting >> + the Base variant for older hardware revisions. >> + >> +endmenu >> diff --git a/board/solidrun/clearfog/clearfog.c >> b/board/solidrun/clearfog/clearfog.c >> index e268ef55a2..e77b9465d4 100644 >> --- a/board/solidrun/clearfog/clearfog.c >> +++ b/board/solidrun/clearfog/clearfog.c >> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = { >> {SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >> {PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >> {USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >> +#if defined (CONFIG_TARGET_CLEARFOG_BASE) >> + {USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >> +#else >> {PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >> +#endif > > I'd prefer run-time test instead of '#ifdefs' that IMO make the code > harder to > read. Something like this (build tested only): > > diff --git a/board/solidrun/clearfog/clearfog.c > b/board/solidrun/clearfog/clearfog.c > index e268ef55a2a0..202c60cb7841 100644 > --- a/board/solidrun/clearfog/clearfog.c > +++ b/board/solidrun/clearfog/clearfog.c > @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map > **serdes_map_array, u8 *count) > { > cf_read_tlv_data(); > > - if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) { > + if (sr_product_is(&cf_tlv_data, "Clearfog GTR") || > + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) { > board_serdes_map[0].serdes_type = PEX0; > board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS; > board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1; > @@ -172,6 +173,9 @@ int checkboard(void) > { > char *board = "ClearFog"; > > + if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) > + board = "ClearFog Base"; > + > cf_read_tlv_data(); > if (strlen(cf_tlv_data.tlv_product_name[0]) > 0) > board = cf_tlv_data.tlv_product_name[0]; > @@ -194,7 +198,8 @@ int board_late_init(void) > { > cf_read_tlv_data(); > > - if (sr_product_is(&cf_tlv_data, "Clearfog Base")) > + if (sr_product_is(&cf_tlv_data, "Clearfog Base") || > + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) > env_set("fdtfile", "armada-388-clearfog-base.dtb"); > else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4")) > env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb"); > > What do you think? > > baruch I'll have to revisit and test to be sure, but I believe the reason I didn't go that route is because I wasn't able to get the CONFIG_IS_ENABLED macro version to trigger when building SPL, although it worked for the main path. I know that was the case for something I was working up, but I don't recall whether it was this patch specifically or not. A bit of a nit, but just using the macro isn't really runtime detection, however adding the separate code path for naming and serdes manipulation dynamically tends that way. I'm already running into several cases (not the default build options) where the SPL doesn't fit in the default 128K offset size, so I'm leery of adding paths that add actual binary size increase. I'm all for switching to CONFIG_IS_ENABLED if I test and can get it to work, but still would lean towards just replacing the ifdef in place; after all it is meant to be the static configuration, not dynamic. Joel
Hi Joel, On Wed, Jan 22 2020, Joel Johnson wrote: > On 2020-01-21 10:49, Baruch Siach wrote: >> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote: >>> Add a unique entry for ClearFog Base variant, reflected in the board >>> name and adjusted SerDes topology. >>> >>> Signed-off-by: Joel Johnson <mrjoel at lixil.net> >>> >>> --- >>> >>> v2 changes: >>> - reworked based on Baruch's run-time TLV EEPROM detection series >>> v3 changes: >>> - rebased on mvebu merged run-time TLV EEPROM detection series >>> - minor update to help test regarding runtime detection failures >>> >>> --- >>> arch/arm/mach-mvebu/Kconfig | 2 ++ >>> board/solidrun/clearfog/Kconfig | 18 ++++++++++++++++++ >>> board/solidrun/clearfog/clearfog.c | 12 ++++++++++++ >>> 3 files changed, 32 insertions(+) >>> create mode 100644 board/solidrun/clearfog/Kconfig >>> >>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig >>> index bc5eaa5a76..161dee937f 100644 >>> --- a/arch/arm/mach-mvebu/Kconfig >>> +++ b/arch/arm/mach-mvebu/Kconfig >>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX >>> default 0 >>> depends on SECURED_MODE_IMAGE >>> >>> +source "board/solidrun/clearfog/Kconfig" >>> + >>> endif >>> diff --git a/board/solidrun/clearfog/Kconfig >>> b/board/solidrun/clearfog/Kconfig >>> new file mode 100644 >>> index 0000000000..936d5918f8 >>> --- /dev/null >>> +++ b/board/solidrun/clearfog/Kconfig >>> @@ -0,0 +1,18 @@ >>> +menu "ClearFog configuration" >>> + depends on TARGET_CLEARFOG >>> + >>> +config TARGET_CLEARFOG_BASE >>> + bool "Use ClearFog Base static configuration" >>> + help >>> + Use the ClearFog Base as the static configuration instead of the >>> + default which uses the ClearFog Pro. >>> + >>> + Runtime board detection is always attempted and used if >>> available. The >>> + static configuration is used as a fallback in cases where runtime >>> + detection is disabled, is not available in hardware, or otherwise >>> fails. >>> + >>> + Only newer revisions of the ClearFog product line support runtime >>> + detection via additional EEPROM hardware. This option enables >>> selecting >>> + the Base variant for older hardware revisions. >>> + >>> +endmenu >>> diff --git a/board/solidrun/clearfog/clearfog.c >>> b/board/solidrun/clearfog/clearfog.c >>> index e268ef55a2..e77b9465d4 100644 >>> --- a/board/solidrun/clearfog/clearfog.c >>> +++ b/board/solidrun/clearfog/clearfog.c >>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = { >>> {SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>> {PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >>> {USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE) >>> + {USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>> +#else >>> {PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >>> +#endif >> >> I'd prefer run-time test instead of '#ifdefs' that IMO make the code harder >> to >> read. Something like this (build tested only): >> >> diff --git a/board/solidrun/clearfog/clearfog.c >> b/board/solidrun/clearfog/clearfog.c >> index e268ef55a2a0..202c60cb7841 100644 >> --- a/board/solidrun/clearfog/clearfog.c >> +++ b/board/solidrun/clearfog/clearfog.c >> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map >> **serdes_map_array, u8 *count) >> { >> cf_read_tlv_data(); >> >> - if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) { >> + if (sr_product_is(&cf_tlv_data, "Clearfog GTR") || >> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) { >> board_serdes_map[0].serdes_type = PEX0; >> board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS; >> board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1; >> @@ -172,6 +173,9 @@ int checkboard(void) >> { >> char *board = "ClearFog"; >> >> + if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) >> + board = "ClearFog Base"; >> + >> cf_read_tlv_data(); >> if (strlen(cf_tlv_data.tlv_product_name[0]) > 0) >> board = cf_tlv_data.tlv_product_name[0]; >> @@ -194,7 +198,8 @@ int board_late_init(void) >> { >> cf_read_tlv_data(); >> >> - if (sr_product_is(&cf_tlv_data, "Clearfog Base")) >> + if (sr_product_is(&cf_tlv_data, "Clearfog Base") || >> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) >> env_set("fdtfile", "armada-388-clearfog-base.dtb"); >> else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4")) >> env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb"); >> >> What do you think? >> >> baruch > > I'll have to revisit and test to be sure, but I believe the reason I didn't go > that route is because I wasn't able to get the CONFIG_IS_ENABLED macro version > to trigger when building SPL, although it worked for the main path. I know > that was the case for something I was working up, but I don't recall whether > it was this patch specifically or not. Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we would need another SPL_FOO config symbol. See include/linux/kconfig.h. > A bit of a nit, but just using the macro isn't really runtime detection, > however adding the separate code path for naming and serdes manipulation > dynamically tends that way. I'm already running into several cases (not the > default build options) where the SPL doesn't fit in the default 128K offset > size, so I'm leery of adding paths that add actual binary size increase. I'm > all for switching to CONFIG_IS_ENABLED if I test and can get it to work, but > still would lean towards just replacing the ifdef in place; after all it is > meant to be the static configuration, not dynamic. It probably makes more sense to reverse the order of ORed conditions: if (IS_ENABLED(TARGET_CLEARFOG_BASE) || sr_product_is(&cf_tlv_data, "Clearfog Base")) IS_ENABLED() is evaluated at build time. When it is true, the compiler drops all other 'if' branches, thus saving space. That also means that the build time configuration overrides the EEPROM set value, which is a Good Thing I believe. I would really like to avoid additional #ifdefs if possible. baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On January 22, 2020 10:32:58 AM UTC, Baruch Siach <baruch at tkos.co.il> wrote: >Hi Joel, > >On Wed, Jan 22 2020, Joel Johnson wrote: >> On 2020-01-21 10:49, Baruch Siach wrote: >>> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote: >>>> Add a unique entry for ClearFog Base variant, reflected in the >board >>>> name and adjusted SerDes topology. >>>> >>>> Signed-off-by: Joel Johnson <mrjoel at lixil.net> >>>> >>>> --- >>>> >>>> v2 changes: >>>> - reworked based on Baruch's run-time TLV EEPROM detection series >>>> v3 changes: >>>> - rebased on mvebu merged run-time TLV EEPROM detection series >>>> - minor update to help test regarding runtime detection failures >>>> >>>> --- >>>> arch/arm/mach-mvebu/Kconfig | 2 ++ >>>> board/solidrun/clearfog/Kconfig | 18 ++++++++++++++++++ >>>> board/solidrun/clearfog/clearfog.c | 12 ++++++++++++ >>>> 3 files changed, 32 insertions(+) >>>> create mode 100644 board/solidrun/clearfog/Kconfig >>>> >>>> diff --git a/arch/arm/mach-mvebu/Kconfig >b/arch/arm/mach-mvebu/Kconfig >>>> index bc5eaa5a76..161dee937f 100644 >>>> --- a/arch/arm/mach-mvebu/Kconfig >>>> +++ b/arch/arm/mach-mvebu/Kconfig >>>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX >>>> default 0 >>>> depends on SECURED_MODE_IMAGE >>>> >>>> +source "board/solidrun/clearfog/Kconfig" >>>> + >>>> endif >>>> diff --git a/board/solidrun/clearfog/Kconfig >>>> b/board/solidrun/clearfog/Kconfig >>>> new file mode 100644 >>>> index 0000000000..936d5918f8 >>>> --- /dev/null >>>> +++ b/board/solidrun/clearfog/Kconfig >>>> @@ -0,0 +1,18 @@ >>>> +menu "ClearFog configuration" >>>> + depends on TARGET_CLEARFOG >>>> + >>>> +config TARGET_CLEARFOG_BASE >>>> + bool "Use ClearFog Base static configuration" >>>> + help >>>> + Use the ClearFog Base as the static configuration instead of >the >>>> + default which uses the ClearFog Pro. >>>> + >>>> + Runtime board detection is always attempted and used if >>>> available. The >>>> + static configuration is used as a fallback in cases where >runtime >>>> + detection is disabled, is not available in hardware, or >otherwise >>>> fails. >>>> + >>>> + Only newer revisions of the ClearFog product line support >runtime >>>> + detection via additional EEPROM hardware. This option enables >>>> selecting >>>> + the Base variant for older hardware revisions. >>>> + >>>> +endmenu >>>> diff --git a/board/solidrun/clearfog/clearfog.c >>>> b/board/solidrun/clearfog/clearfog.c >>>> index e268ef55a2..e77b9465d4 100644 >>>> --- a/board/solidrun/clearfog/clearfog.c >>>> +++ b/board/solidrun/clearfog/clearfog.c >>>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = { >>>> {SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>>> {PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >>>> {USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE) >>>> + {USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, >>>> +#else >>>> {PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, >>>> +#endif >>> >>> I'd prefer run-time test instead of '#ifdefs' that IMO make the code >harder >>> to >>> read. Something like this (build tested only): >>> >>> diff --git a/board/solidrun/clearfog/clearfog.c >>> b/board/solidrun/clearfog/clearfog.c >>> index e268ef55a2a0..202c60cb7841 100644 >>> --- a/board/solidrun/clearfog/clearfog.c >>> +++ b/board/solidrun/clearfog/clearfog.c >>> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map >>> **serdes_map_array, u8 *count) >>> { >>> cf_read_tlv_data(); >>> >>> - if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) { >>> + if (sr_product_is(&cf_tlv_data, "Clearfog GTR") || >>> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) { >>> board_serdes_map[0].serdes_type = PEX0; >>> board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS; >>> board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1; >>> @@ -172,6 +173,9 @@ int checkboard(void) >>> { >>> char *board = "ClearFog"; >>> >>> + if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) >>> + board = "ClearFog Base"; >>> + >>> cf_read_tlv_data(); >>> if (strlen(cf_tlv_data.tlv_product_name[0]) > 0) >>> board = cf_tlv_data.tlv_product_name[0]; >>> @@ -194,7 +198,8 @@ int board_late_init(void) >>> { >>> cf_read_tlv_data(); >>> >>> - if (sr_product_is(&cf_tlv_data, "Clearfog Base")) >>> + if (sr_product_is(&cf_tlv_data, "Clearfog Base") || >>> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) >>> env_set("fdtfile", "armada-388-clearfog-base.dtb"); >>> else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4")) >>> env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb"); >>> >>> What do you think? >>> >>> baruch >> >> I'll have to revisit and test to be sure, but I believe the reason I >didn't go >> that route is because I wasn't able to get the CONFIG_IS_ENABLED >macro version >> to trigger when building SPL, although it worked for the main path. I >know >> that was the case for something I was working up, but I don't recall >whether >> it was this patch specifically or not. > >Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we >would need another SPL_FOO config symbol. See include/linux/kconfig.h. > >> A bit of a nit, but just using the macro isn't really runtime >detection, >> however adding the separate code path for naming and serdes >manipulation >> dynamically tends that way. I'm already running into several cases >(not the >> default build options) where the SPL doesn't fit in the default 128K >offset >> size, so I'm leery of adding paths that add actual binary size >increase. I'm >> all for switching to CONFIG_IS_ENABLED if I test and can get it to >work, but >> still would lean towards just replacing the ifdef in place; after all >it is >> meant to be the static configuration, not dynamic. > >It probably makes more sense to reverse the order of ORed conditions: > > if (IS_ENABLED(TARGET_CLEARFOG_BASE) || > sr_product_is(&cf_tlv_data, "Clearfog Base")) > >IS_ENABLED() is evaluated at build time. When it is true, the compiler >drops all other 'if' branches, thus saving space. That also means that >the build time configuration overrides the EEPROM set value, which is a >Good Thing I believe. I'll take a look and do testing and size comparison in the next day or two. Note that I intended (and wrote the Base target help docs accordingly) that runtime detection, if both enabled and supported in hardware, should be preferred to the static configuration, with static config being only a fallback. This seems to be different from what your thought about it being good for build configuration to override EEPROM detected values, and I'm curious as to your reasoning. There are a few gaps here related to what you point out (e.g. booting on a Pro with EEPROM and Base static config won't give the expected results). Relocating the static adjustment may be fine and help with that case as well. I'll want to add support in such handling for the EEPROM Pro devices but don't have one available. You seem to have them available, can you confirm that "Clearfog Pro" would match those devices using sr_product_is? Thanks, Joel
On 22.01.20 20:28, Joel Johnson wrote: <snip> >> It probably makes more sense to reverse the order of ORed conditions: >> >> if (IS_ENABLED(TARGET_CLEARFOG_BASE) || >> sr_product_is(&cf_tlv_data, "Clearfog Base")) >> >> IS_ENABLED() is evaluated at build time. When it is true, the compiler >> drops all other 'if' branches, thus saving space. That also means that >> the build time configuration overrides the EEPROM set value, which is a >> Good Thing I believe. > > I'll take a look and do testing and size comparison in the next day or > two. > > Note that I intended (and wrote the Base target help docs accordingly) > that runtime detection, if both enabled and supported in hardware, > should be preferred to the static configuration, with static config > being only a fallback. This sounds reasonable. > This seems to be different from what your > thought about it being good for build configuration to override > EEPROM detected values, and I'm curious as to your reasoning. > > There are a few gaps here related to what you point out (e.g. > booting on a Pro with EEPROM and Base static config won't give the > expected results). Relocating the static adjustment may be fine and > help with that case as well. > > I'll want to add support in such handling for the EEPROM Pro > devices but don't have one available. You seem to have them > available, can you confirm that "Clearfog Pro" would match those > devices using sr_product_is? I currently don't have any of these boards available, so I also would like to see some reviewed-by and even better tested-by comments from Baruch (or someone else at SolidRun). Thanks, Stefan
Hi Joel, On Wed, Jan 22, 2020 at 07:28:51PM +0000, Joel Johnson wrote: > On January 22, 2020 10:32:58 AM UTC, Baruch Siach <baruch at tkos.co.il> wrote: > >On Wed, Jan 22 2020, Joel Johnson wrote: > >> On 2020-01-21 10:49, Baruch Siach wrote: > >>> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote: > >>>> Add a unique entry for ClearFog Base variant, reflected in the > >board > >>>> name and adjusted SerDes topology. > >>>> > >>>> Signed-off-by: Joel Johnson <mrjoel at lixil.net> > >>>> > >>>> --- > >>>> > >>>> v2 changes: > >>>> - reworked based on Baruch's run-time TLV EEPROM detection series > >>>> v3 changes: > >>>> - rebased on mvebu merged run-time TLV EEPROM detection series > >>>> - minor update to help test regarding runtime detection failures > >>>> > >>>> --- > >>>> arch/arm/mach-mvebu/Kconfig | 2 ++ > >>>> board/solidrun/clearfog/Kconfig | 18 ++++++++++++++++++ > >>>> board/solidrun/clearfog/clearfog.c | 12 ++++++++++++ > >>>> 3 files changed, 32 insertions(+) > >>>> create mode 100644 board/solidrun/clearfog/Kconfig > >>>> > >>>> diff --git a/arch/arm/mach-mvebu/Kconfig > >b/arch/arm/mach-mvebu/Kconfig > >>>> index bc5eaa5a76..161dee937f 100644 > >>>> --- a/arch/arm/mach-mvebu/Kconfig > >>>> +++ b/arch/arm/mach-mvebu/Kconfig > >>>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX > >>>> default 0 > >>>> depends on SECURED_MODE_IMAGE > >>>> > >>>> +source "board/solidrun/clearfog/Kconfig" > >>>> + > >>>> endif > >>>> diff --git a/board/solidrun/clearfog/Kconfig > >>>> b/board/solidrun/clearfog/Kconfig > >>>> new file mode 100644 > >>>> index 0000000000..936d5918f8 > >>>> --- /dev/null > >>>> +++ b/board/solidrun/clearfog/Kconfig > >>>> @@ -0,0 +1,18 @@ > >>>> +menu "ClearFog configuration" > >>>> + depends on TARGET_CLEARFOG > >>>> + > >>>> +config TARGET_CLEARFOG_BASE > >>>> + bool "Use ClearFog Base static configuration" > >>>> + help > >>>> + Use the ClearFog Base as the static configuration instead of > >the > >>>> + default which uses the ClearFog Pro. > >>>> + > >>>> + Runtime board detection is always attempted and used if > >>>> available. The > >>>> + static configuration is used as a fallback in cases where > >runtime > >>>> + detection is disabled, is not available in hardware, or > >otherwise > >>>> fails. > >>>> + > >>>> + Only newer revisions of the ClearFog product line support > >runtime > >>>> + detection via additional EEPROM hardware. This option enables > >>>> selecting > >>>> + the Base variant for older hardware revisions. > >>>> + > >>>> +endmenu > >>>> diff --git a/board/solidrun/clearfog/clearfog.c > >>>> b/board/solidrun/clearfog/clearfog.c > >>>> index e268ef55a2..e77b9465d4 100644 > >>>> --- a/board/solidrun/clearfog/clearfog.c > >>>> +++ b/board/solidrun/clearfog/clearfog.c > >>>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = { > >>>> {SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0}, > >>>> {PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, > >>>> {USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, > >>>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE) > >>>> + {USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, > >>>> +#else > >>>> {PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, > >>>> +#endif > >>> > >>> I'd prefer run-time test instead of '#ifdefs' that IMO make the code > >harder > >>> to > >>> read. Something like this (build tested only): > >>> > >>> diff --git a/board/solidrun/clearfog/clearfog.c > >>> b/board/solidrun/clearfog/clearfog.c > >>> index e268ef55a2a0..202c60cb7841 100644 > >>> --- a/board/solidrun/clearfog/clearfog.c > >>> +++ b/board/solidrun/clearfog/clearfog.c > >>> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map > >>> **serdes_map_array, u8 *count) > >>> { > >>> cf_read_tlv_data(); > >>> > >>> - if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) { > >>> + if (sr_product_is(&cf_tlv_data, "Clearfog GTR") || > >>> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) { > >>> board_serdes_map[0].serdes_type = PEX0; > >>> board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS; > >>> board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1; > >>> @@ -172,6 +173,9 @@ int checkboard(void) > >>> { > >>> char *board = "ClearFog"; > >>> > >>> + if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) > >>> + board = "ClearFog Base"; > >>> + > >>> cf_read_tlv_data(); > >>> if (strlen(cf_tlv_data.tlv_product_name[0]) > 0) > >>> board = cf_tlv_data.tlv_product_name[0]; > >>> @@ -194,7 +198,8 @@ int board_late_init(void) > >>> { > >>> cf_read_tlv_data(); > >>> > >>> - if (sr_product_is(&cf_tlv_data, "Clearfog Base")) > >>> + if (sr_product_is(&cf_tlv_data, "Clearfog Base") || > >>> + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) > >>> env_set("fdtfile", "armada-388-clearfog-base.dtb"); > >>> else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4")) > >>> env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb"); > >>> > >>> What do you think? > >>> > >>> baruch > >> > >> I'll have to revisit and test to be sure, but I believe the reason I > >didn't go > >> that route is because I wasn't able to get the CONFIG_IS_ENABLED > >macro version > >> to trigger when building SPL, although it worked for the main path. I > >know > >> that was the case for something I was working up, but I don't recall > >whether > >> it was this patch specifically or not. > > > >Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we > >would need another SPL_FOO config symbol. See include/linux/kconfig.h. > > > >> A bit of a nit, but just using the macro isn't really runtime > >detection, > >> however adding the separate code path for naming and serdes > >manipulation > >> dynamically tends that way. I'm already running into several cases > >(not the > >> default build options) where the SPL doesn't fit in the default 128K > >offset > >> size, so I'm leery of adding paths that add actual binary size > >increase. I'm > >> all for switching to CONFIG_IS_ENABLED if I test and can get it to > >work, but > >> still would lean towards just replacing the ifdef in place; after all > >it is > >> meant to be the static configuration, not dynamic. > > > >It probably makes more sense to reverse the order of ORed conditions: > > > > if (IS_ENABLED(TARGET_CLEARFOG_BASE) || > > sr_product_is(&cf_tlv_data, "Clearfog Base")) > > > >IS_ENABLED() is evaluated at build time. When it is true, the compiler > >drops all other 'if' branches, thus saving space. That also means that > >the build time configuration overrides the EEPROM set value, which is a > >Good Thing I believe. > > I'll take a look and do testing and size comparison in the next day or two. > > Note that I intended (and wrote the Base target help docs accordingly) that > runtime detection, if both enabled and supported in hardware, should be > preferred to the static configuration, with static config being only a > fallback. This seems to be different from what your thought about it being > good for build configuration to override EEPROM detected values, and I'm > curious as to your reasoning. I was thinking about forcing board configuration for the manufacturing use case, when the EEPROM is not initialized. But I understand that this use case is not generally useful, unless the EEPROM data is damaged. But if build time configuration serves as fallback, then you have to keep the run-time detection code anyway. How would '#ifdef' make your code smaller? > There are a few gaps here related to what you point out (e.g. booting on a > Pro with EEPROM and Base static config won't give the expected results). > Relocating the static adjustment may be fine and help with that case as > well. > > I'll want to add support in such handling for the EEPROM Pro devices but > don't have one available. You seem to have them available, can you confirm > that "Clearfog Pro" would match those devices using sr_product_is? I considered Clearfog Pro a compatibility fallback. That is why it is not explicitly tested in the code, but assumed. Clearfog Pro rev 2.2 is not in mass production yet to the best of my knowledge. I testing I used the "Clearfog Pro" string, which is a good match to "Clearfog Base" in current code. baruch
diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c index e268ef55a2a0..202c60cb7841 100644 --- a/board/solidrun/clearfog/clearfog.c +++ b/board/solidrun/clearfog/clearfog.c @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map **serdes_map_array, u8 *count) { cf_read_tlv_data(); - if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) { + if (sr_product_is(&cf_tlv_data, "Clearfog GTR") || + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) { board_serdes_map[0].serdes_type = PEX0; board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS; board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1; @@ -172,6 +173,9 @@ int checkboard(void) { char *board = "ClearFog"; + if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) + board = "ClearFog Base"; + cf_read_tlv_data(); if (strlen(cf_tlv_data.tlv_product_name[0]) > 0) board = cf_tlv_data.tlv_product_name[0]; @@ -194,7 +198,8 @@ int board_late_init(void) { cf_read_tlv_data(); - if (sr_product_is(&cf_tlv_data, "Clearfog Base")) + if (sr_product_is(&cf_tlv_data, "Clearfog Base") || + CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) env_set("fdtfile", "armada-388-clearfog-base.dtb"); else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4")) env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");