Re: [PATCH v2 2/2] PCI: mediatek-gen3: Configure PBUS_CSR registers for EN7581 SoC

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

> [+cc Frank, who asked the same question about DT]
> 
> On Thu, Feb 20, 2025 at 08:54:06PM +0100, Lorenzo Bianconi wrote:
> > On Feb 20, Bjorn Helgaas wrote:
> > > On Sun, Feb 02, 2025 at 08:34:24PM +0100, Lorenzo Bianconi wrote:
> > > > Configure PBus base address and address mask to allow the hw
> > > > to detect if a given address is on PCIE0, PCIE1 or PCIE2.
> 
> > > > +#define PCIE_EN7581_PBUS_ADDR(_n)	(0x00 + ((_n) << 3))
> > > > +#define PCIE_EN7581_PBUS_ADDR_MASK(_n)	(0x04 + ((_n) << 3))
> > > > +#define PCIE_EN7581_PBUS_BASE_ADDR(_n)	\
> > > > +	((_n) == 2 ? 0x28000000 :	\
> > > > +	 (_n) == 1 ? 0x24000000 : 0x20000000)
> > > 
> > > Are these addresses something that should be expressed in devicetree?
> > 
> > Do you have any example/pointer for it?
> > 
> > > It seems unusual to encode addresses directly in a driver.
> > 
> > AFAIK they are fixed for EN7581 SoC.
> 
> So this is used to detect if a given address is on PCIE0, PCIE1 or
> PCIE2.  What does that mean?  There are no other mentions of PCIE0 etc
> in the driver, but maybe they match up to "pcie0/1/2" in
> arch/arm64/boot/dts/mediatek/mt7988a.dtsi?
> 
> It looks like you use PCIE_EN7581_PBUS_ADDR(slot), where "slot" came
> from of_get_pci_domain_nr(), which suggests that these might be three
> separate Root Ports?

I was using pci_domain to detect the specific PCIe controller
(something similar to what is done here [0]) but I agree with Frank, it does
not seem completely correct.

[0] https://github.com/torvalds/linux/blob/master/drivers/pci/controller/pcie-mediatek.c#L1048

> 
> Are we talking about an MMIO address that an endpoint driver uses for
> readw() etc, and this code configures the hardware apertures through
> the host bridge?  Seems like that would be related to the "ranges"
> properties in DT.

I guess so, but I do not have any documentation about pbus-csr (adding Hui in
the loop).

As pointed out by Frank, do you agree to add these info in the dts? Something
like:

pcie0: pcie@1fc00000 {
	....
	mediatek,pbus-csr = <&pbus_csr 0x0 0x20000000 0x4 0xfc000000>;
	....
};

pcie1: pcie@1fc20000 {
	....
	mediatek,pbus-csr = <&pbus_csr 0x8 0x24000000 0xc 0xfc000000>;
	....
};

@Hui: can you please provide a better explanation about pbus-csr usage?

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]