diff mbox series

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

Message ID 20250217114831.3225970-4-kevin_chen@aspeedtech.com (mailing list archive)
State New, archived
Headers show
Series Add AST2600 LPC PCC support | expand

Commit Message

Kevin Chen Feb. 17, 2025, 11:48 a.m. UTC
Add LPC PCC controller driver to support POST code capture.

Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
---
 drivers/soc/aspeed/Kconfig          |  10 +
 drivers/soc/aspeed/Makefile         |   1 +
 drivers/soc/aspeed/aspeed-lpc-pcc.c | 439 ++++++++++++++++++++++++++++
 3 files changed, 450 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-lpc-pcc.c

Comments

Krzysztof Kozlowski Feb. 17, 2025, noon UTC | #1
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-d0c71475e909@app.fastmail.com/
and more discussions on LKML

Best regards,
Krzysztof
Andrew Jeffery Feb. 18, 2025, 12:56 a.m. UTC | #2
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-d0c71475e909@app.fastmail.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.git/commit/?id=9f4f9ae81d0affc182f54dd00285ddb90e0b3ae1

Kevin: Did you consider reworking it?

Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
"BPC" ("BIOS POST Code" controller). There should be some consensus on
the binding and userspace interface.

Andrew
Kevin Chen Feb. 18, 2025, 11:11 a.m. UTC | #3
> 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-d0c71475e909@app.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.git/commit/?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. PCC is used for port I/O byte snooping over eSPI.


> 
> Nuvoton have a similar capability in their NPCM BMC SoC(s) with the "BPC"
> ("BIOS POST Code" controller). There should be some consensus on the binding
> and userspace interface.
> 
> Andrew
Andrew Jeffery Feb. 19, 2025, 1:04 a.m. UTC | #4
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-d0c71475e909@app.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.git/commit/?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...

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

> 
> 
> > 
> > Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
> > "BPC"
> > ("BIOS POST Code" controller). There should be some consensus on
> > the binding
> > and userspace interface.

This is still the case.

Andrew
Kevin Chen Feb. 19, 2025, 11:59 a.m. UTC | #5
> 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-d0c71475e909@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.git/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.


> 
> >
> >
> > >
> > > Nuvoton have a similar capability in their NPCM BMC SoC(s) with the
> > > "BPC"
> > > ("BIOS POST Code" controller). There should be some consensus on the
> > > binding and userspace interface.
> 
> This is still the case.
> 
> Andrew
Andrew Jeffery Feb. 20, 2025, 1:22 a.m. UTC | #6
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-d0c71475e909@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.git/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.

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.

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.

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
Kevin Chen Feb. 21, 2025, 12:51 a.m. UTC | #7
> 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@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
Andrew Jeffery Feb. 25, 2025, 4:27 a.m. UTC | #8
On Fri, 2025-02-21 at 00:51 +0000, Kevin Chen wrote:
> > 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@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?

That looks about right.

Note Rob's response there about disparate bindings:

https://lore.kernel.org/linux-kernel//20221130193014.GA2645083-robh@kernel.org/

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

Right, but that's different to how the Aspeed snoop chardev behaves.
Both the snoop and PCC features can be used to present the same data
(POST codes) to userspace. There should be a unified chardev behaviour
for POST codes so we don't end up with a mess in userspace to deal with
the differences in hardware capabilities.

If we've got a chardev it would feel idiomatic to me to have ioctls to
configure the hardware to the required capture mode for the platform.

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

I understand.

> Or, could you please give us come comments about how I can upstream the PCC driver.

Essentially I don't think it's okay that each driver implement a
bespoke chardev, or that the devices don't share a common devicetree
binding. I think what's needed is a small abstraction that provides
common chardev semantics to userspace (e.g. /dev/postcodeN) that can be
connected to the different backends provided by the hardware (Aspeed
snoop and PCC, Nuvoton BPC) and act on the needs of userspace (1, 2, 4-
byte POST codes, configured using ioctls).

Andrew
Andrew Jeffery Feb. 25, 2025, 11:28 p.m. UTC | #9
Hi Mo,

On Mon, 2025-02-24 at 20:34 -0800, Mo Elbadry wrote:
> Hi Andrew,
> 
> I agree that a small layer of abstraction is needed to provide common
> chardev semantics to userspace. I think that effort can come where both
> Nuvoton and Aspeed unify their design and agree on a common abstraction
> layer.
> 
> I think such efforts may take some time for both to unify, is it possible
> to get this upstreamed (after addressing all other comments) while both
> parties work on an agreed unified abstraction layer?
> 

Given Arnd doesn't want bespoke userspace interfaces in the SoC drivers
this will need to go elsewhere, perhaps drivers/char or drivers/misc.
Greg and Arnd maintain both, so the patch needs to make a convincing
argument to them. For my part, my comments are just opinions based on
my understanding of the use-cases and the SoCs involved, and the desire
for reasonable devicetree and userspace interfaces.

I don't think it's right to try to rush things as devicetree and
userspace interfaces can be tricky to change or remove. Rushing tends
to be painful for all involved in the long run.

Cheers,

Andrew
Mo Elbadry Feb. 26, 2025, 12:48 a.m. UTC | #10
Hi Andrew,

Understood, your comments do make sense and are the right way to go.

I agree on not rushing, we want to see things moving forward but
properly as well.

Thank you for your help,
Mo



On Tue, Feb 25, 2025 at 3:28 PM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> Hi Mo,
>
> On Mon, 2025-02-24 at 20:34 -0800, Mo Elbadry wrote:
> > Hi Andrew,
> >
> > I agree that a small layer of abstraction is needed to provide common
> > chardev semantics to userspace. I think that effort can come where both
> > Nuvoton and Aspeed unify their design and agree on a common abstraction
> > layer.
> >
> > I think such efforts may take some time for both to unify, is it possible
> > to get this upstreamed (after addressing all other comments) while both
> > parties work on an agreed unified abstraction layer?
> >
>
> Given Arnd doesn't want bespoke userspace interfaces in the SoC drivers
> this will need to go elsewhere, perhaps drivers/char or drivers/misc.
> Greg and Arnd maintain both, so the patch needs to make a convincing
> argument to them. For my part, my comments are just opinions based on
> my understanding of the use-cases and the SoCs involved, and the desire
> for reasonable devicetree and userspace interfaces.
>
> I don't think it's right to try to rush things as devicetree and
> userspace interfaces can be tricky to change or remove. Rushing tends
> to be painful for all involved in the long run.
>
> Cheers,
>
> Andrew
diff mbox series

Patch

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..dff1a82f4201 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -52,6 +52,16 @@  config ASPEED_SOCINFO
 	help
 	  Say yes to support decoding of ASPEED BMC information.
 
+config ASPEED_LPC_PCC
+	tristate "Aspeed Post Code Capture support"
+	select REGMAP
+	select MFD_SYSCON
+	default ARCH_ASPEED
+	help
+	  Provides a driver to control the LPC PCC interface,
+	  allowing the BMC to capture post code written by the
+	  the host to an arbitrary LPC I/O port.
+
 endmenu
 
 endif
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..1692350b3512 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,4 @@  obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
 obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
 obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
+obj-$(CONFIG_ASPEED_LPC_PCC)		+= aspeed-lpc-pcc.o
diff --git a/drivers/soc/aspeed/aspeed-lpc-pcc.c b/drivers/soc/aspeed/aspeed-lpc-pcc.c
new file mode 100644
index 000000000000..a9733dc932a2
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-lpc-pcc.c
@@ -0,0 +1,439 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ */
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/fs.h>
+#include <linux/kfifo.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/dma-mapping.h>
+#include <linux/sizes.h>
+
+#define DEVICE_NAME "aspeed-lpc-pcc"
+
+static DEFINE_IDA(aspeed_pcc_ida);
+
+#define HICR5	0x80
+#define HICR5_EN_SNP0W			BIT(0)
+#define HICR5_EN_SNP1W			BIT(2)
+#define HICR6	0x084
+#define   HICR6_EN2BMODE		BIT(19)
+#define SNPWADR	0x090
+#define PCCR6	0x0c4
+#define   PCCR6_DMA_CUR_ADDR		GENMASK(27, 0)
+#define PCCR4	0x0d0
+#define   PCCR4_DMA_ADDRL_MASK		GENMASK(31, 0)
+#define   PCCR4_DMA_ADDRL_SHIFT		0
+#define PCCR5	0x0d4
+#define   PCCR5_DMA_ADDRH_MASK		GENMASK(27, 24)
+#define   PCCR5_DMA_ADDRH_SHIFT		24
+#define   PCCR5_DMA_LEN_MASK		GENMASK(23, 0)
+#define   PCCR5_DMA_LEN_SHIFT		0
+#define HICRB	0x100
+#define   HICRB_ENSNP0D			BIT(14)
+#define   HICRB_ENSNP1D			BIT(15)
+#define PCCR0	0x130
+#define   PCCR0_EN_DMA_INT		BIT(31)
+#define   PCCR0_EN_DMA_MODE		BIT(14)
+#define   PCCR0_ADDR_SEL_MASK		GENMASK(13, 12)
+#define   PCCR0_ADDR_SEL_SHIFT		12
+#define   PCCR0_RX_TRIG_LVL_MASK	GENMASK(10, 8)
+#define   PCCR0_RX_TRIG_LVL_SHIFT	8
+#define   PCCR0_CLR_RX_FIFO		BIT(7)
+#define   PCCR0_MODE_SEL_MASK		GENMASK(5, 4)
+#define   PCCR0_MODE_SEL_SHIFT		4
+#define   PCCR0_EN_RX_TMOUT_INT		BIT(2)
+#define   PCCR0_EN_RX_AVAIL_INT		BIT(1)
+#define   PCCR0_EN			BIT(0)
+#define PCCR1	0x134
+#define   PCCR1_BASE_ADDR_MASK		GENMASK(15, 0)
+#define   PCCR1_BASE_ADDR_SHIFT		0
+#define   PCCR1_DONT_CARE_BITS_MASK	GENMASK(21, 16)
+#define   PCCR1_DONT_CARE_BITS_SHIFT	16
+#define PCCR2	0x138
+#define   PCCR2_INT_STATUS_PATTERN_B	BIT(16)
+#define   PCCR2_INT_STATUS_PATTERN_A	BIT(8)
+#define   PCCR2_INT_STATUS_DMA_DONE	BIT(4)
+#define   PCCR2_INT_STATUS_DATA_RDY	PCCR2_INT_STATUS_DMA_DONE
+#define   PCCR2_INT_STATUS_RX_OVER	BIT(3)
+#define   PCCR2_INT_STATUS_RX_TMOUT	BIT(2)
+#define   PCCR2_INT_STATUS_RX_AVAIL	BIT(1)
+#define PCCR3	0x13c
+#define   PCCR3_FIFO_DATA_MASK		GENMASK(7, 0)
+
+#define PCC_DMA_BUFSZ	(256 * SZ_1K)
+
+enum pcc_fifo_threshold {
+	PCC_FIFO_THR_1_BYTE,
+	PCC_FIFO_THR_1_EIGHTH,
+	PCC_FIFO_THR_2_EIGHTH,
+	PCC_FIFO_THR_3_EIGHTH,
+	PCC_FIFO_THR_4_EIGHTH,
+	PCC_FIFO_THR_5_EIGHTH,
+	PCC_FIFO_THR_6_EIGHTH,
+	PCC_FIFO_THR_7_EIGHTH,
+	PCC_FIFO_THR_8_EIGHTH,
+};
+
+enum pcc_record_mode {
+	PCC_REC_1B,
+	PCC_REC_2B,
+	PCC_REC_4B,
+	PCC_REC_FULL,
+};
+
+enum pcc_port_hbits_select {
+	PCC_PORT_HBITS_SEL_NONE,
+	PCC_PORT_HBITS_SEL_45,
+	PCC_PORT_HBITS_SEL_67,
+	PCC_PORT_HBITS_SEL_89,
+};
+
+struct aspeed_pcc_dma {
+	uint32_t rptr;
+	uint8_t *virt;
+	dma_addr_t addr;
+	uint32_t size;
+};
+
+struct aspeed_pcc_ctrl {
+	struct device *dev;
+	struct regmap *regmap;
+	int irq;
+	uint32_t port;
+	struct aspeed_pcc_dma dma;
+	struct kfifo fifo;
+	wait_queue_head_t wq;
+	struct miscdevice mdev;
+	int mdev_id;
+};
+
+static inline bool is_valid_rec_mode(uint32_t mode)
+{
+	return (mode > PCC_REC_FULL) ? false : true;
+}
+
+static inline bool is_valid_high_bits_select(uint32_t sel)
+{
+	return (sel > PCC_PORT_HBITS_SEL_89) ? false : true;
+}
+
+static ssize_t aspeed_pcc_file_read(struct file *file, char __user *buffer,
+				    size_t count, loff_t *ppos)
+{
+	int rc;
+	unsigned int copied;
+	struct aspeed_pcc_ctrl *pcc = container_of(file->private_data,
+					      struct aspeed_pcc_ctrl,
+					      mdev);
+
+	if (kfifo_is_empty(&pcc->fifo)) {
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		rc = wait_event_interruptible(pcc->wq,
+					      !kfifo_is_empty(&pcc->fifo));
+		if (rc == -ERESTARTSYS)
+			return -EINTR;
+	}
+
+	rc = kfifo_to_user(&pcc->fifo, buffer, count, &copied);
+
+	return rc ? rc : copied;
+}
+
+static __poll_t aspeed_pcc_file_poll(struct file *file,
+				     struct poll_table_struct *pt)
+{
+	struct aspeed_pcc_ctrl *pcc = container_of(file->private_data, struct aspeed_pcc_ctrl, mdev);
+
+	poll_wait(file, &pcc->wq, pt);
+
+	return !kfifo_is_empty(&pcc->fifo) ? POLLIN : 0;
+}
+
+static const struct file_operations pcc_fops = {
+	.owner = THIS_MODULE,
+	.read = aspeed_pcc_file_read,
+	.poll = aspeed_pcc_file_poll,
+};
+
+static irqreturn_t aspeed_pcc_dma_isr(int irq, void *arg)
+{
+	uint32_t reg, rptr, wptr;
+	struct aspeed_pcc_ctrl *pcc = (struct aspeed_pcc_ctrl *)arg;
+	struct kfifo *fifo = &pcc->fifo;
+
+	regmap_write_bits(pcc->regmap, PCCR2, PCCR2_INT_STATUS_DMA_DONE, PCCR2_INT_STATUS_DMA_DONE);
+
+	regmap_read(pcc->regmap, PCCR6, &reg);
+	wptr = (reg & PCCR6_DMA_CUR_ADDR) - (pcc->dma.addr & PCCR6_DMA_CUR_ADDR);
+	rptr = pcc->dma.rptr;
+
+	do {
+		if (kfifo_is_full(fifo))
+			kfifo_skip(fifo);
+
+		kfifo_put(fifo, pcc->dma.virt[rptr]);
+
+		rptr = (rptr + 1) % pcc->dma.size;
+	} while (rptr != wptr);
+
+	pcc->dma.rptr = rptr;
+
+	wake_up_interruptible(&pcc->wq);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t aspeed_pcc_isr(int irq, void *arg)
+{
+	uint32_t sts;
+	struct aspeed_pcc_ctrl *pcc = (struct aspeed_pcc_ctrl *)arg;
+
+	regmap_read(pcc->regmap, PCCR2, &sts);
+
+	if (!(sts & (PCCR2_INT_STATUS_RX_TMOUT |
+		     PCCR2_INT_STATUS_RX_AVAIL |
+		     PCCR2_INT_STATUS_DMA_DONE)))
+		return IRQ_NONE;
+
+	return aspeed_pcc_dma_isr(irq, arg);
+}
+
+/*
+ * A2600-15 AP note
+ *
+ * SW workaround to prevent generating Non-Fatal-Error (NFE)
+ * eSPI response when PCC is used for port I/O byte snooping
+ * over eSPI.
+ */
+static int aspeed_a2600_15(struct aspeed_pcc_ctrl *pcc, struct device *dev)
+{
+	u32 hicr5_en, hicrb_en;
+
+	/* abort if snoop is enabled */
+	regmap_read(pcc->regmap, HICR5, &hicr5_en);
+	if (hicr5_en & (HICR5_EN_SNP0W | HICR5_EN_SNP1W)) {
+		dev_err(dev, "A2600-15 should be applied with snoop disabled\n");
+		return -EPERM;
+	}
+
+	/* set SNPWADR of snoop device */
+	regmap_write(pcc->regmap, SNPWADR, pcc->port | ((pcc->port + 2) << 16));
+
+	/* set HICRB[15:14]=11b to enable ACCEPT response for SNPWADR */
+	hicrb_en = HICRB_ENSNP0D | HICRB_ENSNP1D;
+	regmap_update_bits(pcc->regmap, HICRB, hicrb_en, hicrb_en);
+
+	/* set HICR6[19] to extend SNPWADR to 2x range */
+	regmap_update_bits(pcc->regmap, HICR6, HICR6_EN2BMODE, HICR6_EN2BMODE);
+
+	return 0;
+}
+
+static int aspeed_pcc_enable(struct aspeed_pcc_ctrl *pcc, struct device *dev)
+{
+	int rc;
+
+	rc = aspeed_a2600_15(pcc, dev);
+	if (rc)
+		return rc;
+
+	/* record mode: Set 2-Byte mode. */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_MODE_SEL_MASK,
+			   PCC_REC_2B << PCCR0_MODE_SEL_SHIFT);
+
+	/* port address */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_BASE_ADDR_MASK,
+			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
+
+	/* Set address high bits selection to 0b01 for address bit[5:4] */
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_ADDR_SEL_MASK,
+			   PCC_PORT_HBITS_SEL_45 << PCCR0_ADDR_SEL_SHIFT);
+
+	/* Set LPC don't care address to 0x3 for port 80~83h */
+	regmap_update_bits(pcc->regmap, PCCR1,
+			   PCCR1_DONT_CARE_BITS_MASK,
+			   0x3 << PCCR1_DONT_CARE_BITS_SHIFT);
+
+	/* set DMA ring buffer size and enable interrupts */
+	regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
+#ifdef CONFIG_ARM64
+	regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_ADDRH_MASK,
+			   (pcc->dma.addr >> 32) << PCCR5_DMA_ADDRH_SHIFT);
+#endif
+	regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_LEN_MASK,
+			   (pcc->dma.size / 4) << PCCR5_DMA_LEN_SHIFT);
+	regmap_update_bits(pcc->regmap, PCCR0,
+			   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE,
+			   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE);
+
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
+
+	return 0;
+}
+
+static int aspeed_pcc_disable(struct aspeed_pcc_ctrl *pcc)
+{
+	/* Disable PCC and DMA Mode for safety */
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN |  PCCR0_EN_DMA_MODE, 0);
+
+	/* Clear Rx FIFO. */
+	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_CLR_RX_FIFO, 1);
+
+	/* Clear All interrupts status. */
+	regmap_write(pcc->regmap, PCCR2,
+		     PCCR2_INT_STATUS_RX_OVER | PCCR2_INT_STATUS_DMA_DONE |
+		     PCCR2_INT_STATUS_PATTERN_A | PCCR2_INT_STATUS_PATTERN_B);
+
+	return 0;
+}
+
+static int aspeed_pcc_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct aspeed_pcc_ctrl *pcc;
+	struct device *dev;
+	uint32_t fifo_size = PAGE_SIZE;
+
+	dev = &pdev->dev;
+
+	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
+	if (!pcc)
+		return -ENOMEM;
+
+	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(pcc->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		return -ENODEV;
+	}
+
+	rc = of_property_read_u32(dev->of_node, "pcc-ports", &pcc->port);
+	if (rc) {
+		dev_err(dev, "no pcc ports configured\n");
+		return -ENODEV;
+	}
+
+	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (rc) {
+		dev_err(dev, "cannot set 64-bits DMA mask\n");
+		return rc;
+	}
+
+	pcc->dma.size = PCC_DMA_BUFSZ;
+	pcc->dma.virt = dmam_alloc_coherent(dev,
+					    pcc->dma.size,
+					    &pcc->dma.addr,
+					    GFP_KERNEL);
+	if (!pcc->dma.virt) {
+		dev_err(dev, "cannot allocate DMA buffer\n");
+		return -ENOMEM;
+	}
+
+	fifo_size = roundup(pcc->dma.size, PAGE_SIZE);
+	rc = kfifo_alloc(&pcc->fifo, fifo_size, GFP_KERNEL);
+	if (rc) {
+		dev_err(dev, "cannot allocate kFIFO\n");
+		return -ENOMEM;
+	}
+
+	/* Disable PCC to clean up DMA buffer before request IRQ. */
+	rc = aspeed_pcc_disable(pcc);
+	if (rc) {
+		dev_err(dev, "Couldn't disable PCC\n");
+		goto err_free_kfifo;
+	}
+
+	pcc->irq = platform_get_irq(pdev, 0);
+	if (pcc->irq < 0) {
+		dev_err(dev, "Couldn't get IRQ\n");
+		rc = -ENODEV;
+		goto err_free_kfifo;
+	}
+
+	rc = devm_request_irq(dev, pcc->irq, aspeed_pcc_isr, 0, DEVICE_NAME, pcc);
+	if (rc < 0) {
+		dev_err(dev, "Couldn't request IRQ %d\n", pcc->irq);
+		goto err_free_kfifo;
+	}
+
+	init_waitqueue_head(&pcc->wq);
+
+	pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);
+	if (pcc->mdev_id < 0) {
+		dev_err(dev, "Couldn't allocate ID\n");
+		return pcc->mdev_id;
+	}
+
+	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;
+	}
+
+	rc = aspeed_pcc_enable(pcc, dev);
+	if (rc) {
+		dev_err(dev, "Couldn't enable PCC\n");
+		goto err_dereg_mdev;
+	}
+
+	dev_set_drvdata(&pdev->dev, pcc);
+
+	return 0;
+
+err_dereg_mdev:
+	misc_deregister(&pcc->mdev);
+
+err_free_kfifo:
+	kfifo_free(&pcc->fifo);
+
+	return rc;
+}
+
+static void aspeed_pcc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct aspeed_pcc_ctrl *pcc = dev_get_drvdata(dev);
+
+	kfifo_free(&pcc->fifo);
+	misc_deregister(&pcc->mdev);
+}
+
+static const struct of_device_id aspeed_pcc_table[] = {
+	{ .compatible = "aspeed,ast2600-lpc-pcc" },
+	{ },
+};
+
+static struct platform_driver aspeed_pcc_driver = {
+	.driver = {
+		.name = "aspeed-pcc",
+		.of_match_table = aspeed_pcc_table,
+	},
+	.probe = aspeed_pcc_probe,
+	.remove = aspeed_pcc_remove,
+};
+
+module_platform_driver(aspeed_pcc_driver);
+
+MODULE_AUTHOR("Chia-Wei Wang <chiawei_wang@aspeedtech.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Driver for Aspeed Post Code Capture");