[PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
Duc Dang
dhdang at apm.com
Mon Oct 12 16:02:18 PDT 2015
On Sun, Oct 11, 2015 at 7:53 PM, Lian M.H. <Minghuan.Lian at freescale.com> wrote:
> Hi Duc,
>
> I notice your patches removed all the related MSI code and you descriptor said " Remove this unnecessary code. This also avoids a warning message ("failed to enable MSI") during boot ".
> I have a question.
> Do we need a warning when MSI-parent is missing?
Hi Lian,
In case of X-Gene PCIe: No. Initially, X-Gene PCIe driver does not
support MSI. Later on, MSI support was added with the code to look for
msi-parent property. But then thanks to recent works on msi irqdomain,
these code is no longer needed to support MSI for X-Gene PCIe, so I
removed them.
>
> Thanks,
> Minghuan
>
>> -----Original Message-----
>> From: Duc Dang [mailto:dhdang at apm.com]
>> Sent: Monday, October 12, 2015 9:48 AM
>> To: Bjorn Helgaas <helgaas at kernel.org>
>> Cc: Lian Minghuan-B31939 <Minghuan.Lian at freescale.com>;
>> linux-pci at vger.kernel.org; linux-arm <linux-arm-kernel at lists.infradead.org>;
>> Zang Roy-R61911 <tie-fei.zang at freescale.com>; Hu Mingkai-B21284
>> <Mingkai.Hu at freescale.com>; Yoder Stuart-B08248
>> <stuart.yoder at freescale.com>; Li Yang-Leo-R58472 <LeoLi at freescale.com>;
>> Arnd Bergmann <arnd at arndb.de>; Bjorn Helgaas <bhelgaas at google.com>;
>> Jingoo Han <jg1.han at samsung.com>; Zhou Wang
>> <wangzhou1 at hisilicon.com>; Thomas Petazzoni
>> <thomas.petazzoni at free-electrons.com>; Jason Cooper
>> <jason at lakedaemon.net>; Tanmay Inamdar <tinamdar at apm.com>
>> Subject: Re: [PATCH v2] PCI: layerscape: Add PCIe support for LS1043a and
>> LS2080a
>>
>> On Sun, Oct 11, 2015 at 12:10 PM, Bjorn Helgaas <helgaas at kernel.org> wrote:
>> > [+cc Thomas, Jason, Tanmay]
>> >
>> > Hi Minghuan,
>> >
>> > You responded to this message, but your response was rejected by the
>> > mailing list, I think because it was not plain text. See
>> > http://vger.kernel.org/majordomo-info.html
>> >
>> > I went ahead and reconstructed what your response would have looked
>> > like so I could continue the conversation. But please fix your email
>> > setup before responding again :)
>> >
>> > Minghuan wrote:
>> >> On Wed, Oct 07, 2015 at 12:57:25PM -0500, Bjorn Helgaas wrote:
>> >> > Hi Minghuan,
>> >> >
>> >> > On Thu, Sep 17, 2015 at 05:13:39PM +0800, Minghuan Lian wrote:
>> >> > > The patch adds PCIe support for LS1043a and LS2080a.
>> >> > >
>> >> > > Signed-off-by: Minghuan Lian <Minghuan.Lian at freescale.com>
>> >> > > ---
>> >> > > This patch is based on v4.3-rc1 and [PATCH v9 0/6]
>> >> > > PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05 patchset
>> >> > > from Zhou Wang.
>> >> > >
>> >> > > change log
>> >> > > v2:
>> >> > > 1. rename ls2085a to ls2080a
>> >> > > 2. Add ls_pcie_msi_host_init()
>> >> > >
>> >> > > drivers/pci/host/Kconfig | 2 +-
>> >> > > drivers/pci/host/pci-layerscape.c | 227
>> >> > > ++++++++++++++++++++++++++------------
>> >> > > 2 files changed, 157 insertions(+), 72 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> >> > > index ae873be..38fe8a8 100644
>> >> > > --- a/drivers/pci/host/Kconfig
>> >> > > +++ b/drivers/pci/host/Kconfig
>> >> > > @@ -105,7 +105,7 @@ config PCI_XGENE_MSI
>> >> > >
>> >> > > config PCI_LAYERSCAPE
>> >> > > bool "Freescale Layerscape PCIe controller"
>> >> > > - depends on OF && ARM
>> >> > > + depends on OF && (ARM || ARM64)
>> >> >
>> >> > It seems like there are a couple things going on here, and I wonder
>> >> > if you can split them out into separate patches.
>> >> >
>> >> > 1) Making this work on ARM64 as well as on ARM. This may be of
>> >> > interest for other DesignWare-based drivers, so if you split this
>> >> > out, maybe it would be a useful template for converting the other
>> >> > drivers, too.
>> >> >
>> >> > 2) Adding LS1043a and LS2080a. Obviously, this is only of interest
>> >> > to this driver, but maybe it could be separated out into a separate
>> >> > patch?
>> >
>> >> The patch is based on v4.3-rc1 and [PATCH v9 0/6] PCI: hisi: Add PCIe
>> >> host support for HiSilicon SoC Hip05 patchset from Zhou Wang which
>> >> adds arm and
>> >> arm64 support.
>> >>
>> >> The original code with the Zhou Wang's patches can support arm64 as well.
>> >> Because both LS1043a and LS2085 are armv8 (arm64) architecture, the
>> >> patch updates Kconfig to add 'arm64'.
>> >>
>> >> If splitting two patches, the first patch for arm64 support only
>> >> includes one line changes of Kconfig. So, I think it is unnecessary.
>> >
>> > It's OK if the first patch is very small. The point is that the patch
>> > does two orthogonal things, and those two things should be in separate
>> > commits. This makes bisection work better, and it reduces the amount
>> > of functionality removed if we have to revert something.
>> >
>> >> > > +static int ls_pcie_msi_host_init(struct pcie_port *pp,
>> >> > > + struct msi_controller *chip) { struct
>> >> > > +device_node *msi_node; struct device_node *np =
>> >> > > +pp->dev->of_node;
>> >> > > +
>> >> > > + msi_node = of_parse_phandle(np, "msi-parent", 0); if
>> >> > > + (!msi_node) {
>> >> > > + dev_err(pp->dev, "failed to find msi-parent\n");
>> >> > > + return -EINVAL;
>> >> > > + }
>> >> >
>> >> > This doesn't actually *do* anything except complain if "msi-parent"
>> >> > is missing. I'm not sure that's worth doing: if we need "msi-parent"
>> >> > somewhere, we should complain there if we don't find it. If we
>> >> > don't need it, why complain if it's missing?
>> >
>> >> driver/of/irq.c void of_msi_configure(struct device *dev, struct
>> >> device_node *np) will bind "msi-parent" to each device if there is
>> >> "msi-parent" handler. The PCIe driver do not need to do anything. If
>> >> we do not check "msi-parent" here, we will have no chance to check it.
>> >> The common code of 'of' and 'pci' bus driver will not complain,
>> >> because the msi controller may be found by other way.
>> >
>> > Hmm. In mvebu_pcie_msi_enable() and xgene_pcie_msi_enable(), we
>> > also look for "msi-parent". If that fails, mvebu continues silently
>> > and xgene complains (but only if CONFIG_PCI_MSI=y).
>> >
>> > This seems like three unnecessarily different ways of doing the same
>> > thing.
>>
>> For X-Gene MSI, this is the old code where at the time the code was prepared,
>> msi_controller needs to be assigned to a host bridge so that the bridge can
>> support MSI. Until now, with recent changes on MSI irqdomain from Marc and
>> others, we can drop this msi_controller assignment completely as my recent
>> patch that you merged to pci/host-xgene branch:
>> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host
>> -xgene
>>
>> >
>> > I cc'd the maintainers of those drivers; maybe we can agree on a
>> > single strategy.
>> >
>> >> > > static int __init ls_pcie_probe(struct platform_device *pdev) {
>> >> > > ...
>> >> > > - ret = ls_add_pcie_port(pcie);
>> >> > > - if (ret < 0)
>> >> > > + ret = dw_pcie_host_init(pp);
>> >> >
>> >> > We have several DesignWare-based drivers (dra7xx, exynos, imx6, ks,
>> >> > ls, spear13xx), and I'd like to keep their structure as similar as
>> >> > possible. For example, they all have basically this structure:
>> >> >
>> >> > X_pcie_probe
>> >> > X_add_pcie_port
>> >> > dw_pcie_host_init # pp->ops->host_init callback
>> >> > X_pcie_host_init
>> >> > X_pcie_establish_link
>> >> >
>> >> > This patch removes ls_add_pcie_port() and ls_pcie_establish_link(),
>> >> > so we're diverging a bit. That makes it harder to see the
>> >> > similarities across these drivers, which I think is a loss.
>> >
>> >> I will add X_add_pcie_port. But we do not need X_pcie_establish_link
>> >> because we do not need change/reset phy to establish link, besides,
>> >> the PCIe controller slot may be not inserted any PCIe device at all.
>> >> If each controller waits some time for link, the start time will
>> >> increase a lot. In some scenes, long start-up time is not acceptable.
>> >
>> > If we wait for a timeout trying to establish a link when the slot is
>> > empty, it sounds like something's wrong. Can't we tell from presence
>> > detect whether a card is present, even without trying to bring up the
>> > link?
>> >
>> > If we truly do not need a piece like X_pcie_establish_link(), I'm OK
>> > with entirely omitting it. What I really object to is when we have
>> > the same functionality two places with different names or structured
>> > two different ways.
>> >
>> > Bjorn
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-pci"
>> > in the body of a message to majordomo at vger.kernel.org More majordomo
>> > info at http://vger.kernel.org/majordomo-info.html
>>
>> Regards,
>> Duc Dang.
Regards.
Duc Dang.
More information about the linux-arm-kernel
mailing list