Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 27 Jan 2022 19:30:25 +0000
From:   Prashant Malani <pmalani@chromium.org>
To:     "Dustin L. Howett" <dustin@howett.net>
Cc:     linux-kernel@vger.kernel.org, Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Tzung-Bi Shih <tzungbi@google.com>,
        Michael Niksa <michael.niksa@live.com>, swboyd@chromium.org
Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_lpcs: reserve the MEC
 LPC I/O ports first
Message-ID: <YfLy0fAw/XnQ7g4R@chromium.org>
References: <20220126180020.15873-1-dustin@howett.net>
 <20220126180020.15873-3-dustin@howett.net>
 <YfLqloFQpF7bURGi@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YfLqloFQpF7bURGi@chromium.org>
Precedence: bulk

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