Re: [flashrom] Board identification patch for the MSC Q7 Tunnel Creek board

[Carl-Daniel Hailfinger]

Hi,

thanks for this patch.

Am 27.01.2012 18:11 schrieb Idwer Vollering:
> From: "Feldschmid, Ingo" 
> Date: Fri, 27 Jan 2012 15:05:22 +0100
>
> When trying to flash the bios chip on our Tunnel Creek module, there always
> is the “you may be running flashrom on an unsupported laptop” warning.
> Since our board is neither unsupported nor a laptop, I would love to patch
> that.
>
> Basically, I added identification for our module to the board_matches list
> for phase P2, which is just before the laptop / laptop ok check. In the
> board function that is called from this list, I just set the global
> variable “is_laptop” to zero. This will prevent the laptop warning for
> tunnel creek boards which have the dmi string “Q7-TCTC”.
>
> I am not sure if this mechanism is the correct way to do this, since
> entries in this lists are normally used to execute board-specific
> flash-chip-enable functions.

This is exactly what phase 2 board enables are designed for. I'm happy
it works as designed.
> Of course, an easier path would be to just set the chassis information to
> something like “Desktop”, but we are not building complete boards, just a
> COM (Computer-On-Module). Therefore, the module could be used in all
> different kinds of chassis, but the flash chip update mechanism would
> always work, since everything relevant is on the module.

Is there a chassis type which is called Computer-On-Module or something
like that? Then again, the situation here is complicated because even if
such a chassis type can be encoded in DMI data, flashrom can't really
know if flashing can be done safely.
> The attached patch has been built against flashrom trunk version 1486.
>
> I have also included two flashrom logs, just to prove that we can
> flawlessly read and write the bios flash chip on our module.
>
> Signed-off-by: Ingo Feldschmid
> Index: board_enable.c
>
> --- board_enable.c	(revision 1486)
> +++ board_enable.c	(working copy)
> @@ -2045,6 +2045,17 @@
>    return it87_gpio_set(63, 1);
>  }
>
> +/*
> Suited for:
> MSC Q7 Tunnel Creek Module, phase 2 detection patch
> */
>
> +static int msc_q7tctc_p2(void)
> +{
>    /* we are a computer-on-module, not a laptop */
>    is_laptop = 0;
>    return 1;
>
> +}
>
> #endif
>
> /*
> @@ -2220,6 +2231,7 @@
>    {0x1106, 0x0259, 0x1106, 0xAA07,  0x1106, 0x3227, 0x1106, 0xAA07, NULL,         NULL, NULL,           P3, "VIA",         "EPIA EK",               0,   NT, via_vt823x_gpio9_raise},
>    {0x1106, 0x3177, 0x1106, 0xAA01,  0x1106, 0x3123, 0x1106, 0xAA01, NULL,         NULL, NULL,           P3, "VIA",         "EPIA M/MII/...",        0,   OK, via_vt823x_gpio15_raise},
>    {0x1106, 0x0259, 0x1106, 0x3227,  0x1106, 0x3065, 0x1106, 0x3149, NULL,         NULL, NULL,           P3, "VIA",         "EPIA-N/NL",             0,   OK, via_vt823x_gpio9_raise},
> {0x8086, 0x8186, 0x8086, 0x8186,  0x0000, 0x0000, 0x0000, 0x0000, "^Q7-TCTC",   NULL, NULL,           P2, "MSC",         "Q7-TCTC",               0,   OK, msc_q7tctc_p2},
>
> #endif
>    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL, NULL,           P3, NULL,          NULL,                    0,   NT, NULL}, /* end marker */
>  };

The only question I have is whether we can make the match more specific:
- Second set of PCI IDs, preferably one which has a vendor ID of MSC-GE
- Use "^Q7-TCTC$" instead

Otherwise it looks good. Once the question above is answered, this patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

Board identification patch for the MSC Q7 Tunnel Creek board

[Stefan Tauner]

On Sun, 29 Jan 2012 03:21:46 +0100
Carl-Daniel Hailfinger  wrote:
> Hi,
>
> thanks for this patch.
>
> Am 27.01.2012 18:11 schrieb Idwer Vollering:
> > From: "Feldschmid, Ingo" 
> > Date: Fri, 27 Jan 2012 15:05:22 +0100
> >
> > […]
> > Of course, an easier path would be to just set the chassis information to
> > something like “Desktop”, but we are not building complete boards, just a
> > COM (Computer-On-Module). Therefore, the module could be used in all
> > different kinds of chassis, but the flash chip update mechanism would
> > always work, since everything relevant is on the module.
>
> Is there a chassis type which is called Computer-On-Module or something
> like that? Then again, the situation here is complicated because even if
> such a chassis type can be encoded in DMI data, flashrom can't really
> know if flashing can be done safely.

there are a few chassis types that might match, but nothing spot-on and
there is lot of room for interpretation anyway. and as you said it
would not buy us anything, because we have not enough data to deduce if
there are significant numbers of "funny" configurations with ECs etc.
the P2 enable seems to be the way to go for such devices.
> > The attached patch has been built against flashrom trunk version 1486.
> >
> > I have also included two flashrom logs, just to prove that we can
> > flawlessly read and write the bios flash chip on our module.
> >
> > Signed-off-by: Ingo Feldschmid
> > Index: board_enable.c
> >
> > --- board_enable.c	(revision 1486)
> > +++ board_enable.c	(working copy)
> > @@ -2045,6 +2045,17 @@
> >    return it87_gpio_set(63, 1);
> >  }
> >
> > +/*
> > Suited for:
> > MSC Q7 Tunnel Creek Module, phase 2 detection patch
> > */
> >
> > +static int msc_q7tctc_p2(void)

please rename to something more generic, maybe "p2_not_a_laptop", so
that we can reuse the function.
> > +{
> >    /* we are a computer-on-module, not a laptop */

change the comment accordingly.
> >    is_laptop = 0;
> >    return 1;
> >
> > +}
> >
> > #endif
> >
> > /*
> > @@ -2220,6 +2231,7 @@
> >    {0x1106, 0x0259, 0x1106, 0xAA07,  0x1106, 0x3227, 0x1106, 0xAA07, NULL,         NULL, NULL,           P3, "VIA",         "EPIA EK",               0,   NT, via_vt823x_gpio9_raise},
> >    {0x1106, 0x3177, 0x1106, 0xAA01,  0x1106, 0x3123, 0x1106, 0xAA01, NULL,         NULL, NULL,           P3, "VIA",         "EPIA M/MII/...",        0,   OK, via_vt823x_gpio15_raise},
> >    {0x1106, 0x0259, 0x1106, 0x3227,  0x1106, 0x3065, 0x1106, 0x3149, NULL,         NULL, NULL,           P3, "VIA",         "EPIA-N/NL",             0,   OK, via_vt823x_gpio9_raise},
> > {0x8086, 0x8186, 0x8086, 0x8186,  0x0000, 0x0000, 0x0000, 0x0000, "^Q7-TCTC",   NULL, NULL,           P2, "MSC",         "Q7-TCTC",               0,   OK, msc_q7tctc_p2},
> >
> > #endif
> >    {     0,      0,      0,      0,       0,      0,      0,      0, NULL,         NULL, NULL,           P3, NULL,          NULL,                    0,   NT, NULL}, /* end marker */
> >  };
>
> The only question I have is whether we can make the match more specific:
> Second set of PCI IDs, preferably one which has a vendor ID of MSC-GE
> Use "^Q7-TCTC$" instead

another minor request: please sort the list by manufacturer (then by
device) name (and change the function name of course).

thanks for contributing!

Board identification patch for the MSC Q7 Tunnel Creek board

[Stefan Tauner]

ah and i forgot: an entry in print.c that the board is supported might
also be a good idea.