Message ID | 20241017132052.4014605-6-cassel@kernel.org (mailing list archive) |
---|---|
State | New |
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
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(-)