LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Rob Herring <robh+dt@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Chuanjia Liu <chuanjia.liu@mediatek.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Ryder Lee <ryder.lee@mediatek.com>,
	Jianjun Wang <jianjun.wang@mediatek.com>,
	Yong Wu <yong.wu@mediatek.com>, PCI <linux-pci@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>,
	devicetree@vger.kernel.org,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v12 2/6] PCI: mediatek: Add new method to get shared pcie-cfg base address
Date: Tue, 31 Aug 2021 10:04:40 -0500	[thread overview]
Message-ID: <CAL_JsqKEvAypWhiaWMhxM7zVkLAFL9=eMU7_vr=ht+uyxYe0qg@mail.gmail.com> (raw)
In-Reply-To: <20210827164634.GA3779223@bjorn-Precision-5520>

On Fri, Aug 27, 2021 at 11:46 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Aug 23, 2021 at 11:27:56AM +0800, Chuanjia Liu wrote:
> > For the new dts format, add a new method to get
> > shared pcie-cfg base address and use it to configure
> > the PCIECFG controller
>
> Rewrap this to fill 75 columns.
>
> > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 25bee693834f..4296d9e04240 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/msi.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> >  #include <linux/reset.h>
> >
> >  #include "../pci.h"
> > @@ -207,6 +209,7 @@ struct mtk_pcie_port {
> >   * struct mtk_pcie - PCIe host information
> >   * @dev: pointer to PCIe device
> >   * @base: IO mapped register base
> > + * @cfg: IO mapped register map for PCIe config
> >   * @free_ck: free-run reference clock
> >   * @mem: non-prefetchable memory resource
> >   * @ports: pointer to PCIe port information
> > @@ -215,6 +218,7 @@ struct mtk_pcie_port {
> >  struct mtk_pcie {
> >       struct device *dev;
> >       void __iomem *base;
> > +     struct regmap *cfg;
> >       struct clk *free_ck;
> >
> >       struct list_head ports;
> > @@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >               val |= PCIE_CSR_LTSSM_EN(port->slot) |
> >                      PCIE_CSR_ASPM_L1_EN(port->slot);
> >               writel(val, pcie->base + PCIE_SYS_CFG_V2);
> > +     } else if (pcie->cfg) {
> > +             val = PCIE_CSR_LTSSM_EN(port->slot) |
> > +                   PCIE_CSR_ASPM_L1_EN(port->slot);
> > +             regmap_update_bits(pcie->cfg, PCIE_SYS_CFG_V2, val, val);
> >       }
> >
> >       /* Assert all reset signals */
> > @@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
> >       struct device *dev = pcie->dev;
> >       struct platform_device *pdev = to_platform_device(dev);
> >       struct resource *regs;
> > +     struct device_node *cfg_node;
> >       int err;
> >
> >       /* get shared registers, which are optional */
> > @@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct mtk_pcie *pcie)
> >                       return PTR_ERR(pcie->base);
> >       }
> >
> > +     cfg_node = of_find_compatible_node(NULL, NULL,
> > +                                        "mediatek,generic-pciecfg");
>
> This looks wrong to me.  IIUC, since we start at NULL, this searches
> the entire device tree for any node with
>
>   compatible = "mediatek,generic-pciecfg"
>
> but we should only care about the specific device/node this driver
> claimed.
>
> Should this be part of the match data, i.e., struct mtk_pcie_soc?

What would you put in match data exactly?

The other way to do this is to have a DT property with the phandle
which people like to do (have everything in the node 'for their
driver'). If there's only 1 possible node (which is almost always the
case), then there is little benefit to having another property. It's
just redundant data. A phandle lookup might be a bit faster with the
caching we do, but on a miss it would still walk all nodes.

The other thing with these 'extra register bits to twiddle' is that
they tend to be SoC specific and change from chip to chip, so either
way is not very portable. The real question to ask is should there be
a standard interface used or created.

>
> > +     if (cfg_node) {
> > +             pcie->cfg = syscon_node_to_regmap(cfg_node);
>
> Other drivers in drivers/pci/controller/ use
> syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
> layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
> kirin, v3-semi).

There's no phandle to use in this case. As above, I'm trying to break
people of this habit.

Rob

  parent reply	other threads:[~2021-08-31 15:04 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-23  3:27 [PATCH v12 0/6] PCI: mediatek: Spilt PCIe node to comply with hardware design Chuanjia Liu
2021-08-23  3:27 ` [PATCH v12 1/6] dt-bindings: PCI: mediatek: Update the Device tree bindings Chuanjia Liu
2021-08-23  3:27 ` [PATCH v12 2/6] PCI: mediatek: Add new method to get shared pcie-cfg base address Chuanjia Liu
2021-08-27 16:46   ` Bjorn Helgaas
2021-08-30  7:09     ` Chuanjia Liu
2021-08-30 21:43       ` Bjorn Helgaas
2021-08-31  3:31         ` Chuanjia Liu
2021-08-31 15:17           ` Rob Herring
2021-09-02  9:34             ` Chuanjia Liu
2021-08-31 15:04     ` Rob Herring [this message]
2021-08-31 15:47       ` Bjorn Helgaas
2021-08-31 18:24         ` Rob Herring
2021-08-23  3:27 ` [PATCH v12 3/6] PCI: mediatek: Add new method to get irq number Chuanjia Liu
2021-08-31 18:30   ` Bjorn Helgaas
2021-09-02  9:28     ` Chuanjia Liu
2021-08-23  3:27 ` [PATCH v12 4/6] PCI: mediatek: Get pci domain and decide how to parse node Chuanjia Liu
2021-08-23  3:27 ` [PATCH v12 5/6] arm64: dts: mediatek: Split PCIe node for MT2712 and MT7622 Chuanjia Liu
2021-09-21 18:43   ` Matthias Brugger
2021-08-23  3:28 ` [PATCH v12 6/6] ARM: dts: mediatek: Update MT7629 PCIe node for new format Chuanjia Liu
2021-09-21 18:43   ` Matthias Brugger
2021-08-26 12:53 ` [PATCH v12 0/6] PCI: mediatek: Spilt PCIe node to comply with hardware design Lorenzo Pieralisi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL_JsqKEvAypWhiaWMhxM7zVkLAFL9=eMU7_vr=ht+uyxYe0qg@mail.gmail.com' \
    --to=robh+dt@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=chuanjia.liu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=helgaas@kernel.org \
    --cc=jianjun.wang@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=matthias.bgg@gmail.com \
    --cc=ryder.lee@mediatek.com \
    --cc=yong.wu@mediatek.com \
    --subject='Re: [PATCH v12 2/6] PCI: mediatek: Add new method to get shared pcie-cfg base address' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).