Re: [flashrom] rtl8169 support - Final

[Mathias Krause]

On 05.01.2011 10:59, Jon Shadow wrote:
> Fixed a few things. Still need someone to test it.
>
> Signed-off-by: Sergey Lichack

Haven't the hardware to do some testing but like to comment the patch.
> Index: nicrealtek.c
>
> --- nicrealtek.c	(revision 1250)
> +++ nicrealtek.c	(working copy)
> @@ -27,11 +27,11 @@
>  #define PCI_VENDOR_ID_REALTEK	0x10ec
>  #define PCI_VENDOR_ID_SMC1211	0x1113
>
> -#define BIOS_ROM_ADDR		0xD4
> -#define BIOS_ROM_DATA		0xD7
> +static int bios_rom_addr, bios_rom_data;
>
> const struct pcidev_status nics_realtek[] = {
>    {0x10ec, 0x8139, OK, "Realtek", "RTL8139/8139C/8139C+"},
> {0x10ec, 0x8169, OK, "Realtek", "RTL8169"},
> {},
>
> };
>
> @@ -42,6 +42,8 @@
>
> int nicrealtek_init(void)
>  {
> static uint16_t id;

Why static? It's a new variable, only visible in this function and not
checked before being (re)assigned.
> get_io_perms();
>
> io_base_addr = pcidev_init(PCI_VENDOR_ID_REALTEK, PCI_BASE_ADDRESS_0,
>
> @@ -49,6 +51,16 @@
>
> buses_supported = CHIP_BUSTYPE_PARALLEL;
> id = pcidev_dev->device_id;
> if(id == 0x8139) {
> bios_rom_addr = 0xD4;
> bios_rom_data = 0xD7;
> }
> else {
> bios_rom_addr = 0x30;
> bios_rom_data = 0x33;
> }
> return 0;
>
> }
>
> @@ -61,6 +73,9 @@
>
> buses_supported = CHIP_BUSTYPE_PARALLEL;
> bios_rom_addr = 0xD4;
> bios_rom_data = 0xD7;
> return 0;
>
> }
>
> @@ -78,12 +93,12 @@
>     * enable software access.
>     */
>    OUTL(((uint32_t)addr & 0x01FFFF) | 0x0A0000 | (val << 24),
>     io_base_addr + BIOS_ROM_ADDR);
>     io_base_addr + bios_rom_addr);
> /* Output addr and data, set WE to 1, set OE to 1, set CS to 1,
> enable software access.
> */
>  OUTL(((uint32_t)addr & 0x01FFFF) | 0x1E0000 | (val << 24),
>     io_base_addr + BIOS_ROM_ADDR);
>     io_base_addr + bios_rom_addr);
>
> }
>
> uint8_t nicrealtek_chip_readb(const chipaddr addr)
> @@ -92,20 +107,20 @@
>
> /* FIXME: Can we skip reading the old data and simply use 0? */
>    /* Read old data. */
> val = INB(io_base_addr + BIOS_ROM_DATA);
> val = INB(io_base_addr + bios_rom_data);
> /* Output new addr and old data, set WE to 1, set OE to 0, set CS to 0,
> enable software access.
> */
> OUTL(((uint32_t)addr & 0x01FFFF) | 0x060000 | (val << 24),
>     io_base_addr + BIOS_ROM_ADDR);
>     io_base_addr + bios_rom_addr);
>
> /* Read new data. */
> val = INB(io_base_addr + BIOS_ROM_DATA);
> val = INB(io_base_addr + bios_rom_data);
> /* Output addr and new data, set WE to 1, set OE to 1, set CS to 1,
> enable software access.
> */
> OUTL(((uint32_t)addr & 0x01FFFF) | 0x1E0000 | (val << 24),
>     io_base_addr + BIOS_ROM_ADDR);
>     io_base_addr + bios_rom_addr);
>
> return val;
>
> }

Otherwise Acked-by: Mathias Krause