Messages in this thread | | | From | "" <> | Subject | Re: [PATCH v3 1/1] soc: fujitsu: Add A64FX diagnostic interrupt driver | Date | Fri, 8 Apr 2022 10:32:15 +0000 |
| |
Hi Greg,
> > Enable diagnostic interrupts for the A64FX. > > This is done using a pseudo-NMI. > > I do not understand what this driver is, sorry. Can you please provide more > information in the changelog text for what this is, who would use it, and how it will > be interacted with.
I understand. I will add a description in the next version.
> > +config A64FX_DIAG > > + bool "A64FX diag driver" > > + depends on ARM64 > > What about ACPI? Shouldn't this code depend on that?
Okey. I will make it dependent on ACPI.
> > + help > > + Say Y here if you want to enable diag interrupt on A64FX. > > What is A64FX?
A64FX is a processor designed by Fujitsu. For the sake of clarity, I will describe it as "Fujitsu A64FX".
> > + This driver uses pseudo-NMI if available. > > What does this mean?
This driver uses the pseudo-NMI feature to perform diagnostic interrupts for A64FX. However, I felt that it was superfluous to give this explanation here, so I will delete this sentence.
> > + If unsure, say N. > > No module? Why not?
request_nmi() is not EXPORT_SYMBOL. So this driver cannot be a module.
> > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * A64FX diag driver. > > No copyright information? Are you sure?
I will add copyright information.
> > +#define BMC_DIAG_INTERRUPT_STATUS_OFFSET (0x0044) #define > > +BMC_INTERRUPT_STATUS_MASK ((1U) << 31) > > BIT()? > > > +#define BMC_DIAG_INTERRUPT_ENABLE_OFFSET (0x0040) #define > > +BMC_INTERRUPT_ENABLE_MASK ((1U) << 31) > > BIT()?
Okey, I will use BIT().
> > +static irqreturn_t a64fx_diag_handler(int irq, void *dev_id) { > > + handle_sysrq('c'); > > > Why is this calling this sysrq call? From an interrupt? Why? > > And you are hard-coding "c", are you sure?
As mentioned by Arnd, I only called panic () at first, but after receiving his suggestion, I decided to call handle_sysrq ('c'). This driver only supports the function that causes a panic when receiving a diagnostic interrupt from the outside, so "c" is coded. Also, no data is added when a diagnostic interrupt is sent.
> > + if (mmsc & BMC_INTERRUPT_STATUS_MASK) > > + writel(BMC_INTERRUPT_STATUS_MASK, (void > *)diag_status_reg_addr); > > No need to wait for the write to complete? > > You shouldn't have to cast diag_status_reg_addr, right?
Due to the specifications of the machine, there is no problem even if there is no write wait processing.
I will remove constant and (void *). Thank you for pointing out.
> > + enable_irq(priv->irq); > > + priv->has_nmi = false; > > + dev_info(dev, "registered for IRQ %d\n", priv->irq); > > + } else { > > + enable_nmi(priv->irq); > > + priv->has_nmi = true; > > + dev_info(dev, "registered for NMI %d\n", priv->irq); > > When drivers are successful, they are quiet. No need for the noise here.
I will remove the message for a successful NMI/IRQ registration.
Thank you. Hitomi Hasegawa
| |