Patchwork Neuter probe for ITE SuperIO

login
register
about
Submitter Ed Swierk
Date 2010-06-23 06:49:57
Message ID <1277275797.20870.14.camel@localhost.localdomain>
Download mbox | patch
Permalink /patch/1543/
State Bitrotted
Headers show

Comments

Ed Swierk - 2010-06-23 06:49:57
If the system has an SMSC LPC47N217N SuperIO, enter_conf_mode_ite()
enters configuration mode, and then exit_conf_mode_ite() kills power to
the serial UART.

I don't know if there is a proper way to detect what kind of SuperIO the
system has, but probe_superio_ite() is unsafe in its current form.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Carl-Daniel Hailfinger - 2010-06-23 08:30:42
Hi Ed,

On 23.06.2010 08:49, Ed Swierk wrote:
> If the system has an SMSC LPC47N217N SuperIO, enter_conf_mode_ite()
> enters configuration mode, and then exit_conf_mode_ite() kills power to
> the serial UART.
>
> I don't know if there is a proper way to detect what kind of SuperIO the
> system has, but probe_superio_ite() is unsafe in its current form.
>   

This change breaks a few dozen Gigabyte boards with AMD 700 chipset or
Nvidia MCP55 chipset or SiS 761 chipset (and boards from other vendors),
and can't be applied as is.
It also breaks the pending IT8705 autodetection patch which adds support
for a huge number of boards from the beginning of the LPC era.


> Index: flashrom-0.9.2-r1057/internal.c
> ===================================================================
> --- flashrom-0.9.2-r1057.orig/internal.c
> +++ flashrom-0.9.2-r1057/internal.c
> @@ -104,7 +104,10 @@ struct superio superio = {};
>  
>  void probe_superio(void)
>  {
> +#if 0
> +	/* ITE probe causes SMSC LPC47N217 to power off the serial UART! */
>  	superio = probe_superio_ite();
> +#endif
>  #if 0
>  	/* Winbond Super I/O code is not yet available. */
>  	if (superio.vendor == SUPERIO_VENDOR_NONE)
>   

Due to the reasons stated above, this patch can't be applied, but there
are two options which might help:
1. Blacklist ITE Super I/O probing on your board.
This would mean we have to change the code flow to run
chipset_flash_enable(); board_flash_enable(); probe_superio();
it87xx_probe_spi_flash(); instead of the current order probe_superio();
chipset_flash_enable(); it87xx_probe_spi_flash(); board_flash_enable();

2. Add probing support for your Super I/O.
If we probe for the SMSC LPC47N217 before we probe for IT87*, the IT87*
probing will never be executed because we get a match before, and the
code is designed to bail out once the first Super I/O is detected.

Either option looks OK for me, but I personally prefer option 2 because
it will save us the headache of having to individually blacklist every
system with LPC47N217.

In theory, running superiotool should have the same effect as running
current flashrom. Can you please check that?

Regards,
Carl-Daniel
Michael Karcher - 2010-06-23 10:02:39
Am Mittwoch, den 23.06.2010, 10:30 +0200 schrieb Carl-Daniel Hailfinger:
> Due to the reasons stated above, this patch can't be applied, but there
> are two options which might help:
> 1. Blacklist ITE Super I/O probing on your board.
> This would mean we have to change the code flow to run
> chipset_flash_enable(); board_flash_enable(); probe_superio();
> it87xx_probe_spi_flash(); instead of the current order probe_superio();
> chipset_flash_enable(); it87xx_probe_spi_flash(); board_flash_enable();

This breaks the intention of the generic superio code in patchwork. It
assumes you can have a generic "superio_gpio_set" which is called from
the board_enable function, which needs that probe_superio() is run
before. I thus also support option 2 (snipped) that tries to check for
the SMC chip and avoid the ITE probe in that case.

A third route would be (if possible, I didn't check datasheets) to write
a exit_conf_mode_ite which still works on the ITE chip, and does nothing
on the SMC chip.

Regards,
  Michael Karcher
Carl-Daniel Hailfinger - 2010-08-16 16:15:12
Hi Ed,

I would like to get this issue fixed before the next flashrom release,
but I didn't see any response from you yet.

On 23.06.2010 10:30, Carl-Daniel Hailfinger wrote:
> On 23.06.2010 08:49, Ed Swierk wrote:
>   
>> If the system has an SMSC LPC47N217N SuperIO, enter_conf_mode_ite()
>> enters configuration mode, and then exit_conf_mode_ite() kills power to
>> the serial UART.
>>
>> I don't know if there is a proper way to detect what kind of SuperIO the
>> system has, but probe_superio_ite() is unsafe in its current form.
>>     
> In theory, running superiotool should have the same effect as running
> current flashrom. Can you please check that?
>   

If running superiotool has no bad side effects, I plan to just copy that
code over to flashrom and be happy.

Regards,
Carl-Daniel
Carl-Daniel Hailfinger - 2010-08-16 18:19:36
[resending because the original mail to Ed bounced]

On 16.08.2010 18:15, Carl-Daniel Hailfinger wrote:
> Hi Ed,
>
> I would like to get this issue fixed before the next flashrom release,
> but I didn't see any response from you yet.
>
> On 23.06.2010 10:30, Carl-Daniel Hailfinger wrote:
>   
>> On 23.06.2010 08:49, Ed Swierk wrote:
>>   
>>     
>>> If the system has an SMSC LPC47N217N SuperIO, enter_conf_mode_ite()
>>> enters configuration mode, and then exit_conf_mode_ite() kills power to
>>> the serial UART.
>>>
>>> I don't know if there is a proper way to detect what kind of SuperIO the
>>> system has, but probe_superio_ite() is unsafe in its current form.
>>>     
>>>       
>> In theory, running superiotool should have the same effect as running
>> current flashrom. Can you please check that?
>>   
>>     
>
> If running superiotool has no bad side effects, I plan to just copy that
> code over to flashrom and be happy.
>
> Regards,
> Carl-Daniel
>
>
Ed Swierk - 2010-08-16 22:25:06
> On 16.08.2010 18:15, Carl-Daniel Hailfinger wrote:
>> Hi Ed,
>>
>> I would like to get this issue fixed before the next flashrom release,
>> but I didn't see any response from you yet.
>>
>> On 23.06.2010 10:30, Carl-Daniel Hailfinger wrote:
>>
>>> On 23.06.2010 08:49, Ed Swierk wrote:
>>>
>>>
>>>> If the system has an SMSC LPC47N217N SuperIO, enter_conf_mode_ite()
>>>> enters configuration mode, and then exit_conf_mode_ite() kills power to
>>>> the serial UART.
>>>>
>>>> I don't know if there is a proper way to detect what kind of SuperIO the
>>>> system has, but probe_superio_ite() is unsafe in its current form.
>>>>
>>>>
>>> In theory, running superiotool should have the same effect as running
>>> current flashrom. Can you please check that?
>>>
>>>
>>
>> If running superiotool has no bad side effects, I plan to just copy that
>> code over to flashrom and be happy.

Sorry, I didn't get a chance to try out superiotool before I left my
previous employer, and I no longer have access to the board with this
SuperIO.

If you have a patch to flashrom that does the same thing as
superiotool, I can ask my former colleagues to give it a try.

--Ed

Patch

Index: flashrom-0.9.2-r1057/internal.c
===================================================================
--- flashrom-0.9.2-r1057.orig/internal.c
+++ flashrom-0.9.2-r1057/internal.c
@@ -104,7 +104,10 @@  struct superio superio = {};
 
 void probe_superio(void)
 {
+#if 0
+	/* ITE probe causes SMSC LPC47N217 to power off the serial UART! */
 	superio = probe_superio_ite();
+#endif
 #if 0
 	/* Winbond Super I/O code is not yet available. */
 	if (superio.vendor == SUPERIO_VENDOR_NONE)