[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH v5 15/18] PCI: dwc: Make cpu_addr_fixup take struct dw_pcie as argument
    On Mon, Dec 18, 2017 at 10:15:43PM +0100, Niklas Cassel wrote:
    > On Mon, Dec 18, 2017 at 06:10:58PM +0000, Lorenzo Pieralisi wrote:
    > > On Mon, Nov 20, 2017 at 02:32:18PM +0100, Niklas Cassel wrote:
    > > > There is no need to hard code the cpu to bus address fixup mask.
    > >
    > > There is no need or hardcoding it is broken ? There is a difference
    > > between those two.
    > Well, both :P
    > The current hardcoding for ARTPEC-6: GENMASK(27, 0), is wrong.
    > Hardcoding the correct mask GENMASK(28, 0) would be sufficient
    > for ARTPEC-6.
    > However, the ARTPEC-7 is a 32-bit CPU (without LPAE), but has
    > a 64-bit bus. So the ARTPEC-7 can have more devices than fits
    > in the 32-bit range, therefore a lookup table is placed between
    > the bus and the CPU, so you can choose what devices to map.
    > This mapping, which decides what devices to map, is currently
    > done at boot via devicetree.
    > So, on ARTPEC-7, it is not possible to hardcode a CPU_TO_BUS
    > mask, since the PCIe controller's address is only known via DT.
    > Having a hardcoded value is arguably wrong. It should probably
    > have been a DT property called something like "cpu-bus-fixup-mask".
    > However, this property is not needed since we already have
    > pp->cfg0_base and ep->phys_base, which is derived from already
    > existing DT properties.
    > By using pp->cfg0_base and ep->phys_base, we avoid hardcoding a mask
    > in the driver. This should work for ARTPEC-6, DRA7xx and ARTPEC-7.
    > I have not changed the code in DRA7xx though, since their existing
    > code works, but if they want, they could use the same logic as
    > artpec6_pcie_cpu_addr_fixup, and thus remove their hardcoded mask.
    > >
    > > > The PCIe controller has a global address on the AXI bus, however,
    > > > from the perspective of the PCIe controller, its base starts at 0x0,
    > > > so the local address is 0x0. To get the bus address, simply subtract
    > > > the global address from the cpu address. The global address is taken
    > > > from device tree.
    > >
    > > I do not understand this paragraph, I would kindly ask you and Kishon to
    > > explain please this patch and
    > >
    > > commit a660083eb06c ("PCI: dwc: designware: Add new *ops* for CPU addr
    > > fixup")
    > >
    > > What the cpu_addr_fixup() hook is supposed to do ? And what does the
    > > "addr_space" property represent ?
    > The ARTPEC-6 and DRA7xx SoCs both has an interconnect configured is a way
    > where the local address of the PCIe controller is 0x0.
    > For ARTPEC-6 the global address of the PCIe controller is 0xf8050000,
    > so if the CPU writes to address 0xf8050008, from the perspective of the
    > PCIe controller, it will see a write to address 0x8.
    > This is normally not a problem, but when reading/writing from an endpoint,
    > we go via the outbound iATU. The outbound iATU has to be programmed with
    > a base address (reads/writes in the range [base address + limit] will be
    > subject to translation).
    > However, since the address the PCIe controller sees, in reality is
    > "0xf8050000 + the address the PCIe controller sees", we need to subtract
    > the global address before programming the base address in the outbound
    > iATU.
    > Kishon explains the same thing in commit f4c55c5a3f7f68c0.
    > For this patch, I tried make the commit message understandable,
    > without going into too much detail, but there is probably still
    > room for improvement.
    > >
    > > > Also for ARTPEC-7, hard coding the cpu to bus address fixup mask is
    > > > not possible, since it uses a High Address Bits Look Up Table, which
    > > > means that it can, at runtime, map the PCIe window to an arbitrary
    > > > address in the 32-bit address space.
    > >
    > > But you are not changing the ARTPEC-7 mechanism, are you ? I do not
    > > understand this paragraph - I see no change for ARTPEC-7 in this patch.
    > Hopefully this is clarified by the comments above.
    > >
    > > > This also fixes a bug for ARTPEC-6, where the cpu to bus address
    > > > fixup mask previously was off by one (GENMASK(27, 0), rather than
    > > > GENMASK(28, 0)). This is another reason to calculate the mask by
    > > > using values from device tree.
    > >
    > > What this patch does (and how the cpu_addr_fixup() mechanism works)
    > > is not clear to me, please explain, we can rewrite the commit log
    > > with a clear explanation then.
    > Hopefully this is clarified by the comments above.

    It is, thank you - it would be good to convert this thread into commit
    log format, you can send it as an update to this patch.


     \ /
      Last update: 2017-12-19 11:48    [W:3.111 / U:32.772 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site