| Message ID | 20200110144711.81938-2-giulio.benetti@benettiengineering.com |
|---|---|
| State | Accepted |
| Commit | 1054344fa42b1f6568d35eae7ceecc859ad42258 |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Add i.MXRT family support | expand |
> At the moment entry_point is set to image_get_load(header) that sets it > to "load address" instead of "entry point", assuming entry_point is > equal to load_addr, but it's not true. Then load_addr is set to > "entry_point - header_size", but this is wrong too since load_addr is > not an entry point. > So use image_get_ep() for entry_point assignment and image_get_load() > for load_addr assignment. > Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> Applied to u-boot-imx, master, thanks ! Best regards, Stefano Babic
Hi Giulio, > At the moment entry_point is set to image_get_load(header) that sets > it to "load address" instead of "entry point", assuming entry_point is > equal to load_addr, but it's not true. Then load_addr is set to > "entry_point - header_size", but this is wrong too since load_addr is > not an entry point. > > So use image_get_ep() for entry_point assignment and image_get_load() > for load_addr assignment. > > Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> > --- > common/spl/spl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/common/spl/spl.c b/common/spl/spl.c > index c1fce62b91..19085ad270 100644 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -284,9 +284,9 @@ int spl_parse_image_header(struct spl_image_info > *spl_image, spl_image->entry_point = image_get_ep(header); > spl_image->size = > image_get_data_size(header); } else { > - spl_image->entry_point = > image_get_load(header); > + spl_image->entry_point = > image_get_ep(header); /* Load including the header */ > - spl_image->load_addr = > spl_image->entry_point - > + spl_image->load_addr = > image_get_load(header) - header_size; > spl_image->size = > image_get_data_size(header) + header_size; I'm concerned, that this change will silently break several boards - the problem is with assumption that entry point is equal to load_addr. It would be best to pull this change ASAP, so we would have a chance to fix this by next release. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, all patch series has already been applied, anyway I answer to your suggestions since something was missing and I'm going to create a patch for that. So... On 1/28/20 9:09 AM, Lukasz Majewski wrote: > Hi Giulio, > >> At the moment entry_point is set to image_get_load(header) that sets >> it to "load address" instead of "entry point", assuming entry_point is >> equal to load_addr, but it's not true. Then load_addr is set to >> "entry_point - header_size", but this is wrong too since load_addr is >> not an entry point. >> >> So use image_get_ep() for entry_point assignment and image_get_load() >> for load_addr assignment. >> >> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> >> --- >> common/spl/spl.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/common/spl/spl.c b/common/spl/spl.c >> index c1fce62b91..19085ad270 100644 >> --- a/common/spl/spl.c >> +++ b/common/spl/spl.c >> @@ -284,9 +284,9 @@ int spl_parse_image_header(struct spl_image_info >> *spl_image, spl_image->entry_point = image_get_ep(header); >> spl_image->size = >> image_get_data_size(header); } else { >> - spl_image->entry_point = >> image_get_load(header); >> + spl_image->entry_point = >> image_get_ep(header); /* Load including the header */ >> - spl_image->load_addr = >> spl_image->entry_point - >> + spl_image->load_addr = >> image_get_load(header) - header_size; >> spl_image->size = >> image_get_data_size(header) + header_size; > > I'm concerned, that this change will silently break several boards - > the problem is with assumption that entry point is equal to load_addr. > > It would be best to pull this change ASAP, so we would have a chance to > fix this by next release. It's been committed after Patrice fixed a lot of boards: https://gitlab.denx.de/u-boot/u-boot/commit/38a6cce65737096b836d43a22f09b7a54c9d020c and https://gitlab.denx.de/u-boot/u-boot/commit/74bb4570a952b06fecfafc5b961a5cb5147ec544 Indeed at the first moment it's been committed by Tom Rini and started to cause boot failure as you were worried for, then it's been reverted, then it's been re-applied after applying Patrice series. Best regards
On Tue, 28 Jan 2020 17:37:17 +0100 Giulio Benetti <giulio.benetti@benettiengineering.com> wrote: > Hi Lukasz, > > all patch series has already been applied, anyway I answer to your > suggestions since something was missing and I'm going to create a > patch for that. > > So... > > On 1/28/20 9:09 AM, Lukasz Majewski wrote: > > Hi Giulio, > > > >> At the moment entry_point is set to image_get_load(header) that > >> sets it to "load address" instead of "entry point", assuming > >> entry_point is equal to load_addr, but it's not true. Then > >> load_addr is set to "entry_point - header_size", but this is wrong > >> too since load_addr is not an entry point. > >> > >> So use image_get_ep() for entry_point assignment and > >> image_get_load() for load_addr assignment. > >> > >> Signed-off-by: Giulio Benetti > >> <giulio.benetti@benettiengineering.com> --- > >> common/spl/spl.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/common/spl/spl.c b/common/spl/spl.c > >> index c1fce62b91..19085ad270 100644 > >> --- a/common/spl/spl.c > >> +++ b/common/spl/spl.c > >> @@ -284,9 +284,9 @@ int spl_parse_image_header(struct > >> spl_image_info *spl_image, spl_image->entry_point = > >> image_get_ep(header); spl_image->size = > >> image_get_data_size(header); } else { > >> - spl_image->entry_point = > >> image_get_load(header); > >> + spl_image->entry_point = > >> image_get_ep(header); /* Load including the header */ > >> - spl_image->load_addr = > >> spl_image->entry_point - > >> + spl_image->load_addr = > >> image_get_load(header) - header_size; > >> spl_image->size = > >> image_get_data_size(header) + header_size; > > > > I'm concerned, that this change will silently break several boards - > > the problem is with assumption that entry point is equal to > > load_addr. > > > > It would be best to pull this change ASAP, so we would have a > > chance to fix this by next release. > > It's been committed after Patrice fixed a lot of boards: > https://gitlab.denx.de/u-boot/u-boot/commit/38a6cce65737096b836d43a22f09b7a54c9d020c > and > https://gitlab.denx.de/u-boot/u-boot/commit/74bb4570a952b06fecfafc5b961a5cb5147ec544 > > Indeed at the first moment it's been committed by Tom Rini and > started to cause boot failure as you were worried for, then it's been > reverted, then it's been re-applied after applying Patrice series. > Ach... Ok. No problem then. > Best regards Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/common/spl/spl.c b/common/spl/spl.c index c1fce62b91..19085ad270 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -284,9 +284,9 @@ int spl_parse_image_header(struct spl_image_info *spl_image, spl_image->entry_point = image_get_ep(header); spl_image->size = image_get_data_size(header); } else { - spl_image->entry_point = image_get_load(header); + spl_image->entry_point = image_get_ep(header); /* Load including the header */ - spl_image->load_addr = spl_image->entry_point - + spl_image->load_addr = image_get_load(header) - header_size; spl_image->size = image_get_data_size(header) + header_size;
At the moment entry_point is set to image_get_load(header) that sets it to "load address" instead of "entry point", assuming entry_point is equal to load_addr, but it's not true. Then load_addr is set to "entry_point - header_size", but this is wrong too since load_addr is not an entry point. So use image_get_ep() for entry_point assignment and image_get_load() for load_addr assignment. Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> --- common/spl/spl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)