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.
This patch also fixes an issue where we would interact with I/O ports
0x800-0x807 without first making a reservation.
Signed-off-by: Dustin L. Howett <[email protected]>
---
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
Hi Dustin,
On Jan 26 12:00, Dustin L. Howett 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.
>
> This patch also fixes an issue where we would interact with I/O ports
> 0x800-0x807 without first making a reservation.
>
> Signed-off-by: Dustin L. Howett <[email protected]>
> ---
> 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
I can't find this update in the EC code base [1]. Is there any reason
you are not adding this, or is the change in flight (or in some other
location)?
[1] https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h
Thanks,
-Prashant
On Thu, Jan 27, 2022 at 12:55 PM Prashant Malani <[email protected]> wrote:
>
> Hi Dustin,
>
> I can't find this update in the EC code base [1]. Is there any reason
> you are not adding this, or is the change in flight (or in some other
> location)?
>
> [1] https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h
>
Hey Prashant,
The host communication adapters in the EC repo don't support the MEC
protocol at all,
so it did not seem necessary to bring these changes over. I'd be happy
to do so, of
course, if that is desirable.
My understanding (well, my guess) is that protocol support was never
added because
it is already implemented here in cros_ec_lpcs. Userland I/O port
access is(?) less desirable
than having this driver handle it.
d
> Thanks,
>
> -Prashant
+Aseda,
On Jan 27 13:18, Dustin Howett wrote:
> On Thu, Jan 27, 2022 at 12:55 PM Prashant Malani <[email protected]> wrote:
> >
> > Hi Dustin,
> >
> > I can't find this update in the EC code base [1]. Is there any reason
> > you are not adding this, or is the change in flight (or in some other
> > location)?
> >
> > [1] https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h
> >
>
> Hey Prashant,
>
> The host communication adapters in the EC repo don't support the MEC
> protocol at all,
> so it did not seem necessary to bring these changes over. I'd be happy
What source do Framework laptop ECs (MECs?) compile their EC firmware
from?
> to do so, of
> course, if that is desirable.
>
> My understanding (well, my guess) is that protocol support was never
> added because
> it is already implemented here in cros_ec_lpcs. Userland I/O port
> access is(?) less desirable
> than having this driver handle it.
Yeah, I wasn't thinking about userland i/o port access, but just having
this behaviour/different I/O port mapping described in the EC code base
too.
Forgot 1 more minor nit:
On Jan 27 18:55, Prashant Malani wrote:
> Hi Dustin,
>
> On Jan 26 12:00, Dustin L. Howett 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.
> >
> > This patch also fixes an issue where we would interact with I/O ports
> > 0x800-0x807 without first making a reservation.
(borrowing this from swboyd):
$ git grep "This patch" -- Documentation/process
i.e. don't use "This patch"
> >
> > Signed-off-by: Dustin L. Howett <[email protected]>
> > ---
> > 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
> I can't find this update in the EC code base [1]. Is there any reason
> you are not adding this, or is the change in flight (or in some other
> location)?
>
> [1] https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h
>
> Thanks,
>
> -Prashant
On Thu, Jan 27, 2022 at 1:25 PM Prashant Malani <[email protected]> wrote:
>
> What source do Framework laptop ECs (MECs?) compile their EC firmware
> from?
They just released their source here:
https://github.com/FrameworkComputer/EmbeddedController
FWIW, this series was written before they went open, and you're not
likely to see a similar construct
over there.
> Yeah, I wasn't thinking about userland i/o port access, but just having
> this behaviour/different I/O port mapping described in the EC code base
> too.
Happy to submit this over there as well. Are cros_ec_commands.h (here)
and ec_commands.h (there)
intended to be kept in 1:1 sync?
As well, is this a blocking concern?
Thanks!
d
Hi,
On Fri, Jan 28, 2022 at 9:57 AM Aseda Aboagye <[email protected]> wrote:
>
> Generally ec_commands.h is to be kept in sync with the copies in other projects. Periodically when someone modifies it, we should update the copies as well.
> --
> Aseda Aboagye
>
>
> On Thu, Jan 27, 2022 at 9:16 PM Dustin Howett <[email protected]> wrote:
>>
>> On Thu, Jan 27, 2022 at 1:25 PM Prashant Malani <[email protected]> wrote:
>> >
>> > What source do Framework laptop ECs (MECs?) compile their EC firmware
>> > from?
>>
>> They just released their source here:
>> https://github.com/FrameworkComputer/EmbeddedController
>>
>> FWIW, this series was written before they went open, and you're not
>> likely to see a similar construct
>> over there.
>>
>> > Yeah, I wasn't thinking about userland i/o port access, but just having
>> > this behaviour/different I/O port mapping described in the EC code base
>> > too.
>>
>> Happy to submit this over there as well. Are cros_ec_commands.h (here)
>> and ec_commands.h (there)
>> intended to be kept in 1:1 sync?
It's not auto-generated, but FWIW cros_ec_commands.h is a subset of
ec_commands.h (only the parts of ec_commands.h which are relevant to
the kernel drivers are brought over)
>>
>> As well, is this a blocking concern?
I'd prefer it to be first documented in the EC sources via a header
update before we make changes to the kernel here. This ensures that
the authors and maintainers of the various EC sources (Chromium EC,
framework EC etc) are aware of this change and sign-off on it.
I think making this update to the Chromium OS platform/ec code base
will be sufficient (it can flow down to framework when/if they chose
to sync to Chromium platform/ec); but happy to be corrected here by
Aseda or other EC experts.
Thanks,