PCI: designware-ep: Fix ->get_msi() to check MSI_EN bit
PATCH 1/1 PCI: designware-ep: Fix ->get_msi() to check MSI_EN bit
->get_msi() now checks MSI_EN bit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.
This fixes the issue with the following sequence
'modprobe pci_endpoint_test' enables MSI
'rmmod pci_endpoint_test' disables MSI but MSI address has a valid value 'modprobe pci_endpoint_test no_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EP thinks host supports MSI.
--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +3 -9
@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val; -u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep); - val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 ); - -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL ); +if (!(val &MSI_CAP_MSI_EN_MASK ))return -EINVAL ; + val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ;return val; }
drivers/pci/dwc/pcie-designware.h +1 -0
@@ -101,6 +101,7 @@#define MSI_MESSAGE_CONTROL 0x52#define MSI_CAP_MMC_SHIFT 1#define MSI_CAP_MME_SHIFT 4 +#define MSI_CAP_MSI_EN_MASK 0x1#define MSI_CAP_MME_MASK (7 << MSI_CAP_MME_SHIFT)#define MSI_MESSAGE_ADDR_L32 0x54#define MSI_MESSAGE_ADDR_U32 0x58
Signature · 1 line
2.11.0
Quoted 3 lines · Kishon Vijay Abraham I
->get_msi() now checks
MSI_ENbit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.
I think that's a sensible thing to do regardless, given that I do not understand why 0x0 can't be a valid MSI address in the first place.
Quoted 5 lines
This fixes the issue with the following sequence
'modprobepci_endpoint_test' enables MSI
'rmmodpci_endpoint_test' disables MSI but MSI address has a valid value 'modprobepci_endpoint_testno_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EP
Where is the address set ?
Quoted 1 line
thinks host supports MSI.
Add a Fixes: tag please.
Quoted 14 lines
Signed-off-by Kishon Vijay Abraham I <[email protected]>--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)drivers/pci/dwc/pcie-designware-ep.c +0
-0@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val;
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 209
Quoted 10 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-7-u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep); - val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 );
I can't find code that writes these registers, see above for my question.
On top of that I think that what's done the EPF test function driver in struct pci_epf.bind() should be undone in struct pci_epf.unbind() and I do not think that's happening for MSIs.
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 203
Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +1
-2- -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL );
MSI_MESSAGE_CONTROL is written in dw_pcie_ep_set_msi(), I assume a write to bit[0] (ie MSI_CAP_MSI_EN_MASK) is ignored - it would be good if you can clarify that as well please.
Thanks,
Lorenzo
Quoted 22 lines
drivers/pci/dwc/pcie-designware-ep.c +2
-0+if (!(val &MSI_CAP_MSI_EN_MASK ))return -EINVAL ; + val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ;return val; }drivers/pci/dwc/pcie-designware.h +1
-0@@ -101,6 +101,7 @@#define MSI_MESSAGE_CONTROL 0x52#define MSI_CAP_MMC_SHIFT 1#define MSI_CAP_MME_SHIFT 4 +#define MSI_CAP_MSI_EN_MASK 0x1#define MSI_CAP_MME_MASK (7 << MSI_CAP_MME_SHIFT)#define MSI_MESSAGE_ADDR_L32 0x54#define MSI_MESSAGE_ADDR_U32 0x58Signature · 1 line
2.11.0
Hi Lorenzo,
Quoted 13 lines · Lorenzo Pieralisi
Quoted 3 lines · Kishon Vijay Abraham I
->get_msi() now checks
MSI_ENbit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.I think that's a sensible thing to do regardless, given that I do not understand why
0x0can't be a valid MSI address in the first place.Quoted 5 lines
This fixes the issue with the following sequence
'modprobepci_endpoint_test' enables MSI
'rmmodpci_endpoint_test' disables MSI but MSI address has a valid value 'modprobepci_endpoint_testno_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EP
MSI address is written in the MSI capabiltiy register by the host.
Quoted 6 lines
Where is the address set ?
Quoted 1 line
thinks host supports MSI.
Add a Fixes: tag please.
sure.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Kishon Vijay Abraham I <[email protected]>--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)drivers/pci/dwc/pcie-designware-ep.c +0
-2@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val; -u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep);
Quoted 8 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 209
Quoted 5 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-5- val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 );I can't find code that writes these registers, see above for my question.
IIUC the host will write to these registers in __pci_write_msi_msg using the configuration writes.
Quoted 5 lines
On top of that I think that what's done the EPF test function driver in struct
pci_epf.bind()should be undone in structpci_epf.unbind()and I do not think that's happening for MSIs.
Quoted 7 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 203
Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +1
-2- -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL );
MSI_MESSAGE_CONTROLis written indw_pcie_ep_set_msi(), I assume a write to bit[0] (ieMSI_CAP_MSI_EN_MASK) is ignored - it would be good if you can clarify that as well please.
Do you mean dw_pcie_ep_set_msi can overwrite MSI_CAP_MSI_EN and we should avoid that?
Thanks
Kishon
Quoted 70 lines · Kishon Vijay Abraham I
Hi Lorenzo,
Quoted 13 lines · Lorenzo Pieralisi
Quoted 3 lines · Kishon Vijay Abraham I
->get_msi() now checks
MSI_ENbit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.I think that's a sensible thing to do regardless, given that I do not understand why
0x0can't be a valid MSI address in the first place.Quoted 5 lines
This fixes the issue with the following sequence
'modprobepci_endpoint_test' enables MSI
'rmmodpci_endpoint_test' disables MSI but MSI address has a valid value 'modprobepci_endpoint_testno_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EPMSI address is written in the MSI capabiltiy register by the host.
Quoted 6 lines
Where is the address set ?
Quoted 1 line
thinks host supports MSI.
Add a Fixes: tag please.
sure.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Kishon Vijay Abraham I <[email protected]>--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)drivers/pci/dwc/pcie-designware-ep.c +0
-2@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val; -u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep);Quoted 8 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 209
Quoted 5 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-5- val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 );I can't find code that writes these registers, see above for my question.
IIUC the host will write to these registers in
__pci_write_msi_msgusing the configuration writes.Quoted 5 lines
On top of that I think that what's done the EPF test function driver in struct
pci_epf.bind()should be undone in structpci_epf.unbind()and I do not think that's happening for MSIs.Quoted 7 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 203
Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +1
-2- -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL );
MSI_MESSAGE_CONTROLis written indw_pcie_ep_set_msi(), I assume a write to bit[0] (ieMSI_CAP_MSI_EN_MASK) is ignored - it would be good if you can clarify that as well please.
Quoted 2 lines
Do you mean
dw_pcie_ep_set_msican overwriteMSI_CAP_MSI_ENand we should avoid that?
Yes, I think there are a lot of assumptions here. IIUC the sequence you expect this to work is:
1) EP system (ie system where the EP is located) is brought up and EP is configured (inclusive of MSI)
2) host boots and enumerates PCI devices (which includes the EP)
3) host EP PCI driver probes and configure MSI, etc
4) EP system runs EP function test and to understand what the host system enabled on the endpoint (eg PCI EP test function driver using MSIs) you rely on capabilities registers host system settings that are reflected in the EP registers
Is my understanding correct ?
So basically the PCI EP test function works as software IP whose behaviour is controlled by the EP system kernel/userspace.
There are obvious syncronization assumptions here - the whole thing is racy by design (inclusive of the stepwise boot sequence above) but if my understanding above is correct I think this patch is the right way of handling it.
I wonder how this is going to work if the EP function BARs map a real device (eg USB), please let me know your thoughts on that.
Thanks,
Lorenzo
Hi Lorenzo,
Quoted 71 lines · Lorenzo Pieralisi
Quoted 70 lines · Kishon Vijay Abraham I
Hi Lorenzo,
Quoted 13 lines · Lorenzo Pieralisi
Quoted 3 lines · Kishon Vijay Abraham I
->get_msi() now checks
MSI_ENbit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.I think that's a sensible thing to do regardless, given that I do not understand why
0x0can't be a valid MSI address in the first place.Quoted 5 lines
This fixes the issue with the following sequence
'modprobepci_endpoint_test' enables MSI
'rmmodpci_endpoint_test' disables MSI but MSI address has a valid value 'modprobepci_endpoint_testno_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EPMSI address is written in the MSI capabiltiy register by the host.
Quoted 6 lines
Where is the address set ?
Quoted 1 line
thinks host supports MSI.
Add a Fixes: tag please.
sure.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Kishon Vijay Abraham I <[email protected]>--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)drivers/pci/dwc/pcie-designware-ep.c +0
-2@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val; -u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep);Quoted 8 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 209
Quoted 5 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-5- val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 );I can't find code that writes these registers, see above for my question.
IIUC the host will write to these registers in
__pci_write_msi_msgusing the configuration writes.Quoted 12 lines
On top of that I think that what's done the EPF test function driver in struct
pci_epf.bind()should be undone in structpci_epf.unbind()and I do not think that's happening for MSIs.drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 203
Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +1
-2- -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL );
MSI_MESSAGE_CONTROLis written indw_pcie_ep_set_msi(), I assume a write to bit[0] (ieMSI_CAP_MSI_EN_MASK) is ignored - it would be good if you can clarify that as well please.
Quoted 8 lines
Quoted 2 lines
Do you mean
dw_pcie_ep_set_msican overwriteMSI_CAP_MSI_ENand we should avoid that?Yes, I think there are a lot of assumptions here. IIUC the sequence you expect this to work is:
1) EP system (ie system where the EP is located) is brought up and EP is configured (inclusive of MSI)
right. Here only the number of MSI's required by the EP is configured. The host is responsible for enabling MSI after programming the MSI address and data.
The EP function driver should also come up here. The BAR registers and the number of MSIs required by the EP function will be configured by the function driver.
Once the EP function driver has been fully configured, pci_epc_start() should be invoked which starts the link. Without this the host will not see the EP device.
Quoted 2 lines
2) host boots and enumerates PCI devices (which includes the EP)
3) host EP PCI driver probes and configure MSI, etc
right.
Quoted 4 lines
4) EP system runs EP function test and to understand what the host system enabled on the endpoint (eg PCI EP test function driver using MSIs) you rely on capabilities registers host system settings that are reflected in the EP registers
Yeah. it shouldn't try to raise MSI interrupts when host has not enabled it. The pci-epf-test driver which is there in the kernel monitors it's MEM_SPACE registers to see if the host has issued any commands (read or write) and performs the appropriate function.
Quoted 8 lines
Is my understanding correct ?
So basically the PCI EP test function works as software IP whose behaviour is controlled by the EP system kernel/userspace.
There are obvious syncronization assumptions here - the whole thing is racy by design (inclusive of the stepwise boot sequence above)
Given that two different systems can access the same physical memory, the drivers has to be written with caution.
For instance, the EP function drivers shouldn't modify pci configuration space registers after epc_start.
Quoted 5 lines
but if my understanding above is correct I think this patch is the right way of handling it.
I wonder how this is going to work if the EP function BARs map a real device (eg USB), please let me know your thoughts on that.
The EP function driver should setup the endpoint controller to be used as a USB by the host. At a high level, it has to perform the following 4 operations.
1) Identify itself as a USB PCI device.
2) map BAR's to USB registers
3) Forwarding interrupts
4) EP as bus master accessing host's buffer address
1st step can be achieved by using pci_epc_write_header() (vendorid and deviceid should bind it to xhci-pci driver)
2nd step can be done by using pci_epc_set_bar() (here the USB registers base address, size of the USB register space should be passed. These can be obtained from the dt for instance. It has to be made sure if a particular controller is being exposed to the PCI host, it shouldn't be used locally... move the USB dt node from ocp bus to be the child node of endpoint dt node??)
3rd step. Even though the driver for programming the USB controller is in the host system, the interrupts will be received in the EP system. The EP function driver should register an interrupt handler for the USB controllers IRQ (obtained from dt) and the handler should invoke pci_epc_raise_irq() to forward the interrupt.
(What if the USB controller raises lot of interrupts before the first interrupt is handled..?? this can be a separate discussion)
So far there hasn't been any modification to the standard host side driver. But in the 4th step where the host driver might program the URB address (host DDR address) in a register present the EPs MEM_SPACE, If the USB controller can directly access the PCI address, then standard xhci-pci driver should suffice.
However if the USB controller cannot access PCI address space (which is the case in dra7x SoC's) directly then using standard xhci driver won't make much sense and a custom host side driver and a corresponding EP function driver should be written).
This limitation will be for any controllers that can act as a master. But say you have a MMC controller which uses host's system DMA, the programming will be simpler. The MMC controller with ADMA2 might not be so simpler.
Thanks
Kishon
Quoted 154 lines · Kishon Vijay Abraham I
Hi Lorenzo,
Quoted 71 lines · Lorenzo Pieralisi
Quoted 70 lines · Kishon Vijay Abraham I
Hi Lorenzo,
Quoted 13 lines · Lorenzo Pieralisi
Quoted 3 lines · Kishon Vijay Abraham I
->get_msi() now checks
MSI_ENbit in the MSI CAPABILITY register to find whether the host supports MSI instead of using the MSI ADDRESS in the MSI CAPABILITY register.I think that's a sensible thing to do regardless, given that I do not understand why
0x0can't be a valid MSI address in the first place.Quoted 5 lines
This fixes the issue with the following sequence
'modprobepci_endpoint_test' enables MSI
'rmmodpci_endpoint_test' disables MSI but MSI address has a valid value 'modprobepci_endpoint_testno_msi=1' - Since MSI address has a valid
value (set during the previous insertion of the module), EPMSI address is written in the MSI capabiltiy register by the host.
Quoted 6 lines
Where is the address set ?
Quoted 1 line
thinks host supports MSI.
Add a Fixes: tag please.
sure.
Quoted 20 lines
Quoted 20 lines
Signed-off-by Kishon Vijay Abraham I <[email protected]>--- drivers/pci/dwc/pcie-designware-ep.c | 12 +++--------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+), 9 deletions(-)drivers/pci/dwc/pcie-designware-ep.c +0
-2@@ -197,20 +197,14 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr,static int dw_pcie_ep_get_msi (struct pci_epc *epc) {int val; -u32 lower_addr; -u32 upper_addr;struct dw_pcie_ep *ep =epc_get_drvdata (epc);struct dw_pcie *pci =to_dw_pcie_from_ep (ep);Quoted 8 lines
drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 209
Quoted 5 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-5- val =dw_pcie_readb_dbi (pci,MSI_MESSAGE_CONTROL ); - val = (val &MSI_CAP_MME_MASK ) >>MSI_CAP_MME_SHIFT ; - - lower_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_L32 ); - upper_addr =dw_pcie_readl_dbi (pci,MSI_MESSAGE_ADDR_U32 );I can't find code that writes these registers, see above for my question.
IIUC the host will write to these registers in
__pci_write_msi_msgusing the configuration writes.Quoted 12 lines
On top of that I think that what's done the EPF test function driver in struct
pci_epf.bind()should be undone in structpci_epf.unbind()and I do not think that's happening for MSIs.drivers/pci/dwc/pcie-designware-ep.c static int dw_pcie_ep_map_addr(struct pci_epc *epc, phys_addr_t addr, · line 203
Quoted 3 lines
drivers/pci/dwc/pcie-designware-ep.c +1
-2- -if (!(lower_addr || upper_addr)) + val =dw_pcie_readw_dbi (pci,MSI_MESSAGE_CONTROL );
MSI_MESSAGE_CONTROLis written indw_pcie_ep_set_msi(), I assume a write to bit[0] (ieMSI_CAP_MSI_EN_MASK) is ignored - it would be good if you can clarify that as well please.Quoted 8 lines
Quoted 2 lines
Do you mean
dw_pcie_ep_set_msican overwriteMSI_CAP_MSI_ENand we should avoid that?Yes, I think there are a lot of assumptions here. IIUC the sequence you expect this to work is:
1) EP system (ie system where the EP is located) is brought up and EP is configured (inclusive of MSI)
right. Here only the number of MSI's required by the EP is configured. The host is responsible for enabling MSI after programming the MSI address and data.
The EP function driver should also come up here. The BAR registers and the number of MSIs required by the EP function will be configured by the function driver.
Once the EP function driver has been fully configured,
pci_epc_start()should be invoked which starts the link. Without this the host will not see the EP device.Quoted 2 lines
2) host boots and enumerates PCI devices (which includes the EP)
3) host EP PCI driver probes and configure MSI, etcright.
Quoted 4 lines
4) EP system runs EP function test and to understand what the host system enabled on the endpoint (eg PCI EP test function driver using MSIs) you rely on capabilities registers host system settings that are reflected in the EP registers
Yeah. it shouldn't try to raise MSI interrupts when host has not enabled it. The pci-epf-test driver which is there in the kernel monitors it's
MEM_SPACEregisters to see if the host has issued any commands (read or write) and performs the appropriate function.Quoted 8 lines
Is my understanding correct ?
So basically the PCI EP test function works as software IP whose behaviour is controlled by the EP system kernel/userspace.
There are obvious syncronization assumptions here - the whole thing is racy by design (inclusive of the stepwise boot sequence above)
Given that two different systems can access the same physical memory, the drivers has to be written with caution.
For instance, the EP function drivers shouldn't modify pci configuration space registers after
epc_start.Quoted 5 lines
but if my understanding above is correct I think this patch is the right way of handling it.
I wonder how this is going to work if the EP function BARs map a real device (eg USB), please let me know your thoughts on that.
The EP function driver should setup the endpoint controller to be used as a USB by the host. At a high level, it has to perform the following 4 operations.
1) Identify itself as a USB PCI device.
2) map BAR's to USB registers
3) Forwarding interrupts
4) EP as bus master accessing host's buffer address1st step can be achieved by using
pci_epc_write_header()(vendorid and deviceid should bind it to xhci-pci driver)2nd step can be done by using
pci_epc_set_bar()(here the USB registers base address, size of the USB register space should be passed. These can be obtained from the dt for instance. It has to be made sure if a particular controller is being exposed to the PCI host, it shouldn't be used locally... move the USB dt node from ocp bus to be the child node of endpoint dt node??)3rd step. Even though the driver for programming the USB controller is in the host system, the interrupts will be received in the EP system. The EP function driver should register an interrupt handler for the USB controllers IRQ (obtained from dt) and the handler should invoke
pci_epc_raise_irq()to forward the interrupt.
(What if the USB controller raises lot of interrupts before the first interrupt is handled..?? this can be a separate discussion)So far there hasn't been any modification to the standard host side driver. But in the 4th step where the host driver might program the URB address (host DDR address) in a register present the EPs
MEM_SPACE, If the USB controller can directly access the PCI address, then standard xhci-pci driver should suffice.
Quoted 8 lines
However if the USB controller cannot access PCI address space (which is the case in dra7x SoC's) directly then using standard xhci driver won't make much sense and a custom host side driver and a corresponding EP function driver should be written).
This limitation will be for any controllers that can act as a master. But say you have a MMC controller which uses host's system DMA, the programming will be simpler. The MMC controller with ADMA2 might not be so simpler.
Thank you for the explanation - yes we need to see what's the best way to tackle some of the issues you reported.
Mind updating the tags and sending the patch back (v2) so that we can ask Bjorn to queue the fix please ?
Thanks,
Lorenzo