Hello Bjorn, On Wed, Dec 04, 2024 at 11:33:52AM -0600, Bjorn Helgaas wrote: > In subject, maybe "Write BAR_MASK before iATU registers" Ok. Will fix in v6. > > I guess writing BAR_MASK is really configuring the *size* of the BAR? I am quite sure that you know how host software determines the size of a BAR :) But yes, writing the BAR_MASK will directly decide a BARs size: https://wiki.osdev.org/PCI#Address_and_size_of_the_BAR So BAR_MASK is "BAR size - 1". > Maybe the size is the important semantic connection with iATU config? The connection is: "Field size depends on log2(BAR_MASK+1) in BAR match mode." So I think it makes sense for the subject to include BAR_MASK rather than BAR size. > > On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote: > > The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields > > in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that: > > "Field size depends on log2(BAR_MASK+1) in BAR match mode." > > Can we include a databook revision and section here to help future > maintainers? Ok. Will fix in v6. > > > I.e. only the upper bits are writable, and the number of writable bits is > > dependent on the configured BAR_MASK. > > > > If we do not write the BAR_MASK before writing the iATU registers, we are > > relying the reset value of the BAR_MASK being larger than the requested > > size of the first set_bar() call. The reset value of the BAR_MASK is SoC > > dependent. > > > > Thus, if the first set_bar() call requests a size that is larger than the > > reset value of the BAR_MASK, the iATU will try to write to read-only bits, > > which will cause the iATU to end up redirecting to a physical address that > > is different from the address that was intended. > > > > Thus, we should always write the iATU registers after writing the BAR_MASK. > > Apparently we write BAR_MASK and the iATU registers in the wrong > order? I assume dw_pcie_ep_inbound_atu() writes the iATU registers. Yes. > > I can't quite connect the commit log with the code change. I assume > the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update > BAR_MASK? dw_pcie_ep_writel_dbi2() writes the BAR_MASK. dw_pcie_ep_writel_dbi() writes the BAR type. > > And I guess the problem is that the previous code does: > > dw_pcie_ep_inbound_atu # iATU > dw_pcie_ep_writel_dbi2 # BAR_MASK (?) > dw_pcie_ep_writel_dbi > > and the new code basically does this: > > if (ep->epf_bar[bar]) { > dw_pcie_ep_writel_dbi2 # BAR_MASK (?) > dw_pcie_ep_writel_dbi > } > dw_pcie_ep_inbound_atu # iATU > ep->epf_bar[bar] = epf_bar > > so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK > before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the > BAR_MASK update? The problem is as described in the commit message: "If we do not write the BAR_MASK before writing the iATU registers, we are relying the reset value of the BAR_MASK being larger than the requested size of the first set_bar() call. The reset value of the BAR_MASK is SoC dependent." Before: dw_pcie_ep_inbound_atu() # iATU - the writable bits in this write depends on # BAR_MASK, which has not been written yet, thus the # write will be at the mercy of the reset value of # BAR_MASK, which is SoC dependent. dw_pcie_ep_writel_dbi2() # BAR_MASK dw_pcie_ep_writel_dbi() # BAR type After: dw_pcie_ep_writel_dbi2() # BAR_MASK dw_pcie_ep_writel_dbi() # BAR type dw_pcie_ep_inbound_atu() # iATU - this write is now done after BAR_MASK has # been written, so we know that all the bits in this # write is writable. Kind regards, Niklas
