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:   Wed, 26 Jan 2022 14:24:10 +0800
From:   Tzung-Bi Shih <tzungbi@google.com>
To:     Dustin Howett <dustin@howett.net>
Cc:     linux-kernel@vger.kernel.org, Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH 2/2] platform/chrome: reserve only the I/O ports required
 for the MEC EC
Message-ID: <YfDpCqffa+5Zypg4@google.com>
References: <20220105031242.287751-1-dustin@howett.net>
 <20220105031242.287751-3-dustin@howett.net>
 <CALtnz63d7utST13=S7UbExDzSE0GnoFA7JowQkkFSqrSUTmCdQ@mail.gmail.com>
 <Ye95vRREnbaLM+EJ@google.com>
 <CA+BfgNKRNXVxLhQ2mAxiq-c3ipkFPdYoZee6myO8djmXWnfrYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+BfgNKRNXVxLhQ2mAxiq-c3ipkFPdYoZee6myO8djmXWnfrYg@mail.gmail.com>
Precedence: bulk

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.