Re: [PATCH RFC NOT TESTED] PCI: intel-gw: Use use_parent_dt_ranges and clean up intel_pcie_cpu_addr_fixup()

From: Bjorn Helgaas
Date: Wed Mar 19 2025 - 15:23:52 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH v4 2/2] ARM: dts: aspeed: Add Initial device tree for AMD Onyx Platform"
Previous message: Jason Gunthorpe: "Re: [PATCH v3 1/1] KVM: arm64: Allow cacheable stage 2 mapping using VMA flags"
In reply to: Lei Chuan Hua: "RE: [PATCH RFC NOT TESTED] PCI: intel-gw: Use use_parent_dt_ranges and clean up intel_pcie_cpu_addr_fixup()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Mar 19, 2025 at 06:10:57AM +0000, Lei Chuan Hua wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > Sent: Tuesday, 18 March 2025 11:32 pm
> > To: Lei Chuan Hua <lchuanhua@xxxxxxxxxxxxx>
> > Cc: Frank Li <Frank.Li@xxxxxxx>; Lorenzo Pieralisi
> > <lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński <kw@xxxxxxxxx>; Manivannan
> > Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>; Rob Herring
> > <robh@xxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; linux-
> > pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH RFC NOT TESTED] PCI: intel-gw: Use
> > use_parent_dt_ranges and clean up intel_pcie_cpu_addr_fixup()
> >
> > This email was sent from outside of MaxLinear.
> >
> > On Tue, Mar 18, 2025 at 01:49:46AM +0000, Lei Chuan Hua wrote:
> > > Hi Bjorn,
> > >
> > > I did a quick test with necessary change in dts. It worked, please
> > > move on.
> >
> > What does this mean? By "move on", do you mean that I should merge the
> > patch below (the removal of intel_pcie_cpu_addr())?
>
> I mean you can merge the patch with removal of intel_pcie_cpu_addr()
>
> > I do not want to merge a change that will break any existing intel-gw
> > platform. When you say "with necessary change in dts", it makes me
> > think the removal of intel_pcie_cpu_addr() forces a change to dts, which
> > would not be acceptable. We can't force users to upgrade the dts just
> > to run a newer kernel.
>
> Actually, intel_pcie_cpu_addr() did the address translation, so in our case,
> Dts has to adapt to this change.
>
> > I assume 250318 linux-next, which includes Frank's v12 series, should
> > work with no change to dts required. (It would be awesome if you can
> > verify that.)
>
> I will try 250318 linux-next and let you know the result once it is done.
>
> > If you apply this patch to remove intel_pcie_cpu_addr() on top of
> > 250318 linux-next, does it still work with no changes to dts?
>
> I think we need to adapt dts change. Even this series patch has dts
> adaption part.
>
> > If you have to make a dts change for it to work after removing
> > intel_pcie_cpu_addr(), then we have a problem.
>
> We can update the dts yaml file.
>
> > I do not see a .dts file in the upstream tree that contains "intel,lgm-
> > pcie", so I don't know what the .dts contains or how it is distributed.
> >
> > I do see the binding at
> > Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml,
> > but the example there does not include anything about address
> > translation between the CPU and the PCI controller, so my guess is that
> > there are .dts files in the field that will not work if we remove
> > intel_pcie_cpu_addr().
>
> This driver is for x86 atom platform, no upstream dts file even in
> arch/x86/boot Since upstream x86 platforms use acpi, even several
> platforms use dts, but never create dts directory in
> arch/x86/boot.
>
> As I mentioned earlier, dts needs a minor change.

If there are end users that have a dts that must be changed before
intel_pcie_cpu_addr() can be removed, we can't remove it because we
can't force those users to upgrade their dts.

If this driver is only used internally to Intel, or if the hardware
has never been shipped to end users, it's OK to remove
intel_pcie_cpu_addr() and assume those internal users will update dts.

> > > ________________________________________
> > > From: Bjorn Helgaas <mailto:helgaas@xxxxxxxxxx>
> > > Sent: Tuesday, March 18, 2025 1:59 AM
> > > To: Frank Li <mailto:Frank.Li@xxxxxxx>
> > > Cc: Lei Chuan Hua <mailto:lchuanhua@xxxxxxxxxxxxx>; Lorenzo Pieralisi
> > > <mailto:lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński <mailto:kw@xxxxxxxxx>;
> > > Manivannan Sadhasivam <mailto:manivannan.sadhasivam@xxxxxxxxxx>; Rob Herring
> > > <mailto:robh@xxxxxxxxxx>; Bjorn Helgaas <mailto:bhelgaas@xxxxxxxxxx>;
> > > mailto:linux-pci@xxxxxxxxxxxxxxx <mailto:linux-pci@xxxxxxxxxxxxxxx>;
> > > mailto:linux-kernel@xxxxxxxxxxxxxxx <mailto:linux-kernel@xxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH RFC NOT TESTED] PCI: intel-gw: Use
> > > use_parent_dt_ranges and clean up intel_pcie_cpu_addr_fixup()
> > >
> > >
> > >
> > > On Wed, Mar 05, 2025 at 12:07:54PM -0500, Frank Li wrote:
> > > > Remove intel_pcie_cpu_addr_fixup() as the DT bus fabric should
> > > > provide correct address translation. Set use_parent_dt_ranges to
> > > > allow the DWC core driver to fetch address translation from the
> > device tree.
> > > >
> > > > Signed-off-by: Frank Li <mailto:Frank.Li@xxxxxxx>
> > >
> > > Any update on this, Chuanhua?
> > >
> > > I plan to merge v12 of Frank's series [1] for v6.15. We need to know
> > > ASAP if that would break intel-gw.
> > >
> > > If we knew that it was safe to also apply this patch to remove
> > > intel_pcie_cpu_addr(), that would be even better.
> > >
> > > I will plan to apply the patch below on top of Frank's series [1] for
> > > v6.15 unless I hear that it would break something.
> > >
> > > Bjorn
> > >
> > > [1]
> > > https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fr%2F20250315201548.858189-1-helgaas%40kernel.org&data=05
> > > %7C02%7Clchuanhua%40maxlinear.com%7C1612d73ded5741bbd37508dd66320100%7
> > > Cdac2800513e041b882807663835f2b1d%7C0%7C0%7C638779087153570342%7CUnkno
> > > wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXa
> > > W4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=aIuZWzwFy2r
> > > rzsJ5KfbxWKMx%2BPn1WHx2KvpSR0nxsl8%3D&reserved=0
> > >
> > > > ---
> > > > This patches basic on
> > > > https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > > re.kernel.org%2Fimx%2F20250128-pci_fixup_addr-v9-0-3c4bb506f665%40nx
> > > > p.com%2F&data=05%7C02%7Clchuanhua%40maxlinear.com%7C1612d73ded5741bb
> > > > d37508dd66320100%7Cdac2800513e041b882807663835f2b1d%7C0%7C0%7C638779
> > > > 087153596851%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiI
> > > > wLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> > > > %7C%7C&sdata=mht19VSB24Znpvtz1pOlmtHYec%2BDBDH70zuLOZmwlSI%3D&reserv
> > > > ed=0
> > > >
> > > > I have not hardware to test and there are not intel,lgm-pcie in
> > > > kernel tree.
> > > >
> > > > Your dts should correct reflect hardware behavor, ref:
> > > > https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > > re.kernel.org%2Flinux-pci%2FZ8huvkENIBxyPKJv%40axis.com%2FT%2F%23mb7
> > > > ae78c3a22324b37567d24ecc1c810c1b3f55c5&data=05%7C02%7Clchuanhua%40ma
> > > > xlinear.com%7C1612d73ded5741bbd37508dd66320100%7Cdac2800513e041b8828
> > > > 07663835f2b1d%7C0%7C0%7C638779087153612764%7CUnknown%7CTWFpbGZsb3d8e
> > > > yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiT
> > > > WFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=DUsGCW%2FZpvx4whLteoIjYqw
> > > > d6oOk9rXks%2BV40i5sovI%3D&reserved=0
> > > >
> > > > According to your intel_pcie_cpu_addr_fixup()
> > > >
> > > > Basically, config space/io/mem space need minus SZ_256. parent bus
> > > > range convert it to original value.
> > > >
> > > > Look for driver owner, who help test this and start move forward to
> > > > remove
> > > > cpu_addr_fixup() work.
> > > > ---
> > > > Frank Li <mailto:Frank.Li@xxxxxxx>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-intel-gw.c | 8 +-------
> > > > 1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > > b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > > index 9b53b8f6f268e..c21906eced618 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > > @@ -57,7 +57,6 @@
> > > > PCIE_APP_IRN_INTA | PCIE_APP_IRN_INTB | \
> > > > PCIE_APP_IRN_INTC | PCIE_APP_IRN_INTD)
> > > >
> > > > -#define BUS_IATU_OFFSET SZ_256M
> > > > #define RESET_INTERVAL_MS 100
> > > >
> > > > struct intel_pcie {
> > > > @@ -381,13 +380,7 @@ static int intel_pcie_rc_init(struct dw_pcie_rp
> > *pp)
> > > > return intel_pcie_host_setup(pcie); }
> > > >
> > > > -static u64 intel_pcie_cpu_addr(struct dw_pcie *pcie, u64 cpu_addr)
> > > > -{
> > > > - return cpu_addr + BUS_IATU_OFFSET;
> > > > -}
> > > > -
> > > > static const struct dw_pcie_ops intel_pcie_ops = {
> > > > - .cpu_addr_fixup = intel_pcie_cpu_addr,
> > > > };
> > > >
> > > > static const struct dw_pcie_host_ops intel_pcie_dw_ops = { @@
> > > > -409,6 +402,7 @@ static int intel_pcie_probe(struct platform_device
> > *pdev)
> > > > platform_set_drvdata(pdev, pcie);
> > > > pci = &pcie->pci;
> > > > pci->dev = dev;
> > > > + pci->use_parent_dt_ranges = true;
> > > > pp = &pci->pp;
> > > >
> > > > ret = intel_pcie_get_resources(pdev);
> > > >
> > > > ---
> > > > base-commit: 1552be4855dacca5ea39b15b1ef0b96c91dbea0d
> > > > change-id: 20250305-intel-7c25bfb498b1
> > > >
> > > > Best regards,
> > > >

Next message: Krzysztof Kozlowski: "Re: [PATCH v4 2/2] ARM: dts: aspeed: Add Initial device tree for AMD Onyx Platform"
Previous message: Jason Gunthorpe: "Re: [PATCH v3 1/1] KVM: arm64: Allow cacheable stage 2 mapping using VMA flags"
In reply to: Lei Chuan Hua: "RE: [PATCH RFC NOT TESTED] PCI: intel-gw: Use use_parent_dt_ranges and clean up intel_pcie_cpu_addr_fixup()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]