Re: [flashrom] [PATCH 7/8] ichspi: unlock PR register restrictions if not locked down

[Carl-Daniel Hailfinger]

Am 20.08.2011 12:39 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner

Acked-by: Carl-Daniel Hailfinger 
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".
> @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.
> 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.
> 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.
> for(i = 0; i < 5; i++)
> 	prettyprint_ich9_reg_pr(i);

[PATCH 7/8] ichspi: unlock PR register restrictions if not locked down

[Stefan Tauner]

On Sat, 17 Sep 2011 16:21:12 +0200
Carl-Daniel Hailfinger  wrote:
> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > Signed-off-by: Stefan Tauner
>
> Acked-by: Carl-Daniel Hailfinger 
> 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.