Wondering if, Greg you approves Hitomi’s updated series, then you pick up
it whatever the tree you maintain?
Thanks,
Itaru.
On Fri, Apr 8, 2022 at 19:36 hasegawa-hit...@fujitsu.com <
hasegawa-hit...@fujitsu.com> wrote:
> 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
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
