Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Li Chen]

Hi, Tom

From these function:
static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
				struct pci_epf_bar *epf_bar)
{
	......
	if ((flags & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
		ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS;
	} else {
		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
		bool is_64bits = sz > SZ_2G;
		if (is_64bits && (bar & 1))
			return -EINVAL;
		if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;

		if (is_64bits && is_prefetch)
			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
		else if (is_prefetch)
			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
		else if (is_64bits)
			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS;
		else
			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS;
	}

	......
}


I don't understand why should sz > SZ_2G be taken into account for 64_bits. From my personal practice, there is no problem to use CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS or CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS when sz < SZ_2G.


Regards,
Li

**********************************************************************
This email and attachments contain Ambarella Proprietary and/or Confidential Information and is intended solely for the use of the individual(s) to whom it is addressed. Any unauthorized review, use, disclosure, distribute, copy, or print is prohibited. If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message. Thank you.

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Tom Joseph]

Hi Li,

 For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use as upper bits).
I see that the code is assuming 32_bits if size < 2G , so all bars could be enabled.

As I understand, you have a use case where you want to set the bar as 64 bit, actually use small size.
Is it possible to describe bit more about this use case (just curious)?

Thanks,
Tom  

> -----Original Message-----
> From: Li Chen <lchen@ambarella.com>
> Sent: 19 January 2022 09:28
> To: Tom Joseph <tjoseph@cadence.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in
> pcie-cadence-ep.c?
> 
> EXTERNAL MAIL
> 
> 
> Hi, Tom
> 
> From these function:
> static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> 				struct pci_epf_bar *epf_bar)
> {
> 	......
> 	if ((flags & PCI_BASE_ADDRESS_SPACE) ==
> PCI_BASE_ADDRESS_SPACE_IO) {
> 		ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_IO_32BITS;
> 	} else {
> 		bool is_prefetch = !!(flags &
> PCI_BASE_ADDRESS_MEM_PREFETCH);
> 		bool is_64bits = sz > SZ_2G;
> 		if (is_64bits && (bar & 1))
> 			return -EINVAL;
> 		if (is_64bits && !(flags &
> PCI_BASE_ADDRESS_MEM_TYPE_64))
> 			epf_bar->flags |=
> PCI_BASE_ADDRESS_MEM_TYPE_64;
> 
> 		if (is_64bits && is_prefetch)
> 			ctrl =
> CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
> 		else if (is_prefetch)
> 			ctrl =
> CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_32BITS;
> 		else if (is_64bits)
> 			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS;
> 		else
> 			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_32BITS;
> 	}
> 
> 	......
> }
> 
> 
> I don't understand why should sz > SZ_2G be taken into account for 64_bits.
> From my personal practice, there is no problem to use
> CDNS_PCIE_LM_BAR_CFG_CTRL_MEM_64BITS or
> CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS when sz < SZ_2G.
> 
> 
> Regards,
> Li
> 
> **********************************************************
> ************
> This email and attachments contain Ambarella Proprietary and/or
> Confidential Information and is intended solely for the use of the
> individual(s) to whom it is addressed. Any unauthorized review, use,
> disclosure, distribute, copy, or print is prohibited. If you are not an intended
> recipient, please contact the sender by reply email and destroy all copies of
> the original message. Thank you.

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Li Chen]

Hi, Tom

> -----Original Message-----
> From: Tom Joseph [mailto:tjoseph@cadence.com]
> Sent: Thursday, January 20, 2022 9:11 PM
> To: Li Chen
> Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits
> in pcie-cadence-ep.c?
> 
> Hi Li,
> 
>  For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use as upper
> bits).

Yes, I get it. 
> I see that the code is assuming 32_bits if size < 2G , so all bars could be enabled.
> 

Ok, but I still wonder why 2G instead of other sizes like 1G or 512M? IMO if there is no obvious limitation, 64 or 32 bit should be left to the user to decide(like bar_fixed_64bit and bar_fixed_size in pci_epc_features), instead of hardcode 2G here.

> As I understand, you have a use case where you want to set the bar as 64 bit,
> actually use small size.
> Is it possible to describe bit more about this use case (just curious)?

It's because our SoC use 0-64G AMBA address space for our dram(system memory), so if I want to use 32 bit bar like 16M bus address, I must reserve this 16M area with kernel's reserve-memory, otherwise endpoints like nvme will report unsupported request when it do dma and the dma address is also located under this 16M area. More details have been put in this thread: https://lore.kernel.org/lkml/CH2PR19MB40245BF88CF2F7210DCB1AA4A0669@CH2PR19MB4024.namprd19.prod.outlook.com/T/#m0dd09b7e6f868b9692185ec57c1986b3c786e8d3


So, if I don't want to reserve much memory for BAR, I have to use 64-bit bar, and it must be prefetch 64 bit, not non-prefetch in my case, because my virtual p2p bridge has three windows: io, mem(32bit), prefetch mem(64 bit, because CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS is set), and if the controller running under ep-mode use 64 non-prefetch, this region will fallback to bridge's 32-bit mem window but I don't(and cannot) reserve so much 32bit memory for it).


Regards,
Li

**********************************************************************
This email and attachments contain Ambarella Proprietary and/or Confidential Information and is intended solely for the use of the individual(s) to whom it is addressed. Any unauthorized review, use, disclosure, distribute, copy, or print is prohibited. If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message. Thank you.

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Tom Joseph]

Hi Li,

> -----Original Message-----
> From: Li Chen <lchen@ambarella.com>
> Sent: 21 January 2022 02:56
> To: Tom Joseph <tjoseph@cadence.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in
> pcie-cadence-ep.c?
> 
> EXTERNAL MAIL
> 
> 
> Hi, Tom
> 
> > -----Original Message-----
> > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > Sent: Thursday, January 20, 2022 9:11 PM
> > To: Li Chen
> > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> is_64bits
> > in pcie-cadence-ep.c?
> >
> > Hi Li,
> >
> >  For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use as
> upper
> > bits).
> 
> Yes, I get it.
> > I see that the code is assuming 32_bits if size < 2G , so all bars could be
> enabled.
> >
> 
> Ok, but I still wonder why 2G instead of other sizes like 1G or 512M? IMO if
> there is no obvious limitation, 64 or 32 bit should be left to the user to
> decide(like bar_fixed_64bit and bar_fixed_size in pci_epc_features), instead
> of hardcode 2G here.

Check for 2G is important as this is the max valid aperture size encoding (0x11000)
allowed by the controller for 32 bit BARs.

> 
> > As I understand, you have a use case where you want to set the bar as 64
> bit,
> > actually use small size.
> > Is it possible to describe bit more about this use case (just curious)?
> 
> It's because our SoC use 0-64G AMBA address space for our dram(system
> memory), so if I want to use 32 bit bar like 16M bus address, I must reserve
> this 16M area with kernel's reserve-memory, otherwise endpoints like nvme
> will report unsupported request when it do dma and the dma address is also
> located under this 16M area. More details have been put in this thread:
> https://urldefense.com/v3/__https://lore.kernel.org/lkml/CH2PR19MB4024
> 5BF88CF2F7210DCB1AA4A0669@CH2PR19MB4024.namprd19.prod.outlook.c
> om/T/*m0dd09b7e6f868b9692185ec57c1986b3c786e8d3__;Iw!!EHscmS1ygiU
> 1lA!SVgmPO0MrmUUnZzlmc-fCPsSBddkfUgT-
> Y7EmlLAgz9AoQSVZXa_18TIdT6O7kY$
> 
> 
> So, if I don't want to reserve much memory for BAR, I have to use 64-bit bar,
> and it must be prefetch 64 bit, not non-prefetch in my case, because my
> virtual p2p bridge has three windows: io, mem(32bit), prefetch mem(64 bit,
> because CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS is set), and
> if the controller running under ep-mode use 64 non-prefetch, this region will
> fallback to bridge's 32-bit mem window but I don't(and cannot) reserve so
> much 32bit memory for it).
> 
Got your point. Does a change in the code as below will be good enough ?

- bool is_64bits = sz > SZ_2G; 
+bool is_64bits = (sz > SZ_2G) ||(!!( flags &  PCI_BASE_ADDRESS_MEM_TYPE_64)) ;


Thanks,
Tom

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Li Chen]

Hi, Tom

> -----Original Message-----
> From: Tom Joseph [mailto:tjoseph@cadence.com]
> Sent: Saturday, January 22, 2022 2:09 AM
> To: Li Chen
> Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits
> in pcie-cadence-ep.c?
> 
> Hi Li,
> 
> > -----Original Message-----
> > From: Li Chen <lchen@ambarella.com>
> > Sent: 21 January 2022 02:56
> > To: Tom Joseph <tjoseph@cadence.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> > <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> > <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in
> > pcie-cadence-ep.c?
> >
> > EXTERNAL MAIL
> >
> >
> > Hi, Tom
> >
> > > -----Original Message-----
> > > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > > Sent: Thursday, January 20, 2022 9:11 PM
> > > To: Li Chen
> > > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> > > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> > is_64bits
> > > in pcie-cadence-ep.c?
> > >
> > > Hi Li,
> > >
> > >  For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use as
> > upper
> > > bits).
> >
> > Yes, I get it.
> > > I see that the code is assuming 32_bits if size < 2G , so all bars could be
> > enabled.
> > >
> >
> > Ok, but I still wonder why 2G instead of other sizes like 1G or 512M? IMO if
> > there is no obvious limitation, 64 or 32 bit should be left to the user to
> > decide(like bar_fixed_64bit and bar_fixed_size in pci_epc_features), instead
> > of hardcode 2G here.
> 
> Check for 2G is important as this is the max valid aperture size encoding (0x11000)
> allowed by the controller for 32 bit BARs.

Thanks, I get it now, and also find it in 7.3.1.50. Physical Function BAR Configuration Register 0 just now.
> 
> >
> > > As I understand, you have a use case where you want to set the bar as 64
> > bit,
> > > actually use small size.
> > > Is it possible to describe bit more about this use case (just curious)?
> >
> > It's because our SoC use 0-64G AMBA address space for our dram(system
> > memory), so if I want to use 32 bit bar like 16M bus address, I must reserve
> > this 16M area with kernel's reserve-memory, otherwise endpoints like nvme
> > will report unsupported request when it do dma and the dma address is also
> > located under this 16M area. More details have been put in this thread:
> > https://urldefense.com/v3/__https://lore.kernel.org/lkml/CH2PR19MB4024
> > 5BF88CF2F7210DCB1AA4A0669@CH2PR19MB4024.namprd19.prod.outlook.c
> > om/T/*m0dd09b7e6f868b9692185ec57c1986b3c786e8d3__;Iw!!EHscmS1ygiU
> > 1lA!SVgmPO0MrmUUnZzlmc-fCPsSBddkfUgT-
> > Y7EmlLAgz9AoQSVZXa_18TIdT6O7kY$
> >
> >
> > So, if I don't want to reserve much memory for BAR, I have to use 64-bit bar,
> > and it must be prefetch 64 bit, not non-prefetch in my case, because my
> > virtual p2p bridge has three windows: io, mem(32bit), prefetch mem(64 bit,
> > because CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS is set), and
> > if the controller running under ep-mode use 64 non-prefetch, this region will
> > fallback to bridge's 32-bit mem window but I don't(and cannot) reserve so
> > much 32bit memory for it).
> >
> Got your point. Does a change in the code as below will be good enough ?
> 
> - bool is_64bits = sz > SZ_2G;
> +bool is_64bits = (sz > SZ_2G) ||(!!( flags &
> PCI_BASE_ADDRESS_MEM_TYPE_64)) ;

Before answering this question, I want to raise another question, why bar 1 cannot be 64 bit?
		bool is_prefetch = !!(flags & PCI_BASE_ADDRESS_MEM_PREFETCH);
		bool is_64bits = sz > SZ_2G;

		if (is_64bits && (bar & 1))
			return -EINVAL;
I would be very grateful if you can tell me. 

I don't know the answer and will this change break something of bar 1, so my current way to handle this issue is:

PCI: cadence: respect 64bti when size is smaller than 2G

Original codes force size under 2G to be 32 bit somehow.
Signed-off-by: Li Chen <lchen@ambarella.com>

1 file changed, 2 insertions(+)
drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 ++

modified   drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -107,6 +107,8 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
 		if (is_64bits && !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
 			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
 
+		is_64bits = epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64;
+
 		if (is_64bits && is_prefetch)
 			ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
 		else if (is_prefetch)
> 
> 
> Thanks,
> Tom
> 
> ##############################################################
> ########
> This EXTERNAL email has been scanned by Proofpoint Email Protect service.

Regards,
Li

**********************************************************************
This email and attachments contain Ambarella Proprietary and/or Confidential Information and is intended solely for the use of the individual(s) to whom it is addressed. Any unauthorized review, use, disclosure, distribute, copy, or print is prohibited. If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message. Thank you.

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Tom Joseph]

Hi Li,

> -----Original Message-----
> From: Li Chen <lchen@ambarella.com>
> Sent: 24 January 2022 02:26
> To: Tom Joseph <tjoseph@cadence.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in
> pcie-cadence-ep.c?
> 
> EXTERNAL MAIL
> 
> 
> Hi, Tom
> 
> > -----Original Message-----
> > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > Sent: Saturday, January 22, 2022 2:09 AM
> > To: Li Chen
> > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> is_64bits
> > in pcie-cadence-ep.c?
> >
> > Hi Li,
> >
> > > -----Original Message-----
> > > From: Li Chen <lchen@ambarella.com>
> > > Sent: 21 January 2022 02:56
> > > To: Tom Joseph <tjoseph@cadence.com>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> > > <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> is_64bits in
> > > pcie-cadence-ep.c?
> > >
> > > EXTERNAL MAIL
> > >
> > >
> > > Hi, Tom
> > >
> > > > -----Original Message-----
> > > > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > > > Sent: Thursday, January 20, 2022 9:11 PM
> > > > To: Li Chen
> > > > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas;
> linux-
> > > > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> > > is_64bits
> > > > in pcie-cadence-ep.c?
> > > >
> > > > Hi Li,
> > > >
> > > >  For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use
> as
> > > upper
> > > > bits).
> > >
> > > Yes, I get it.
> > > > I see that the code is assuming 32_bits if size < 2G , so all bars could be
> > > enabled.
> > > >
> > >
> > > Ok, but I still wonder why 2G instead of other sizes like 1G or 512M? IMO
> if
> > > there is no obvious limitation, 64 or 32 bit should be left to the user to
> > > decide(like bar_fixed_64bit and bar_fixed_size in pci_epc_features),
> instead
> > > of hardcode 2G here.
> >
> > Check for 2G is important as this is the max valid aperture size encoding
> (0x11000)
> > allowed by the controller for 32 bit BARs.
> 
> Thanks, I get it now, and also find it in 7.3.1.50. Physical Function BAR
> Configuration Register 0 just now.
> >
> > >
> > > > As I understand, you have a use case where you want to set the bar as
> 64
> > > bit,
> > > > actually use small size.
> > > > Is it possible to describe bit more about this use case (just curious)?
> > >
> > > It's because our SoC use 0-64G AMBA address space for our dram(system
> > > memory), so if I want to use 32 bit bar like 16M bus address, I must
> reserve
> > > this 16M area with kernel's reserve-memory, otherwise endpoints like
> nvme
> > > will report unsupported request when it do dma and the dma address is
> also
> > > located under this 16M area. More details have been put in this thread:
> > >
> https://urldefense.com/v3/__https://lore.kernel.org/lkml/CH2PR19MB4024
> > >
> 5BF88CF2F7210DCB1AA4A0669@CH2PR19MB4024.namprd19.prod.outlook.c
> > >
> om/T/*m0dd09b7e6f868b9692185ec57c1986b3c786e8d3__;Iw!!EHscmS1ygiU
> > > 1lA!SVgmPO0MrmUUnZzlmc-fCPsSBddkfUgT-
> > > Y7EmlLAgz9AoQSVZXa_18TIdT6O7kY$
> > >
> > >
> > > So, if I don't want to reserve much memory for BAR, I have to use 64-bit
> bar,
> > > and it must be prefetch 64 bit, not non-prefetch in my case, because my
> > > virtual p2p bridge has three windows: io, mem(32bit), prefetch mem(64
> bit,
> > > because CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS is set),
> and
> > > if the controller running under ep-mode use 64 non-prefetch, this region
> will
> > > fallback to bridge's 32-bit mem window but I don't(and cannot) reserve
> so
> > > much 32bit memory for it).
> > >
> > Got your point. Does a change in the code as below will be good enough ?
> >
> > - bool is_64bits = sz > SZ_2G;
> > +bool is_64bits = (sz > SZ_2G) ||(!!( flags &
> > PCI_BASE_ADDRESS_MEM_TYPE_64)) ;
> 
> Before answering this question, I want to raise another question, why bar 1
> cannot be 64 bit?
> 		bool is_prefetch = !!(flags &
> PCI_BASE_ADDRESS_MEM_PREFETCH);
> 		bool is_64bits = sz > SZ_2G;
> 
> 		if (is_64bits && (bar & 1))
> 			return -EINVAL;
> I would be very grateful if you can tell me.
> 

As mentioned earlier,  if the user indicate a 64_bit bar aperture (with the encoding) , 
all the odd bars will be used up. User should then be denied to program odd bars.
The above check is not only for bar 1 , but for all odd bars.
This check is important, hence below change list doesn't look agreeable.

> I don't know the answer and will this change break something of bar 1, so my
> current way to handle this issue is:
> 
> PCI: cadence: respect 64bti when size is smaller than 2G
> 
> Original codes force size under 2G to be 32 bit somehow.
> Signed-off-by: Li Chen <lchen@ambarella.com>
> 
> 1 file changed, 2 insertions(+)
> drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 ++
> 
> modified   drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -107,6 +107,8 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc,
> u8 fn, u8 vfn,
>  		if (is_64bits && !(flags &
> PCI_BASE_ADDRESS_MEM_TYPE_64))
>  			epf_bar->flags |=
> PCI_BASE_ADDRESS_MEM_TYPE_64;
> 
> +		is_64bits = epf_bar->flags &
> PCI_BASE_ADDRESS_MEM_TYPE_64;
> +
>  		if (is_64bits && is_prefetch)
>  			ctrl =
> CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
>  		else if (is_prefetch)
> >
> >

Thanks,
Tom

RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in pcie-cadence-ep.c?

[Li Chen]

Hi, Tom

> -----Original Message-----
> From: Tom Joseph [mailto:tjoseph@cadence.com]
> Sent: Tuesday, January 25, 2022 1:28 AM
> To: Li Chen
> Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits
> in pcie-cadence-ep.c?
> 
> Hi Li,
> 
> > -----Original Message-----
> > From: Li Chen <lchen@ambarella.com>
> > Sent: 24 January 2022 02:26
> > To: Tom Joseph <tjoseph@cadence.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> > <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> > <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for is_64bits in
> > pcie-cadence-ep.c?
> >
> > EXTERNAL MAIL
> >
> >
> > Hi, Tom
> >
> > > -----Original Message-----
> > > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > > Sent: Saturday, January 22, 2022 2:09 AM
> > > To: Li Chen
> > > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas; linux-
> > > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> > is_64bits
> > > in pcie-cadence-ep.c?
> > >
> > > Hi Li,
> > >
> > > > -----Original Message-----
> > > > From: Li Chen <lchen@ambarella.com>
> > > > Sent: 21 January 2022 02:56
> > > > To: Tom Joseph <tjoseph@cadence.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring
> > > > <robh@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> > > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> > is_64bits in
> > > > pcie-cadence-ep.c?
> > > >
> > > > EXTERNAL MAIL
> > > >
> > > >
> > > > Hi, Tom
> > > >
> > > > > -----Original Message-----
> > > > > From: Tom Joseph [mailto:tjoseph@cadence.com]
> > > > > Sent: Thursday, January 20, 2022 9:11 PM
> > > > > To: Li Chen
> > > > > Cc: Lorenzo Pieralisi; Rob Herring; Krzysztof Wilczyński; Bjorn Helgaas;
> > linux-
> > > > > pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > Subject: [EXT] RE: Why does cdns_pcie_ep_set_bar use sz > SZ_2G for
> > > > is_64bits
> > > > > in pcie-cadence-ep.c?
> > > > >
> > > > > Hi Li,
> > > > >
> > > > >  For 64_bits ,  all the odd bars (BAR1, 3 ,5) will be disabled ( so as to use
> > as
> > > > upper
> > > > > bits).
> > > >
> > > > Yes, I get it.
> > > > > I see that the code is assuming 32_bits if size < 2G , so all bars could be
> > > > enabled.
> > > > >
> > > >
> > > > Ok, but I still wonder why 2G instead of other sizes like 1G or 512M? IMO
> > if
> > > > there is no obvious limitation, 64 or 32 bit should be left to the user to
> > > > decide(like bar_fixed_64bit and bar_fixed_size in pci_epc_features),
> > instead
> > > > of hardcode 2G here.
> > >
> > > Check for 2G is important as this is the max valid aperture size encoding
> > (0x11000)
> > > allowed by the controller for 32 bit BARs.
> >
> > Thanks, I get it now, and also find it in 7.3.1.50. Physical Function BAR
> > Configuration Register 0 just now.
> > >
> > > >
> > > > > As I understand, you have a use case where you want to set the bar as
> > 64
> > > > bit,
> > > > > actually use small size.
> > > > > Is it possible to describe bit more about this use case (just curious)?
> > > >
> > > > It's because our SoC use 0-64G AMBA address space for our dram(system
> > > > memory), so if I want to use 32 bit bar like 16M bus address, I must
> > reserve
> > > > this 16M area with kernel's reserve-memory, otherwise endpoints like
> > nvme
> > > > will report unsupported request when it do dma and the dma address is
> > also
> > > > located under this 16M area. More details have been put in this thread:
> > > >
> > https://urldefense.com/v3/__https://lore.kernel.org/lkml/CH2PR19MB4024
> > > >
> > 5BF88CF2F7210DCB1AA4A0669@CH2PR19MB4024.namprd19.prod.outlook.c
> > > >
> > om/T/*m0dd09b7e6f868b9692185ec57c1986b3c786e8d3__;Iw!!EHscmS1ygiU
> > > > 1lA!SVgmPO0MrmUUnZzlmc-fCPsSBddkfUgT-
> > > > Y7EmlLAgz9AoQSVZXa_18TIdT6O7kY$
> > > >
> > > >
> > > > So, if I don't want to reserve much memory for BAR, I have to use 64-bit
> > bar,
> > > > and it must be prefetch 64 bit, not non-prefetch in my case, because my
> > > > virtual p2p bridge has three windows: io, mem(32bit), prefetch mem(64
> > bit,
> > > > because CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS is set),
> > and
> > > > if the controller running under ep-mode use 64 non-prefetch, this region
> > will
> > > > fallback to bridge's 32-bit mem window but I don't(and cannot) reserve
> > so
> > > > much 32bit memory for it).
> > > >
> > > Got your point. Does a change in the code as below will be good enough ?
> > >
> > > - bool is_64bits = sz > SZ_2G;
> > > +bool is_64bits = (sz > SZ_2G) ||(!!( flags &
> > > PCI_BASE_ADDRESS_MEM_TYPE_64)) ;
> >
> > Before answering this question, I want to raise another question, why bar 1
> > cannot be 64 bit?
> > 		bool is_prefetch = !!(flags &
> > PCI_BASE_ADDRESS_MEM_PREFETCH);
> > 		bool is_64bits = sz > SZ_2G;
> >
> > 		if (is_64bits && (bar & 1))
> > 			return -EINVAL;
> > I would be very grateful if you can tell me.
> >
> 
> As mentioned earlier,  if the user indicate a 64_bit bar aperture (with the
> encoding) ,
> all the odd bars will be used up. User should then be denied to program odd bars.

Is this decided by cadence PCIe controller's design? I used to see the device that uses bar 1 + bar 2 as paired 64 bit bar, and bar 1 is for type, size, lower bit, bar 2 is for upper 32 bits. 

> The above check is not only for bar 1 , but for all odd bars.

Thanks! I forgot this important thing, get it now. 

> This check is important, hence below change list doesn't look agreeable.

> > I don't know the answer and will this change break something of bar 1, so my
> > current way to handle this issue is:
> >
> > PCI: cadence: respect 64bti when size is smaller than 2G
> >
> > Original codes force size under 2G to be 32 bit somehow.
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> >
> > 1 file changed, 2 insertions(+)
> > drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 ++
> >
> > modified   drivers/pci/controller/cadence/pcie-cadence-ep.c
> > @@ -107,6 +107,8 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc,
> > u8 fn, u8 vfn,
> >  		if (is_64bits && !(flags &
> > PCI_BASE_ADDRESS_MEM_TYPE_64))
> >  			epf_bar->flags |=
> > PCI_BASE_ADDRESS_MEM_TYPE_64;
> >
> > +		is_64bits = epf_bar->flags &
> > PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +
> >  		if (is_64bits && is_prefetch)
> >  			ctrl =
> > CDNS_PCIE_LM_BAR_CFG_CTRL_PREFETCH_MEM_64BITS;
> >  		else if (is_prefetch)
> > >
> > >
> 
> Thanks,
> Tom
> 
> ##############################################################
> ########
> This EXTERNAL email has been scanned by Proofpoint Email Protect service.

Regards,
Li

**********************************************************************
This email and attachments contain Ambarella Proprietary and/or Confidential Information and is intended solely for the use of the individual(s) to whom it is addressed. Any unauthorized review, use, disclosure, distribute, copy, or print is prohibited. If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message. Thank you.