| Message ID | 20241014130710.413-5-svarbanov@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add PCIe support for bcm2712 | expand |
On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: > BCM2712 memory map can supports up to 64GB of system > memory, thus expand the inbound size calculation in > helper function up to 64GB. The fact that the calculation is done in a helper isn't important here. Can you make the subject line say something about supporting DMA for up to 64GB of system memory? This is being done specifically for BCM2712, but I assume it's safe for *all* brcmstb devices, right? s/can supports/can support/ Rewrap commit log to fill 75 columns. > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > --- > v2 -> v3: > - Added Reviewed-by tags. > - Improved patch description (Florian). > > drivers/pci/controller/pcie-brcmstb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 9321280f6edb..b0ef2f31914d 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -309,8 +309,8 @@ static int brcm_pcie_encode_ibar_size(u64 size) > if (log2_in >= 12 && log2_in <= 15) > /* Covers 4KB to 32KB (inclusive) */ > return (log2_in - 12) + 0x1c; > - else if (log2_in >= 16 && log2_in <= 35) > - /* Covers 64KB to 32GB, (inclusive) */ > + else if (log2_in >= 16 && log2_in <= 36) > + /* Covers 64KB to 64GB, (inclusive) */ > return log2_in - 15; > /* Something is awry so disable */ > return 0; > -- > 2.43.0 >
On 10/14/24 09:57, Bjorn Helgaas wrote: > On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: >> BCM2712 memory map can supports up to 64GB of system >> memory, thus expand the inbound size calculation in >> helper function up to 64GB. > > The fact that the calculation is done in a helper isn't important > here. Can you make the subject line say something about supporting > DMA for up to 64GB of system memory? > > This is being done specifically for BCM2712, but I assume it's safe > for *all* brcmstb devices, right? It is safe in the sense that all brcmstb devices with this PCIe controller will adopt the same encoding of the size, all of the currently supported brcmstb devices have a variety of limitations when it comes to the amount of addressable DRAM however. Typically we have a hard limit at 4GB of DRAM per memory controller, some devices can do 2GB x3, 4GB x2, or 4GB x1. Does that answer your question?
On Mon, Oct 14, 2024 at 10:10:11AM -0700, Florian Fainelli wrote: > On 10/14/24 09:57, Bjorn Helgaas wrote: > > On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: > > > BCM2712 memory map can supports up to 64GB of system > > > memory, thus expand the inbound size calculation in > > > helper function up to 64GB. > > > > The fact that the calculation is done in a helper isn't important > > here. Can you make the subject line say something about supporting > > DMA for up to 64GB of system memory? > > > > This is being done specifically for BCM2712, but I assume it's safe > > for *all* brcmstb devices, right? > > It is safe in the sense that all brcmstb devices with this PCIe controller > will adopt the same encoding of the size, all of the currently supported > brcmstb devices have a variety of limitations when it comes to the amount of > addressable DRAM however. Typically we have a hard limit at 4GB of DRAM per > memory controller, some devices can do 2GB x3, 4GB x2, or 4GB x1. > > Does that answer your question? I'd like something in the commit log to the effect that while we're doing this to support more system memory on BCM2712, this change is safe for other SoCs that don't support as much system memory.
On Mon, Oct 14, 2024 at 1:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Oct 14, 2024 at 10:10:11AM -0700, Florian Fainelli wrote: > > On 10/14/24 09:57, Bjorn Helgaas wrote: > > > On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: > > > > BCM2712 memory map can supports up to 64GB of system > > > > memory, thus expand the inbound size calculation in > > > > helper function up to 64GB. > > > > > > The fact that the calculation is done in a helper isn't important > > > here. Can you make the subject line say something about supporting > > > DMA for up to 64GB of system memory? > > > > > > This is being done specifically for BCM2712, but I assume it's safe > > > for *all* brcmstb devices, right? > > > > It is safe in the sense that all brcmstb devices with this PCIe controller > > will adopt the same encoding of the size, all of the currently supported > > brcmstb devices have a variety of limitations when it comes to the amount of > > addressable DRAM however. Typically we have a hard limit at 4GB of DRAM per > > memory controller, some devices can do 2GB x3, 4GB x2, or 4GB x1. > > > > Does that answer your question? > > I'd like something in the commit log to the effect that while we're > doing this to support more system memory on BCM2712, this change is > safe for other SoCs that don't support as much system memory. Hello, This setting configures the size of an RC's inbound window to system memory. Any inbound access outside of all of the inbound windows will be discarded. Some existing SoCs cannot support the 64GB size. Configuring such an SoC to 64GB will effectively disable the entire window. Regards, Jim Quinlan Broadcom STB/CM
On Wed, Oct 16, 2024 at 01:09:00PM -0400, Jim Quinlan wrote: > On Mon, Oct 14, 2024 at 1:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, Oct 14, 2024 at 10:10:11AM -0700, Florian Fainelli wrote: > > > On 10/14/24 09:57, Bjorn Helgaas wrote: > > > > On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: > > > > > BCM2712 memory map can supports up to 64GB of system > > > > > memory, thus expand the inbound size calculation in > > > > > helper function up to 64GB. > > > > > > > > The fact that the calculation is done in a helper isn't important > > > > here. Can you make the subject line say something about supporting > > > > DMA for up to 64GB of system memory? > > > > > > > > This is being done specifically for BCM2712, but I assume it's safe > > > > for *all* brcmstb devices, right? > > > > > > It is safe in the sense that all brcmstb devices with this PCIe > > > controller will adopt the same encoding of the size, all of the > > > currently supported brcmstb devices have a variety of > > > limitations when it comes to the amount of addressable DRAM > > > however. Typically we have a hard limit at 4GB of DRAM per > > > memory controller, some devices can do 2GB x3, 4GB x2, or 4GB > > > x1. > > > > > > Does that answer your question? > > > > I'd like something in the commit log to the effect that while > > we're doing this to support more system memory on BCM2712, this > > change is safe for other SoCs that don't support as much system > > memory. > > This setting configures the size of an RC's inbound window to system > memory. Any inbound access outside of all of the inbound windows > will be discarded. > > Some existing SoCs cannot support the 64GB size. Configuring such > an SoC to 64GB will effectively disable the entire window. So I *think* you're saying that this patch will break existing SoCs that don't support the 64GB size, right? Bjorn
Hi Bjorn, On 10/16/24 22:38, Bjorn Helgaas wrote: > On Wed, Oct 16, 2024 at 01:09:00PM -0400, Jim Quinlan wrote: >> On Mon, Oct 14, 2024 at 1:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote: >>> On Mon, Oct 14, 2024 at 10:10:11AM -0700, Florian Fainelli wrote: >>>> On 10/14/24 09:57, Bjorn Helgaas wrote: >>>>> On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: >>>>>> BCM2712 memory map can supports up to 64GB of system >>>>>> memory, thus expand the inbound size calculation in >>>>>> helper function up to 64GB. >>>>> >>>>> The fact that the calculation is done in a helper isn't important >>>>> here. Can you make the subject line say something about supporting >>>>> DMA for up to 64GB of system memory? >>>>> >>>>> This is being done specifically for BCM2712, but I assume it's safe >>>>> for *all* brcmstb devices, right? >>>> >>>> It is safe in the sense that all brcmstb devices with this PCIe >>>> controller will adopt the same encoding of the size, all of the >>>> currently supported brcmstb devices have a variety of >>>> limitations when it comes to the amount of addressable DRAM >>>> however. Typically we have a hard limit at 4GB of DRAM per >>>> memory controller, some devices can do 2GB x3, 4GB x2, or 4GB >>>> x1. >>>> >>>> Does that answer your question? >>> >>> I'd like something in the commit log to the effect that while >>> we're doing this to support more system memory on BCM2712, this >>> change is safe for other SoCs that don't support as much system >>> memory. >> >> This setting configures the size of an RC's inbound window to system >> memory. Any inbound access outside of all of the inbound windows >> will be discarded. >> >> Some existing SoCs cannot support the 64GB size. Configuring such >> an SoC to 64GB will effectively disable the entire window. > > So I *think* you're saying that this patch will break existing SoCs > that don't support the 64GB size, right? Existing SoCs will not be impacted. It could be theoretically possible to break inbound window translations only if you wrongly populate window sizes in DT. ~Stan
On Thu, Oct 17, 2024 at 11:02:33AM +0300, Stanimir Varbanov wrote: > On 10/16/24 22:38, Bjorn Helgaas wrote: > > On Wed, Oct 16, 2024 at 01:09:00PM -0400, Jim Quinlan wrote: > >> On Mon, Oct 14, 2024 at 1:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > >>> On Mon, Oct 14, 2024 at 10:10:11AM -0700, Florian Fainelli wrote: > >>>> On 10/14/24 09:57, Bjorn Helgaas wrote: > >>>>> On Mon, Oct 14, 2024 at 04:07:03PM +0300, Stanimir Varbanov wrote: > >>>>>> BCM2712 memory map can supports up to 64GB of system > >>>>>> memory, thus expand the inbound size calculation in > >>>>>> helper function up to 64GB. > >>>>> > >>>>> The fact that the calculation is done in a helper isn't important > >>>>> here. Can you make the subject line say something about supporting > >>>>> DMA for up to 64GB of system memory? > >>>>> > >>>>> This is being done specifically for BCM2712, but I assume it's safe > >>>>> for *all* brcmstb devices, right? > >>>> > >>>> It is safe in the sense that all brcmstb devices with this PCIe > >>>> controller will adopt the same encoding of the size, all of the > >>>> currently supported brcmstb devices have a variety of > >>>> limitations when it comes to the amount of addressable DRAM > >>>> however. Typically we have a hard limit at 4GB of DRAM per > >>>> memory controller, some devices can do 2GB x3, 4GB x2, or 4GB > >>>> x1. > >>>> > >>>> Does that answer your question? > >>> > >>> I'd like something in the commit log to the effect that while > >>> we're doing this to support more system memory on BCM2712, this > >>> change is safe for other SoCs that don't support as much system > >>> memory. > >> > >> This setting configures the size of an RC's inbound window to system > >> memory. Any inbound access outside of all of the inbound windows > >> will be discarded. > >> > >> Some existing SoCs cannot support the 64GB size. Configuring such > >> an SoC to 64GB will effectively disable the entire window. > > > > So I *think* you're saying that this patch will break existing SoCs > > that don't support the 64GB size, right? > > Existing SoCs will not be impacted. It could be theoretically possible > to break inbound window translations only if you wrongly populate window > sizes in DT. I guess this is the part that I missed -- the inbound window sizes come from DT (via bridge->dma_ranges, IIUC), and the patch merely supports encoding of larger windows than previously. Bjorn
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 9321280f6edb..b0ef2f31914d 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -309,8 +309,8 @@ static int brcm_pcie_encode_ibar_size(u64 size) if (log2_in >= 12 && log2_in <= 15) /* Covers 4KB to 32KB (inclusive) */ return (log2_in - 12) + 0x1c; - else if (log2_in >= 16 && log2_in <= 35) - /* Covers 64KB to 32GB, (inclusive) */ + else if (log2_in >= 16 && log2_in <= 36) + /* Covers 64KB to 64GB, (inclusive) */ return log2_in - 15; /* Something is awry so disable */ return 0;