Re: [PATCH net-next v4 01/10] net: wwan: tmi: Add PCIe core

[Yanchao Yang (杨彦超)]

Hi Loic,

On Fri, 2023-03-17 at 13:34 +0100, Loic Poulain wrote:
> Hi Yanchao,
> 
> On Fri, 17 Mar 2023 at 09:10, Yanchao Yang <yanchao.yang@xxxxxxxxxxxx
> > wrote:
> > 
> > Registers the TMI device driver with the kernel. Set up all the
> > fundamental
> > configurations for the device: PCIe layer, Modem Host Cross Core
> > Interface
> > (MHCCIF), Reset Generation Unit (RGU), modem common control
> > operations and
> > build infrastructure.
> > 
> > * PCIe layer code implements driver probe and removal, MSI-X
> > interrupt
> > initialization and de-initialization, and the way of resetting the
> > device.
> > * MHCCIF provides interrupt channels to communicate events such as
> > handshake,
> > PM and port enumeration.
> > * RGU provides interrupt channels to generate notifications from
> > the device
> > so that the TMI driver could get the device reset.
> > * Modem common control operations provide the basic read/write
> > functions of
> > the device's hardware registers, mask/unmask/get/clear functions of
> > the
> > device's interrupt registers and inquiry functions of the device's
> > status.
> > 
> > Signed-off-by: Yanchao Yang <yanchao.yang@xxxxxxxxxxxx>
> > Signed-off-by: Ting Wang <ting.wang@xxxxxxxxxxxx>
> > ---
> >  drivers/net/wwan/Kconfig                 |  14 +
> >  drivers/net/wwan/Makefile                |   1 +
> >  drivers/net/wwan/mediatek/Makefile       |   8 +
> >  drivers/net/wwan/mediatek/mtk_dev.h      | 203 ++++++
> >  drivers/net/wwan/mediatek/pcie/mtk_pci.c | 887
> > +++++++++++++++++++++++
> >  drivers/net/wwan/mediatek/pcie/mtk_pci.h | 144 ++++
> >  drivers/net/wwan/mediatek/pcie/mtk_reg.h |  69 ++
> >  7 files changed, 1326 insertions(+)
> >  create mode 100644 drivers/net/wwan/mediatek/Makefile
> >  create mode 100644 drivers/net/wwan/mediatek/mtk_dev.h
> >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_pci.c
> >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_pci.h
> >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_reg.h
> > 
> 
> [...]
> 
> > +static int mtk_pci_get_virq_id(struct mtk_md_dev *mdev, int
> > irq_id)
> > +{
> > +       struct pci_dev *pdev = to_pci_dev(mdev->dev);
> > +       int nr = 0;
> > +
> > +       if (pdev->msix_enabled)
> > +               nr = irq_id % mdev->msi_nvecs;
> > +
> > +       return pci_irq_vector(pdev, nr);
> > +}
> > +
> > +static int mtk_pci_register_irq(struct mtk_md_dev *mdev, int
> > irq_id,
> > +                               int (*irq_cb)(int irq_id, void
> > *data), void *data)
> > +{
> > +       struct mtk_pci_priv *priv = mdev->hw_priv;
> > +
> > +       if (unlikely((irq_id < 0 || irq_id >= MTK_IRQ_CNT_MAX) ||
> > !irq_cb))
> > +               return -EINVAL;
> > +
> > +       if (priv->irq_cb_list[irq_id]) {
> > +               dev_err(mdev->dev,
> > +                       "Unable to register irq, irq_id=%d, it's
> > already been register by %ps.\n",
> > +                       irq_id, priv->irq_cb_list[irq_id]);
> > +               return -EFAULT;
> > +       }
> > +       priv->irq_cb_list[irq_id] = irq_cb;
> > +       priv->irq_cb_data[irq_id] = data;
> 
> So it looks like you re-implement your own irq chip internally. What
> about creating a new irq-chip/domain for this (cf
> irq_domain_add_simple)?
> That would allow the client code to use the regular irq interface and
> helpers
> and it should simply code and improve its debuggability
> (/proc/irq...).
We will check it and update you later.
> 
> [...]
> 
> > +static int mtk_mhccif_register_evt(struct mtk_md_dev *mdev, u32
> > chs,
> > +                                  int (*evt_cb)(u32 status, void
> > *data), void *data)
> > +{
> > +       struct mtk_pci_priv *priv = mdev->hw_priv;
> > +       struct mtk_mhccif_cb *cb;
> > +       unsigned long flag;
> > +       int ret = 0;
> > +
> > +       if (!chs || !evt_cb)
> > +               return -EINVAL;
> > +
> > +       spin_lock_irqsave(&priv->mhccif_lock, flag);
> 
> Why spinlock here and not mutex. AFAIU, you always take this lock in
> a
> non-atomic/process context.
Currently, the function is only called in the FSM initialization and
PM(power management) initialization process. Both are atomic.
On the other hand, this registration function will operate the global
variables “mhccif_cb_list”, but it takes very little time. So, we think
spinlock is preferred.

> 
> > +       list_for_each_entry(cb, &priv->mhccif_cb_list, entry) {
> > +               if (cb->chs & chs) {
> > +                       ret = -EFAULT;
> > +                       dev_err(mdev->dev,
> > +                               "Unable to register evt,
> > chs=0x%08X&0x%08X registered_cb=%ps\n",
> > +                               chs, cb->chs, cb->evt_cb);
> > +                       goto err;
> > +               }
> > +       }
> > +       cb = devm_kzalloc(mdev->dev, sizeof(*cb), GFP_ATOMIC);
> > +       if (!cb) {
> > +               ret = -ENOMEM;
> > +               goto err;
> > +       }
> > +       cb->evt_cb = evt_cb;
> > +       cb->data = data;
> > +       cb->chs = chs;
> > +       list_add_tail(&cb->entry, &priv->mhccif_cb_list);
> > +
> > +err:
> > +       spin_unlock_irqrestore(&priv->mhccif_lock, flag);
> > +
> > +       return ret;
> > +}
> 
> [...]
> 
> > +
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > b/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > new file mode 100644
> > index 000000000000..b487ca9b302e
> > --- /dev/null
> > +++ b/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> 
> Why a separated header file, isn't the content (e.g. mtk_pci_priv)
> used only from mtk_pci.c?
Do you mean that we should move all contents of “mtk_pci.h” into
“mtk_pci.c” directly? The “mtk_pci.h” seems to be redundant, right?

> 
> > @@ -0,0 +1,144 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause-Clear
> > + *
> > + * Copyright (c) 2022, MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_PCI_H__
> > +#define __MTK_PCI_H__
> > +
> > +#include <linux/pci.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include "../mtk_dev.h"
> > +
> > +enum mtk_atr_type {
> > +       ATR_PCI2AXI = 0,
> > +       ATR_AXI2PCI
> > +};
> 
> [...]
> 
> Regards,
> Loic

Many thanks.
Yanchao.Yang