Messages in this thread | | | Subject | Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB | From | Vidya Sagar <> | Date | Wed, 20 May 2020 23:21:50 +0530 |
| |
On 20-May-20 6:46 PM, Thierry Reding wrote: > 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.Yes. That is true. In case of Tegra194, for 1-to-1 memory translations, there is no need to use any ATU regions at all as the HW is capable of generating a memory transaction on the bus on its own for any CPU generated reads/writes falling in these apertures and are not captured by ATU windows for generating other (config/IO) types of transactions. Since a part of 64-bit region is used for mapping non-prefetchable BARs of endpoints (which are only 32-bit in size), a translation is required from 64-bit CPU/AXI address to 32-bit PCIe bus address and ATU region is used precisely for this purpose. Since there is no need for ATU translation to do 1-to-1 memory mapping, rest of the 64-bit aperture is used for mapping prefetchable BARs. I'm not sure if this HW behavior is same across DWC implementations. I took a quick look at the 'ranges' property of other DWC implementations and none of them have a region marked as prefetchable...! I'm not sure how kernel handles mapping an endpoint's prefetchable BAR in this case (does it use the 'non-prefetchable' mapping of 'ranges' property for 'prefetchable' BARs as well??)
> > 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 >
| |