| Message ID | 20241017132052.4014605-6-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: dwc: ep: Minor alignment cleanups | expand |
On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 20f67fd85e83..9bafa62bed1d 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > u32 msg_addr_lower, msg_addr_upper, reg; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > - unsigned int aligned_offset; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u16 msg_ctrl, msg_data; > bool has_upper; > u64 msg_addr; > @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > } > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > - epc->mem->window.page_size); > + msi_mem_size); > if (ret) > return ret; > > - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); > + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > struct pci_epf_msix_tbl *msix_tbl; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u32 reg, msg_data, vec_ctrl; > - unsigned int aligned_offset; why not direct use 'aligned_offset' ? just change to size_t. > u32 tbl_offset; > u64 msg_addr; > int ret; > @@ -615,14 +616,13 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > return -EPERM; > } > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > epc->mem->window.page_size); > if (ret) > return ret; > > - writel(msg_data, ep->msi_mem + aligned_offset); > + writel(msg_data, ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > -- > 2.47.0 >
Hello Frank, On Thu, Oct 17, 2024 at 11:36:53AM -0400, Frank Li wrote: > On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 20f67fd85e83..9bafa62bed1d 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > > u32 msg_addr_lower, msg_addr_upper, reg; > > struct dw_pcie_ep_func *ep_func; > > struct pci_epc *epc = ep->epc; > > - unsigned int aligned_offset; > > + size_t msi_mem_size = epc->mem->window.page_size; > > + size_t offset; > > u16 msg_ctrl, msg_data; > > bool has_upper; > > u64 msg_addr; > > @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > > } > > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > > - epc->mem->window.page_size); > > + msi_mem_size); > > if (ret) > > return ret; > > > > - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); > > + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); > > > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > > > @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > > struct pci_epf_msix_tbl *msix_tbl; > > struct dw_pcie_ep_func *ep_func; > > struct pci_epc *epc = ep->epc; > > + size_t msi_mem_size = epc->mem->window.page_size; > > + size_t offset; > > u32 reg, msg_data, vec_ctrl; > > - unsigned int aligned_offset; > > why not direct use 'aligned_offset' ? just change to size_t. Because I think that that name was really bad. aligned_offset sounds like the offset is aligned, but that is not the case. Now when we have a dw_pcie_ep_align_addr() function, I think that simply calling the variable offset is less ambiguous. Anyone who isn't sure what the offset represents can simply read the documentation for the .align_addr endpoint controller operation. Kind regards, Niklas
On Thu, Oct 17, 2024 at 08:54:01PM +0200, Niklas Cassel wrote: > Hello Frank, > > On Thu, Oct 17, 2024 at 11:36:53AM -0400, Frank Li wrote: > > On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > > > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > > > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > --- > > > .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > index 20f67fd85e83..9bafa62bed1d 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > > > u32 msg_addr_lower, msg_addr_upper, reg; > > > struct dw_pcie_ep_func *ep_func; > > > struct pci_epc *epc = ep->epc; > > > - unsigned int aligned_offset; > > > + size_t msi_mem_size = epc->mem->window.page_size; > > > + size_t offset; > > > u16 msg_ctrl, msg_data; > > > bool has_upper; > > > u64 msg_addr; > > > @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > > > } > > > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > > > > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > > > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > > > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > > > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > > > - epc->mem->window.page_size); > > > + msi_mem_size); > > > if (ret) > > > return ret; > > > > > > - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); > > > + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); > > > > > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > > > > > @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > > > struct pci_epf_msix_tbl *msix_tbl; > > > struct dw_pcie_ep_func *ep_func; > > > struct pci_epc *epc = ep->epc; > > > + size_t msi_mem_size = epc->mem->window.page_size; > > > + size_t offset; > > > u32 reg, msg_data, vec_ctrl; > > > - unsigned int aligned_offset; > > > > why not direct use 'aligned_offset' ? just change to size_t. > > Because I think that that name was really bad. > aligned_offset sounds like the offset is aligned, but that is not the case. > > Now when we have a dw_pcie_ep_align_addr() function, I think that simply > calling the variable offset is less ambiguous. Anyone who isn't sure what > the offset represents can simply read the documentation for the .align_addr > endpoint controller operation. Make sense. Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > Kind regards, > Niklas
On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 20f67fd85e83..9bafa62bed1d 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > u32 msg_addr_lower, msg_addr_upper, reg; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > - unsigned int aligned_offset; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u16 msg_ctrl, msg_data; > bool has_upper; > u64 msg_addr; > @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > } > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > - epc->mem->window.page_size); > + msi_mem_size); I haven't worked through this; just double checking that this is correct. Previously we did ALIGN_DOWN() here, but dw_pcie_ep_align_addr() uses ALIGN() (not ALIGN_DOWN()). Similar below in dw_pcie_ep_raise_msix_irq(). > if (ret) > return ret; > > - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); > + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > struct pci_epf_msix_tbl *msix_tbl; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u32 reg, msg_data, vec_ctrl; > - unsigned int aligned_offset; > u32 tbl_offset; > u64 msg_addr; > int ret; > @@ -615,14 +616,13 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > return -EPERM; > } > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > epc->mem->window.page_size); > if (ret) > return ret; > > - writel(msg_data, ep->msi_mem + aligned_offset); > + writel(msg_data, ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > -- > 2.47.0 >
On 11/1/24 05:28, Bjorn Helgaas wrote: > On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: >> Use the dw_pcie_ep_align_addr() function to calculate the alignment in >> dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. >> >> Signed-off-by: Niklas Cassel <cassel@kernel.org> >> --- >> .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c >> index 20f67fd85e83..9bafa62bed1d 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c >> @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, >> u32 msg_addr_lower, msg_addr_upper, reg; >> struct dw_pcie_ep_func *ep_func; >> struct pci_epc *epc = ep->epc; >> - unsigned int aligned_offset; >> + size_t msi_mem_size = epc->mem->window.page_size; >> + size_t offset; >> u16 msg_ctrl, msg_data; >> bool has_upper; >> u64 msg_addr; >> @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, >> } >> msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; >> >> - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); >> - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); >> + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); >> ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, >> - epc->mem->window.page_size); >> + msi_mem_size); > > I haven't worked through this; just double checking that this is > correct. Previously we did ALIGN_DOWN() here, but > dw_pcie_ep_align_addr() uses ALIGN() (not ALIGN_DOWN()). Similar > below in dw_pcie_ep_raise_msix_irq(). The ALIGN() in dw_pcie_ep_align_addr() is for the mapping size. The address is aligned down manually: return pci_addr & ~mask; So it is the same. We could change dw_pcie_ep_align_addr() to do: return ALIGN_DOWN(pci_addr, epc->mem->window.page_size); But given that the offset calculation needs the alignment mask anyway, using the mask variable directly seems natural. So this is functionnally identical for the PCI address being mapped, and it is even better for the mapping size since this was passing epc->mem->window.page_size before but if the PCI address range crosses over a page_size boundary, we actually need 2 x page_size as the mapping size... Which makes me realized that there is something still wrong: the memory being mapped (ep->msi_mem) is at most one page: ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys, epc->mem->window.page_size); if (!ep->msi_mem) { ret = -ENOMEM; dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n"); goto err_exit_epc_mem; } But we may need up to 2 pages depending on the PCI address we get for the MSI/MSIX. So we need to fix that as well I think. Niklas, thoughts ?
On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 20f67fd85e83..9bafa62bed1d 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > u32 msg_addr_lower, msg_addr_upper, reg; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > - unsigned int aligned_offset; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u16 msg_ctrl, msg_data; > bool has_upper; > u64 msg_addr; > @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > } > msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > - epc->mem->window.page_size); > + msi_mem_size); > if (ret) > return ret; > > - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); > + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > struct pci_epf_msix_tbl *msix_tbl; > struct dw_pcie_ep_func *ep_func; > struct pci_epc *epc = ep->epc; > + size_t msi_mem_size = epc->mem->window.page_size; > + size_t offset; > u32 reg, msg_data, vec_ctrl; > - unsigned int aligned_offset; > u32 tbl_offset; > u64 msg_addr; > int ret; > @@ -615,14 +616,13 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > return -EPERM; > } > > - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > epc->mem->window.page_size); > if (ret) > return ret; > > - writel(msg_data, ep->msi_mem + aligned_offset); > + writel(msg_data, ep->msi_mem + offset); > > dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); > > -- > 2.47.0 >
On Fri, Nov 01, 2024 at 07:36:51AM +0900, Damien Le Moal wrote: > On 11/1/24 05:28, Bjorn Helgaas wrote: > > On Thu, Oct 17, 2024 at 03:20:55PM +0200, Niklas Cassel wrote: > >> Use the dw_pcie_ep_align_addr() function to calculate the alignment in > >> dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. > >> > >> Signed-off-by: Niklas Cassel <cassel@kernel.org> > >> --- > >> .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- > >> 1 file changed, 9 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > >> index 20f67fd85e83..9bafa62bed1d 100644 > >> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > >> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > >> @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > >> u32 msg_addr_lower, msg_addr_upper, reg; > >> struct dw_pcie_ep_func *ep_func; > >> struct pci_epc *epc = ep->epc; > >> - unsigned int aligned_offset; > >> + size_t msi_mem_size = epc->mem->window.page_size; > >> + size_t offset; > >> u16 msg_ctrl, msg_data; > >> bool has_upper; > >> u64 msg_addr; > >> @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, > >> } > >> msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; > >> > >> - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); > >> - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); > >> + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); > >> ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, > >> - epc->mem->window.page_size); > >> + msi_mem_size); > > > > I haven't worked through this; just double checking that this is > > correct. Previously we did ALIGN_DOWN() here, but > > dw_pcie_ep_align_addr() uses ALIGN() (not ALIGN_DOWN()). Similar > > below in dw_pcie_ep_raise_msix_irq(). > > The ALIGN() in dw_pcie_ep_align_addr() is for the mapping size. The address is > aligned down manually: > > return pci_addr & ~mask; > > So it is the same. We could change dw_pcie_ep_align_addr() to do: > > return ALIGN_DOWN(pci_addr, epc->mem->window.page_size); > > But given that the offset calculation needs the alignment mask anyway, using > the mask variable directly seems natural. I agree. > > So this is functionnally identical for the PCI address being mapped, and it is > even better for the mapping size since this was passing > epc->mem->window.page_size before but if the PCI address range crosses over a > page_size boundary, we actually need 2 x page_size as the mapping size... > > Which makes me realized that there is something still wrong: the memory being > mapped (ep->msi_mem) is at most one page: > > ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys, > epc->mem->window.page_size); > if (!ep->msi_mem) { > ret = -ENOMEM; > dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n"); > goto err_exit_epc_mem; > } > > But we may need up to 2 pages depending on the PCI address we get for the > MSI/MSIX. So we need to fix that as well I think. > > Niklas, thoughts ? Hmm.. the MSI data that we want to map is just two bytes. It is just that the minimum mapping we can do is epc->mem->window.page_size. On e.g. rk3588 the page_size is 64k. I guess that we might be unlucky and get the MSI data at an offset that is at the end of the page. But considering that we just need 2 bytes (for MSI, 4 bytes for MSI-X), and that the PCI word size is 4 bytes, I don't see a problem with the current code. While the allocation of the msi_mem should still be page_size: ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys, epc->mem->window.page_size); I guess that we could change dw_pcie_ep_raise_msi_irq() from: size_t msi_mem_size = epc->mem->window.page_size; to: size_t msi_mem_size = 2; and change dw_pcie_ep_raise_msix_irq() from: size_t msi_mem_size = epc->mem->window.page_size; to: size_t msi_mem_size = 4; To make the code strictly more correct for a reader, but I don't think that there is a problem with the current code. Kind regards, Niklas
Hello, > Use the dw_pcie_ep_align_addr() function to calculate the alignment in > dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. [1/1] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq() https://git.kernel.org/pci/pci/c/f68da9a67173 Krzysztof
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 20f67fd85e83..9bafa62bed1d 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -503,7 +503,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, u32 msg_addr_lower, msg_addr_upper, reg; struct dw_pcie_ep_func *ep_func; struct pci_epc *epc = ep->epc; - unsigned int aligned_offset; + size_t msi_mem_size = epc->mem->window.page_size; + size_t offset; u16 msg_ctrl, msg_data; bool has_upper; u64 msg_addr; @@ -531,14 +532,13 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, } msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower; - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, - epc->mem->window.page_size); + msi_mem_size); if (ret) return ret; - writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset); + writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset); dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys); @@ -589,8 +589,9 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, struct pci_epf_msix_tbl *msix_tbl; struct dw_pcie_ep_func *ep_func; struct pci_epc *epc = ep->epc; + size_t msi_mem_size = epc->mem->window.page_size; + size_t offset; u32 reg, msg_data, vec_ctrl; - unsigned int aligned_offset; u32 tbl_offset; u64 msg_addr; int ret; @@ -615,14 +616,13 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, return -EPERM; } - aligned_offset = msg_addr & (epc->mem->window.page_size - 1); - msg_addr = ALIGN_DOWN(msg_addr, epc->mem->window.page_size); + msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &msi_mem_size, &offset); ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr, epc->mem->window.page_size); if (ret) return ret; - writel(msg_data, ep->msi_mem + aligned_offset); + writel(msg_data, ep->msi_mem + offset); dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);
Use the dw_pcie_ep_align_addr() function to calculate the alignment in dw_pcie_ep_raise_{msi,msix}_irq() instead of open coding the same. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- .../pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)