[PATCH v1 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

[Kevin Chen]

> On Wed, 2025-02-19 at 11:59 +0000, Kevin Chen wrote:
> > > On Tue, 2025-02-18 at 11:11 +0000, Kevin Chen wrote:
> > > > > On Mon, 2025-02-17 at 13:00 +0100, Krzysztof Kozlowski wrote:
> > > > > > On 17/02/2025 12:48, Kevin Chen wrote:
> > > > > > > +
> > > > > > > +       pcc->mdev.parent = dev;
> > > > > > > +       pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > > > > > > +       pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > > > "%s%d",
> > > > > > > DEVICE_NAME,
> > > > > > >
> > > > >
> > >
> +                                       pcc->mdev_id);
> > > > > > > +       pcc->mdev.fops = &pcc_fops;
> > > > > > > +       rc = misc_register(&pcc->mdev);
> > > > > > > +       if (rc) {
> > > > > > > +               dev_err(dev, "Couldn't register misc
> > > > > > > device\n");
> > > > > > > +               goto err_free_kfifo;
> > > > > > > +       }
> > > > > >
> > > > > > You cannot expose user-space interfaces from SoC drivers. Use
> > > > > > appropriate subsystem for this with proper ABI documentation.
> > > > > >
> > > > > > See:
> > > > > > https://lore.kernel.org/all/bc5118f2-8982-46ff-bc75-d0c71475e9
> > > > > > 09 at a
> > > > > > pp.f
> > > > > > astmail.com/
> > > > > > and more discussions on LKML
> > > > >
> > > > > Further, drivers/misc/aspeed-lpc-snoop.c already exists:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.g
> > > > > it/c
> > > > > ommit/?id=
> > > > > 9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1
> > > > >
> > > > > Kevin: Did you consider reworking it?
> > > > Andrew: No, I do not rework it but add the post code capture
> > > > driver using the SNOOP registers. As a result, I add some code in
> > > > aspeed_a2600_15 to check the SNOOP enable bit. PCC driver probe
> > > > abort if snoop is enabled.
> > >
> > > Hmm, I think OpenBMC's history regarding POST code support caused
> > > some confusion on my part. For whatever reason, the snoop device was
> > > used as a source of POST codes despite the existence of the
> > > dedicated POST code hardware since at least the AST2400, but...
> > What I know about the dedicated POST code hardware in ASPEED should be
> > the same one in LPC module.
> >
> > >
> > > > PCC is used for port I/O byte snooping over eSPI.
> > >
> > > ... it seems that they're largely interchangeable, just with
> > > different hardware features (PCC has DMA)? My impression is that the
> > > snoop device could also be used over eSPI?
> > Yes, PCC has DMA to capture the POST code.
> > And snoop device also can be used over eSPI.
> >
> > These two devices of PCC and snoop use the same port I/O of 80h and
> > 81h.
> > But, in current usage of PCC, it can support a continuous, 4-bytes
> > maximum region from port I/O 80h to 83h.
> > What I know about PCC or snoop usage, depends on INTEL platform or AMD
> > platform.
> >
> > For ASPEED, we want to upstream the PCC driver for the PCC usage.
> 
> Yeah, that's fine, but I think some work needs to be done to provide coherence
> in the devicetree binding and userspace APIs across both the ASPEED snoop
> and PCC bits, as well as the Nuvoton BPC. Bespoke designs create pain.
https://lore.kernel.org/linux-kernel//7661de74-f68c-6617-6a4e-3b0eb76a2a2e@linaro.org/T/
Andrew, I find the "NPCM BPC driver" to get the link. Are these patches match what you mentioned?

> 
> The PCC driver above reads the data out of the DMA ring-buffer straight into
> the kfifo hooked up the the miscdev read callback. The datasheet
> notes: "the data structure of the FIFO is mode dependent" in the description of
> PCCR3, but no in-band or out-of-band mechanism (sysfs,
> ioctl) is provided for userspace to query whether it's 1B, 2B, 4B or "full" mode.
For the data structure in PCCR3, I checked with designer. We only need 2B mode to get the information about data and related addresses.
For example, from espi master send the port 80h~83h with first data 0x11223344 and second data 0x55667788. The PCC kfifo would be written in the following output from hexdump.
# hexdump  /dev/aspeed-lpc-pcc0
0000000 4044 4133 4222 4311 4088 4177 4266 4355
> 
> The situation with the snoop driver is similar (1 or 2 1B channels multiplexed
> into the one data stream). It also looks a bit quirky with multiple channels
> enabled, as what userspace reads will depend on the host access patterns, but
> no metadata is provided to userspace about what it's reading.
Yes, for the snoop driver and PCC driver, some mechanism is the same. But snoop only supports 2 bytes data from the 2 1B channels multiplexed.
So, we need to add PCC driver to upstream for customer's 4 Bytes POST code capture usage even the PCC driver needs to check the snoop enabled or not.
Or, could you please give us come comments about how I can upstream the PCC driver.

> 
> This should be fixed so userspace can have some certainty and a useful API to
> work against (one that provides a direct description of what's being read out).
> 
> I expect we could similarly consolidate the devicetree bindings, covering what
> IO port range to attach to.
> 
> Andrew