* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 13:20 ` [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq() Niklas Cassel
@ 2024-10-17 15:36 ` Frank Li
2024-10-17 18:54 ` Niklas Cassel
2024-10-31 20:28 ` Bjorn Helgaas
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Frank Li @ 2024-10-17 15:36 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, linux-pci
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 15:36 ` Frank Li
@ 2024-10-17 18:54 ` Niklas Cassel
2024-10-17 19:36 ` Frank Li
0 siblings, 1 reply; 16+ messages in thread
From: Niklas Cassel @ 2024-10-17 18:54 UTC (permalink / raw)
To: Frank Li
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 18:54 ` Niklas Cassel
@ 2024-10-17 19:36 ` Frank Li
0 siblings, 0 replies; 16+ messages in thread
From: Frank Li @ 2024-10-17 19:36 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 13:20 ` [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq() Niklas Cassel
2024-10-17 15:36 ` Frank Li
@ 2024-10-31 20:28 ` Bjorn Helgaas
2024-10-31 22:36 ` Damien Le Moal
2024-11-01 7:16 ` Manivannan Sadhasivam
2024-11-02 11:41 ` Krzysztof Wilczyński
3 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2024-10-31 20:28 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, linux-pci
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-31 20:28 ` Bjorn Helgaas
@ 2024-10-31 22:36 ` Damien Le Moal
2024-11-01 18:25 ` Niklas Cassel
0 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2024-10-31 22:36 UTC (permalink / raw)
To: Bjorn Helgaas, Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas, linux-pci
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 ?
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-31 22:36 ` Damien Le Moal
@ 2024-11-01 18:25 ` Niklas Cassel
0 siblings, 0 replies; 16+ messages in thread
From: Niklas Cassel @ 2024-11-01 18:25 UTC (permalink / raw)
To: Damien Le Moal
Cc: Bjorn Helgaas, Jingoo Han, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 13:20 ` [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq() Niklas Cassel
2024-10-17 15:36 ` Frank Li
2024-10-31 20:28 ` Bjorn Helgaas
@ 2024-11-01 7:16 ` Manivannan Sadhasivam
2024-11-02 11:41 ` Krzysztof Wilczyński
3 siblings, 0 replies; 16+ messages in thread
From: Manivannan Sadhasivam @ 2024-11-01 7:16 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Lorenzo Pieralisi, Krzysztof Wilczyński,
Rob Herring, Bjorn Helgaas, Damien Le Moal, linux-pci
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
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq()
2024-10-17 13:20 ` [PATCH 2/2] PCI: dwc: ep: Use align addr function for dw_pcie_ep_raise_{msi,msix}_irq() Niklas Cassel
` (2 preceding siblings ...)
2024-11-01 7:16 ` Manivannan Sadhasivam
@ 2024-11-02 11:41 ` Krzysztof Wilczyński
3 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Wilczyński @ 2024-11-02 11:41 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi, Rob Herring,
Bjorn Helgaas, Damien Le Moal, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread