Message ID | 20250217114831.3225970-4-kevin_chen@aspeedtech.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add AST2600 LPC PCC support | expand |
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
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
> 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
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
> 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
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
> 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
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
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
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 --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, ®); + 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");
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