Patchwork [3/3] X60: use I/O 0x1600/0x1604 for ACPI accesses

login
register
about
Submitter Sven Schnelle
Date 2011-03-12 00:18:07
Message ID <1299889087-3174-3-git-send-email-svens@stackframe.org>
Download mbox | patch
Permalink /patch/2773/
State New
Headers show

Comments

Stefan Reinauer - 2011-03-11 23:40:49
* Sven Schnelle <svens@stackframe.org> [110312 01:18]:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  src/mainboard/lenovo/x60/mainboard.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
> index b8e6a49..79e4a83 100644
> --- a/src/mainboard/lenovo/x60/mainboard.c
> +++ b/src/mainboard/lenovo/x60/mainboard.c
> @@ -50,6 +50,11 @@ static void wlan_enable(void)
>  
>  static void mainboard_enable(device_t dev)
>  {
> +	/* Enable 1600/1604 register pair */
> +	ec_set_bit(0x00, 0x05);
> +	/* switch to just enabled registers for ACPI */
> +	ec_set_ports(0x1604, 0x1600);
> +

I think you only need to use the high ports in SMM code to avoid race
conditions.

Stefan
Stefan Reinauer - 2011-03-11 23:41:40
* Sven Schnelle <svens@stackframe.org> [110312 01:18]:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  src/mainboard/lenovo/x60/mainboard.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
> index b8e6a49..79e4a83 100644
> --- a/src/mainboard/lenovo/x60/mainboard.c
> +++ b/src/mainboard/lenovo/x60/mainboard.c
> @@ -50,6 +50,11 @@ static void wlan_enable(void)
>  
>  static void mainboard_enable(device_t dev)
>  {
> +	/* Enable 1600/1604 register pair */
> +	ec_set_bit(0x00, 0x05);
> +	/* switch to just enabled registers for ACPI */

I think the comment is misleading. ACPI has nothing to do with this.

> +	ec_set_ports(0x1604, 0x1600);
> +
Sven Schnelle - 2011-03-12 00:18:07
Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 src/mainboard/lenovo/x60/mainboard.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Sven Schnelle - 2011-03-12 08:16:45
Stefan Reinauer <stefan.reinauer@coreboot.org> writes:

> * Sven Schnelle <svens@stackframe.org> [110312 01:18]:
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>>  src/mainboard/lenovo/x60/mainboard.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>> 
>> diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
>> index b8e6a49..79e4a83 100644
>> --- a/src/mainboard/lenovo/x60/mainboard.c
>> +++ b/src/mainboard/lenovo/x60/mainboard.c
>> @@ -50,6 +50,11 @@ static void wlan_enable(void)
>>  
>>  static void mainboard_enable(device_t dev)
>>  {
>> +	/* Enable 1600/1604 register pair */
>> +	ec_set_bit(0x00, 0x05);
>> +	/* switch to just enabled registers for ACPI */
>> +	ec_set_ports(0x1604, 0x1600);
>> +
>
> I think you only need to use the high ports in SMM code to avoid race
> conditions.

Yes. But the original BIOS switches to those ports at the same time, and
i want to do it the same way. Makes comparing bus cycles easier, and has
no disadvantages IMHO.
Sven Schnelle - 2011-03-12 08:20:35
Stefan Reinauer <stefan.reinauer@coreboot.org> writes:

> * Sven Schnelle <svens@stackframe.org> [110312 01:18]:
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>>  src/mainboard/lenovo/x60/mainboard.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>> 
>> diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
>> index b8e6a49..79e4a83 100644
>> --- a/src/mainboard/lenovo/x60/mainboard.c
>> +++ b/src/mainboard/lenovo/x60/mainboard.c
>> @@ -50,6 +50,11 @@ static void wlan_enable(void)
>>  
>>  static void mainboard_enable(device_t dev)
>>  {
>> +	/* Enable 1600/1604 register pair */
>> +	ec_set_bit(0x00, 0x05);
>> +	/* switch to just enabled registers for ACPI */
>
> I think the comment is misleading. ACPI has nothing to do with this.

You're right. This is obviously wrong. Those are the EC registers, not
ACPI ;)

Sven.
Stefan Reinauer - 2011-03-13 22:59:03
* Sven Schnelle <svens@stackframe.org> [110312 09:16]:
> Stefan Reinauer <stefan.reinauer@coreboot.org> writes:
> 
> > * Sven Schnelle <svens@stackframe.org> [110312 01:18]:
> >> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> >> ---
> >>  src/mainboard/lenovo/x60/mainboard.c |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
> >> index b8e6a49..79e4a83 100644
> >> --- a/src/mainboard/lenovo/x60/mainboard.c
> >> +++ b/src/mainboard/lenovo/x60/mainboard.c
> >> @@ -50,6 +50,11 @@ static void wlan_enable(void)
> >>  
> >>  static void mainboard_enable(device_t dev)
> >>  {
> >> +	/* Enable 1600/1604 register pair */
> >> +	ec_set_bit(0x00, 0x05);
> >> +	/* switch to just enabled registers for ACPI */
> >> +	ec_set_ports(0x1604, 0x1600);
> >> +
> >
> > I think you only need to use the high ports in SMM code to avoid race
> > conditions.
> 
> Yes. But the original BIOS switches to those ports at the same time, and
> i want to do it the same way. Makes comparing bus cycles easier, and has
> no disadvantages IMHO.

It does have a disadvantage if there is an SMI that accesses the EC
while you access the EC in your code, too.

Also be sure to explicitly re-set the right address in SMM mode,
otherwise SMM will still use the original registers as SMM uses an extra
copy of the EC access code.

Last but not least, make sure that 0x1600 is blocked from use of the
resource allocator for anything else.

Patch

diff --git a/src/mainboard/lenovo/x60/mainboard.c b/src/mainboard/lenovo/x60/mainboard.c
index b8e6a49..79e4a83 100644
--- a/src/mainboard/lenovo/x60/mainboard.c
+++ b/src/mainboard/lenovo/x60/mainboard.c
@@ -50,6 +50,11 @@  static void wlan_enable(void)
 
 static void mainboard_enable(device_t dev)
 {
+	/* Enable 1600/1604 register pair */
+	ec_set_bit(0x00, 0x05);
+	/* switch to just enabled registers for ACPI */
+	ec_set_ports(0x1604, 0x1600);
+
 	backlight_enable();
 	trackpoint_enable();
 	/* FIXME: this should be ACPI's task