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@xxxxxxxxxx>
> > ---
> > .../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
