| Message ID | 1568020220-7758-3-git-send-email-talel@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Amazon's Annapurna Labs POS Driver | expand |
On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote: > > The Amazon's Annapurna Labs SoCs includes Point Of Serialization error > logging unit that reports an error in case write error (e.g. attempt to > write to a read only register). > This patch introduces the support for this unit. > > Signed-off-by: Talel Shenhar <talel@amazon.com> Looks ok overall, juts a few minor comments: > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Talel Shenhar"); > +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); These usually go to the end of the file. > + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1); > + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) > + return IRQ_NONE; > + > + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0); > + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1); Why do you require _relaxed() accessors here? Please add a comment explaining that, or use the regular readl()/writel(). > + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource); This can be simplified to devm_platform_ioremap_resource(). > + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); And this is usually written as platform_get_irq() Arnd
On 9/9/2019 12:44 PM, Arnd Bergmann wrote: > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote: >> The Amazon's Annapurna Labs SoCs includes Point Of Serialization error >> logging unit that reports an error in case write error (e.g. attempt to >> write to a read only register). >> This patch introduces the support for this unit. >> >> Signed-off-by: Talel Shenhar <talel@amazon.com> > Looks ok overall, juts a few minor comments: Thanks. > >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Talel Shenhar"); >> +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); > These usually go to the end of the file. Ack, Will move them as part of v2. > >> + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1); >> + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) >> + return IRQ_NONE; >> + >> + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0); >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > Why do you require _relaxed() accessors here? Please add a comment > explaining that, or use the regular readl()/writel(). I don't think commenting is needed here as there is nothing special in this type of access. I don't see this is common to comment the use of the _relaxed accessors. This driver is for SoC using arm64 cpu. If one uses the non-relaxed version of readl while running on arm64, he shall cause read barrier, which is then doing dsm(ld).. This barrier is not needed here, so we spare the use of the more heavy readl in favor of the less "harmful" one. Let me know what you think. > >> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource); > This can be simplified to devm_platform_ioremap_resource(). Ack, Will simplify them in v2. > >> + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > And this is usually written as platform_get_irq() Ack, Will replace them with platform_get_irq() in v2. > > Arnd
On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote: > On 9/9/2019 12:44 PM, Arnd Bergmann wrote: > > On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote: > >> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1); > > Why do you require _relaxed() accessors here? Please add a comment > > explaining that, or use the regular readl()/writel(). > > I don't think commenting is needed here as there is nothing special in > this type of access. > > I don't see this is common to comment the use of the _relaxed accessors. I usually mention it in driver reviews, but most authors revert back to the normal accessors when there is no difference. > This driver is for SoC using arm64 cpu. > > If one uses the non-relaxed version of readl while running on arm64, he > shall cause read barrier, which is then doing dsm(ld).. This barrier is > not needed here, so we spare the use of the more heavy readl in favor of > the less "harmful" one. > > Let me know what you think. If the barrier causes no harm, just leave it in to keep the code more readable. Most developers don't need to know the difference between the two, so using the less common interface just makes the reader curious about why it was picked. Avoiding the barrier can make a huge performance difference in a hot code path, but the downside is that it can behave in unexpected ways if the same code is run on a different CPU architecture that does not have the exact same rules about what _relaxed() means. In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead to slower rather than faster code when the explicit barrier is heavier than the implied one (e.g. on x86), or readl_relaxed() does not skip the barrier. The general rule with kernel interfaces when you have two versions that both do what you want is to pick the one with the shorter name. See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(), or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also exceptions) Arnd
On 9/9/2019 4:41 PM, Arnd Bergmann wrote: > On Mon, Sep 9, 2019 at 1:13 PM Shenhar, Talel <talel@amazon.com> wrote: >> On 9/9/2019 12:44 PM, Arnd Bergmann wrote: >>> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@amazon.com> wrote: >>>> + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1); >>> Why do you require _relaxed() accessors here? Please add a comment >>> explaining that, or use the regular readl()/writel(). >> I don't think commenting is needed here as there is nothing special in >> this type of access. >> >> I don't see this is common to comment the use of the _relaxed accessors. > I usually mention it in driver reviews, but most authors revert back > to the normal accessors when there is no difference. > >> This driver is for SoC using arm64 cpu. >> >> If one uses the non-relaxed version of readl while running on arm64, he >> shall cause read barrier, which is then doing dsm(ld).. This barrier is >> not needed here, so we spare the use of the more heavy readl in favor of >> the less "harmful" one. >> >> Let me know what you think. > If the barrier causes no harm, just leave it in to keep the code more > readable. Most developers don't need to know the difference between > the two, so using the less common interface just makes the reader > curious about why it was picked. > > Avoiding the barrier can make a huge performance difference in a > hot code path, but the downside is that it can behave in unexpected > ways if the same code is run on a different CPU architecture that > does not have the exact same rules about what _relaxed() means. > > In fact, replacing a 'readl()' with 'readl_relaxed() + rmb()' can lead > to slower rather than faster code when the explicit barrier is heavier > than the implied one (e.g. on x86), or readl_relaxed() does not skip > the barrier. > > The general rule with kernel interfaces when you have two versions > that both do what you want is to pick the one with the shorter name. > See spin_lock()/spin_lock_irqsave(), ioremap()/ioremap_nocache(), > or ktime_get()/ktime_get_clocktai_ts64(). (yes, there are also > exceptions) > > Arnd Thanks for the detailed response. In current implementation of v1, I am not doing any read barrier, Hence, using the non-relaxed will add unneeded memory barrier. I have no strong objection moving to the non-relaxed version and have an unneeded memory barrier, as this path is not "hot" one. Beside of avoiding the unneeded memory barrier, I would be happy to keep common behavior for our drivers: e.g. https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49 So what do you think we should go with? relaxed or non-relaxed?
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote: > On 9/9/2019 4:41 PM, Arnd Bergmann wrote: > > In current implementation of v1, I am not doing any read barrier, Hence, > using the non-relaxed will add unneeded memory barrier. > > I have no strong objection moving to the non-relaxed version and have an > unneeded memory barrier, as this path is not "hot" one. Ok, then please add it. > Beside of avoiding the unneeded memory barrier, I would be happy to keep > common behavior for our drivers: > > e.g. > > https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49 > > > So what do you think we should go with? relaxed or non-relaxed? The al_fic_set_trigger() function is clearly a slow-path and should use the non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier might introduce a measurable overhead, but at the same time I'm not sure if that one is correct without the barrier: If you have an MSI-type interrupt for notifying a device driver of a DMA completion, there might not be any other barrier between the arrival of the MSI message and the CPU accessing the data. Depending on how strict the hardware implements MSI and how the IRQ is chained, this could lead to data corruption. If the interrupt is only used for level or edge triggered interrupts, this is ok since you already need another register read in the driver before it can safely access a DMA buffer. In either case, if you can prove that it's safe to use the relaxed version here and you think that it may help, it would be good to add a comment explaining the reasoning. Arnd
On 9/9/2019 6:16 PM, Arnd Bergmann wrote: > On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote: >> On 9/9/2019 4:41 PM, Arnd Bergmann wrote: >> >> In current implementation of v1, I am not doing any read barrier, Hence, >> using the non-relaxed will add unneeded memory barrier. >> >> I have no strong objection moving to the non-relaxed version and have an >> unneeded memory barrier, as this path is not "hot" one. > Ok, then please add it. ok, shall be part of v2 > >> Beside of avoiding the unneeded memory barrier, I would be happy to keep >> common behavior for our drivers: >> >> e.g. >> >> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49 >> >> >> So what do you think we should go with? relaxed or non-relaxed? > The al_fic_set_trigger() function is clearly a slow-path and should use the > non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier > might introduce a measurable overhead, but at the same time I'm > not sure if that one is correct without the barrier: > > If you have an MSI-type interrupt for notifying a device driver of > a DMA completion, there might not be any other barrier between > the arrival of the MSI message and the CPU accessing the data. > Depending on how strict the hardware implements MSI and how > the IRQ is chained, this could lead to data corruption. > > If the interrupt is only used for level or edge triggered interrupts, > this is ok since you already need another register read in > the driver before it can safely access a DMA buffer. > > In either case, if you can prove that it's safe to use the relaxed > version here and you think that it may help, it would be good to > add a comment explaining the reasoning. Decided to go with the non-relaxed version as this is not hot path and likely be more clear to the common reader to have non relaxed version. > > Arnd
diff --git a/MAINTAINERS b/MAINTAINERS index e7a47b5..627af40 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -751,6 +751,12 @@ F: drivers/tty/serial/altera_jtaguart.c F: include/linux/altera_uart.h F: include/linux/altera_jtaguart.h +AMAZON ANNAPURNA LABS POS +M: Talel Shenhar <talel@amazon.com> +S: Maintained +F: Documentation/devicetree/bindings/soc/amazon/amazon,al-pos.txt +F: drivers/soc/amazon/al_pos.c + AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER M: Talel Shenhar <talel@amazon.com> S: Maintained diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig index 833e04a..913a6b1 100644 --- a/drivers/soc/Kconfig +++ b/drivers/soc/Kconfig @@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers" source "drivers/soc/actions/Kconfig" +source "drivers/soc/amazon/Kconfig" source "drivers/soc/amlogic/Kconfig" source "drivers/soc/aspeed/Kconfig" source "drivers/soc/atmel/Kconfig" diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile index 2ec3550..c1c5c64 100644 --- a/drivers/soc/Makefile +++ b/drivers/soc/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_ACTIONS) += actions/ obj-$(CONFIG_SOC_ASPEED) += aspeed/ obj-$(CONFIG_ARCH_AT91) += atmel/ +obj-y += amazon/ obj-y += bcm/ obj-$(CONFIG_ARCH_DOVE) += dove/ obj-$(CONFIG_MACH_DOVE) += dove/ diff --git a/drivers/soc/amazon/Kconfig b/drivers/soc/amazon/Kconfig new file mode 100644 index 00000000..fdd4cdd --- /dev/null +++ b/drivers/soc/amazon/Kconfig @@ -0,0 +1,5 @@ +config AL_POS + bool "Amazon's Annapurna Labs POS driver" + depends on ARCH_ALPINE || COMPILE_TEST + help + Include support for the SoC POS error capability. diff --git a/drivers/soc/amazon/Makefile b/drivers/soc/amazon/Makefile new file mode 100644 index 00000000..a31441a --- /dev/null +++ b/drivers/soc/amazon/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_AL_POS) += al_pos.o diff --git a/drivers/soc/amazon/al_pos.c b/drivers/soc/amazon/al_pos.c new file mode 100644 index 00000000..6d0bdff --- /dev/null +++ b/drivers/soc/amazon/al_pos.c @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + */ +#include <linux/bitfield.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Talel Shenhar"); +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver"); + +/* Registers Offset */ +#define AL_POS_ERROR_LOG_1 0x0 +#define AL_POS_ERROR_LOG_0 0x4 + +/* Registers Fields */ +#define AL_POS_ERROR_LOG_1_VALID GENMASK(31, 31) +#define AL_POS_ERROR_LOG_1_BRESP GENMASK(18, 17) +#define AL_POS_ERROR_LOG_1_REQUEST_ID GENMASK(16, 8) +#define AL_POS_ERROR_LOG_1_ADDR_HIGH GENMASK(7, 0) + +#define AL_POS_ERROR_LOG_0_ADDR_LOW GENMASK(31, 0) + +static int al_pos_panic; +module_param(al_pos_panic, int, 0); +MODULE_PARM_DESC(al_pos_panic, "Defines if POS error is causing panic()"); + +struct al_pos { + struct platform_device *pdev; + void __iomem *mmio_base; + int irq; +}; + +static irqreturn_t al_pos_irq_handler(int irq, void *info) +{ + struct platform_device *pdev = info; + struct al_pos *pos = platform_get_drvdata(pdev); + u32 log1; + u32 log0; + u64 addr; + u16 request_id; + u8 bresp; + + log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1); + if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1)) + return IRQ_NONE; + + log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0); + writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1); + + addr = FIELD_GET(AL_POS_ERROR_LOG_0_ADDR_LOW, log0); + addr |= (FIELD_GET(AL_POS_ERROR_LOG_1_ADDR_HIGH, log1) << 32); + request_id = FIELD_GET(AL_POS_ERROR_LOG_1_REQUEST_ID, log1); + bresp = FIELD_GET(AL_POS_ERROR_LOG_1_BRESP, log1); + + dev_err(&pdev->dev, "addr=0x%llx request_id=0x%x bresp=0x%x\n", + addr, request_id, bresp); + + if (al_pos_panic) + panic("POS"); + + return IRQ_HANDLED; +} + +static int al_pos_probe(struct platform_device *pdev) +{ + struct al_pos *pos; + struct resource *resource; + int ret; + + pos = devm_kzalloc(&pdev->dev, sizeof(*pos), GFP_KERNEL); + if (!pos) + return -ENOMEM; + + platform_set_drvdata(pdev, pos); + pos->pdev = pdev; + + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource); + if (IS_ERR(pos->mmio_base)) { + dev_err(&pdev->dev, "failed to ioremap memory (%ld)\n", + PTR_ERR(pos->mmio_base)); + return PTR_ERR(pos->mmio_base); + } + + pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (pos->irq <= 0) { + dev_err(&pdev->dev, "fail to parse and map irq\n"); + return -EINVAL; + } + + ret = devm_request_irq(&pdev->dev, + pos->irq, + al_pos_irq_handler, + 0, + pdev->name, + pdev); + if (ret != 0) { + dev_err(&pdev->dev, + "failed to register to irq %d (%d)\n", + pos->irq, ret); + return ret; + } + + dev_info(&pdev->dev, "successfully loaded\n"); + + return 0; +} + +static const struct of_device_id al_pos_of_match[] = { + { .compatible = "amazon,al-pos", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, al_pos_of_match); + +static struct platform_driver al_pos_driver = { + .probe = al_pos_probe, + .driver = { + .name = "al-pos", + .of_match_table = al_pos_of_match, + }, +}; + +module_platform_driver(al_pos_driver);
The Amazon's Annapurna Labs SoCs includes Point Of Serialization error logging unit that reports an error in case write error (e.g. attempt to write to a read only register). This patch introduces the support for this unit. Signed-off-by: Talel Shenhar <talel@amazon.com> --- MAINTAINERS | 6 +++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/amazon/Kconfig | 5 ++ drivers/soc/amazon/Makefile | 1 + drivers/soc/amazon/al_pos.c | 129 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 143 insertions(+) create mode 100644 drivers/soc/amazon/Kconfig create mode 100644 drivers/soc/amazon/Makefile create mode 100644 drivers/soc/amazon/al_pos.c