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

[Loic Poulain]

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...).

[...]

> +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.

> +       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?

> @@ -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