On Sat, 17 Sep 2011 16:21:12 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 20.08.2011 12:39 schrieb Stefan Tauner: > > Signed-off-by: Stefan Tauner <[email protected]> > > Acked-by: Carl-Daniel Hailfinger <[email protected]> > with comments. > > > diff --git a/ichspi.c b/ichspi.c > > index c38fbfc..a9327de 100644 > > --- a/ichspi.c > > +++ b/ichspi.c > > @@ -1507,6 +1507,24 @@ static void prettyprint_ich9_reg_pr(int i) > > msg_pdbg(", unused)\n"); > > } > > > > +/* Set the read and write protection enable bits of PR register @i > > according to > > Change "Set" to "Set/clear". unneeded imho.. "set to ... according to <boolean>" clearly means set to zero or one respectively, but changed anyway since it makes this explicit. > > + * @read_prot and @write_prot. */ > > +static void ich8_set_pr(int i, int read_prot, int write_prot) > > +{ > > + void *addr = ich_spibar + ICH9_REG_PR0 + (i * 4); > > + uint32_t pr = mmio_readl(addr); > > + > > + msg_gspew("PR%u is 0x%08x, ", i, pr); > > + pr &= ~((1 << PR_RP_OFF) | (1 << PR_WP_OFF)); > > + if (read_prot) > > + pr |= (1 << PR_RP_OFF); > > + if (write_prot) > > + pr |= (1 << PR_WP_OFF); > > Having some logic which eliminated unnecessary changes of pr would be > nice. You could even print "unchanged\n" for that case. done as discussed on IRC. > > + msg_gspew("trying to set it to 0x%08x ", pr); > > + mmio_writel(pr, addr); > > Really not rmmio_writel? We want to undo all non-permanent config > changes on shutdown. thanks! > > + msg_gspew("resulted in 0x%08x.\n", mmio_readl(addr)); > > +} > > + > > static const struct spi_programmer spi_programmer_ich7 = { > > .type = SPI_CONTROLLER_ICH7, > > .max_data_read = 64, > > @@ -1648,6 +1666,10 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, > > void *rcrb, > > for(i = 0; i < 5; i++) > > do_ich9_spi_frap(tmp, i); > > > > + /* try to disable PR locks before printing them */ > > + if (!ichspi_lock) > > + for(i = 0; i < 5; i++) > > + ich8_set_pr(i, 0, 0); > > ich8_ is an unusual prefix. Most other names use either ich7_ or ich9_ > prefixes even if the functionality is present in ICH8. and that is a big mistake actually (in the case of ich9_ prefixes for functions/constants that are already available in ICH8) imho. changed to ich9_ for now and committed in r1447. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
