[PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[Dustin L. Howett]

This patch series adds support for the Framework Laptop to the cros_ec
LPC driver.

The Framework Laptop is a non-Chromebook laptop that uses the ChromeOS
Embedded Controller. Since the machine was designed to present a more
normal device profile, it does not report all 512 I/O ports that are
typically used by cros_ec_lpcs. Because of this, changes to the driver's
port reservation scheme were required.

Dustin L. Howett (2):
  platform/chrome: cros-ec: detect the Framework Laptop
  platform/chrome: reserve only the I/O ports required for the MEC EC

 drivers/platform/chrome/cros_ec_lpc.c          |   47 ++++++++++-----
 include/linux/platform_data/cros_ec_commands.h |    4 +
 2 files changed, 38 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH 1/2] platform/chrome: cros-ec: detect the Framework Laptop

[Dustin L. Howett]

The Framework Laptop identifies itself in DMI with manufacturer
"Framework" and product "Laptop".

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 d6306d2a096f..458eb59db2ff 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -500,6 +500,14 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Glimmer"),
 		},
 	},
+	/* A small number of non-Chromebook/box machines also use the ChromeOS EC */
+	{
+		/* the Framework Laptop */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Laptop"),
+		},
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(dmi, cros_ec_lpc_dmi_table);
-- 
2.34.1

[PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[Dustin L. Howett]

Some ChromeOS EC devices (such as the Framework Laptop) only map I/O
ports 0x800-0x807. Making the larger reservation required by the non-MEC
LPC (the 0xFF ports for the memory map, and the 0xFF ports for the
parameter region) is non-viable on these devices.

Since we probe the MEC EC first, we can get away with a smaller
reservation that covers the MEC EC ports. If we fall back to classic
LPC, we can grow the reservation to cover the memory map and the
parameter region.

Signed-off-by: Dustin L. Howett <dustin@howett.net>
---
 drivers/platform/chrome/cros_ec_lpc.c         | 39 ++++++++++++-------
 .../linux/platform_data/cros_ec_commands.h    |  4 ++
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 458eb59db2ff..06fdfe365710 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -341,9 +341,14 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	u8 buf[2];
 	int irq, ret;
 
-	if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
-				 dev_name(dev))) {
-		dev_err(dev, "couldn't reserve memmap region\n");
+	/*
+	 * The Framework Laptop (and possibly other non-ChromeOS devices)
+	 * only exposes the eight I/O ports that are required for the Microchip EC.
+	 * Requesting a larger reservation will fail.
+	 */
+	if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
+				 EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
+		dev_err(dev, "couldn't reserve MEC region\n");
 		return -EBUSY;
 	}
 
@@ -357,6 +362,12 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
 	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
 	if (buf[0] != 'E' || buf[1] != 'C') {
+		if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
+					 dev_name(dev))) {
+			dev_err(dev, "couldn't reserve memmap region\n");
+			return -EBUSY;
+		}
+
 		/* Re-assign read/write operations for the non MEC variant */
 		cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
 		cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
@@ -366,17 +377,19 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 			dev_err(dev, "EC ID not detected\n");
 			return -ENODEV;
 		}
-	}
 
-	if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
-				 EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
-		dev_err(dev, "couldn't reserve region0\n");
-		return -EBUSY;
-	}
-	if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
-				 EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
-		dev_err(dev, "couldn't reserve region1\n");
-		return -EBUSY;
+		/* Reserve the remaining I/O ports required by the non-MEC protocol. */
+		if (!devm_request_region(dev, EC_HOST_CMD_REGION0 + EC_HOST_CMD_MEC_REGION_SIZE,
+					 EC_HOST_CMD_REGION_SIZE - EC_HOST_CMD_MEC_REGION_SIZE,
+					 dev_name(dev))) {
+			dev_err(dev, "couldn't reserve remainder of region0\n");
+			return -EBUSY;
+		}
+		if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
+					 EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
+			dev_err(dev, "couldn't reserve region1\n");
+			return -EBUSY;
+		}
 	}
 
 	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 271bd87bff0a..a85b1176e6c0 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -55,6 +55,10 @@
 #define EC_HOST_CMD_REGION0    0x800
 #define EC_HOST_CMD_REGION1    0x880
 #define EC_HOST_CMD_REGION_SIZE 0x80
+/*
+ * Other machines report only the region spanned by the Microchip MEC EC.
+ */
+#define EC_HOST_CMD_MEC_REGION_SIZE 0x08
 
 /* EC command register bit functions */
 #define EC_LPC_CMDR_DATA	BIT(0)  /* Data ready for host to read */
-- 
2.34.1

Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11214 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220105031242.287751-3-dustin@howett.net>
References: <20220105031242.287751-3-dustin@howett.net>
TO: "Dustin L. Howett" <dustin@howett.net>
TO: linux-kernel(a)vger.kernel.org
CC: Benson Leung <bleung@chromium.org>
CC: Guenter Roeck <groeck@chromium.org>
CC: "Dustin L. Howett" <dustin@howett.net>

Hi "Dustin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on linux/master linus/master v5.16-rc8 next-20220107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dustin-L-Howett/platform-chrome-Add-support-for-the-Framework-Laptop/20220105-111340
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: i386-randconfig-m021-20220107 (https://download.01.org/0day-ci/archive/20220109/202201090725.PnArnVcv-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/platform/chrome/cros_ec_lpc.c:382 cros_ec_lpc_probe() warn: consider using resource_size() here

vim +382 drivers/platform/chrome/cros_ec_lpc.c

a6df7798d03b29 Gwendal Grignou        2017-05-16  334  
ec2f33ab582bf6 Bill Richardson        2015-02-02  335  static int cros_ec_lpc_probe(struct platform_device *pdev)
ec2f33ab582bf6 Bill Richardson        2015-02-02  336  {
ec2f33ab582bf6 Bill Richardson        2015-02-02  337  	struct device *dev = &pdev->dev;
a6df7798d03b29 Gwendal Grignou        2017-05-16  338  	struct acpi_device *adev;
a6df7798d03b29 Gwendal Grignou        2017-05-16  339  	acpi_status status;
ec2f33ab582bf6 Bill Richardson        2015-02-02  340  	struct cros_ec_device *ec_dev;
bce70fef727924 Shawn Nematbakhsh      2017-05-16  341  	u8 buf[2];
da1cf5a1cf124f Enrico Granata         2018-10-09  342  	int irq, ret;
ec2f33ab582bf6 Bill Richardson        2015-02-02  343  
268c7bd0087331 Dustin L. Howett       2022-01-04  344  	/*
268c7bd0087331 Dustin L. Howett       2022-01-04  345  	 * The Framework Laptop (and possibly other non-ChromeOS devices)
268c7bd0087331 Dustin L. Howett       2022-01-04  346  	 * only exposes the eight I/O ports that are required for the Microchip EC.
268c7bd0087331 Dustin L. Howett       2022-01-04  347  	 * Requesting a larger reservation will fail.
268c7bd0087331 Dustin L. Howett       2022-01-04  348  	 */
268c7bd0087331 Dustin L. Howett       2022-01-04  349  	if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
268c7bd0087331 Dustin L. Howett       2022-01-04  350  				 EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
268c7bd0087331 Dustin L. Howett       2022-01-04  351  		dev_err(dev, "couldn't reserve MEC region\n");
ec2f33ab582bf6 Bill Richardson        2015-02-02  352  		return -EBUSY;
ec2f33ab582bf6 Bill Richardson        2015-02-02  353  	}
ec2f33ab582bf6 Bill Richardson        2015-02-02  354  
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  355  	/*
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  356  	 * Read the mapped ID twice, the first one is assuming the
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  357  	 * EC is a Microchip Embedded Controller (MEC) variant, if the
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  358  	 * protocol fails, fallback to the non MEC variant and try to
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  359  	 * read again the ID.
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  360  	 */
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  361  	cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  362  	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  363  	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  364  	if (buf[0] != 'E' || buf[1] != 'C') {
268c7bd0087331 Dustin L. Howett       2022-01-04  365  		if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
268c7bd0087331 Dustin L. Howett       2022-01-04  366  					 dev_name(dev))) {
268c7bd0087331 Dustin L. Howett       2022-01-04  367  			dev_err(dev, "couldn't reserve memmap region\n");
268c7bd0087331 Dustin L. Howett       2022-01-04  368  			return -EBUSY;
268c7bd0087331 Dustin L. Howett       2022-01-04  369  		}
268c7bd0087331 Dustin L. Howett       2022-01-04  370  
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  371  		/* Re-assign read/write operations for the non MEC variant */
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  372  		cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  373  		cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  374  		cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2,
22c040fa21b604 Enric Balletbo i Serra 2019-06-14  375  				     buf);
bce70fef727924 Shawn Nematbakhsh      2017-05-16  376  		if (buf[0] != 'E' || buf[1] != 'C') {
ec2f33ab582bf6 Bill Richardson        2015-02-02  377  			dev_err(dev, "EC ID not detected\n");
ec2f33ab582bf6 Bill Richardson        2015-02-02  378  			return -ENODEV;
ec2f33ab582bf6 Bill Richardson        2015-02-02  379  		}
ec2f33ab582bf6 Bill Richardson        2015-02-02  380  
268c7bd0087331 Dustin L. Howett       2022-01-04  381  		/* Reserve the remaining I/O ports required by the non-MEC protocol. */
268c7bd0087331 Dustin L. Howett       2022-01-04 @382  		if (!devm_request_region(dev, EC_HOST_CMD_REGION0 + EC_HOST_CMD_MEC_REGION_SIZE,
268c7bd0087331 Dustin L. Howett       2022-01-04  383  					 EC_HOST_CMD_REGION_SIZE - EC_HOST_CMD_MEC_REGION_SIZE,
268c7bd0087331 Dustin L. Howett       2022-01-04  384  					 dev_name(dev))) {
268c7bd0087331 Dustin L. Howett       2022-01-04  385  			dev_err(dev, "couldn't reserve remainder of region0\n");
ec2f33ab582bf6 Bill Richardson        2015-02-02  386  			return -EBUSY;
ec2f33ab582bf6 Bill Richardson        2015-02-02  387  		}
ec2f33ab582bf6 Bill Richardson        2015-02-02  388  		if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
ec2f33ab582bf6 Bill Richardson        2015-02-02  389  					 EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
ec2f33ab582bf6 Bill Richardson        2015-02-02  390  			dev_err(dev, "couldn't reserve region1\n");
ec2f33ab582bf6 Bill Richardson        2015-02-02  391  			return -EBUSY;
ec2f33ab582bf6 Bill Richardson        2015-02-02  392  		}
268c7bd0087331 Dustin L. Howett       2022-01-04  393  	}
ec2f33ab582bf6 Bill Richardson        2015-02-02  394  
ec2f33ab582bf6 Bill Richardson        2015-02-02  395  	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
ec2f33ab582bf6 Bill Richardson        2015-02-02  396  	if (!ec_dev)
ec2f33ab582bf6 Bill Richardson        2015-02-02  397  		return -ENOMEM;
ec2f33ab582bf6 Bill Richardson        2015-02-02  398  
ec2f33ab582bf6 Bill Richardson        2015-02-02  399  	platform_set_drvdata(pdev, ec_dev);
ec2f33ab582bf6 Bill Richardson        2015-02-02  400  	ec_dev->dev = dev;
ec2f33ab582bf6 Bill Richardson        2015-02-02  401  	ec_dev->phys_name = dev_name(dev);
ec2f33ab582bf6 Bill Richardson        2015-02-02  402  	ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
d365407079d331 Stephen Barber         2015-06-09  403  	ec_dev->pkt_xfer = cros_ec_pkt_xfer_lpc;
ec2f33ab582bf6 Bill Richardson        2015-02-02  404  	ec_dev->cmd_readmem = cros_ec_lpc_readmem;
2c7589af3c4dee Stephen Barber         2015-06-09  405  	ec_dev->din_size = sizeof(struct ec_host_response) +
2c7589af3c4dee Stephen Barber         2015-06-09  406  			   sizeof(struct ec_response_get_protocol_info);
2c7589af3c4dee Stephen Barber         2015-06-09  407  	ec_dev->dout_size = sizeof(struct ec_host_request);
ec2f33ab582bf6 Bill Richardson        2015-02-02  408  
da1cf5a1cf124f Enrico Granata         2018-10-09  409  	/*
da1cf5a1cf124f Enrico Granata         2018-10-09  410  	 * Some boards do not have an IRQ allotted for cros_ec_lpc,
da1cf5a1cf124f Enrico Granata         2018-10-09  411  	 * which makes ENXIO an expected (and safe) scenario.
da1cf5a1cf124f Enrico Granata         2018-10-09  412  	 */
a69b4eebe513b8 Enric Balletbo i Serra 2019-11-29  413  	irq = platform_get_irq_optional(pdev, 0);
da1cf5a1cf124f Enrico Granata         2018-10-09  414  	if (irq > 0)
da1cf5a1cf124f Enrico Granata         2018-10-09  415  		ec_dev->irq = irq;
da1cf5a1cf124f Enrico Granata         2018-10-09  416  	else if (irq != -ENXIO) {
da1cf5a1cf124f Enrico Granata         2018-10-09  417  		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
da1cf5a1cf124f Enrico Granata         2018-10-09  418  		return irq;
da1cf5a1cf124f Enrico Granata         2018-10-09  419  	}
da1cf5a1cf124f Enrico Granata         2018-10-09  420  
ec2f33ab582bf6 Bill Richardson        2015-02-02  421  	ret = cros_ec_register(ec_dev);
ec2f33ab582bf6 Bill Richardson        2015-02-02  422  	if (ret) {
ec2f33ab582bf6 Bill Richardson        2015-02-02  423  		dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
ec2f33ab582bf6 Bill Richardson        2015-02-02  424  		return ret;
ec2f33ab582bf6 Bill Richardson        2015-02-02  425  	}
ec2f33ab582bf6 Bill Richardson        2015-02-02  426  
a6df7798d03b29 Gwendal Grignou        2017-05-16  427  	/*
a6df7798d03b29 Gwendal Grignou        2017-05-16  428  	 * Connect a notify handler to process MKBP messages if we have a
a6df7798d03b29 Gwendal Grignou        2017-05-16  429  	 * companion ACPI device.
a6df7798d03b29 Gwendal Grignou        2017-05-16  430  	 */
a6df7798d03b29 Gwendal Grignou        2017-05-16  431  	adev = ACPI_COMPANION(dev);
a6df7798d03b29 Gwendal Grignou        2017-05-16  432  	if (adev) {
a6df7798d03b29 Gwendal Grignou        2017-05-16  433  		status = acpi_install_notify_handler(adev->handle,
a6df7798d03b29 Gwendal Grignou        2017-05-16  434  						     ACPI_ALL_NOTIFY,
a6df7798d03b29 Gwendal Grignou        2017-05-16  435  						     cros_ec_lpc_acpi_notify,
a6df7798d03b29 Gwendal Grignou        2017-05-16  436  						     ec_dev);
a6df7798d03b29 Gwendal Grignou        2017-05-16  437  		if (ACPI_FAILURE(status))
a6df7798d03b29 Gwendal Grignou        2017-05-16  438  			dev_warn(dev, "Failed to register notifier %08x\n",
a6df7798d03b29 Gwendal Grignou        2017-05-16  439  				 status);
a6df7798d03b29 Gwendal Grignou        2017-05-16  440  	}
a6df7798d03b29 Gwendal Grignou        2017-05-16  441  
ec2f33ab582bf6 Bill Richardson        2015-02-02  442  	return 0;
ec2f33ab582bf6 Bill Richardson        2015-02-02  443  }
ec2f33ab582bf6 Bill Richardson        2015-02-02  444  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

Hi Dustin,

On Tue, Jan 04, 2022 at 09:12:40PM -0600, Dustin L. Howett wrote:
> This patch series adds support for the Framework Laptop to the cros_ec
> LPC driver.
> 
> The Framework Laptop is a non-Chromebook laptop that uses the ChromeOS
> Embedded Controller. Since the machine was designed to present a more
> normal device profile, it does not report all 512 I/O ports that are
> typically used by cros_ec_lpcs. Because of this, changes to the driver's
> port reservation scheme were required.
> 
> Dustin L. Howett (2):
>   platform/chrome: cros-ec: detect the Framework Laptop
>   platform/chrome: reserve only the I/O ports required for the MEC EC
> 
>  drivers/platform/chrome/cros_ec_lpc.c          |   47 ++++++++++-----
>  include/linux/platform_data/cros_ec_commands.h |    4 +
>  2 files changed, 38 insertions(+), 13 deletions(-)
> 

Thanks for sending this! I saw that Framework also posted their embedded
controller source code as well, so we've got both sides of this process
in the open now!

I'd like to have someone else at Google ramp up on reviewing chrome/platform
patches.

Tzung-Bi, could you help take a look at Dustin's series?

Thanks,
Benson

> -- 
> 2.34.1
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[Tzung-Bi Shih]

Hi Benson,

On Tue, Jan 25, 2022 at 4:52 AM Benson Leung <bleung@google.com> wrote:
> Tzung-Bi, could you help take a look at Dustin's series?

Pardon me, I didn't subscribe LKML due to it is too noisy.  Could you
also forward the 2 patches?

I will try some magic
(https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#Downloading-a-patch-from-patchwork-into-IMAP)
to get the mail thread.  And subscribe LKML to get further patch
series.

Re: [PATCH 1/2] platform/chrome: cros-ec: detect the Framework Laptop

[Tzung-Bi Shih]

On Tue, Jan 25, 2022 at 9:35 AM Dustin L. Howett <dustin@howett.net> wrote:
>
> The Framework Laptop identifies itself in DMI with manufacturer
> "Framework" and product "Laptop".
>
> 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 d6306d2a096f..458eb59db2ff 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -500,6 +500,14 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
>                         DMI_MATCH(DMI_PRODUCT_NAME, "Glimmer"),
>                 },
>         },
> +       /* A small number of non-Chromebook/box machines also use the ChromeOS EC */
> +       {
> +               /* the Framework Laptop */
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Laptop"),
> +               },
> +       },
>         { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(dmi, cros_ec_lpc_dmi_table);
> --
> 2.34.1
>
>

Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[Tzung-Bi Shih]

On Tue, Jan 25, 2022 at 9:35 AM Dustin L. Howett <dustin@howett.net> wrote:
>
> Some ChromeOS EC devices (such as the Framework Laptop) only map I/O
> ports 0x800-0x807. Making the larger reservation required by the non-MEC
> LPC (the 0xFF ports for the memory map, and the 0xFF ports for the
> parameter region) is non-viable on these devices.
>
> Since we probe the MEC EC first, we can get away with a smaller
> reservation that covers the MEC EC ports. If we fall back to classic
> LPC, we can grow the reservation to cover the memory map and the
> parameter region.
>
> Signed-off-by: Dustin L. Howett <dustin@howett.net>
> ---
>  drivers/platform/chrome/cros_ec_lpc.c         | 39 ++++++++++++-------
>  .../linux/platform_data/cros_ec_commands.h    |  4 ++
>  2 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 458eb59db2ff..06fdfe365710 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -341,9 +341,14 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>         u8 buf[2];
>         int irq, ret;
>
> -       if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
> -                                dev_name(dev))) {
> -               dev_err(dev, "couldn't reserve memmap region\n");
> +       /*
> +        * The Framework Laptop (and possibly other non-ChromeOS devices)
> +        * only exposes the eight I/O ports that are required for the Microchip EC.
> +        * Requesting a larger reservation will fail.
> +        */
> +       if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> +                                EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
> +               dev_err(dev, "couldn't reserve MEC region\n");
>                 return -EBUSY;
>         }
>
> @@ -357,6 +362,12 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>         cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
>         cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
>         if (buf[0] != 'E' || buf[1] != 'C') {
> +               if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
> +                                        dev_name(dev))) {
> +                       dev_err(dev, "couldn't reserve memmap region\n");
> +                       return -EBUSY;
> +               }
> +
>                 /* Re-assign read/write operations for the non MEC variant */
>                 cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
>                 cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
> @@ -366,17 +377,19 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>                         dev_err(dev, "EC ID not detected\n");
>                         return -ENODEV;
>                 }
> -       }
>
> -       if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> -                                EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> -               dev_err(dev, "couldn't reserve region0\n");
> -               return -EBUSY;
> -       }
> -       if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
> -                                EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> -               dev_err(dev, "couldn't reserve region1\n");
> -               return -EBUSY;
> +               /* Reserve the remaining I/O ports required by the non-MEC protocol. */
> +               if (!devm_request_region(dev, EC_HOST_CMD_REGION0 + EC_HOST_CMD_MEC_REGION_SIZE,
> +                                        EC_HOST_CMD_REGION_SIZE - EC_HOST_CMD_MEC_REGION_SIZE,
> +                                        dev_name(dev))) {
> +                       dev_err(dev, "couldn't reserve remainder of region0\n");
> +                       return -EBUSY;
> +               }
> +               if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
> +                                        EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> +                       dev_err(dev, "couldn't reserve region1\n");
> +                       return -EBUSY;
> +               }
>         }
>
>         ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 271bd87bff0a..a85b1176e6c0 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -55,6 +55,10 @@
>  #define EC_HOST_CMD_REGION0    0x800
>  #define EC_HOST_CMD_REGION1    0x880
>  #define EC_HOST_CMD_REGION_SIZE 0x80
> +/*
> + * Other machines report only the region spanned by the Microchip MEC EC.
> + */
> +#define EC_HOST_CMD_MEC_REGION_SIZE 0x08
>
>  /* EC command register bit functions */
>  #define EC_LPC_CMDR_DATA       BIT(0)  /* Data ready for host to read */
> --
> 2.34.1
>
>

Re: [PATCH 1/2] platform/chrome: cros-ec: detect the Framework Laptop

[Tzung-Bi Shih]

On Tue, Jan 25, 2022 at 11:41:51AM +0800, Tzung-Bi Shih wrote:
> On Tue, Jan 25, 2022 at 9:35 AM Dustin L. Howett <dustin@howett.net> wrote:
> >
> > The Framework Laptop identifies itself in DMI with manufacturer
> > "Framework" and product "Laptop".
> >
> > Signed-off-by: Dustin L. Howett <dustin@howett.net>

Nit: "platform/chrome: cros_ec_lpc:" would be a better commit title prefix.

With that,
Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>

Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[Tzung-Bi Shih]

On Tue, Jan 25, 2022 at 11:42:29AM +0800, Tzung-Bi Shih wrote:
> On Tue, Jan 25, 2022 at 9:35 AM Dustin L. Howett <dustin@howett.net> wrote:
> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> > index 458eb59db2ff..06fdfe365710 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc.c
> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
> > @@ -341,9 +341,14 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> >         u8 buf[2];
> >         int irq, ret;
> >
> > -       if (!devm_request_region(dev, EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE,
> > -                                dev_name(dev))) {
> > -               dev_err(dev, "couldn't reserve memmap region\n");
> > +       /*
> > +        * The Framework Laptop (and possibly other non-ChromeOS devices)
> > +        * only exposes the eight I/O ports that are required for the Microchip EC.
> > +        * Requesting a larger reservation will fail.
> > +        */
> > +       if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> > +                                EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
> > +               dev_err(dev, "couldn't reserve MEC region\n");
> >                 return -EBUSY;

The original code:
- devm_request_region(dev, EC_LPC_ADDR_MEMMAP, ...) and then
- cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).

After the patch:
- devm_request_region(dev, EC_HOST_CMD_REGION0, ...) and then
- cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).

Does it work if it reads out of request_region range?

> > @@ -366,17 +377,19 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> >                         dev_err(dev, "EC ID not detected\n");
> >                         return -ENODEV;
> >                 }
> > -       }
> >
> > -       if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> > -                                EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> > -               dev_err(dev, "couldn't reserve region0\n");
> > -               return -EBUSY;
> > -       }
> > -       if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
> > -                                EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> > -               dev_err(dev, "couldn't reserve region1\n");
> > -               return -EBUSY;
> > +               /* Reserve the remaining I/O ports required by the non-MEC protocol. */
> > +               if (!devm_request_region(dev, EC_HOST_CMD_REGION0 + EC_HOST_CMD_MEC_REGION_SIZE,
> > +                                        EC_HOST_CMD_REGION_SIZE - EC_HOST_CMD_MEC_REGION_SIZE,
> > +                                        dev_name(dev))) {
> > +                       dev_err(dev, "couldn't reserve remainder of region0\n");
> > +                       return -EBUSY;
> > +               }
> > +               if (!devm_request_region(dev, EC_HOST_CMD_REGION1,
> > +                                        EC_HOST_CMD_REGION_SIZE, dev_name(dev))) {
> > +                       dev_err(dev, "couldn't reserve region1\n");
> > +                       return -EBUSY;
> > +               }

The 2 request_region are now guarded by the first "if (buf[0] != 'E' || buf[1] != 'C')".  That is, only non-MEC will request the 2 regions.

Doesn't other MECs (e.g. non-Framework Laptop) need the 2 regions?

Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[Dustin Howett]

On Mon, Jan 24, 2022 at 10:17 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
>
> The original code:
> - devm_request_region(dev, EC_LPC_ADDR_MEMMAP, ...) and then
> - cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).
>
> After the patch:
> - devm_request_region(dev, EC_HOST_CMD_REGION0, ...) and then
> - cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).
>
> Does it work if it reads out of request_region range?
>
>
> The 2 request_region are now guarded by the first "if (buf[0] != 'E' || buf[1] != 'C')".  That is, only non-MEC will request the 2 regions.
>
> Doesn't other MECs (e.g. non-Framework Laptop) need the 2 regions?

So, in both cases this should be legal. Here's why:

The MEC protocol multiplexes memory-mapped reads through the same 8
I/O ports (0x800 - 0x807)
as it does host commands. It works by adjusting the base address,
EC_LPC_ADDR_MEMMAP (0x900),
to 0x100 before it initiates a standard MEC LPC xfer.
See cros_ec_lpc_mec_read_bytes line ~101 (as of 881007522c8fcc3785).

Therefore, the adjusted flow in the patch is:

0. Default cros_ec_lpc_ops to the MEC version of read/xfer [unchanged in patch]
1. Request 0x800 - 0x807 (MEC region)
2. read() using the MEC read function (using only the above ports)
3. if it succeeds, great! we have a MEC EC.
--- if it failed ---
4. Map the non-MEC port range 0x900 - 0x9FF for memory-mapped reads
5. read() using the NON-MEC read function (using ports 0x900 - 0x9FF)
6. if it succeeds, map the remaining host command ports 0x808 - 0x8FF

In short, only non-MEC needs the 0x900 - 0x9FF mapping for "mapped
memory". Therefore we can defer the
port allocation until after we've failed to read mapped memory the MEC way. :)

Based on my understanding of the MEC protocol, non-Framework Laptop
MECs hold this invariant true as well.
They should only need ports 0x800 - 0x807.

Hope that helps!

Want me to send a v2 with updated commit messages?
d

Re: [PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[Guenter Roeck]

If nothing else helps, you should be able to find and download the
series from lore.kernel.org/lkml
(https://lore.kernel.org/lkml/?q=%22framework+laptop%22). Long term it
might be useful to set up patchwork to track cros related patches.

Guenter

On Mon, Jan 24, 2022 at 5:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> Hi Benson,
>
> On Tue, Jan 25, 2022 at 4:52 AM Benson Leung <bleung@google.com> wrote:
> > Tzung-Bi, could you help take a look at Dustin's series?
>
> Pardon me, I didn't subscribe LKML due to it is too noisy.  Could you
> also forward the 2 patches?
>
> I will try some magic
> (https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#Downloading-a-patch-from-patchwork-into-IMAP)
> to get the mail thread.  And subscribe LKML to get further patch
> series.

Re: [PATCH 0/2] platform/chrome: Add support for the Framework Laptop

[Benson Leung]

Hi Guenter,

On Tue, Jan 25, 2022 at 8:52 AM Guenter Roeck <groeck@google.com> wrote:
>
> If nothing else helps, you should be able to find and download the
> series from lore.kernel.org/lkml
> (https://lore.kernel.org/lkml/?q=%22framework+laptop%22). Long term it
> might be useful to set up patchwork to track cros related patches.

I've inquired about creating a patchwork for chrome/platform.
Definitely agree that this could be organized better.

Benson

>
> Guenter
>
> On Mon, Jan 24, 2022 at 5:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > Hi Benson,
> >
> > On Tue, Jan 25, 2022 at 4:52 AM Benson Leung <bleung@google.com> wrote:
> > > Tzung-Bi, could you help take a look at Dustin's series?
> >
> > Pardon me, I didn't subscribe LKML due to it is too noisy.  Could you
> > also forward the 2 patches?
> >
> > I will try some magic
> > (https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#Downloading-a-patch-from-patchwork-into-IMAP)
> > to get the mail thread.  And subscribe LKML to get further patch
> > series.



-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC

[Tzung-Bi Shih]

On Tue, Jan 25, 2022 at 09:15:45AM -0600, Dustin Howett wrote:
> On Mon, Jan 24, 2022 at 10:17 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> >
> > The original code:
> > - devm_request_region(dev, EC_LPC_ADDR_MEMMAP, ...) and then
> > - cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).
> >
> > After the patch:
> > - devm_request_region(dev, EC_HOST_CMD_REGION0, ...) and then
> > - cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...).
> >
> > Does it work if it reads out of request_region range?
> >
> >
> > The 2 request_region are now guarded by the first "if (buf[0] != 'E' || buf[1] != 'C')".  That is, only non-MEC will request the 2 regions.
> >
> > Doesn't other MECs (e.g. non-Framework Laptop) need the 2 regions?
> 
> So, in both cases this should be legal. Here's why:
> 
> The MEC protocol multiplexes memory-mapped reads through the same 8
> I/O ports (0x800 - 0x807)
> as it does host commands. It works by adjusting the base address,
> EC_LPC_ADDR_MEMMAP (0x900),
> to 0x100 before it initiates a standard MEC LPC xfer.
> See cros_ec_lpc_mec_read_bytes line ~101 (as of 881007522c8fcc3785).
> 
> Therefore, the adjusted flow in the patch is:
> 
> 0. Default cros_ec_lpc_ops to the MEC version of read/xfer [unchanged in patch]
> 1. Request 0x800 - 0x807 (MEC region)
> 2. read() using the MEC read function (using only the above ports)
> 3. if it succeeds, great! we have a MEC EC.
> --- if it failed ---
> 4. Map the non-MEC port range 0x900 - 0x9FF for memory-mapped reads
> 5. read() using the NON-MEC read function (using ports 0x900 - 0x9FF)
> 6. if it succeeds, map the remaining host command ports 0x808 - 0x8FF
> 
> In short, only non-MEC needs the 0x900 - 0x9FF mapping for "mapped
> memory". Therefore we can defer the
> port allocation until after we've failed to read mapped memory the MEC way. :)
> 
> Based on my understanding of the MEC protocol, non-Framework Laptop
> MECs hold this invariant true as well.
> They should only need ports 0x800 - 0x807.

Thanks for the detail explanation.  After reading cros_ec_lpc_mec_read_bytes() carefully, I guess I got it.

The patch actually fixes 2 issues:
1. The original code accesses the 8 IO ports (i.e. 0x800 - 0x807) via cros_ec_lpc_mec_read_bytes(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, ...) without requesting the region in advance.

2. MEC variants only need to request the 8 IO ports.  However, the rest of ports (i.e. 0x808 - 0x9ff) are for non-MECs.

> Want me to send a v2 with updated commit messages?

Yes, that would be helpful.