* [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
2022-01-05 3:12 [PATCH 0/2] platform/chrome: Add support for the Framework Laptop Dustin L. Howett
@ 2022-01-05 3:12 ` Dustin L. Howett
2022-01-25 3:42 ` Tzung-Bi Shih
0 siblings, 1 reply; 6+ messages in thread
From: Dustin L. Howett @ 2022-01-05 3:12 UTC (permalink / raw)
To: linux-kernel; +Cc: Benson Leung, Guenter Roeck, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
@ 2022-01-08 23:03 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-01-08 23:03 UTC (permalink / raw)
To: kbuild
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
2022-01-05 3:12 ` [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC Dustin L. Howett
@ 2022-01-25 3:42 ` Tzung-Bi Shih
2022-01-25 4:17 ` Tzung-Bi Shih
0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-01-25 3:42 UTC (permalink / raw)
To: Dustin L. Howett; +Cc: linux-kernel, Benson Leung, Guenter Roeck, 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
2022-01-25 3:42 ` Tzung-Bi Shih
@ 2022-01-25 4:17 ` Tzung-Bi Shih
2022-01-25 15:15 ` Dustin Howett
0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-01-25 4:17 UTC (permalink / raw)
To: Dustin L. Howett
Cc: Dustin L. Howett, linux-kernel, Benson Leung, Guenter Roeck
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
2022-01-25 4:17 ` Tzung-Bi Shih
@ 2022-01-25 15:15 ` Dustin Howett
2022-01-26 6:24 ` Tzung-Bi Shih
0 siblings, 1 reply; 6+ messages in thread
From: Dustin Howett @ 2022-01-25 15:15 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: linux-kernel, Benson Leung, Guenter Roeck
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC
2022-01-25 15:15 ` Dustin Howett
@ 2022-01-26 6:24 ` Tzung-Bi Shih
0 siblings, 0 replies; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-01-26 6:24 UTC (permalink / raw)
To: Dustin Howett; +Cc: linux-kernel, Benson Leung, Guenter Roeck
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-26 6:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-08 23:03 [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2022-01-05 3:12 [PATCH 0/2] platform/chrome: Add support for the Framework Laptop Dustin L. Howett
2022-01-05 3:12 ` [PATCH 2/2] platform/chrome: reserve only the I/O ports required for the MEC EC Dustin L. Howett
2022-01-25 3:42 ` Tzung-Bi Shih
2022-01-25 4:17 ` Tzung-Bi Shih
2022-01-25 15:15 ` Dustin Howett
2022-01-26 6:24 ` Tzung-Bi Shih
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.