[PATCH net-next v4 01/10] net: wwan: tmi: Add PCIe core
Loic Poulain
loic.poulain at linaro.org
Tue Apr 25 06:56:09 PDT 2023
Hello,
On Mon, 10 Apr 2023 at 14:16, Yanchao Yang (杨彦超)
<Yanchao.Yang at mediatek.com> wrote:
>
> Hi Loic,
>
> sorry for late response, please check following reply.
>
> On Mon, 2023-03-20 at 21:26 +0800, Yanchao Yang wrote:
> > 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 at mediatek.com
> > > > 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 at mediatek.com>
> > > > Signed-off-by: Ting Wang <ting.wang at mediatek.com>
> > > > ---
> > > > 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.
> No, we don’t re-implement irq chip. After studying the irq_domain
> interface you suggest, the TMI driver leverages on MSI irq domain. We
> use pci_alloc_irq_vectors to allocate MSI-X irq desc and use
> pci_request_irq to bind interrupt sources with irq handlers.
What I mean is that you're implementing some interrupt muxing into
your driver, which could possibly be abstracted with a 'virtual' irq
domain and generic ops (e.g with proper irq domain your
mtk_pci_register_irq method could be replaced with the usual
request_irq), Think about it as future improvement, I assume we can go
with the current solution for now.
> > >
> > > [...]
> > >
> > > > +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.
> Any ideas or comments for this? Please help share it at your
> convenience.
They do not seem atomic... moreover you're using the _irqsave variant
while you're never accessing the list from (hard|soft)irq context.
> >
> > >
> > > > + 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);
Maybe alloc this outside the lock, so that you can use the 'regular'
GFP_KERNEL alloc.
> > > > + 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?
> Any ideas or comments for this? Please help share it at your
> convenience.
Yes, keep that in the .c file.
Regards,
Loic
More information about the Linux-mediatek
mailing list