diff mbox series

[2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()

Message ID 20241017132052.4014605-6-cassel@kernel.org (mailing list archive)
State New
Headers show
Series PCI: dwc: ep: Minor alignment cleanups | expand

Commit Message

Niklas Cassel Oct. 17, 2024, 1:20 p.m. UTC 
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(-)

Comments

Frank Li Oct. 17, 2024, 3:36 p.m. UTC | #1 
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
>
Niklas Cassel Oct. 17, 2024, 6:54 p.m. UTC | #2 
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
Frank Li Oct. 17, 2024, 7:36 p.m. UTC | #3 
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 mbox series

Patch

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);