Re: [PATCH v3 04/11] PCI: brcmstb: Expand inbound size calculation helper

[Bjorn Helgaas <helgaas@kernel.org>]

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