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

[Yanchao Yang (杨彦超)]

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@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.
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.
> > 
> > [...]
> > 
> > > +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.
> 
> > 
> > > +       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?
Any ideas or comments for this? Please help share it at your
convenience.
> 
> > 
> > > @@ -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

BTW, can you help check other patches and share your suggestions?

Many thanks.
Yanchao.Yang