[1/1] ACPI: EC: Allow multibyte access to EC (v2)

Submitted by Andy Whitcroft on April 13, 2010, 9:54 a.m.

Details

Message ID 1271152487-4339-2-git-send-email-apw@canonical.com
State Superseded
Delegated to: Andy Whitcroft
Headers show

Commit Message

Andy Whitcroft April 13, 2010, 9:54 a.m.
Allow more than a single byte to be read or written to the EC.  This
is needed by some Dell BIOSs.

Based on the original version by Alexey Starikovskiy.

BugLink: http://bugs.launchpad.net/bugs/526354
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/acpi/acpica/exprep.c |   12 ++++++++++++
 drivers/acpi/ec.c            |    7 ++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

Comments

Stefan Bader April 13, 2010, 10:28 a.m.
All in all this seems to be the better approach

Andy Whitcroft wrote:
> Allow more than a single byte to be read or written to the EC.  This
> is needed by some Dell BIOSs.
> 
> Based on the original version by Alexey Starikovskiy.
> 
> BugLink: http://bugs.launchpad.net/bugs/526354
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/acpi/acpica/exprep.c |   12 ++++++++++++
>  drivers/acpi/ec.c            |    7 ++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/exprep.c b/drivers/acpi/acpica/exprep.c
> index 52fec07..c02d543 100644
> --- a/drivers/acpi/acpica/exprep.c
> +++ b/drivers/acpi/acpica/exprep.c
> @@ -468,6 +468,18 @@ acpi_status acpi_ex_prep_field_value(struct acpi_create_field_info *info)
>  
>  		acpi_ut_add_reference(obj_desc->field.region_obj);
>  
> +		/* allow full data read from EC address space */
> +		if (obj_desc->field.region_obj->region.space_id ==
> +			ACPI_ADR_SPACE_EC) {
> +			if (obj_desc->common_field.bit_length > 8)
> +				obj_desc->common_field.access_bit_width =
> +				ACPI_ROUND_UP(obj_desc->common_field.
> +							bit_length, 8);
> +				obj_desc->common_field.access_byte_width =
> +				ACPI_DIV_8(obj_desc->common_field.
> +							access_bit_width);
> +		}

It would be interesting to find out whether it would not be better to have the
bit length _always_ roounded up to a multiple of 8 and not only when multiple
bytes are needed.

> +
>  		ACPI_DEBUG_PRINT((ACPI_DB_BFIELD,
>  				  "RegionField: BitOff %X, Off %X, Gran %X, Region %p\n",
>  				  obj_desc->field.start_field_bit_offset,
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index f1670e0..b654a96 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -601,10 +601,7 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
>  	if (function != ACPI_READ && function != ACPI_WRITE)
>  		return AE_BAD_PARAMETER;
>  
> -	if (bits != 8 && acpi_strict)
> -		return AE_BAD_PARAMETER;
> -
> -	if (EC_FLAGS_MSI)
> +	if (EC_FLAGS_MSI || bits > 8)
>  		acpi_ec_burst_enable(ec);
>  
>  	if (function == ACPI_READ) {
> @@ -626,7 +623,7 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
>  		}
>  	}
>  
> -	if (EC_FLAGS_MSI)
> +	if (EC_FLAGS_MSI || bits > 8)
>  		acpi_ec_burst_disable(ec);
>  
>  	switch (result) {


As the new loop had issues with not reading anything when the bit width was
below 8 bits and also assumes little endianess (can we assume acpi is only
running on x86?) it sounds generally to be better to leave it as is.
Leann Ogasawara April 13, 2010, 4:03 p.m.
On Tue, 2010-04-13 at 10:54 +0100, Andy Whitcroft wrote:
> Allow more than a single byte to be read or written to the EC.  This
> is needed by some Dell BIOSs.
> 
> Based on the original version by Alexey Starikovskiy.
> 
> BugLink: http://bugs.launchpad.net/bugs/526354
> Signed-off-by: Andy Whitcroft <apw@canonical.com>

Indeed this appears to be a modified version of the original patch and
initial testing seems positive.

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

> ---
>  drivers/acpi/acpica/exprep.c |   12 ++++++++++++
>  drivers/acpi/ec.c            |    7 ++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/exprep.c b/drivers/acpi/acpica/exprep.c
> index 52fec07..c02d543 100644
> --- a/drivers/acpi/acpica/exprep.c
> +++ b/drivers/acpi/acpica/exprep.c
> @@ -468,6 +468,18 @@ acpi_status acpi_ex_prep_field_value(struct acpi_create_field_info *info)
>  
>  		acpi_ut_add_reference(obj_desc->field.region_obj);
>  
> +		/* allow full data read from EC address space */
> +		if (obj_desc->field.region_obj->region.space_id ==
> +			ACPI_ADR_SPACE_EC) {
> +			if (obj_desc->common_field.bit_length > 8)
> +				obj_desc->common_field.access_bit_width =
> +				ACPI_ROUND_UP(obj_desc->common_field.
> +							bit_length, 8);
> +				obj_desc->common_field.access_byte_width =
> +				ACPI_DIV_8(obj_desc->common_field.
> +							access_bit_width);
> +		}
> +
>  		ACPI_DEBUG_PRINT((ACPI_DB_BFIELD,
>  				  "RegionField: BitOff %X, Off %X, Gran %X, Region %p\n",
>  				  obj_desc->field.start_field_bit_offset,
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index f1670e0..b654a96 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -601,10 +601,7 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
>  	if (function != ACPI_READ && function != ACPI_WRITE)
>  		return AE_BAD_PARAMETER;
>  
> -	if (bits != 8 && acpi_strict)
> -		return AE_BAD_PARAMETER;
> -
> -	if (EC_FLAGS_MSI)
> +	if (EC_FLAGS_MSI || bits > 8)
>  		acpi_ec_burst_enable(ec);
>  
>  	if (function == ACPI_READ) {
> @@ -626,7 +623,7 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
>  		}
>  	}
>  
> -	if (EC_FLAGS_MSI)
> +	if (EC_FLAGS_MSI || bits > 8)
>  		acpi_ec_burst_disable(ec);
>  
>  	switch (result) {
> -- 
> 1.7.0
> 
>

Patch hide | download patch | download mbox

diff --git a/drivers/acpi/acpica/exprep.c b/drivers/acpi/acpica/exprep.c
index 52fec07..c02d543 100644
--- a/drivers/acpi/acpica/exprep.c
+++ b/drivers/acpi/acpica/exprep.c
@@ -468,6 +468,18 @@  acpi_status acpi_ex_prep_field_value(struct acpi_create_field_info *info)
 
 		acpi_ut_add_reference(obj_desc->field.region_obj);
 
+		/* allow full data read from EC address space */
+		if (obj_desc->field.region_obj->region.space_id ==
+			ACPI_ADR_SPACE_EC) {
+			if (obj_desc->common_field.bit_length > 8)
+				obj_desc->common_field.access_bit_width =
+				ACPI_ROUND_UP(obj_desc->common_field.
+							bit_length, 8);
+				obj_desc->common_field.access_byte_width =
+				ACPI_DIV_8(obj_desc->common_field.
+							access_bit_width);
+		}
+
 		ACPI_DEBUG_PRINT((ACPI_DB_BFIELD,
 				  "RegionField: BitOff %X, Off %X, Gran %X, Region %p\n",
 				  obj_desc->field.start_field_bit_offset,
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f1670e0..b654a96 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -601,10 +601,7 @@  acpi_ec_space_handler(u32 function, acpi_physical_address address,
 	if (function != ACPI_READ && function != ACPI_WRITE)
 		return AE_BAD_PARAMETER;
 
-	if (bits != 8 && acpi_strict)
-		return AE_BAD_PARAMETER;
-
-	if (EC_FLAGS_MSI)
+	if (EC_FLAGS_MSI || bits > 8)
 		acpi_ec_burst_enable(ec);
 
 	if (function == ACPI_READ) {
@@ -626,7 +623,7 @@  acpi_ec_space_handler(u32 function, acpi_physical_address address,
 		}
 	}
 
-	if (EC_FLAGS_MSI)
+	if (EC_FLAGS_MSI || bits > 8)
 		acpi_ec_burst_disable(ec);
 
 	switch (result) {