Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5016 bytes --]

On Wed, May 20, 2020 at 12:06:40PM +0100, Lorenzo Pieralisi wrote:
> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> 
> [...]
> 
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > >                      pp->mem = win->res;
> > > > > > >                      pp->mem->name = "MEM";
> > > > > > >                      mem_size = resource_size(pp->mem);
> > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > >                      pp->mem_size = mem_size;
> > > > > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > 
> > > > > That warning was added for a reason - why should not we log legitimate
> > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > behaviour given the current ATU programming API.
> > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > non-prefetchable window as one of the ATU outbound translation
> > > > channels is being used,
> > > 
> > > Is it true for all DWC host controllers ? Or there may be another
> > > exception whereby we would be forced to disable this warning altogether
> > > ?
> > > 
> > > > but, we are not employing any ATU outbound translation channel for
> > > 
> > > What does this mean ? "we are not employing any ATU outbound...", is
> > > this the tegra driver ? And what guarantees that this warning is not
> > > legitimate on DWC host controllers that do use the ATU outbound
> > > translation for prefetchable windows ?
> > > 
> > > Can DWC maintainers chime in and clarify please ?
> > 
> > Before this code section, there is the following function call 
> > pci_parse_request_of_pci_ranges(), which performs a simple validation for 
> > the IORESOURCE_MEM resource type.
> > This validation checks if the resource is marked as prefetchable, if so, 
> > an error message "non-prefetchable memory resource required" is given and 
> > a return code with the -EINVAL value.
> 
> That code checks if there is *at least* a non-prefetchable resource,
> that's all it does.
> 
> > In other words, to reach the code that Vidya is changing, it can be only 
> > if the resource is a non-prefetchable, any prefetchable resource will be 
> > blocked by the previous call, if I'm not mistaken.
> 
> I think you are mistaken sorry.
> 
> > Having this in mind, Vidya's change will not make the expected result 
> > aimed by him.
> 
> I think Vidya's patch does what he expects, the question is whether
> it is widely applicable to ALL DWC hosts, that's what I want to know.
> 
> > I don't see any problem by having resources larger than 4GB, from what 
> > I'm seeing in the databook there isn't any restricting related to that as 
> > long they don't consume the maximum space that is addressable by the 
> > system (depending on if they are 32-bit or 64-bit system address).
> > 
> > To be honest, I'm not seeing a system that could have this resource 
> > larger than 4GB, but it might exist some exception that I don't know of, 
> > that's why I accepted Alan's patch to warn the user that the resource 
> > exceeds the maximum for the 32 bits so that he can be aware that he 
> > *might* be consuming the maximum space addressable.
> 
> I think it is most certainly a possibility to have > 4GB prefetchable
> address spaces so we ought to fix this for good. I still have to
> understand how the DWC host detects the memory region to be programmed
> into the ATU given that there is more than one but only 1 ATU memory
> region AFAICS.

Probably best to wait for Vidya to confirm since I'm not altogether
familiar with PCI on Tegra194, but looking at the DTS files and the
Tegra194 TRM, the prefetchable memory regions are set to a range in
0x1200000000-0x1fffffffff which is a region of the address map that
is reserved for "PCIe aperture for > 32-bit OS". Part of that is in
use for non-prefetchable memory (and ends up being programmed into
the ATU) whereas a much larger part is used for prefetchable memory
and is not programmed anywhere, as far as I can tell.

But I think given that this is a designated region of the address
map this is probably automatically redirected to the PCIe controller.
What I don't know is if that's something Tegra-specific or whether all
instantiations have something similar set up.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]