Message-ID: <20250723082111.GO11056@google.com>
Date: Wed, 23 Jul 2025 09:21:11 +0100
From: Lee Jones <lee@...nel.org>
To: Binbin Zhou <zhoubb.aaron@...il.com>
Cc: Binbin Zhou <zhoubinbin@...ngson.cn>,
	Huacai Chen <chenhuacai@...ngson.cn>,
	Corey Minyard <minyard@....org>,
	Huacai Chen <chenhuacai@...nel.org>,
	Xuerui Wang <kernel@...0n.name>, loongarch@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	openipmi-developer@...ts.sourceforge.net, jeffbai@...c.io,
	kexybiscuit@...c.io, wangyao@...ote.com,
	Chong Qiao <qiaochong@...ngson.cn>,
	Corey Minyard <corey@...yard.net>
Subject: Re: [PATCH v7 2/3] mfd: ls2kbmc: Add Loongson-2K BMC reset function
 support

On Wed, 23 Jul 2025, Binbin Zhou wrote:

> Hi Lee:
> 
> Thanks for your reply again.
> 
> On Wed, Jul 23, 2025 at 3:31 PM Lee Jones <lee@...nel.org> wrote:
> >
> > On Mon, 21 Jul 2025, Binbin Zhou wrote:
> >
> > > Hi Lee:
> > >
> > > Thanks for your reply.
> > >
> > >
> > > On Fri, Jul 18, 2025 at 9:52 PM Lee Jones <lee@...nel.org> wrote:
> > > >
> > > > On Fri, 11 Jul 2025, Binbin Zhou wrote:
> > > >
> > > > > Hi Lee:
> > > > >
> > > > > Thanks for your review.
> > > > >
> > > > > On Thu, Jul 10, 2025 at 6:03 PM Lee Jones <lee@...nel.org> wrote:
> > > > > >
> > > > > > On Fri, 04 Jul 2025, Binbin Zhou wrote:
> > > > > >
> > > > > > > Since the display is a sub-function of the Loongson-2K BMC, when the
> > > > > > > BMC reset, the entire BMC PCIe is disconnected, including the display
> > > > > > > which is interrupted.
> > > > > > >
> > > > > > > Quick overview of the entire LS2K BMC reset process:
> > > > > > >
> > > > > > > There are two types of reset methods: soft reset (BMC-initiated reboot
> > > > > > > of IPMI reset command) and BMC watchdog reset (watchdog timeout).
> > > > > > >
> > > > > > > First, regardless of the method, an interrupt is generated (PCIe interrupt
> > > > > > > for soft reset/GPIO interrupt for watchdog reset);
> > > > > > >
> > > > > > > Second, during the interrupt process, the system enters bmc_reset_work,
> > > > > > > clears the bus/IO/mem resources of the LS7A PCI-E bridge, waits for the BMC
> > > > > > > reset to begin, then restores the parent device's PCI configuration space,
> > > > > > > waits for the BMC reset to complete, and finally restores the BMC PCI
> > > > > > > configuration space.
> > > > > > >
> > > > > > > Display restoration occurs last.
> > > > > > >
> > > > > > > Co-developed-by: Chong Qiao <qiaochong@...ngson.cn>
> > > > > > > Signed-off-by: Chong Qiao <qiaochong@...ngson.cn>
> > > > > > > Reviewed-by: Huacai Chen <chenhuacai@...ngson.cn>
> > > > > > > Acked-by: Corey Minyard <corey@...yard.net>
> > > > > > > Signed-off-by: Binbin Zhou <zhoubinbin@...ngson.cn>
> > > > > > > ---
> > > > > > >  drivers/mfd/ls2k-bmc-core.c | 328 ++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 328 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> > > > > > > index 50d560a4611c..1ae673f6a196 100644
> > > > > > > --- a/drivers/mfd/ls2k-bmc-core.c
> > > > > > > +++ b/drivers/mfd/ls2k-bmc-core.c
> > > > > > > @@ -10,8 +10,12 @@
> > > > > > >   */
> > > > > > >
> > > > > > >  #include <linux/aperture.h>
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > >  #include <linux/errno.h>
> > > > > > >  #include <linux/init.h>
> > > > > > > +#include <linux/iopoll.h>
> > > > > > > +#include <linux/kbd_kern.h>
> > > > > > >  #include <linux/kernel.h>
> > > > > > >  #include <linux/mfd/core.h>
> > > > > > >  #include <linux/module.h>
> > > > > > > @@ -19,6 +23,8 @@
> > > > > > >  #include <linux/pci_ids.h>
> > > > > > >  #include <linux/platform_data/simplefb.h>
> > > > > > >  #include <linux/platform_device.h>
> > > > > > > +#include <linux/stop_machine.h>
> > > > > > > +#include <linux/vt_kern.h>
> > > > > > >
> > > > > > >  /* LS2K BMC resources */
> > > > > > >  #define LS2K_DISPLAY_RES_START               (SZ_16M + SZ_2M)
> > > > > > > @@ -29,6 +35,48 @@
> > > > > > >  #define LS2K_IPMI3_RES_START         (LS2K_IPMI2_RES_START + LS2K_IPMI_RES_SIZE)
> > > > > > >  #define LS2K_IPMI4_RES_START         (LS2K_IPMI3_RES_START + LS2K_IPMI_RES_SIZE)
> > > > > > >
> > > > > > > +#define LS7A_PCI_CFG_SIZE            0x100
> > > > > > > +
> > > > > > > +/* LS7A bridge registers */
> > > > > > > +#define LS7A_PCIE_PORT_CTL0          0x0
> > > > > > > +#define LS7A_PCIE_PORT_STS1          0xC
> > > > > > > +#define LS7A_GEN2_CTL                        0x80C
> > > > > > > +#define LS7A_SYMBOL_TIMER            0x71C
> > > > > > > +
> > > > > > > +/* Bits of LS7A_PCIE_PORT_CTL0 */
> > > > > > > +#define LS2K_BMC_PCIE_LTSSM_ENABLE   BIT(3)
> > > > > > > +
> > > > > > > +/* Bits of LS7A_PCIE_PORT_STS1 */
> > > > > > > +#define LS2K_BMC_PCIE_LTSSM_STS              GENMASK(5, 0)
> > > > > > > +#define LS2K_BMC_PCIE_CONNECTED              0x11
> > > > > > > +
> > > > > > > +#define LS2K_BMC_PCIE_DELAY_US               1000
> > > > > > > +#define LS2K_BMC_PCIE_TIMEOUT_US     1000000
> > > > > > > +
> > > > > > > +/* Bits of LS7A_GEN2_CTL */
> > > > > > > +#define LS7A_GEN2_SPEED_CHANG                BIT(17)
> > > > > > > +#define LS7A_CONF_PHY_TX             BIT(18)
> > > > > > > +
> > > > > > > +/* Bits of LS7A_SYMBOL_TIMER */
> > > > > > > +#define LS7A_MASK_LEN_MATCH          BIT(26)
> > > > > > > +
> > > > > > > +/* Interval between interruptions */
> > > > > > > +#define LS2K_BMC_INT_INTERVAL                (60 * HZ)
> > > > > > > +
> > > > > > > +/* Maximum time to wait for U-Boot and DDR to be ready with ms. */
> > > > > > > +#define LS2K_BMC_RESET_WAIT_TIME     10000
> > > > > > > +
> > > > > > > +/* It's an experience value */
> > > > > > > +#define LS7A_BAR0_CHECK_MAX_TIMES    2000
> > > > > > > +
> > > > > > > +#define LS2K_BMC_RESET_GPIO          14
> > > > > > > +#define LOONGSON_GPIO_REG_BASE               0x1FE00500
> > > > > > > +#define LOONGSON_GPIO_REG_SIZE               0x18
> > > > > > > +#define LOONGSON_GPIO_OEN            0x0
> > > > > > > +#define LOONGSON_GPIO_FUNC           0x4
> > > > > > > +#define LOONGSON_GPIO_INTPOL         0x10
> > > > > > > +#define LOONGSON_GPIO_INTEN          0x14
> > > > > > > +
> > > > > > >  static struct resource ls2k_display_resources[] = {
> > > > > > >       DEFINE_RES_MEM_NAMED(LS2K_DISPLAY_RES_START, SZ_4M, "simpledrm-res"),
> > > > > > >  };
> > > > > > > @@ -62,6 +110,273 @@ static struct mfd_cell ls2k_bmc_cells[] = {
> > > > > > >       MFD_CELL_RES("ls2k-ipmi-si", ls2k_ipmi4_resources),
> > > > > > >  };
> > > > > > >
> > > > > > > +/* Index of the BMC PCI configuration space to be restored at BMC reset. */
> > > > > > > +struct ls2k_bmc_pci_data {
> > > > > > > +     u32 pci_command;
> > > > > > > +     u32 base_address0;
> > > > > > > +     u32 interrupt_line;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/* Index of the parent PCI configuration space to be restored at BMC reset. */
> > > > > > > +struct ls2k_bmc_bridge_pci_data {
> > > > > > > +     u32 pci_command;
> > > > > > > +     u32 base_address[6];
> > > > > > > +     u32 rom_addreess;
> > > > > > > +     u32 interrupt_line;
> > > > > > > +     u32 msi_hi;
> > > > > > > +     u32 msi_lo;
> > > > > > > +     u32 devctl;
> > > > > > > +     u32 linkcap;
> > > > > > > +     u32 linkctl_sts;
> > > > > > > +     u32 symbol_timer;
> > > > > > > +     u32 gen2_ctrl;
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct ls2k_bmc_pdata {
> > > > > > > +     struct device *dev;
> > > > > > > +     struct work_struct bmc_reset_work;
> > > > > > > +     struct ls2k_bmc_pci_data bmc_pci_data;
> > > > > > > +     struct ls2k_bmc_bridge_pci_data bridge_pci_data;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static bool ls2k_bmc_bar0_addr_is_set(struct pci_dev *ppdev)
> > > > > >
> > > > > > Nit: This is usually called pdev.
> > > > >
> > > > > OK.
> > > >
> > > > Snip things you agree with please.
> > > >
> > > > [...]
> > > >
> > > > > > > +static void ls2k_bmc_events_fn(struct work_struct *work)
> > > > > > > +{
> > > > > > > +     struct ls2k_bmc_pdata *priv = container_of(work, struct ls2k_bmc_pdata, bmc_reset_work);
> > > > > > > +
> > > > > > > +     /*
> > > > > > > +      * The PCI-E is lost when the BMC resets, at which point access to the PCI-E
> > > > > > > +      * from other CPUs is suspended to prevent a crash.
> > > > > > > +      */
> > > > > > > +     stop_machine(ls2k_bmc_recover_pci_data, priv, NULL);
> > > > > > > +
> > > > > > > +#ifdef CONFIG_VT
> > > > > >
> > > > > > #ifery in C-files is generally frowned upon.
> > > > > >
> > > > > > Is the any pieces of run-time data you can use instead?
> > > > > >
> > > > > > Or a stub which culminated in a no-op if !CONFIG_VT?
> > > > >
> > > > > Emm, I'm not sure if I understand correctly, but is the following way suitable?
> > > > >
> > > > >         if (IS_ENABLED(CONFIG_VT))
> > > >
> > > > It's better than #if, but even better would be a stub in a header file.
> > >
> > > Hmm... The declarations for vt_move_to_console()/set_console() are in
> > > two VT-related header files [1]. Adding the relevant stub functions
> > > involves the VT subsystem, which does not seem to be relevant to the
> > > subject of our patch set.
> > > Therefore, I think the above modification is more suitable for us.
> >
> > All of the subsystems in the kernel are open source, last time I checked. :)
> >
> > But okay, I won't insist on it.
> >
> > > [1]:
> > > vt_move_to_console:
> > > https://elixir.bootlin.com/linux/v6.15/source/include/linux/vt_kern.h#L141
> > > set_console: https://elixir.bootlin.com/linux/v6.15/source/include/linux/kbd_kern.h#L69
> > >
> > > >
> > > > >                 /* Re-push the display due to previous PCI-E loss. */
> > > > >                 set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> > > > >
> > > > > >
> > > > > > > +     /* Re-push the display due to previous PCI-E loss. */
> > > > > > > +     set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> > > > > > > +#endif
> > > > > > > +}
> > > > > > > +
> > > > > > > +static irqreturn_t ls2k_bmc_interrupt(int irq, void *arg)
> > > > > > > +{
> > > > > > > +     struct ls2k_bmc_pdata *priv = arg;
> > > > > > > +     static unsigned long last_jiffies;
> > > > > > > +
> > > > > > > +     if (system_state != SYSTEM_RUNNING)
> > > > > > > +             return IRQ_HANDLED;
> > > > > > > +
> > > > > > > +     /* Skip interrupt in LS2K_BMC_INT_INTERVAL */
> > > > > > > +     if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
> > > > > > > +             schedule_work(&priv->bmc_reset_work);
> > > > > > > +             last_jiffies = jiffies;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return IRQ_HANDLED;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Saves the BMC parent device (LS7A) and its own PCI configuration space registers
> > > > > > > + * that need to be restored after BMC reset.
> > > > > > > + */
> > > > > > > +static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_pdata *priv)
> > > > > > > +{
> > > > > > > +     struct pci_dev *parent = pdev->bus->self;
> > > > > > > +     int base, i = 0;
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, PCI_COMMAND, &priv->bridge_pci_data.pci_command);
> > > > > > > +
> > > > > > > +     for (base = PCI_BASE_ADDRESS_0; base <= PCI_BASE_ADDRESS_5; base += 4, i++)
> > > > > > > +             pci_read_config_dword(parent, base, &priv->bridge_pci_data.base_address[i]);
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, PCI_ROM_ADDRESS, &priv->bridge_pci_data.rom_addreess);
> > > > > > > +     pci_read_config_dword(parent, PCI_INTERRUPT_LINE, &priv->bridge_pci_data.interrupt_line);
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, parent->msi_cap + PCI_MSI_ADDRESS_LO,
> > > > > > > +                           &priv->bridge_pci_data.msi_lo);
> > > > > > > +     pci_read_config_dword(parent, parent->msi_cap + PCI_MSI_ADDRESS_HI,
> > > > > > > +                           &priv->bridge_pci_data.msi_hi);
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, parent->pcie_cap + PCI_EXP_DEVCTL,
> > > > > > > +                           &priv->bridge_pci_data.devctl);
> > > > > > > +     pci_read_config_dword(parent, parent->pcie_cap + PCI_EXP_LNKCAP,
> > > > > > > +                           &priv->bridge_pci_data.linkcap);
> > > > > > > +     pci_read_config_dword(parent, parent->pcie_cap + PCI_EXP_LNKCTL,
> > > > > > > +                           &priv->bridge_pci_data.linkctl_sts);
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, LS7A_GEN2_CTL, &priv->bridge_pci_data.gen2_ctrl);
> > > > > > > +     priv->bridge_pci_data.gen2_ctrl |= FIELD_PREP(LS7A_GEN2_SPEED_CHANG, 0x1)
> > > > > > > +                                     | FIELD_PREP(LS7A_CONF_PHY_TX, 0x0);
> > > > > > > +
> > > > > > > +     pci_read_config_dword(parent, LS7A_SYMBOL_TIMER, &priv->bridge_pci_data.symbol_timer);
> > > > > > > +     priv->bridge_pci_data.symbol_timer |= LS7A_MASK_LEN_MATCH;
> > > > > > > +
> > > > > > > +     pci_read_config_dword(pdev, PCI_COMMAND, &priv->bmc_pci_data.pci_command);
> > > > > > > +     pci_read_config_dword(pdev, PCI_BASE_ADDRESS_0, &priv->bmc_pci_data.base_address0);
> > > > > > > +     pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &priv->bmc_pci_data.interrupt_line);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int ls2k_bmc_pdata_initial(struct pci_dev *pdev, struct ls2k_bmc_pdata *priv)
> > > > > > > +{
> > > > > > > +     int gsi = 16 + (LS2K_BMC_RESET_GPIO & 7);
> > > > > > > +     void __iomem *gpio_base;
> > > > > > > +     int irq, ret;
> > > > > > > +
> > > > > > > +     ls2k_bmc_save_pci_data(pdev, priv);
> > > > > > > +
> > > > > > > +     INIT_WORK(&priv->bmc_reset_work, ls2k_bmc_events_fn);
> > > > > > > +
> > > > > > > +     ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
> > > > > > > +                            IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", priv);
> > > > > > > +     if (ret) {
> > > > > > > +             dev_err(priv->dev, "LS2KBMC PCI-E request_irq(%d) failed\n", pdev->irq);
> > > > > >
> > > > > > Please don't use function names in error messages.
> > > > > >
> > > > > > Make them human readable inclusive of non-kernel engineers.
> > > > >
> > > > > How about:
> > > > >
> > > > > dev_err(ddata->dev, "Failed to request LS2KBMC PCI-E IRQ %d.\n", pdev->irq);
> > > > >
> > > > > also, the error message regarding GPIO IRQ is as follows:
> > > > >
> > > > > dev_err(ddata->dev, "Failed to request LS2KBMC GPIO IRQ %d.\n", irq);
> > > >
> > > > Yes, much better.
> > > >
> > > > [...]
> > > >
> > > > > > > +     priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
> > > > > > > +     if (IS_ERR(priv)) {
> > > > > > > +             ret = -ENOMEM;
> > > > > > > +             goto disable_pci;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     priv->dev = &dev->dev;
> > > > > > > +
> > > > > > > +     ret = ls2k_bmc_pdata_initial(dev, priv);
> > > > > >
> > > > > > priv (ddata) already contains dev - you don't need both.
> > > > >
> > > > > Yes, we just pass priv(ddata), and
> > > > >
> > > > > struct pci_dev *pdev = to_pci_dev(ddata->dev);
> > > >
> > > > I would pass dev ... hold on, where do you store priv for reuse?
> > >
> > > Sorry, I am not entirely sure I understand your question. It does not
> > > appear that priv needs to be reused.
> >
> > Then why does it exist at all?
> 
> Emm...
> Perhaps I have strayed from the topic of our discussion. Let's try to
> get back on track.
> After replying to your previous email, I thought about whether your
> question was whether we need something similar to
> 
> pci_set_drvdata(dev, priv);
> 
> Looking at the code as a whole, perhaps it is because I am used to
> using `priv` as a function parameter, so in most cases I use `priv`
> directly, except in interrupt routines and INIT_WORK
> I hope we are on the right track with our discussion.

Okay, use priv for now.  But only pass other parameters when they cannot
be derived from priv.  Also ddata tends to be preferred over priv in
this subsystem.

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists