On Tue, Nov 19, 2024 at 10:44:38AM +0100, Niklas Cassel wrote: > Hello DWC PCIe endpoint developers, > > > As I wrote in patch [1] (please review): > 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." > > I.e. only the upper bits are writable, and the number of writable bits is > dependent on the configured BAR_MASK. > > While patch [1] is a nice fail-safe that we definitely want to have... > I can see that the DWC PCIe EP driver is currently broken (and has been > since the beginning). > > We are programming the iATU _before_ configuring the BAR: > https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L232-L247 > > The problem is of course that the iATU registers depend on the BAR mask > (the number of read-only bits depends on the currently set BAR mask), > so the iATU registers should be done _after_ configuring the BAR. > > Doing it in this was makes a lot of sense. > First you configure the BAR, then you configure the address translation > for that BAR. (Since the iATU in BAR match mode obviously depends on how > the BAR is configured.) > > > Now, I would like to send a patch to change this order, so that we actually > write things in the only order that makes sense, my problem is this line: > https://github.com/torvalds/linux/blob/v6.12/drivers/pci/controller/dwc/pcie-designware-ep.c#L236-L237 > > This code makes no sense to me whatsoever. > > If I look at the commit that introduced this early return: > 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address") > > It is not signed off by any of the regular PCI maintainers, neither does it > have any Acked-by by any of the regular PCI maintainers, so I kind of wonder > why this patch is there is the first place... > (Taking something via a different tree is fine, but that would still require > an Acked-by by one of the maintainers for the tree which owns that file.) > > While I am tempted to simply send a revert for this commit, so that we can > write the registers in the correct order (iATU after BAR mask), I still do > not want to regress the NTB EPF driver (even if the commit appears to have > bypassed the regular PCI maintainers). > > Frank, since you are the author of: > 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address") > > Please advice on what to do here. The only thing that makes sense to me is > that dw_pcie_ep_set_bar() always writes the BAR MASK. If dw_pcie_ep_set_bar() > does NOT write the BAR MASK, we depend on whatever BAR MASK was there before > dw_pcie_ep_set_bar(), which no matter how I look at it, seems horribly wrong. dw_pcie_ep_set_bar() don't change BAR MASK because host side also use it to allocate resource when probe. But it just change iATU map address, for example 1M BAR1, map to EP's side 0x1000_0000, it may change to another address 0x2000_0000. In doorbell MSI case, you tested, it dynamtic change to ITS address, then change back when do normal bar test, which easiest way to understand the behavior. I think the order write iATU and mask doesn't matter because hardware should just ignores some bits according to mask register. If there are another APIs, which change inbound map address, should be better. Maybe off topic, I think if support combine some segmement address to one bar should be wonderful, such as combine MSI ITS address and other register to BAR0. It is not worth to waste one bar, which just for doorbell. Frank > > > Kind regards, > Niklas > > [1] https://lore.kernel.org/linux-pci/20241116163558.2606874-10-cassel@xxxxxxxxxx/
