Re: [flashrom] [PATCH 08/12] simplify ich_set_bbar

[Carl-Daniel Hailfinger]

Am 08.06.2011 04:55 schrieb Stefan Tauner:
> ~10 lines less, documenting better what the differences are (i.e. offset of BBAR only).
>
> Signed-off-by: Stefan Tauner

Nice cleanup!
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH 08/12] simplify ich_set_bbar

[Stefan Tauner]

On Mon, 13 Jun 2011 19:24:55 +0200
Carl-Daniel Hailfinger  wrote:
> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> > ~10 lines less, documenting better what the differences are (i.e. offset of BBAR only).
> >
> > Signed-off-by: Stefan Tauner
>
> Nice cleanup!
> Acked-by: Carl-Daniel Hailfinger

ahem! :)

because of idwer's remark that "msg_pdbg("Reserved bits in BBAR not
zero: 0x%04x"..." should get a \n (thx!), i reviewed that patch again
and noticed that it is quite broken.

in the original code the real bbar is read from the register and
checked for reserved bits before we try to set the requested min
address. i somehow missed that, sorry. :/

the new, attached patch has following changes:
 - check for reserved bits
 - output 8 hex characters instead of 4 (i.e. 32b instead of 16b)
 - add \n to the message above

[PATCH 08/12] simplify ich_set_bbar

[Carl-Daniel Hailfinger]

Am 13.06.2011 20:31 schrieb Stefan Tauner:
> On Mon, 13 Jun 2011 19:24:55 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 08.06.2011 04:55 schrieb Stefan Tauner:
> > > ~10 lines less, documenting better what the differences are (i.e. offset of BBAR only).
> > >
> > > Signed-off-by: Stefan Tauner
> >
> > Nice cleanup!
> > Acked-by: Carl-Daniel Hailfinger
>
> ahem! :)
>
> because of idwer's remark that "msg_pdbg("Reserved bits in BBAR not
> zero: 0x%04x"..." should get a \n (thx!), i reviewed that patch again
> and noticed that it is quite broken.
>
> in the original code the real bbar is read from the register and
> checked for reserved bits before we try to set the requested min
> address. i somehow missed that, sorry. :/

Ouch. I overlooked that during review. The patch looked so simple.
> the new, attached patch has following changes:
> check for reserved bits
> output 8 hex characters instead of 4 (i.e. 32b instead of 16b)
> add \n to the message above

Since you're already changing the function prototype of ich_set_bbar,
can you make it static?

And another point about the coding style:
Can you use int bbar_offs instead of void *bbar_addr?
That way, it is easier to check directly which physmap region the
accesses correspond to.

Regards,
Carl-Daniel

[PATCH 08/12] simplify ich_set_bbar

[Stefan Tauner]

On Mon, 13 Jun 2011 22:09:35 +0200
Carl-Daniel Hailfinger  wrote:
> Am 13.06.2011 20:31 schrieb Stefan Tauner:
> > On Mon, 13 Jun 2011 19:24:55 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 08.06.2011 04:55 schrieb Stefan Tauner:
> > > > ~10 lines less, documenting better what the differences are (i.e. offset of BBAR only).
> > > >
> > > > Signed-off-by: Stefan Tauner
> > >
> > > Nice cleanup!
> > > Acked-by: Carl-Daniel Hailfinger
> >
> > ahem! :)
> >
> > because of idwer's remark that "msg_pdbg("Reserved bits in BBAR not
> > zero: 0x%04x"..." should get a \n (thx!), i reviewed that patch again
> > and noticed that it is quite broken.
> >
> > in the original code the real bbar is read from the register and
> > checked for reserved bits before we try to set the requested min
> > address. i somehow missed that, sorry. :/
>
> Ouch. I overlooked that during review. The patch looked so simple.

famous last words ;)
> > the new, attached patch has following changes:
> > check for reserved bits
> > output 8 hex characters instead of 4 (i.e. 32b instead of 16b)
> > add \n to the message above
>
> Since you're already changing the function prototype of ich_set_bbar,
> can you make it static?
>
> And another point about the coding style:
> Can you use int bbar_offs instead of void *bbar_addr?
> That way, it is easier to check directly which physmap region the
> accesses correspond to.

like so?

[SPAM?] Re: [PATCH 08/12] simplify ich_set_bbar

[Stefan Tauner]

On Mon, 13 Jun 2011 23:12:45 +0200
Stefan Tauner  wrote:
> On Mon, 13 Jun 2011 22:09:35 +0200
> Carl-Daniel Hailfinger  wrote:
> > Since you're already changing the function prototype of ich_set_bbar,
> > can you make it static?
> >
> > And another point about the coding style:
> > Can you use int bbar_offs instead of void *bbar_addr?
> > That way, it is easier to check directly which physmap region the
> > accesses correspond to.
>
> like so?

and:
-	ichspi_bbar = mmio_readl(ich_spibar + bbar_off);
+	ichspi_bbar = mmio_readl(ich_spibar + bbar_off) & BBAR_MASK;
at the end else we would use the reserved bits too