| Message ID | 20201111153559.19050-12-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement NTB Controller using multiple PCI EP | expand |
> -----Original Message----- > From: Kishon Vijay Abraham I <kishon@ti.com> > > Implement ->msi_map_irq() ops in order to map physical address to > MSI address and return MSI data. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > .../pci/controller/cadence/pcie-cadence-ep.c | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > Reviewed-by: Tom Joseph <tjoseph@cadence.com>
On Wed, Nov 11, 2020 at 9:37 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Implement ->msi_map_irq() ops in order to map physical address to > MSI address and return MSI data. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > .../pci/controller/cadence/pcie-cadence-ep.c | 53 +++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > index 84cc58dc8512..1fe6b8baca97 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > @@ -382,6 +382,57 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, > return 0; > } > > +static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, > + phys_addr_t addr, u8 interrupt_num, > + u32 entry_size, u32 *msi_data, > + u32 *msi_addr_offset) > +{ > + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); > + u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET; > + struct cdns_pcie *pcie = &ep->pcie; > + u64 pci_addr, pci_addr_mask = 0xff; > + u16 flags, mme, data, data_mask; > + u8 msi_count; > + int ret; > + int i; > + > + /* Check whether the MSI feature has been enabled by the PCI host. */ > + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_FLAGS); > + if (!(flags & PCI_MSI_FLAGS_ENABLE)) > + return -EINVAL; > + > + /* Get the number of enabled MSIs */ > + mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4; > + msi_count = 1 << mme; > + if (!interrupt_num || interrupt_num > msi_count) > + return -EINVAL; > + > + /* Compute the data value to be written. */ > + data_mask = msi_count - 1; > + data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64); > + data = data & ~data_mask; > + > + /* Get the PCI address where to write the data into. */ > + pci_addr = cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_HI); > + pci_addr <<= 32; > + pci_addr |= cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_LO); > + pci_addr &= GENMASK_ULL(63, 2); Wouldn't all of the above be the same code for any endpoint driver? We just need endpoint config space accessors for the same 32-bit only access issues. Not asking for that in this series, but if that's the direction we should go. > + > + for (i = 0; i < interrupt_num; i++) { > + ret = cdns_pcie_ep_map_addr(epc, fn, addr, > + pci_addr & ~pci_addr_mask, > + entry_size); > + if (ret) > + return ret; > + addr = addr + entry_size; > + } > + > + *msi_data = data; > + *msi_addr_offset = pci_addr & pci_addr_mask; > + > + return 0; > +} > + > static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, > u16 interrupt_num) > { > @@ -481,6 +532,7 @@ static const struct pci_epc_features cdns_pcie_epc_features = { > .linkup_notifier = false, > .msi_capable = true, > .msix_capable = true, > + .align = 256, > }; > > static const struct pci_epc_features* > @@ -500,6 +552,7 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { > .set_msix = cdns_pcie_ep_set_msix, > .get_msix = cdns_pcie_ep_get_msix, > .raise_irq = cdns_pcie_ep_raise_irq, > + .map_msi_irq = cdns_pcie_ep_map_msi_irq, > .start = cdns_pcie_ep_start, > .get_features = cdns_pcie_ep_get_features, > }; > -- > 2.17.1 >
Hi Rob, On 15/12/20 9:31 pm, Rob Herring wrote: > On Wed, Nov 11, 2020 at 9:37 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> Implement ->msi_map_irq() ops in order to map physical address to >> MSI address and return MSI data. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> .../pci/controller/cadence/pcie-cadence-ep.c | 53 +++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c >> index 84cc58dc8512..1fe6b8baca97 100644 >> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c >> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c >> @@ -382,6 +382,57 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, >> return 0; >> } >> >> +static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, >> + phys_addr_t addr, u8 interrupt_num, >> + u32 entry_size, u32 *msi_data, >> + u32 *msi_addr_offset) >> +{ >> + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); >> + u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET; >> + struct cdns_pcie *pcie = &ep->pcie; >> + u64 pci_addr, pci_addr_mask = 0xff; >> + u16 flags, mme, data, data_mask; >> + u8 msi_count; >> + int ret; >> + int i; >> + > > >> + /* Check whether the MSI feature has been enabled by the PCI host. */ >> + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_FLAGS); >> + if (!(flags & PCI_MSI_FLAGS_ENABLE)) >> + return -EINVAL; >> + >> + /* Get the number of enabled MSIs */ >> + mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4; >> + msi_count = 1 << mme; >> + if (!interrupt_num || interrupt_num > msi_count) >> + return -EINVAL; >> + >> + /* Compute the data value to be written. */ >> + data_mask = msi_count - 1; >> + data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64); >> + data = data & ~data_mask; >> + >> + /* Get the PCI address where to write the data into. */ >> + pci_addr = cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_HI); >> + pci_addr <<= 32; >> + pci_addr |= cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_LO); >> + pci_addr &= GENMASK_ULL(63, 2); > > Wouldn't all of the above be the same code for any endpoint driver? We > just need endpoint config space accessors for the same 32-bit only > access issues. Not asking for that in this series, but if that's the > direction we should go. Do you mean "endpoint" variant of pci_generic_config_read() which takes function number and capability offset? That could be done but we have to add support to traverse the linked list of capabilities though the capabilities are going to be at a fixed location for a given IP. Also in some cases, the writes are to a different register than the configuration space registers like vendor_id in Cadence EP should be written to Local Management register instead of the configuration space register. Thank You, Kishon
On Mon, Jan 4, 2021 at 6:13 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi Rob, > > On 15/12/20 9:31 pm, Rob Herring wrote: > > On Wed, Nov 11, 2020 at 9:37 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > >> > >> Implement ->msi_map_irq() ops in order to map physical address to > >> MSI address and return MSI data. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> .../pci/controller/cadence/pcie-cadence-ep.c | 53 +++++++++++++++++++ > >> 1 file changed, 53 insertions(+) > >> > >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > >> index 84cc58dc8512..1fe6b8baca97 100644 > >> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > >> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > >> @@ -382,6 +382,57 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, > >> return 0; > >> } > >> > >> +static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, > >> + phys_addr_t addr, u8 interrupt_num, > >> + u32 entry_size, u32 *msi_data, > >> + u32 *msi_addr_offset) > >> +{ > >> + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); > >> + u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET; > >> + struct cdns_pcie *pcie = &ep->pcie; > >> + u64 pci_addr, pci_addr_mask = 0xff; > >> + u16 flags, mme, data, data_mask; > >> + u8 msi_count; > >> + int ret; > >> + int i; > >> + > > > > > >> + /* Check whether the MSI feature has been enabled by the PCI host. */ > >> + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_FLAGS); > >> + if (!(flags & PCI_MSI_FLAGS_ENABLE)) > >> + return -EINVAL; > >> + > >> + /* Get the number of enabled MSIs */ > >> + mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4; > >> + msi_count = 1 << mme; > >> + if (!interrupt_num || interrupt_num > msi_count) > >> + return -EINVAL; > >> + > >> + /* Compute the data value to be written. */ > >> + data_mask = msi_count - 1; > >> + data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64); > >> + data = data & ~data_mask; > >> + > >> + /* Get the PCI address where to write the data into. */ > >> + pci_addr = cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_HI); > >> + pci_addr <<= 32; > >> + pci_addr |= cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_LO); > >> + pci_addr &= GENMASK_ULL(63, 2); > > > > Wouldn't all of the above be the same code for any endpoint driver? We > > just need endpoint config space accessors for the same 32-bit only > > access issues. Not asking for that in this series, but if that's the > > direction we should go. > > Do you mean "endpoint" variant of pci_generic_config_read() which takes > function number and capability offset? That could be done but we have to > add support to traverse the linked list of capabilities though the > capabilities are going to be at a fixed location for a given IP. Well, the above code would call the equivalent of pci_bus_read_config_*() functions which then calls driver specific read/write ops like pci_generic_config_read(). Once we have common accessors, then functions to get the capability offsets would be common too. It shouldn't matter that they happen to be fixed, walking the linked list should work either way. Getting rid of fixed offsets for the host side drivers is something I've been doing too. > Also in some cases, the writes are to a different register than the > configuration space registers like vendor_id in Cadence EP should be > written to Local Management register instead of the configuration space > register. We have the same issue on the host side as well. That just means we need to wrap the generic ops functions. Rob
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c index 84cc58dc8512..1fe6b8baca97 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c @@ -382,6 +382,57 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, return 0; } +static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, + phys_addr_t addr, u8 interrupt_num, + u32 entry_size, u32 *msi_data, + u32 *msi_addr_offset) +{ + struct cdns_pcie_ep *ep = epc_get_drvdata(epc); + u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET; + struct cdns_pcie *pcie = &ep->pcie; + u64 pci_addr, pci_addr_mask = 0xff; + u16 flags, mme, data, data_mask; + u8 msi_count; + int ret; + int i; + + /* Check whether the MSI feature has been enabled by the PCI host. */ + flags = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_FLAGS); + if (!(flags & PCI_MSI_FLAGS_ENABLE)) + return -EINVAL; + + /* Get the number of enabled MSIs */ + mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4; + msi_count = 1 << mme; + if (!interrupt_num || interrupt_num > msi_count) + return -EINVAL; + + /* Compute the data value to be written. */ + data_mask = msi_count - 1; + data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64); + data = data & ~data_mask; + + /* Get the PCI address where to write the data into. */ + pci_addr = cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_HI); + pci_addr <<= 32; + pci_addr |= cdns_pcie_ep_fn_readl(pcie, fn, cap + PCI_MSI_ADDRESS_LO); + pci_addr &= GENMASK_ULL(63, 2); + + for (i = 0; i < interrupt_num; i++) { + ret = cdns_pcie_ep_map_addr(epc, fn, addr, + pci_addr & ~pci_addr_mask, + entry_size); + if (ret) + return ret; + addr = addr + entry_size; + } + + *msi_data = data; + *msi_addr_offset = pci_addr & pci_addr_mask; + + return 0; +} + static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u16 interrupt_num) { @@ -481,6 +532,7 @@ static const struct pci_epc_features cdns_pcie_epc_features = { .linkup_notifier = false, .msi_capable = true, .msix_capable = true, + .align = 256, }; static const struct pci_epc_features* @@ -500,6 +552,7 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { .set_msix = cdns_pcie_ep_set_msix, .get_msix = cdns_pcie_ep_get_msix, .raise_irq = cdns_pcie_ep_raise_irq, + .map_msi_irq = cdns_pcie_ep_map_msi_irq, .start = cdns_pcie_ep_start, .get_features = cdns_pcie_ep_get_features, };
Implement ->msi_map_irq() ops in order to map physical address to MSI address and return MSI data. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- .../pci/controller/cadence/pcie-cadence-ep.c | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+)