Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Nov 08, 2024 at 11:22:52AM +0100, neil.armstrong@linaro.org wrote:
> On 07/11/2024 18:45, Mayank Rana wrote:
> > 
> > 
> > On 11/7/2024 12:45 AM, neil.armstrong@linaro.org wrote:
> > > Hi,
> > > 
> > > On 06/11/2024 23:13, Mayank Rana wrote:
> > > > On SA8255p ride platform, PCIe root complex is firmware managed as well
> > > > configured into ECAM compliant mode. This change adds functionality to
> > > > enable resource management (system resource as well PCIe controller and
> > > > PHY configuration) through firmware, and enumerating ECAM compliant root
> > > > complex.
> > > > 
> > > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/Kconfig     |   1 +
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
> > > >   2 files changed, 108 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index b6d6778b0698..0fe76bd39d69 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -275,6 +275,7 @@ config PCIE_QCOM
> > > >       select PCIE_DW_HOST
> > > >       select CRC8
> > > >       select PCIE_QCOM_COMMON
> > > > +    select PCI_HOST_COMMON
> > > >       help
> > > >         Say Y here to enable PCIe controller support on Qualcomm SoCs. The
> > > >         PCIe controller uses the DesignWare core plus Qualcomm-specific
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index ef44a82be058..2cb74f902baf 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -21,7 +21,9 @@
> > > >   #include <linux/limits.h>
> > > >   #include <linux/init.h>
> > > >   #include <linux/of.h>
> > > > +#include <linux/of_pci.h>
> > > >   #include <linux/pci.h>
> > > > +#include <linux/pci-ecam.h>
> > > >   #include <linux/pm_opp.h>
> > > >   #include <linux/pm_runtime.h>
> > > >   #include <linux/platform_device.h>
> > > > @@ -254,10 +256,12 @@ struct qcom_pcie_ops {
> > > >     * @ops: qcom PCIe ops structure
> > > >     * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
> > > >     * snooping
> > > > +  * @firmware_managed: Set if PCIe root complex is firmware managed
> > > >     */
> > > >   struct qcom_pcie_cfg {
> > > >       const struct qcom_pcie_ops *ops;
> > > >       bool override_no_snoop;
> > > > +    bool firmware_managed;
> > > >       bool no_l0s;
> > > >   };
> > > > @@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> > > >       .no_l0s = true,
> > > >   };
> > > > +static const struct qcom_pcie_cfg cfg_fw_managed = {
> > > > +    .firmware_managed = true,
> > > > +};
> > > > +
> > > >   static const struct dw_pcie_ops dw_pcie_ops = {
> > > >       .link_up = qcom_pcie_link_up,
> > > >       .start_link = qcom_pcie_start_link,
> > > > @@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> > > >       return IRQ_HANDLED;
> > > >   }
> > > > +static void qcom_pci_free_msi(void *ptr)
> > > > +{
> > > > +    struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
> > > > +
> > > > +    if (pp && pp->has_msi_ctrl)
> > > > +        dw_pcie_free_msi(pp);
> > > > +}
> > > > +
> > > > +static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
> > > > +{
> > > > +    struct device *dev = cfg->parent;
> > > > +    struct dw_pcie_rp *pp;
> > > > +    struct dw_pcie *pci;
> > > > +    int ret;
> > > > +
> > > > +    pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > +    if (!pci)
> > > > +        return -ENOMEM;
> > > > +
> > > > +    pci->dev = dev;
> > > > +    pp = &pci->pp;
> > > > +    pci->dbi_base = cfg->win;
> > > > +    pp->num_vectors = MSI_DEF_NUM_VECTORS;
> > > > +
> > > > +    ret = dw_pcie_msi_host_init(pp);
> > > > +    if (ret)
> > > > +        return ret;
> > > > +
> > > > +    pp->has_msi_ctrl = true;
> > > > +    dw_pcie_msi_init(pp);
> > > > +
> > > > +    ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +/* ECAM ops */
> > > > +const struct pci_ecam_ops pci_qcom_ecam_ops = {
> > > > +    .init        = qcom_pcie_ecam_host_init,
> > > > +    .pci_ops    = {
> > > > +        .map_bus    = pci_ecam_map_bus,
> > > > +        .read        = pci_generic_config_read,
> > > > +        .write        = pci_generic_config_write,
> > > > +    }
> > > > +};
> > > > +
> > > >   static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   {
> > > >       const struct qcom_pcie_cfg *pcie_cfg;
> > > > @@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >       char *name;
> > > >       pcie_cfg = of_device_get_match_data(dev);
> > > > -    if (!pcie_cfg || !pcie_cfg->ops) {
> > > > -        dev_err(dev, "Invalid platform data\n");
> > > > +    if (!pcie_cfg) {
> > > > +        dev_err(dev, "No platform data\n");
> > > > +        return -EINVAL;
> > > > +    }
> > > > +
> > > > +    if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
> > > > +        dev_err(dev, "No platform ops\n");
> > > >           return -EINVAL;
> > > >       }
> > > > +    pm_runtime_enable(dev);
> > > > +    ret = pm_runtime_get_sync(dev);
> > > > +    if (ret < 0)
> > > > +        goto err_pm_runtime_put;
> > > > +
> > > > +    if (pcie_cfg->firmware_managed) {
> > > > +        struct pci_host_bridge *bridge;
> > > > +        struct pci_config_window *cfg;
> > > > +
> > > > +        bridge = devm_pci_alloc_host_bridge(dev, 0);
> > > > +        if (!bridge) {
> > > > +            ret = -ENOMEM;
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        of_pci_check_probe_only();
> > > > +        /* Parse and map our Configuration Space windows */
> > > > +        cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
> > > > +        if (IS_ERR(cfg)) {
> > > > +            ret = PTR_ERR(cfg);
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        bridge->sysdata = cfg;
> > > > +        bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
> > > > +        bridge->msi_domain = true;
> > > > +
> > > > +        ret = pci_host_probe(bridge);
> > > > +        if (ret) {
> > > > +            dev_err(dev, "pci_host_probe() failed:%d\n", ret);
> > > > +            goto err_pm_runtime_put;
> > > > +        }
> > > > +
> > > > +        return ret;
> > > > +    }
> > > > +
> > > >       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > >       if (!pcie)
> > > >           return -ENOMEM;
> > > > @@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >       if (!pci)
> > > >           return -ENOMEM;
> > > > -    pm_runtime_enable(dev);
> > > > -    ret = pm_runtime_get_sync(dev);
> > > > -    if (ret < 0)
> > > > -        goto err_pm_runtime_put;
> > > > -
> > > >       pci->dev = dev;
> > > >       pci->ops = &dw_pcie_ops;
> > > >       pp = &pci->pp;
> > > > @@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   static int qcom_pcie_suspend_noirq(struct device *dev)
> > > >   {
> > > > -    struct qcom_pcie *pcie = dev_get_drvdata(dev);
> > > > +    struct qcom_pcie *pcie;
> > > >       int ret = 0;
> > > > +    if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
> > > 
> > > Can't you use if (pcie_cfg->firmware_managed) here instead ?
> > yes, although with firmware managed mode, struct qcom_pcie *pcie is not allocated, and just
> > to get access to pcie_cfg for this check, I took this approach. I am thiking to do allocating struct qcom_pcie *pcie and using it in future if we need more other related functionality which needs usage of this structure for functionality like global interrupt etc.
> > 
> > Although if you still prefer to allocate struct qcom_pcie based memory to access pcie_cfg, then I can consider to update in next patchset. Please suggest.
> 
> I understand, but running of_device_is_compatible() in runtime PM is not something we should do,
> so either allocate pcie_cfg, or add a firmware_managed bool to qcom_pcie copied from pcie_cfg,
> or move runtime pm callbacks in qcom_pcie_ops and don't declare any in cfg_fw_managed->ops.
> 
> I think the latter would be more scalable so we could add runtime pm variant handling
> for each IP versions. But it may be quite quite useless for now.
> 

Or just bail out if dev_get_drvdata() return NULL?

- Mani

-- 
மணிவண்ணன் சதாசிவம்