[3/3] acpi: Fix-up patch to correct sandbox test errors

Message ID 20200427170236.156351-4-sjg@chromium.org
State New
Headers show
Series
  • Fix-up series for x86/master
Related show

Commit Message

Simon Glass April 27, 2020, 5:02 p.m.
Move the alignment code into acpi_setup_base_tables() so that test and
production code are in alignment.

This brings x86/master into line with patch series v8.

Signed-off-by: Simon Glass <sjg at chromium.org>
---

 arch/x86/lib/acpi_table.c | 5 -----
 lib/acpi/acpi_table.c     | 7 ++++++-
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Bin Meng April 28, 2020, 1:30 a.m. | #1
Hi Simon,

On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
>
> Move the alignment code into acpi_setup_base_tables() so that test and
> production code are in alignment.
>
> This brings x86/master into line with patch series v8.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>  arch/x86/lib/acpi_table.c | 5 -----
>  lib/acpi/acpi_table.c     | 7 ++++++-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index 600bde2f5f..13f1409de8 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
>         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
>
>         acpi_setup_base_tables(ctx, start);
> -       /*
> -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> -        * boundary (Windows checks this, but Linux does not).
> -        */
> -       acpi_align64(ctx);
>
>         debug("ACPI:    * FACS\n");
>         facs = ctx->current;
> diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> index 5abf1cad50..1c253af3bf 100644
> --- a/lib/acpi/acpi_table.c
> +++ b/lib/acpi/acpi_table.c
> @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
>         }
>
>         if (i >= entries_num) {
> -               debug("ACPI: Error: too many tables\n");
> +               log_err("ACPI: Error: too many tables\n");
>                 return -E2BIG;
>         }
>
> @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
>         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
>         acpi_write_rsdt(ctx->rsdt);
>         acpi_write_xsdt(ctx->xsdt);
> +       /*
> +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> +        * boundary (Windows checks this, but Linux does not).
> +        */
> +       acpi_align64(ctx);
>  }
> --

Could you please point out which commit in u-boot-x86/master should
squash in this patch to fix the build error on sandbox?

Regards,
Bin
Simon Glass April 28, 2020, 2:28 a.m. | #2
Hi Bin,

On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Move the alignment code into acpi_setup_base_tables() so that test and
> > production code are in alignment.
> >
> > This brings x86/master into line with patch series v8.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  arch/x86/lib/acpi_table.c | 5 -----
> >  lib/acpi/acpi_table.c     | 7 ++++++-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > index 600bde2f5f..13f1409de8 100644
> > --- a/arch/x86/lib/acpi_table.c
> > +++ b/arch/x86/lib/acpi_table.c
> > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> >
> >         acpi_setup_base_tables(ctx, start);
> > -       /*
> > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > -        * boundary (Windows checks this, but Linux does not).
> > -        */
> > -       acpi_align64(ctx);
> >
> >         debug("ACPI:    * FACS\n");
> >         facs = ctx->current;
> > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > index 5abf1cad50..1c253af3bf 100644
> > --- a/lib/acpi/acpi_table.c
> > +++ b/lib/acpi/acpi_table.c
> > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> >         }
> >
> >         if (i >= entries_num) {
> > -               debug("ACPI: Error: too many tables\n");
> > +               log_err("ACPI: Error: too many tables\n");
> >                 return -E2BIG;
> >         }
> >
> > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> >         acpi_write_rsdt(ctx->rsdt);
> >         acpi_write_xsdt(ctx->xsdt);
> > +       /*
> > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > +        * boundary (Windows checks this, but Linux does not).
> > +        */
> > +       acpi_align64(ctx);
> >  }
> > --
>
> Could you please point out which commit in u-boot-x86/master should
> squash in this patch to fix the build error on sandbox?

It might be easier to pick up v8 in that case. I think there are three
patches that need to change because of conflicts caused by the first
one.

So can you pick up the v8 patches? Also you do need to rebase on
master because of the str_to_upper patches.

Regards,
Simon
Bin Meng April 28, 2020, 2:42 a.m. | #3
Hi Simon,

On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > production code are in alignment.
> > >
> > > This brings x86/master into line with patch series v8.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > >
> > >  arch/x86/lib/acpi_table.c | 5 -----
> > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > index 600bde2f5f..13f1409de8 100644
> > > --- a/arch/x86/lib/acpi_table.c
> > > +++ b/arch/x86/lib/acpi_table.c
> > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > >
> > >         acpi_setup_base_tables(ctx, start);
> > > -       /*
> > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > -        * boundary (Windows checks this, but Linux does not).
> > > -        */
> > > -       acpi_align64(ctx);
> > >
> > >         debug("ACPI:    * FACS\n");
> > >         facs = ctx->current;
> > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > index 5abf1cad50..1c253af3bf 100644
> > > --- a/lib/acpi/acpi_table.c
> > > +++ b/lib/acpi/acpi_table.c
> > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > >         }
> > >
> > >         if (i >= entries_num) {
> > > -               debug("ACPI: Error: too many tables\n");
> > > +               log_err("ACPI: Error: too many tables\n");
> > >                 return -E2BIG;
> > >         }
> > >
> > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > >         acpi_write_rsdt(ctx->rsdt);
> > >         acpi_write_xsdt(ctx->xsdt);
> > > +       /*
> > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > +        * boundary (Windows checks this, but Linux does not).
> > > +        */
> > > +       acpi_align64(ctx);
> > >  }
> > > --
> >
> > Could you please point out which commit in u-boot-x86/master should
> > squash in this patch to fix the build error on sandbox?
>
> It might be easier to pick up v8 in that case. I think there are three
> patches that need to change because of conflicts caused by the first
> one.
>
> So can you pick up the v8 patches? Also you do need to rebase on
> master because of the str_to_upper patches.

But v8 is a complete new series for part B? I think we'd better re-tag
v8 as v1. It's quite confusing.

Regards,
Bin
Simon Glass April 28, 2020, 3:06 a.m. | #4
Hi Bin,

On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > production code are in alignment.
> > > >
> > > > This brings x86/master into line with patch series v8.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >
> > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > index 600bde2f5f..13f1409de8 100644
> > > > --- a/arch/x86/lib/acpi_table.c
> > > > +++ b/arch/x86/lib/acpi_table.c
> > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > >
> > > >         acpi_setup_base_tables(ctx, start);
> > > > -       /*
> > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > -        * boundary (Windows checks this, but Linux does not).
> > > > -        */
> > > > -       acpi_align64(ctx);
> > > >
> > > >         debug("ACPI:    * FACS\n");
> > > >         facs = ctx->current;
> > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > index 5abf1cad50..1c253af3bf 100644
> > > > --- a/lib/acpi/acpi_table.c
> > > > +++ b/lib/acpi/acpi_table.c
> > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > >         }
> > > >
> > > >         if (i >= entries_num) {
> > > > -               debug("ACPI: Error: too many tables\n");
> > > > +               log_err("ACPI: Error: too many tables\n");
> > > >                 return -E2BIG;
> > > >         }
> > > >
> > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > >         acpi_write_rsdt(ctx->rsdt);
> > > >         acpi_write_xsdt(ctx->xsdt);
> > > > +       /*
> > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > +        * boundary (Windows checks this, but Linux does not).
> > > > +        */
> > > > +       acpi_align64(ctx);
> > > >  }
> > > > --
> > >
> > > Could you please point out which commit in u-boot-x86/master should
> > > squash in this patch to fix the build error on sandbox?
> >
> > It might be easier to pick up v8 in that case. I think there are three
> > patches that need to change because of conflicts caused by the first
> > one.
> >
> > So can you pick up the v8 patches? Also you do need to rebase on
> > master because of the str_to_upper patches.
>
> But v8 is a complete new series for part B? I think we'd better re-tag
> v8 as v1. It's quite confusing.

No I mean the part A series. Let's get that applied and then we will
be in a brave new world.

I could resent part B as v1 if you like, but note that it has some
reviewed-by tags and some v3, etc. comments. The patches languished
for quite a while which is why I decided to try to split them up.

Regards,
Simon
Bin Meng April 28, 2020, 3:25 a.m. | #5
Hi Simon,

On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > production code are in alignment.
> > > > >
> > > > > This brings x86/master into line with patch series v8.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > index 600bde2f5f..13f1409de8 100644
> > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > >
> > > > >         acpi_setup_base_tables(ctx, start);
> > > > > -       /*
> > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > -        */
> > > > > -       acpi_align64(ctx);
> > > > >
> > > > >         debug("ACPI:    * FACS\n");
> > > > >         facs = ctx->current;
> > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > --- a/lib/acpi/acpi_table.c
> > > > > +++ b/lib/acpi/acpi_table.c
> > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > >         }
> > > > >
> > > > >         if (i >= entries_num) {
> > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > >                 return -E2BIG;
> > > > >         }
> > > > >
> > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > +       /*
> > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > +        */
> > > > > +       acpi_align64(ctx);
> > > > >  }
> > > > > --
> > > >
> > > > Could you please point out which commit in u-boot-x86/master should
> > > > squash in this patch to fix the build error on sandbox?
> > >
> > > It might be easier to pick up v8 in that case. I think there are three
> > > patches that need to change because of conflicts caused by the first
> > > one.
> > >
> > > So can you pick up the v8 patches? Also you do need to rebase on
> > > master because of the str_to_upper patches.
> >
> > But v8 is a complete new series for part B? I think we'd better re-tag
> > v8 as v1. It's quite confusing.
>
> No I mean the part A series. Let's get that applied and then we will
> be in a brave new world.

Is the v8 this one?
http://patchwork.ozlabs.org/user/todo/uboot/?series=172777

The part A series are already applied, but it has a build error as I mentioned.

>
> I could resent part B as v1 if you like, but note that it has some
> reviewed-by tags and some v3, etc. comments. The patches languished
> for quite a while which is why I decided to try to split them up.


Regards,
Bin
Simon Glass April 28, 2020, 2:18 p.m. | #6
Hi Bin,

On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > production code are in alignment.
> > > > > >
> > > > > > This brings x86/master into line with patch series v8.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > >
> > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > -       /*
> > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > -        */
> > > > > > -       acpi_align64(ctx);
> > > > > >
> > > > > >         debug("ACPI:    * FACS\n");
> > > > > >         facs = ctx->current;
> > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > >         }
> > > > > >
> > > > > >         if (i >= entries_num) {
> > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > >                 return -E2BIG;
> > > > > >         }
> > > > > >
> > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > +       /*
> > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > +        */
> > > > > > +       acpi_align64(ctx);
> > > > > >  }
> > > > > > --
> > > > >
> > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > squash in this patch to fix the build error on sandbox?
> > > >
> > > > It might be easier to pick up v8 in that case. I think there are three
> > > > patches that need to change because of conflicts caused by the first
> > > > one.
> > > >
> > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > master because of the str_to_upper patches.
> > >
> > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > v8 as v1. It's quite confusing.
> >
> > No I mean the part A series. Let's get that applied and then we will
> > be in a brave new world.
>
> Is the v8 this one?
> http://patchwork.ozlabs.org/user/todo/uboot/?series=172777

No it is this one:

http://patchwork.ozlabs.org/project/uboot/list/?series=172776

>
> The part A series are already applied, but it has a build error as I mentioned.

Right, but the options are to do a fixup patch or to pick up the v8
series instead. Unfortunately there are merge conflicts in patches so
if you just pick up one patch it won't apply cleanly.

>
> >
> > I could resent part B as v1 if you like, but note that it has some
> > reviewed-by tags and some v3, etc. comments. The patches languished
> > for quite a while which is why I decided to try to split them up.
>

Regards,
Simon
Bin Meng April 28, 2020, 2:34 p.m. | #7
Hi Simon,

On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > production code are in alignment.
> > > > > > >
> > > > > > > This brings x86/master into line with patch series v8.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > >
> > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > -       /*
> > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > -        */
> > > > > > > -       acpi_align64(ctx);
> > > > > > >
> > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > >         facs = ctx->current;
> > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > >         }
> > > > > > >
> > > > > > >         if (i >= entries_num) {
> > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > >                 return -E2BIG;
> > > > > > >         }
> > > > > > >
> > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > +       /*
> > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > +        */
> > > > > > > +       acpi_align64(ctx);
> > > > > > >  }
> > > > > > > --
> > > > > >
> > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > squash in this patch to fix the build error on sandbox?
> > > > >
> > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > patches that need to change because of conflicts caused by the first
> > > > > one.
> > > > >
> > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > master because of the str_to_upper patches.
> > > >
> > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > v8 as v1. It's quite confusing.
> > >
> > > No I mean the part A series. Let's get that applied and then we will
> > > be in a brave new world.
> >
> > Is the v8 this one?
> > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
>
> No it is this one:
>
> http://patchwork.ozlabs.org/project/uboot/list/?series=172776
>
> >
> > The part A series are already applied, but it has a build error as I mentioned.
>
> Right, but the options are to do a fixup patch or to pick up the v8
> series instead. Unfortunately there are merge conflicts in patches so
> if you just pick up one patch it won't apply cleanly.
>

But we still need maintain bisectablity right? To do that we need a
fix patch that can be squashed into the u-boot-x86/master. Picking up
the v8 series does not fix the bisectiablity I think.

> >
> > >
> > > I could resent part B as v1 if you like, but note that it has some
> > > reviewed-by tags and some v3, etc. comments. The patches languished
> > > for quite a while which is why I decided to try to split them up.
> >

Regards,
Bin
Simon Glass April 28, 2020, 2:39 p.m. | #8
Hi Bin,

On Tue, 28 Apr 2020 at 08:34, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > > production code are in alignment.
> > > > > > > >
> > > > > > > > This brings x86/master into line with patch series v8.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > > >
> > > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > > -       /*
> > > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > > -        */
> > > > > > > > -       acpi_align64(ctx);
> > > > > > > >
> > > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > > >         facs = ctx->current;
> > > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > > >         }
> > > > > > > >
> > > > > > > >         if (i >= entries_num) {
> > > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > > >                 return -E2BIG;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > > +       /*
> > > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > > +        */
> > > > > > > > +       acpi_align64(ctx);
> > > > > > > >  }
> > > > > > > > --
> > > > > > >
> > > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > > squash in this patch to fix the build error on sandbox?
> > > > > >
> > > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > > patches that need to change because of conflicts caused by the first
> > > > > > one.
> > > > > >
> > > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > > master because of the str_to_upper patches.
> > > > >
> > > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > > v8 as v1. It's quite confusing.
> > > >
> > > > No I mean the part A series. Let's get that applied and then we will
> > > > be in a brave new world.
> > >
> > > Is the v8 this one?
> > > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
> >
> > No it is this one:
> >
> > http://patchwork.ozlabs.org/project/uboot/list/?series=172776
> >
> > >
> > > The part A series are already applied, but it has a build error as I mentioned.
> >
> > Right, but the options are to do a fixup patch or to pick up the v8
> > series instead. Unfortunately there are merge conflicts in patches so
> > if you just pick up one patch it won't apply cleanly.
> >
>
> But we still need maintain bisectablity right? To do that we need a
> fix patch that can be squashed into the u-boot-x86/master. Picking up
> the v8 series does not fix the bisectiablity I think.

But my suggestion is to pick up the v8 patch *and drop what you
currently have in x86/master*. Otherwise, as you say, bisect might not
work.

I should have objected at the time to Andy refactoring patch, but I
wasn't sure when my stuff would land so it did not seem reasonable to
do so.

Regards,
SImon
Bin Meng April 28, 2020, 2:58 p.m. | #9
Hi Simon,

On Tue, Apr 28, 2020 at 10:39 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 28 Apr 2020 at 08:34, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > > > production code are in alignment.
> > > > > > > > >
> > > > > > > > > This brings x86/master into line with patch series v8.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > > > >
> > > > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > > > -       /*
> > > > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > -        */
> > > > > > > > > -       acpi_align64(ctx);
> > > > > > > > >
> > > > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > > > >         facs = ctx->current;
> > > > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > > > >         }
> > > > > > > > >
> > > > > > > > >         if (i >= entries_num) {
> > > > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > > > >                 return -E2BIG;
> > > > > > > > >         }
> > > > > > > > >
> > > > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > > > +       /*
> > > > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > +        */
> > > > > > > > > +       acpi_align64(ctx);
> > > > > > > > >  }
> > > > > > > > > --
> > > > > > > >
> > > > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > > > squash in this patch to fix the build error on sandbox?
> > > > > > >
> > > > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > > > patches that need to change because of conflicts caused by the first
> > > > > > > one.
> > > > > > >
> > > > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > > > master because of the str_to_upper patches.
> > > > > >
> > > > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > > > v8 as v1. It's quite confusing.
> > > > >
> > > > > No I mean the part A series. Let's get that applied and then we will
> > > > > be in a brave new world.
> > > >
> > > > Is the v8 this one?
> > > > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
> > >
> > > No it is this one:
> > >
> > > http://patchwork.ozlabs.org/project/uboot/list/?series=172776
> > >
> > > >
> > > > The part A series are already applied, but it has a build error as I mentioned.
> > >
> > > Right, but the options are to do a fixup patch or to pick up the v8
> > > series instead. Unfortunately there are merge conflicts in patches so
> > > if you just pick up one patch it won't apply cleanly.
> > >
> >
> > But we still need maintain bisectablity right? To do that we need a
> > fix patch that can be squashed into the u-boot-x86/master. Picking up
> > the v8 series does not fix the bisectiablity I think.
>
> But my suggestion is to pick up the v8 patch *and drop what you
> currently have in x86/master*. Otherwise, as you say, bisect might not
> work.

Oops, I missed the v8 patch in patchwork (not assigned to me yet)

>
> I should have objected at the time to Andy refactoring patch, but I
> wasn't sure when my stuff would land so it did not seem reasonable to
> do so.

I will drop what is applied in x86/master, and apply the v8 part A
series. But it's better to re-tag v8 part B as v1 part B. I got quite
confused until now :)

Regards,
Bin
Simon Glass April 28, 2020, 3:41 p.m. | #10
Hi Bin,

On Tue, 28 Apr 2020 at 08:59, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 10:39 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 28 Apr 2020 at 08:34, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > > > > production code are in alignment.
> > > > > > > > > >
> > > > > > > > > > This brings x86/master into line with patch series v8.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > > > > >
> > > > > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > > > > -       /*
> > > > > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > -        */
> > > > > > > > > > -       acpi_align64(ctx);
> > > > > > > > > >
> > > > > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > > > > >         facs = ctx->current;
> > > > > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > > > > >         }
> > > > > > > > > >
> > > > > > > > > >         if (i >= entries_num) {
> > > > > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > > > > >                 return -E2BIG;
> > > > > > > > > >         }
> > > > > > > > > >
> > > > > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > > > > +       /*
> > > > > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > +        */
> > > > > > > > > > +       acpi_align64(ctx);
> > > > > > > > > >  }
> > > > > > > > > > --
> > > > > > > > >
> > > > > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > > > > squash in this patch to fix the build error on sandbox?
> > > > > > > >
> > > > > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > > > > patches that need to change because of conflicts caused by the first
> > > > > > > > one.
> > > > > > > >
> > > > > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > > > > master because of the str_to_upper patches.
> > > > > > >
> > > > > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > > > > v8 as v1. It's quite confusing.
> > > > > >
> > > > > > No I mean the part A series. Let's get that applied and then we will
> > > > > > be in a brave new world.
> > > > >
> > > > > Is the v8 this one?
> > > > > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
> > > >
> > > > No it is this one:
> > > >
> > > > http://patchwork.ozlabs.org/project/uboot/list/?series=172776
> > > >
> > > > >
> > > > > The part A series are already applied, but it has a build error as I mentioned.
> > > >
> > > > Right, but the options are to do a fixup patch or to pick up the v8
> > > > series instead. Unfortunately there are merge conflicts in patches so
> > > > if you just pick up one patch it won't apply cleanly.
> > > >
> > >
> > > But we still need maintain bisectablity right? To do that we need a
> > > fix patch that can be squashed into the u-boot-x86/master. Picking up
> > > the v8 series does not fix the bisectiablity I think.
> >
> > But my suggestion is to pick up the v8 patch *and drop what you
> > currently have in x86/master*. Otherwise, as you say, bisect might not
> > work.
>
> Oops, I missed the v8 patch in patchwork (not assigned to me yet)
>
> >
> > I should have objected at the time to Andy refactoring patch, but I
> > wasn't sure when my stuff would land so it did not seem reasonable to
> > do so.
>
> I will drop what is applied in x86/master, and apply the v8 part A
> series. But it's better to re-tag v8 part B as v1 part B. I got quite
> confused until now :)

OK I see. Shall I send out part B as v1 now? Or wait until there are
comments and then send v2?

Regards,
Simon
Bin Meng April 29, 2020, 1:30 a.m. | #11
Hi Simon,

On Tue, Apr 28, 2020 at 11:41 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 28 Apr 2020 at 08:59, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 28, 2020 at 10:39 PM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Tue, 28 Apr 2020 at 08:34, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin,
> > > > > > > > >
> > > > > > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > > > > > production code are in alignment.
> > > > > > > > > > >
> > > > > > > > > > > This brings x86/master into line with patch series v8.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > > > > > >
> > > > > > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > > > > > -       /*
> > > > > > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > > -        */
> > > > > > > > > > > -       acpi_align64(ctx);
> > > > > > > > > > >
> > > > > > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > > > > > >         facs = ctx->current;
> > > > > > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > > > > > >         }
> > > > > > > > > > >
> > > > > > > > > > >         if (i >= entries_num) {
> > > > > > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > > > > > >                 return -E2BIG;
> > > > > > > > > > >         }
> > > > > > > > > > >
> > > > > > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > > > > > +       /*
> > > > > > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > > +        */
> > > > > > > > > > > +       acpi_align64(ctx);
> > > > > > > > > > >  }
> > > > > > > > > > > --
> > > > > > > > > >
> > > > > > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > > > > > squash in this patch to fix the build error on sandbox?
> > > > > > > > >
> > > > > > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > > > > > patches that need to change because of conflicts caused by the first
> > > > > > > > > one.
> > > > > > > > >
> > > > > > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > > > > > master because of the str_to_upper patches.
> > > > > > > >
> > > > > > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > > > > > v8 as v1. It's quite confusing.
> > > > > > >
> > > > > > > No I mean the part A series. Let's get that applied and then we will
> > > > > > > be in a brave new world.
> > > > > >
> > > > > > Is the v8 this one?
> > > > > > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
> > > > >
> > > > > No it is this one:
> > > > >
> > > > > http://patchwork.ozlabs.org/project/uboot/list/?series=172776
> > > > >
> > > > > >
> > > > > > The part A series are already applied, but it has a build error as I mentioned.
> > > > >
> > > > > Right, but the options are to do a fixup patch or to pick up the v8
> > > > > series instead. Unfortunately there are merge conflicts in patches so
> > > > > if you just pick up one patch it won't apply cleanly.
> > > > >
> > > >
> > > > But we still need maintain bisectablity right? To do that we need a
> > > > fix patch that can be squashed into the u-boot-x86/master. Picking up
> > > > the v8 series does not fix the bisectiablity I think.
> > >
> > > But my suggestion is to pick up the v8 patch *and drop what you
> > > currently have in x86/master*. Otherwise, as you say, bisect might not
> > > work.
> >
> > Oops, I missed the v8 patch in patchwork (not assigned to me yet)
> >
> > >
> > > I should have objected at the time to Andy refactoring patch, but I
> > > wasn't sure when my stuff would land so it did not seem reasonable to
> > > do so.
> >
> > I will drop what is applied in x86/master, and apply the v8 part A
> > series. But it's better to re-tag v8 part B as v1 part B. I got quite
> > confused until now :)
>
> OK I see. Shall I send out part B as v1 now? Or wait until there are
> comments and then send v2?

Please resend it as v1. I suspect people might get confused like me.

Regards,
Bin
Simon Glass April 29, 2020, 4:55 p.m. | #12
Hi Bin,

On Tue, 28 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Apr 28, 2020 at 11:41 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 28 Apr 2020 at 08:59, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Apr 28, 2020 at 10:39 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Tue, 28 Apr 2020 at 08:34, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Apr 28, 2020 at 10:19 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Mon, 27 Apr 2020 at 21:25, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Tue, Apr 28, 2020 at 11:06 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Mon, 27 Apr 2020 at 20:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Tue, Apr 28, 2020 at 10:29 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Bin,
> > > > > > > > > >
> > > > > > > > > > On Mon, 27 Apr 2020 at 19:30, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Apr 28, 2020 at 1:02 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Move the alignment code into acpi_setup_base_tables() so that test and
> > > > > > > > > > > > production code are in alignment.
> > > > > > > > > > > >
> > > > > > > > > > > > This brings x86/master into line with patch series v8.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > >  arch/x86/lib/acpi_table.c | 5 -----
> > > > > > > > > > > >  lib/acpi/acpi_table.c     | 7 ++++++-
> > > > > > > > > > > >  2 files changed, 6 insertions(+), 6 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> > > > > > > > > > > > index 600bde2f5f..13f1409de8 100644
> > > > > > > > > > > > --- a/arch/x86/lib/acpi_table.c
> > > > > > > > > > > > +++ b/arch/x86/lib/acpi_table.c
> > > > > > > > > > > > @@ -375,11 +375,6 @@ ulong write_acpi_tables(ulong start_addr)
> > > > > > > > > > > >         debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
> > > > > > > > > > > >
> > > > > > > > > > > >         acpi_setup_base_tables(ctx, start);
> > > > > > > > > > > > -       /*
> > > > > > > > > > > > -        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > > > -        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > > > -        */
> > > > > > > > > > > > -       acpi_align64(ctx);
> > > > > > > > > > > >
> > > > > > > > > > > >         debug("ACPI:    * FACS\n");
> > > > > > > > > > > >         facs = ctx->current;
> > > > > > > > > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> > > > > > > > > > > > index 5abf1cad50..1c253af3bf 100644
> > > > > > > > > > > > --- a/lib/acpi/acpi_table.c
> > > > > > > > > > > > +++ b/lib/acpi/acpi_table.c
> > > > > > > > > > > > @@ -145,7 +145,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> > > > > > > > > > > >         }
> > > > > > > > > > > >
> > > > > > > > > > > >         if (i >= entries_num) {
> > > > > > > > > > > > -               debug("ACPI: Error: too many tables\n");
> > > > > > > > > > > > +               log_err("ACPI: Error: too many tables\n");
> > > > > > > > > > > >                 return -E2BIG;
> > > > > > > > > > > >         }
> > > > > > > > > > > >
> > > > > > > > > > > > @@ -256,4 +256,9 @@ void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> > > > > > > > > > > >         acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> > > > > > > > > > > >         acpi_write_rsdt(ctx->rsdt);
> > > > > > > > > > > >         acpi_write_xsdt(ctx->xsdt);
> > > > > > > > > > > > +       /*
> > > > > > > > > > > > +        * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> > > > > > > > > > > > +        * boundary (Windows checks this, but Linux does not).
> > > > > > > > > > > > +        */
> > > > > > > > > > > > +       acpi_align64(ctx);
> > > > > > > > > > > >  }
> > > > > > > > > > > > --
> > > > > > > > > > >
> > > > > > > > > > > Could you please point out which commit in u-boot-x86/master should
> > > > > > > > > > > squash in this patch to fix the build error on sandbox?
> > > > > > > > > >
> > > > > > > > > > It might be easier to pick up v8 in that case. I think there are three
> > > > > > > > > > patches that need to change because of conflicts caused by the first
> > > > > > > > > > one.
> > > > > > > > > >
> > > > > > > > > > So can you pick up the v8 patches? Also you do need to rebase on
> > > > > > > > > > master because of the str_to_upper patches.
> > > > > > > > >
> > > > > > > > > But v8 is a complete new series for part B? I think we'd better re-tag
> > > > > > > > > v8 as v1. It's quite confusing.
> > > > > > > >
> > > > > > > > No I mean the part A series. Let's get that applied and then we will
> > > > > > > > be in a brave new world.
> > > > > > >
> > > > > > > Is the v8 this one?
> > > > > > > http://patchwork.ozlabs.org/user/todo/uboot/?series=172777
> > > > > >
> > > > > > No it is this one:
> > > > > >
> > > > > > http://patchwork.ozlabs.org/project/uboot/list/?series=172776
> > > > > >
> > > > > > >
> > > > > > > The part A series are already applied, but it has a build error as I mentioned.
> > > > > >
> > > > > > Right, but the options are to do a fixup patch or to pick up the v8
> > > > > > series instead. Unfortunately there are merge conflicts in patches so
> > > > > > if you just pick up one patch it won't apply cleanly.
> > > > > >
> > > > >
> > > > > But we still need maintain bisectablity right? To do that we need a
> > > > > fix patch that can be squashed into the u-boot-x86/master. Picking up
> > > > > the v8 series does not fix the bisectiablity I think.
> > > >
> > > > But my suggestion is to pick up the v8 patch *and drop what you
> > > > currently have in x86/master*. Otherwise, as you say, bisect might not
> > > > work.
> > >
> > > Oops, I missed the v8 patch in patchwork (not assigned to me yet)
> > >
> > > >
> > > > I should have objected at the time to Andy refactoring patch, but I
> > > > wasn't sure when my stuff would land so it did not seem reasonable to
> > > > do so.
> > >
> > > I will drop what is applied in x86/master, and apply the v8 part A
> > > series. But it's better to re-tag v8 part B as v1 part B. I got quite
> > > confused until now :)
> >
> > OK I see. Shall I send out part B as v1 now? Or wait until there are
> > comments and then send v2?
>
> Please resend it as v1. I suspect people might get confused like me.

OK this is done. Hoping people can find time to review soon! I first
posted all this in January.

Regards,
Simon

Patch

diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index 600bde2f5f..13f1409de8 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -375,11 +375,6 @@  ulong write_acpi_tables(ulong start_addr)
 	debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
 
 	acpi_setup_base_tables(ctx, start);
-	/*
-	 * Per ACPI spec, the FACS table address must be aligned to a 64 byte
-	 * boundary (Windows checks this, but Linux does not).
-	 */
-	acpi_align64(ctx);
 
 	debug("ACPI:    * FACS\n");
 	facs = ctx->current;
diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
index 5abf1cad50..1c253af3bf 100644
--- a/lib/acpi/acpi_table.c
+++ b/lib/acpi/acpi_table.c
@@ -145,7 +145,7 @@  int acpi_add_table(struct acpi_ctx *ctx, void *table)
 	}
 
 	if (i >= entries_num) {
-		debug("ACPI: Error: too many tables\n");
+		log_err("ACPI: Error: too many tables\n");
 		return -E2BIG;
 	}
 
@@ -256,4 +256,9 @@  void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
 	acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
 	acpi_write_rsdt(ctx->rsdt);
 	acpi_write_xsdt(ctx->xsdt);
+	/*
+	 * Per ACPI spec, the FACS table address must be aligned to a 64 byte
+	 * boundary (Windows checks this, but Linux does not).
+	 */
+	acpi_align64(ctx);
 }