[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 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.