Re: [coreboot] [PATCH] flashrom: Add external programmer delay functions

[Uwe Hermann]

On Fri, Jun 05, 2009 at 06:28:41PM +0200, Carl-Daniel Hailfinger wrote:
> Add external programmer delay functions so external programmers can
> handle the delay on their own if needed.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Uwe Hermann

...but see below.
> Index: flashrom-programmer_delay/flash.h
>
> --- flashrom-programmer_delay/flash.h	(Revision 576)
> +++ flashrom-programmer_delay/flash.h	(Arbeitskopie)
> @@ -103,6 +103,8 @@
>    uint8_t (*chip_readb) (const chipaddr addr);
>    uint16_t (*chip_readw) (const chipaddr addr);
>    uint32_t (*chip_readl) (const chipaddr addr);
> void (*delay) (int usecs);
>
> };
>
> extern const struct programmer_entry programmer_table[];
> @@ -118,6 +120,7 @@
>  uint8_t chip_readb(const chipaddr addr);
>  uint16_t chip_readw(const chipaddr addr);
>  uint32_t chip_readl(const chipaddr addr);
> +void programmer_delay(uint32_t usecs);

Why uint32_t here and int above?
> Index: flashrom-programmer_delay/spi.c
>
> --- flashrom-programmer_delay/spi.c	(Revision 576)
> +++ flashrom-programmer_delay/spi.c	(Arbeitskopie)
> @@ -425,7 +425,7 @@
>     */
>    /* FIXME: We assume spi_read_status_register will never fail. */
>    while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> sleep(1);
> programmer_delay(1000000);

I'd also make this 100 * 1000 as below, easier on the eyes.
> return 0;
>  }
>
> @@ -453,7 +453,7 @@
>     */
>    /* FIXME: We assume spi_read_status_register will never fail. */
>    while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> sleep(1);
> programmer_delay(1000000);

Ditto.
> return 0;
>  }
>
> @@ -485,7 +485,7 @@
>     * This usually takes 100-4000 ms, so wait in 100 ms steps.
>     */
>    while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> usleep(100 * 1000);
> programmer_delay(100 * 1000);
> return 0;
>
> }
> Index: flashrom-programmer_delay/am29f040b.c
>
> --- flashrom-programmer_delay/am29f040b.c	(Revision 576)
> +++ flashrom-programmer_delay/am29f040b.c	(Arbeitskopie)
> @@ -29,7 +29,7 @@
>    chip_writeb(0x55, bios + 0x2AA);
>    chip_writeb(0x30, bios + address);
> sleep(2);
> programmer_delay(1000000 * 2);

Maybe 2 * 1000 * 1000, same format as above.
> Index: flashrom-programmer_delay/internal.c
>
> --- flashrom-programmer_delay/internal.c	(Revision 576)
> +++ flashrom-programmer_delay/internal.c	(Arbeitskopie)
> @@ -195,6 +195,18 @@
>    return *(volatile uint32_t *) addr;
>  }
>
> +void internal_delay(int usecs)
> +{
> /* If the delay is >1 s, use usleep because timing does not need to
> * be so precise.

Hm, but why not use myusec_delay() always? Does it have drawbacks?
> */
> if (usecs > 1000000) {
> usleep(usecs);
> } else {
> myusec_delay(usecs);
> }
>
> +}

Uwe.