| Message ID | 1518041832-337-2-git-send-email-york.sun@nxp.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/2] env: Fix env_load_location | expand |
On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote: > Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env > lookup function") dropped the default action if driver doesn't have > get_char() defined. This causes failure to get environmental > variables from NOR flash. Add back this default action for now. > > Signed-off-by: York Sun <york.sun@nxp.com> > CC: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > Limited test on LS1043ARDB. > > env/env.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/env/env.c b/env/env.c > index edfb575..210bae2 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -159,7 +159,7 @@ int env_get_char(int index) > int ret; > > if (!drv->get_char) > - continue; > + return *(uchar *)(gd->env_addr + index); Thinking more about this, I think this would break the case where the first environment in your list has no get_char method, but the second might. Would something like that work? ------------ 8< -------- diff --git a/env/env.c b/env/env.c index 9a89832c1aaf..391c9c0df2f0 100644 --- a/env/env.c +++ b/env/env.c @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio) int env_get_char(int index) { struct env_driver *drv; + bool has_get_char = false; int prio; if (gd->env_valid == ENV_INVALID) @@ -166,6 +167,7 @@ int env_get_char(int index) if (!env_has_inited(drv->location)) continue; + has_get_char = true; ret = drv->get_char(index); if (!ret) return 0; @@ -174,6 +176,9 @@ int env_get_char(int index) drv->name, ret); } + if (!has_get_char) + return *(uchar *)(gd->env_addr + index); + return -ENODEV; } ------ 8< ------- Thanks! Maxime
[e-news abonnieren]<http://www.pepperl-fuchs.de/e-news> [YouTube: Pepperl+Fuchs auf YouTube] <http://www.youtube.com/user/PepperlFuchsGmbH> [@PepperlFuchsDE folgen] <https://twitter.com/PepperlFuchsDE> [Pepperl+Fuchs auf XING] <https://www.xing.com/companies/pepperl%252bfuchsgmbh>
Sorry for the html mess, I'll resend in a minute. Simon Pepperl+Fuchs GmbH, Mannheim Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Claus Michael Registergericht/Register Court: AG Mannheim HRB 4713 On 08.02.2018 10:42, Simon Goldschmidt wrote: > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot Wichtiger Hinweis: Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails. Important Information: This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails.
On 08.02.2018 09:47, Maxime Ripard wrote: > On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote: >> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env >> lookup function") dropped the default action if driver doesn't have >> get_char() defined. This causes failure to get environmental >> variables from NOR flash. Add back this default action for now. >> >> Signed-off-by: York Sun <york.sun@nxp.com> >> CC: Maxime Ripard <maxime.ripard@free-electrons.com> >> --- >> Limited test on LS1043ARDB. >> >> env/env.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/env/env.c b/env/env.c >> index edfb575..210bae2 100644 >> --- a/env/env.c >> +++ b/env/env.c >> @@ -159,7 +159,7 @@ int env_get_char(int index) >> int ret; >> >> if (!drv->get_char) >> - continue; >> + return *(uchar *)(gd->env_addr + index); > Thinking more about this, I think this would break the case where the > first environment in your list has no get_char method, but the second > might. How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority. That's why I suggested removing the 'get_char' callback from the env drivers :-) Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'. Simon > Would something like that work? > > ------------ 8< -------- > diff --git a/env/env.c b/env/env.c > index 9a89832c1aaf..391c9c0df2f0 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio) > int env_get_char(int index) > { > struct env_driver *drv; > + bool has_get_char = false; > int prio; > > if (gd->env_valid == ENV_INVALID) > @@ -166,6 +167,7 @@ int env_get_char(int index) > if (!env_has_inited(drv->location)) > continue; > > + has_get_char = true; > ret = drv->get_char(index); > if (!ret) > return 0; > @@ -174,6 +176,9 @@ int env_get_char(int index) > drv->name, ret); > } > > + if (!has_get_char) > + return *(uchar *)(gd->env_addr + index); > + > return -ENODEV; > } > ------ 8< ------- > > Thanks! > Maxime > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote: > On 08.02.2018 09:47, Maxime Ripard wrote: > > On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote: > > > Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env > > > lookup function") dropped the default action if driver doesn't have > > > get_char() defined. This causes failure to get environmental > > > variables from NOR flash. Add back this default action for now. > > > > > > Signed-off-by: York Sun <york.sun@nxp.com> > > > CC: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > Limited test on LS1043ARDB. > > > > > > env/env.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/env/env.c b/env/env.c > > > index edfb575..210bae2 100644 > > > --- a/env/env.c > > > +++ b/env/env.c > > > @@ -159,7 +159,7 @@ int env_get_char(int index) > > > int ret; > > > if (!drv->get_char) > > > - continue; > > > + return *(uchar *)(gd->env_addr + index); > > Thinking more about this, I think this would break the case where the > > first environment in your list has no get_char method, but the second > > might. > > > How can we decide which way is wanted? With your patch below, we might end > up loading chars from a low-prio environment (which is not CRC checked) in > the early boot stage. Later, we load the environment from another env driver > with higher priority. Ah, right. > That's why I suggested removing the 'get_char' callback from the env drivers > :-) Early boot stage environment lookups would still work the old way > reading from 'gd->env_addr + index'. If that works on York's board, I'm all in. Maxime
Maxime, York, I've just sent a patch that removes 'get_char' callback from the env drivers. This should restore the old behaviour we had before supporting multiple environment drivers (for all but eeprom, of course). Simon On 08.02.2018 23:10, Maxime Ripard wrote: > On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote: >> On 08.02.2018 09:47, Maxime Ripard wrote: >>> On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote: >>>> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env >>>> lookup function") dropped the default action if driver doesn't have >>>> get_char() defined. This causes failure to get environmental >>>> variables from NOR flash. Add back this default action for now. >>>> >>>> Signed-off-by: York Sun <york.sun@nxp.com> >>>> CC: Maxime Ripard <maxime.ripard@free-electrons.com> >>>> --- >>>> Limited test on LS1043ARDB. >>>> >>>> env/env.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/env/env.c b/env/env.c >>>> index edfb575..210bae2 100644 >>>> --- a/env/env.c >>>> +++ b/env/env.c >>>> @@ -159,7 +159,7 @@ int env_get_char(int index) >>>> int ret; >>>> if (!drv->get_char) >>>> - continue; >>>> + return *(uchar *)(gd->env_addr + index); >>> Thinking more about this, I think this would break the case where the >>> first environment in your list has no get_char method, but the second >>> might. >> >> How can we decide which way is wanted? With your patch below, we might end >> up loading chars from a low-prio environment (which is not CRC checked) in >> the early boot stage. Later, we load the environment from another env driver >> with higher priority. > Ah, right. > >> That's why I suggested removing the 'get_char' callback from the env drivers >> :-) Early boot stage environment lookups would still work the old way >> reading from 'gd->env_addr + index'. > If that works on York's board, I'm all in. > > Maxime >
diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret; if (!drv->get_char) - continue; + return *(uchar *)(gd->env_addr + index); if (!env_has_inited(drv->location)) continue;
Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env lookup function") dropped the default action if driver doesn't have get_char() defined. This causes failure to get environmental variables from NOR flash. Add back this default action for now. Signed-off-by: York Sun <york.sun@nxp.com> CC: Maxime Ripard <maxime.ripard@free-electrons.com> --- Limited test on LS1043ARDB. env/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)