Re: [PATCH v1 4/4] cros_ec_lpc: add quirks for the Framework Laptop

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/5/2023 11:07, Dustin L. Howett wrote:
> The original Framework Laptop contains a Microchip EC, but the newer
> ones have switched to an NPCX EC which uses the standard linear MMIO
> protocol for host commands and EC memory.
> 
> However, those newer Framework Laptops diverge from the standard
> ChromeOS EC in two important ways:
> 
> 1. The ACPI table for PNP0C09 only claims I/O ports 0x800 to 0x8FE
>     instead of the traditional 0x8FF
> 2. The MMIO region for EC memory begins at port 0xE00 rather than the
>     traditional 0x900.
> 
> cros_ec_lpc's quirks system addresses both of these issues.
> 
> Signed-off-by: Dustin L. Howett <dustin@howett.net>
> ---
>   drivers/platform/chrome/cros_ec_lpc.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index c06575625d2f..66285c66ff22 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -537,6 +537,13 @@ static const struct acpi_device_id cros_ec_lpc_acpi_device_ids[] = {
>   };
>   MODULE_DEVICE_TABLE(acpi, cros_ec_lpc_acpi_device_ids);
>   
> +static const struct lpc_driver_data framework_laptop_lpc_driver_data __initconst = {
> +	.quirks =
> +		CROS_EC_LPC_QUIRK_REMAP_MEMORY |
> +		CROS_EC_LPC_QUIRK_SHORT_HOSTCMD_RESERVATION,
> +	.quirk_mmio_memory_base = 0xE00,
> +};

I think it would be really helpful for anyone that looks at this commit 
and scratches their head to have a decoder here as a comment along these 
lines to explain how this works (correct it for any inaccuracies please):

/*
  * 13" Gen 11 Intel uses Microchip EC
  * 13" Gen 12 Intel and newer use NPCX EC
  * 13"/16" Gen 1 AMD and newer use NPCX EC
  * These quirks set up the values used in cros_ec_lpc_probe() to be 
able to discover them properly with accordance to the fallback probing 
logic.
  */

If those comments grow stale because Gen 14 Intel or Gen 2 AMD change 
offsets or vendors, then the comment can be changed too, or a second 
quirk be introduced.

In the event that these need to change again in any way in the future 
models, it may be better to introduce a _DSD in the BIOS to allow the 
kernel to be able to discover values that would otherwise be included in 
a quirk.

> +
>   static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
>   	{
>   		/*
> @@ -596,6 +603,7 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
>   			DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
>   			DMI_MATCH(DMI_PRODUCT_NAME, "Laptop"),
>   		},
> +		.driver_data = (void *)&framework_laptop_lpc_driver_data,
>   	},
>   	{ /* sentinel */ }
>   };