| Message ID | 20241031-ep-msi-v4-0-717da2d99b28@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: EP: Add RC-to-EP doorbell with platform MSI controller | expand |
Hello Frank, On Thu, Oct 31, 2024 at 12:26:59PM -0400, Frank Li wrote: > ┌────────────┐ ┌───────────────────────────────────┐ ┌────────────────┐ > │ │ │ │ │ │ > │ │ │ PCI Endpoint │ │ PCI Host │ > │ │ │ │ │ │ > │ │◄──┤ 1.platform_msi_domain_alloc_irqs()│ │ │ > │ │ │ │ │ │ > │ MSI ├──►│ 2.write_msi_msg() ├──►├─BAR<n> │ > │ Controller │ │ update doorbell register address│ │ │ > │ │ │ for BAR │ │ │ > │ │ │ │ │ 3. Write BAR<n>│ > │ │◄──┼───────────────────────────────────┼───┤ │ > │ │ │ │ │ │ > │ ├──►│ 4.Irq Handle │ │ │ > │ │ │ │ │ │ > │ │ │ │ │ │ > └────────────┘ └───────────────────────────────────┘ └────────────────┘ > > This patches based on old https://lore.kernel.org/imx/20221124055036.1630573-1-Frank.Li@nxp.com/ > > Original patch only target to vntb driver. But actually it is common > method. > > This patches add new API to pci-epf-core, so any EP driver can use it. > > Previous v2 discussion here. > https://lore.kernel.org/imx/20230911220920.1817033-1-Frank.Li@nxp.com/ > > Changes in v4: > - Remove patch genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs() > - Use new method to avoid compatible problem. > Add new command DOORBELL_ENABLE and DOORBELL_DISABLE. > pcitest -B send DOORBELL_ENABLE first, EP test function driver try to > remap one of BAR_N (except test register bar) to ITS MSI MMIO space. Old > driver don't support new command, so failure return, not side effect. > After test, DOORBELL_DISABLE command send out to recover original map, so > pcitest bar test can pass as normal. > - Other detail change see each patches's change log > - Link to v3: https://lore.kernel.org/r/20241015-ep-msi-v3-0-cedc89a16c1a@nxp.com > > Change from v2 to v3 > - Fixed manivannan's comments > - Move common part to pci-ep-msi.c and pci-ep-msi.h > - rebase to 6.12-rc1 > - use RevID to distingiush old version > > mkdir /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 > echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/msi_interrupts > echo 0x080c > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/deviceid > echo 0x1957 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/vendorid > echo 1 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/revid > ^^^^^^ to enable platform msi support. > ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 /sys/kernel/config/pci_ep/controllers/4c380000.pcie-ep Perhaps drop these steps, to not confuse the reader. They are no longer relevant with the latest revision anyway. > > - use new device ID, which identify support doorbell to avoid broken > compatility. > > Enable doorbell support only for PCI_DEVICE_ID_IMX8_DB, while other devices > keep the same behavior as before. > > EP side RC with old driver RC with new driver > PCI_DEVICE_ID_IMX8_DB no probe doorbell enabled > Other device ID doorbell disabled* doorbell disabled* > > * Behavior remains unchanged. > > Change from v1 to v2 > - Add missed patch for endpont/pci-epf-test.c > - Move alloc and free to epc driver from epf. > - Provide general help function for EPC driver to alloc platform msi irq. > - Fixed manivannan's comments. I wanted to try this series on rk3588, which has a MSI controller as part of the GIC using Interrupt Translation Services (ITS), for the Root Complex DT node: https://github.com/torvalds/linux/blob/v6.12-rc6/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi#L164 https://github.com/torvalds/linux/blob/v6.12-rc6/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi#L1981-L1999 There isn't any reference to a MSI controller in the Endpoint DT node: https://github.com/torvalds/linux/blob/v6.12-rc6/arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi#L189 When testing your series, I get an error in platform_device_msi_init_and_alloc_irqs(), because no domain is found. I assume that is it "msi-parent" that we should add here. However, looking at: $ git grep -p msi-parent arch/arm64/boot/dts/freescale/ I do not see any freescale/iMX platform specifying a msi-parent for the EP node. Is there any additional DTS changes needed for iMX that is not part of this series, or what am I missing? When adding "msi-parent = <&its1 0x0000>; to the EP node (this is just a guess since RC node has: msi-map = <0x0000 &its1 0x0000 0x1000>;) I do get a domain, but I do not get any IRQ on the EP side when the RC side is writing the doorbell (as part of pcitest -B), [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 [ 21.114613] pci_epf_enable_doorbell db_bar: 1 [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 [ 21.115994] pci_epf_enable_doorbell: success # cat /proc/interrupts | grep epc 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 Even if I try to write the doorbell manually from EP side using devmem: # devmem 0xfe670040 32 1 it still doesn't trigger a IRQ on the EP side: # cat /proc/interrupts | grep epc 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 Any ideas? Kind regards, Niklas
On Wed, Nov 06, 2024 at 10:15:27AM +0100, Niklas Cassel wrote: > > I do get a domain, but I do not get any IRQ on the EP side when the RC side is > writing the doorbell (as part of pcitest -B), > > [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 > [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 > [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 > [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 > [ 21.114613] pci_epf_enable_doorbell db_bar: 1 > [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 > [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 > [ 21.115994] pci_epf_enable_doorbell: success > > # cat /proc/interrupts | grep epc > 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 > > Even if I try to write the doorbell manually from EP side using devmem: > > # devmem 0xfe670040 32 1 Sorry, this should of course have been: # devmem 0xfe650040 32 1 But the result is the name, no IRQ triggered on the EP side. (My command above was from testing "msi-parent = <&its0 0x0000>", rather than &its1, but that also didn't work when writing the corresponding "doorbell_addr" using e.g. devmem.) Considering that the RC node is using &its1, that is probably also what should be used in the EP node when running the controller in EP mode instead of RC mode. Kind regards, Niklas
On Wed, Nov 06, 2024 at 10:36:42AM +0100, Niklas Cassel wrote: > On Wed, Nov 06, 2024 at 10:15:27AM +0100, Niklas Cassel wrote: > > > > I do get a domain, but I do not get any IRQ on the EP side when the RC side is > > writing the doorbell (as part of pcitest -B), > > > > [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 > > [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 > > [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 > > [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 > > [ 21.114613] pci_epf_enable_doorbell db_bar: 1 > > [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 > > [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 > > [ 21.115994] pci_epf_enable_doorbell: success > > > > # cat /proc/interrupts | grep epc > > 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 > > > > Even if I try to write the doorbell manually from EP side using devmem: > > > > # devmem 0xfe670040 32 1 > > Sorry, this should of course have been: > # devmem 0xfe650040 32 1 Thank you test it. You can't write it at EP side. ITS identify the bus master. master ID (streamid) of CPU is the diffference with PCI master's ID (streamid). You set msi-parent = <&its0 0x0000>, not sure if 0x0000 is validate stream. You have to run at RC side, "devmem (Bar1+0x40) 32 0". So PCIe EP controller can use EP streamid. some system need special register to config stream id, you can refer host side's settings. Frank > > But the result is the name, no IRQ triggered on the EP side. > > (My command above was from testing "msi-parent = <&its0 0x0000>", > rather than &its1, but that also didn't work when writing the > corresponding "doorbell_addr" using e.g. devmem.) <&its0 0x0000>, second argument is your PCIe controller's stream ID. You can ref RC side. > > Considering that the RC node is using &its1, that is probably > also what should be used in the EP node when running the controller > in EP mode instead of RC mode. Generally, RC node should use smmu-map, instead &its1. Or your PCI controller direct use 16bit RID as streamid. > > > Kind regards, > Niklas
On Wed, Nov 06, 2024 at 12:13:09PM -0500, Frank Li wrote: > On Wed, Nov 06, 2024 at 10:36:42AM +0100, Niklas Cassel wrote: > > On Wed, Nov 06, 2024 at 10:15:27AM +0100, Niklas Cassel wrote: > > > > > > I do get a domain, but I do not get any IRQ on the EP side when the RC side is > > > writing the doorbell (as part of pcitest -B), > > > > > > [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 > > > [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 > > > [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 > > > [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 > > > [ 21.114613] pci_epf_enable_doorbell db_bar: 1 > > > [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 > > > [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 > > > [ 21.115994] pci_epf_enable_doorbell: success > > > > > > # cat /proc/interrupts | grep epc > > > 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 > > > > > > Even if I try to write the doorbell manually from EP side using devmem: > > > > > > # devmem 0xfe670040 32 1 > > > > Sorry, this should of course have been: > > # devmem 0xfe650040 32 1 > > Thank you test it. You can't write it at EP side. ITS identify the bus > master. master ID (streamid) of CPU is the diffference with PCI master's > ID (streamid). You set msi-parent = <&its0 0x0000>, not sure if 0x0000 is > validate stream. I see, this makes sense since the ITS converts BDF to an MSI specifier. > > You have to run at RC side, "devmem (Bar1+0x40) 32 0". So PCIe EP > controller can use EP streamid. > > some system need special register to config stream id, you can refer host > side's settings. > <&its0 0x0000>, second argument is your PCIe controller's stream ID. You > can ref RC side. The RC node looks like this: msi-map = <0x0000 &its1 0x0000 0x1000>; So it does indeed use 0x0 as the MSI specifier. > > > > > Considering that the RC node is using &its1, that is probably > > also what should be used in the EP node when running the controller > > in EP mode instead of RC mode. > > Generally, RC node should use smmu-map, instead &its1. Or your PCI > controller direct use 16bit RID as streamid. smmu-map? Do you mean iommu-map? I don't see why we would need to have the SMMU enabled to use ITS. The iommu is currently disabled on my platform. I did enable the iommu, and all BAR tests, read tests, write tests, and copy tests pass. However I get an iommu error when the RC is writing the doorbell. Perhaps you need to do dma_map_single() on the address that you are setting the inbound address translation to? Without the IOMMU, if I modify pci_endpoint_test.c to not send the DISABLE_DOORBELL command on error (so that EP side still has DB enabled), I can read all BARs except BAR1 (which was used for the doorbell): [ 21.077417] pci 0000:01:00.0: BAR 0 [mem 0xf0300000-0xf03fffff]: assigned [ 21.078029] pci 0000:01:00.0: BAR 1 [mem 0xf0400000-0xf04fffff]: assigned [ 21.078640] pci 0000:01:00.0: BAR 2 [mem 0xf0500000-0xf05fffff]: assigned [ 21.079250] pci 0000:01:00.0: BAR 3 [mem 0xf0600000-0xf06fffff]: assigned [ 21.079860] pci 0000:01:00.0: BAR 5 [mem 0xf0700000-0xf07fffff]: assigned # pcitest -B [ 25.156623] COMMAND_ENABLE_DOORBELL complete - status: 0x440 [ 25.157131] db_bar: 1 addr: 0x40 data: 0x0 [ 25.157501] setting irq_type to: 1 [ 25.157802] writing: 0x0 to offset: 0x40 in BAR: 1 [ 35.300676] we wrote to the BAR, status is now: 0x0 status is not updated after writing to DB, and /proc/interrupts on EP side is not incrementing. # devmem 0xf0300000 0x00000000 # devmem 0xf0400040 0xFFFFFFFF # devmem 0xf0500000 0x00000000 # devmem 0xf0600000 0x00000000 # devmem 0xf0700000 0x00000000 So there does seem to be something wrong with the inbound translation, at least when testing on rk3588 which only uses 1MB fixed size BARs: https://github.com/torvalds/linux/blob/v6.12-rc6/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L276-L281 You also didn't answer which imx platform that you are using to test this, I don't see a single imx platform that specifies "msi-parent". Kind regards, Niklas
On Thu, Nov 07, 2024 at 12:57:16AM +0100, Niklas Cassel wrote: > On Wed, Nov 06, 2024 at 12:13:09PM -0500, Frank Li wrote: > > On Wed, Nov 06, 2024 at 10:36:42AM +0100, Niklas Cassel wrote: > > > On Wed, Nov 06, 2024 at 10:15:27AM +0100, Niklas Cassel wrote: > > > > > > > > I do get a domain, but I do not get any IRQ on the EP side when the RC side is > > > > writing the doorbell (as part of pcitest -B), > > > > > > > > [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 > > > > [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 > > > > [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 > > > > [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 > > > > [ 21.114613] pci_epf_enable_doorbell db_bar: 1 > > > > [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 > > > > [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 > > > > [ 21.115994] pci_epf_enable_doorbell: success > > > > > > > > # cat /proc/interrupts | grep epc > > > > 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 > > > > > > > > Even if I try to write the doorbell manually from EP side using devmem: > > > > > > > > # devmem 0xfe670040 32 1 > > > > > > Sorry, this should of course have been: > > > # devmem 0xfe650040 32 1 > > > > Thank you test it. You can't write it at EP side. ITS identify the bus > > master. master ID (streamid) of CPU is the diffference with PCI master's > > ID (streamid). You set msi-parent = <&its0 0x0000>, not sure if 0x0000 is > > validate stream. > > I see, this makes sense since the ITS converts BDF to an MSI specifier. > > > > > > You have to run at RC side, "devmem (Bar1+0x40) 32 0". So PCIe EP > > controller can use EP streamid. > > > > some system need special register to config stream id, you can refer host > > side's settings. > > > <&its0 0x0000>, second argument is your PCIe controller's stream ID. You > > can ref RC side. > > The RC node looks like this: > msi-map = <0x0000 &its1 0x0000 0x1000>; > So it does indeed use 0x0 as the MSI specifier. > > > > > > > > > > Considering that the RC node is using &its1, that is probably > > > also what should be used in the EP node when running the controller > > > in EP mode instead of RC mode. > > > > Generally, RC node should use smmu-map, instead &its1. Or your PCI > > controller direct use 16bit RID as streamid. > > smmu-map? Do you mean iommu-map? Sorry, typo, my means msi-map. > > I don't see why we would need to have the SMMU enabled to use ITS. > The iommu is currently disabled on my platform. > > I did enable the iommu, and all BAR tests, read tests, write tests, > and copy tests pass. However I get an iommu error when the RC is > writing the doorbell. Perhaps you need to do dma_map_single() on > the address that you are setting the inbound address translation to? > > > > Without the IOMMU, if I modify pci_endpoint_test.c to not send the > DISABLE_DOORBELL command on error (so that EP side still has DB enabled), > I can read all BARs except BAR1 (which was used for the doorbell): > [ 21.077417] pci 0000:01:00.0: BAR 0 [mem 0xf0300000-0xf03fffff]: assigned > [ 21.078029] pci 0000:01:00.0: BAR 1 [mem 0xf0400000-0xf04fffff]: assigned > [ 21.078640] pci 0000:01:00.0: BAR 2 [mem 0xf0500000-0xf05fffff]: assigned > [ 21.079250] pci 0000:01:00.0: BAR 3 [mem 0xf0600000-0xf06fffff]: assigned > [ 21.079860] pci 0000:01:00.0: BAR 5 [mem 0xf0700000-0xf07fffff]: assigned > # pcitest -B > [ 25.156623] COMMAND_ENABLE_DOORBELL complete - status: 0x440 > [ 25.157131] db_bar: 1 addr: 0x40 data: 0x0 > [ 25.157501] setting irq_type to: 1 > [ 25.157802] writing: 0x0 to offset: 0x40 in BAR: 1 > [ 35.300676] we wrote to the BAR, status is now: 0x0 > > status is not updated after writing to DB, > and /proc/interrupts on EP side is not incrementing. > > # devmem 0xf0300000 > 0x00000000 > # devmem 0xf0400040 > 0xFFFFFFFF > # devmem 0xf0500000 > 0x00000000 > # devmem 0xf0600000 > 0x00000000 > # devmem 0xf0700000 > 0x00000000 > > So there does seem to be something wrong with the inbound translation, > at least when testing on rk3588 which only uses 1MB fixed size BARs: > https://github.com/torvalds/linux/blob/v6.12-rc6/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L276-L281 > It should be fine. Some hardware many append some stream id bits before send to ITS. > > > You also didn't answer which imx platform that you are using to test this, > I don't see a single imx platform that specifies "msi-parent". Only imx95 support its now. LUT part code is still under review. EP support part should after lut merged. Preivous use layerscape, which uboot set it. Frank > > > Kind regards, > Niklas
On Thu, Nov 07, 2024 at 12:57:16AM +0100, Niklas Cassel wrote: > On Wed, Nov 06, 2024 at 12:13:09PM -0500, Frank Li wrote: > > On Wed, Nov 06, 2024 at 10:36:42AM +0100, Niklas Cassel wrote: > > > On Wed, Nov 06, 2024 at 10:15:27AM +0100, Niklas Cassel wrote: > > > > > > > > I do get a domain, but I do not get any IRQ on the EP side when the RC side is > > > > writing the doorbell (as part of pcitest -B), > > > > > > > > [ 7.978984] pci_epc_alloc_doorbell: num_db: 1 > > > > [ 7.979545] pci_epf_test_bind: doorbell_addr: 0x40 > > > > [ 7.979978] pci_epf_test_bind: doorbell_data: 0x0 > > > > [ 7.980397] pci_epf_test_bind: doorbell_bar: 0x1 > > > > [ 21.114613] pci_epf_enable_doorbell db_bar: 1 > > > > [ 21.115001] pci_epf_enable_doorbell: doorbell_addr: 0xfe650040 > > > > [ 21.115512] pci_epf_enable_doorbell: phys_addr: 0xfe650000 > > > > [ 21.115994] pci_epf_enable_doorbell: success > > > > > > > > # cat /proc/interrupts | grep epc > > > > 117: 0 0 0 0 0 0 0 0 ITS-pMSI-a40000000.pcie-ep 0 Edge pci-epc-doorbell0 > > > > > > > > Even if I try to write the doorbell manually from EP side using devmem: > > > > > > > > # devmem 0xfe670040 32 1 > > > > > > Sorry, this should of course have been: > > > # devmem 0xfe650040 32 1 > > > > Thank you test it. You can't write it at EP side. ITS identify the bus > > master. master ID (streamid) of CPU is the diffference with PCI master's > > ID (streamid). You set msi-parent = <&its0 0x0000>, not sure if 0x0000 is > > validate stream. > > I see, this makes sense since the ITS converts BDF to an MSI specifier. > > > > > > You have to run at RC side, "devmem (Bar1+0x40) 32 0". So PCIe EP > > controller can use EP streamid. > > > > some system need special register to config stream id, you can refer host > > side's settings. > > > <&its0 0x0000>, second argument is your PCIe controller's stream ID. You > > can ref RC side. > > The RC node looks like this: > msi-map = <0x0000 &its1 0x0000 0x1000>; > So it does indeed use 0x0 as the MSI specifier. > > > > > > > > > > Considering that the RC node is using &its1, that is probably > > > also what should be used in the EP node when running the controller > > > in EP mode instead of RC mode. > > > > Generally, RC node should use smmu-map, instead &its1. Or your PCI > > controller direct use 16bit RID as streamid. > > smmu-map? Do you mean iommu-map? > > I don't see why we would need to have the SMMU enabled to use ITS. > The iommu is currently disabled on my platform. > > I did enable the iommu, and all BAR tests, read tests, write tests, > and copy tests pass. However I get an iommu error when the RC is > writing the doorbell. Perhaps you need to do dma_map_single() on > the address that you are setting the inbound address translation to? > > > > Without the IOMMU, if I modify pci_endpoint_test.c to not send the > DISABLE_DOORBELL command on error (so that EP side still has DB enabled), > I can read all BARs except BAR1 (which was used for the doorbell): If you enable IOMMU, please double check pci_epc_write_msi_msg() pass down iommu mapped address. I have not test iommu enabled's case yet. Frank > [ 21.077417] pci 0000:01:00.0: BAR 0 [mem 0xf0300000-0xf03fffff]: assigned > [ 21.078029] pci 0000:01:00.0: BAR 1 [mem 0xf0400000-0xf04fffff]: assigned > [ 21.078640] pci 0000:01:00.0: BAR 2 [mem 0xf0500000-0xf05fffff]: assigned > [ 21.079250] pci 0000:01:00.0: BAR 3 [mem 0xf0600000-0xf06fffff]: assigned > [ 21.079860] pci 0000:01:00.0: BAR 5 [mem 0xf0700000-0xf07fffff]: assigned > # pcitest -B > [ 25.156623] COMMAND_ENABLE_DOORBELL complete - status: 0x440 > [ 25.157131] db_bar: 1 addr: 0x40 data: 0x0 > [ 25.157501] setting irq_type to: 1 > [ 25.157802] writing: 0x0 to offset: 0x40 in BAR: 1 > [ 35.300676] we wrote to the BAR, status is now: 0x0 > > status is not updated after writing to DB, > and /proc/interrupts on EP side is not incrementing. > > # devmem 0xf0300000 > 0x00000000 > # devmem 0xf0400040 > 0xFFFFFFFF > # devmem 0xf0500000 > 0x00000000 > # devmem 0xf0600000 > 0x00000000 > # devmem 0xf0700000 > 0x00000000 > > So there does seem to be something wrong with the inbound translation, > at least when testing on rk3588 which only uses 1MB fixed size BARs: > https://github.com/torvalds/linux/blob/v6.12-rc6/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L276-L281 > > > > You also didn't answer which imx platform that you are using to test this, > I don't see a single imx platform that specifies "msi-parent". > > > Kind regards, > Niklas
On Wed, Nov 06, 2024 at 07:41:42PM -0500, Frank Li wrote: > > > > So there does seem to be something wrong with the inbound translation, > > at least when testing on rk3588 which only uses 1MB fixed size BARs: > > https://github.com/torvalds/linux/blob/v6.12-rc6/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L276-L281 > > > > It should be fine. Some hardware many append some stream id bits before > send to ITS. Some more debugging with the IOMMU on. EP side start: [ 14.601081] pci_epc_alloc_doorbell: num_db: 1 [ 14.601588] pci_epf_test_bind: doorbell_addr: 0xf040 (align: 0x10000) [ 14.602162] pci_epf_test_bind: doorbell_data: 0x0 [ 14.602573] pci_epf_test_bind: doorbell_bar: 0x1 When RC side does: pcitest -B [ 109.252900] COMMAND_ENABLE_DOORBELL complete - status: 0x440 [ 109.253406] db_bar: 1 addr: 0xf040 data: 0x0 [ 109.254094] writing: 0x0 to offset: 0xf040 in BAR: 1 [ 119.268887] we wrote to the BAR, status is now: 0x0 EP side results in: [ 117.894997] pci_epf_enable_doorbell db_bar: 1 [ 117.895399] pci_epf_enable_doorbell: using doorbell_addr: 0xffffff9ff040 [ 117.896517] pci_epf_enable_doorbell: phys_addr: 0xffffff9f0000 [ 117.897037] dw_pcie_ep_set_bar: set_bar: bar: 1 phys_addr: ffffff9f0000 flags: 0x0 size: 0x100000 [ 117.898504] pci_epf_enable_doorbell: success [ 118.912433] arm-smmu-v3 fc900000.iommu: event 0x10 received: [ 118.912965] arm-smmu-v3 fc900000.iommu: 0x0000000000000010 [ 118.913508] arm-smmu-v3 fc900000.iommu: 0x0000020000000000 [ 118.914018] arm-smmu-v3 fc900000.iommu: 0x0000ffffff90f040 [ 118.914534] arm-smmu-v3 fc900000.iommu: 0x0000000000000000 Looking at the doorbell_addr, it seems to be a IOMMU address already. If we look at the ARM SMMU-v3 specification, event 0x10 is: Translation fault: The address provided to a stage of translation failed the range check defined by TxSZ/SLx, the address was within a disabled TTBx, or a valid translation table descriptor was not found for the address. for event F_TRANSLATION: 0x0000ffffff90f040 is input address. StreamID is: 0x0, so that looks as expected for rk3588. (And if the SteamID was incorrect, I would have expected a C_BAD_STREAMID event instead.) Comparing the address of the IOMMU error: 0xffffff90f040 with the doorbell addr: 0xffffff9ff040 XOR value: 0x0000000f0000 We can see that they are not identical. When RC side does: devmem $((0xf0400000+0xf040)) 32 0 it results in the exact same IOMMU error on the EP side as the one above. However, if I manually append the XOR value: devmem $((0xf0400000+0xf040+0xf0000)) 32 0 I can see on the EP side: [ 631.399530] pci_epf_doorbell_handler [ 631.399850] pci_epf_test_doorbell So why is the inbound translation incorrect? Like I told you earlier, rk3588 has fixed size 1MB BARs, so the BAR_MASK will be: ~(SZ_1M-1) 0xfff00000 So the physical address that you write in the iATU: 0xffffff9f0000 will actually be: 0xfffffff00000 after reading back the same register from the iATU, since the lower bits will be masked away. I'm guessing that you would need to do something like: diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index e5b6a65e3e16f..0ab5d61bf0493 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -675,6 +675,9 @@ static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_ep return; } + if (epf_test->epc_features->bar[bar].type == BAR_FIXED) + align = max(epf_test->epc_features->bar[bar].fixed_size, align); + msg = &epf->db_msg[0].msg; doorbell_addr = msg->address_hi; doorbell_addr <<= 32; @@ -1016,15 +1021,22 @@ static int pci_epf_test_bind(struct pci_epf *epf) struct msi_msg *msg = &epf->db_msg[0].msg; u32 align = epc_features->align; u64 doorbell_addr; + enum pci_barno bar; + + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); align = align ? align : 128; + if (epf_test->epc_features->bar[bar].type == BAR_FIXED) + align = max(epf_test->epc_features->bar[bar].fixed_size, + align); + doorbell_addr = msg->address_hi; doorbell_addr <<= 32; doorbell_addr |= msg->address_lo; reg->doorbell_addr = doorbell_addr & (align - 1); reg->doorbell_data = msg->data; - reg->doorbell_bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); + reg->doorbell_bar = bar; } return 0; I tested the above on top of your series, and now pcitest -B works as expected :) yay! Kind regards, Niklas
┌────────────┐ ┌───────────────────────────────────┐ ┌────────────────┐ │ │ │ │ │ │ │ │ │ PCI Endpoint │ │ PCI Host │ │ │ │ │ │ │ │ │◄──┤ 1.platform_msi_domain_alloc_irqs()│ │ │ │ │ │ │ │ │ │ MSI ├──►│ 2.write_msi_msg() ├──►├─BAR<n> │ │ Controller │ │ update doorbell register address│ │ │ │ │ │ for BAR │ │ │ │ │ │ │ │ 3. Write BAR<n>│ │ │◄──┼───────────────────────────────────┼───┤ │ │ │ │ │ │ │ │ ├──►│ 4.Irq Handle │ │ │ │ │ │ │ │ │ │ │ │ │ │ │ └────────────┘ └───────────────────────────────────┘ └────────────────┘ This patches based on old https://lore.kernel.org/imx/20221124055036.1630573-1-Frank.Li@nxp.com/ Original patch only target to vntb driver. But actually it is common method. This patches add new API to pci-epf-core, so any EP driver can use it. Previous v2 discussion here. https://lore.kernel.org/imx/20230911220920.1817033-1-Frank.Li@nxp.com/ Changes in v4: - Remove patch genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs() - Use new method to avoid compatible problem. Add new command DOORBELL_ENABLE and DOORBELL_DISABLE. pcitest -B send DOORBELL_ENABLE first, EP test function driver try to remap one of BAR_N (except test register bar) to ITS MSI MMIO space. Old driver don't support new command, so failure return, not side effect. After test, DOORBELL_DISABLE command send out to recover original map, so pcitest bar test can pass as normal. - Other detail change see each patches's change log - Link to v3: https://lore.kernel.org/r/20241015-ep-msi-v3-0-cedc89a16c1a@nxp.com Change from v2 to v3 - Fixed manivannan's comments - Move common part to pci-ep-msi.c and pci-ep-msi.h - rebase to 6.12-rc1 - use RevID to distingiush old version mkdir /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/msi_interrupts echo 0x080c > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/deviceid echo 0x1957 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/vendorid echo 1 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/revid ^^^^^^ to enable platform msi support. ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 /sys/kernel/config/pci_ep/controllers/4c380000.pcie-ep - use new device ID, which identify support doorbell to avoid broken compatility. Enable doorbell support only for PCI_DEVICE_ID_IMX8_DB, while other devices keep the same behavior as before. EP side RC with old driver RC with new driver PCI_DEVICE_ID_IMX8_DB no probe doorbell enabled Other device ID doorbell disabled* doorbell disabled* * Behavior remains unchanged. Change from v1 to v2 - Add missed patch for endpont/pci-epf-test.c - Move alloc and free to epc driver from epf. - Provide general help function for EPC driver to alloc platform msi irq. - Fixed manivannan's comments. To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> To: Krzysztof Wilczyński <kw@linux.com> To: Kishon Vijay Abraham I <kishon@kernel.org> To: Bjorn Helgaas <bhelgaas@google.com> To: Arnd Bergmann <arnd@arndb.de> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-kernel@vger.kernel.org Cc: linux-pci@vger.kernel.org Cc: imx@lists.linux.dev Cc: Niklas Cassel <cassel@kernel.org> Cc: cassel@kernel.org Cc: dlemoal@kernel.org Cc: maz@kernel.org Cc: tglx@linutronix.de Cc: jdmason@kudzu.us Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Frank Li (5): PCI: endpoint: Add pci_epc_get_fn() API for customizable filtering PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller PCI: endpoint: pci-epf-test: Add doorbell test support misc: pci_endpoint_test: Add doorbell test case tools: PCI: Add 'B' option for test doorbell drivers/misc/pci_endpoint_test.c | 63 +++++++++++++ drivers/pci/endpoint/Makefile | 2 +- drivers/pci/endpoint/functions/pci-epf-test.c | 104 +++++++++++++++++++++ drivers/pci/endpoint/pci-ep-msi.c | 128 ++++++++++++++++++++++++++ drivers/pci/endpoint/pci-epc-core.c | 23 ++++- include/linux/pci-ep-msi.h | 15 +++ include/linux/pci-epc.h | 2 + include/linux/pci-epf.h | 19 ++++ include/uapi/linux/pcitest.h | 1 + tools/pci/pcitest.c | 16 +++- 10 files changed, 369 insertions(+), 4 deletions(-) --- base-commit: f231847d7f5a171be4566099f654521606b3ec37 change-id: 20241010-ep-msi-8b4cab33b1be Best regards, --- Frank Li <Frank.Li@nxp.com>