Messages in this thread | | | Date | Thu, 7 Jan 2016 14:33:23 +0800 | From | Jisheng Zhang <> | Subject | Re: [PATCH v5 1/5] PCI: designware: ensure ATU is enabled before IO/conf space accesses |
| |
Dear Bjorn,
On Wed, 6 Jan 2016 12:20:03 -0600 Bjorn Helgaas wrote:
> [+cc Jisheng] > > On Fri, Dec 18, 2015 at 02:38:55PM +0200, Stanimir Varbanov wrote: > > There is no guarantees that enabling ATU will hit the hardware > > immediately, and subsequent accesses to configuration / IO spaces > > are reliable. So fixing this by read back PCIE_ATU_CR2 register > > just after writing. > > > > Without such a fix the PCI device enumeration during kernel boot > > is not reliable, and reading configuration space for particular > > PCI device on the bus returns zero aka no device. > > > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > > --- > > drivers/pci/host/pcie-designware.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > > index 02a7452bdf23..7880de63895d 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -154,6 +154,8 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, > > static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index, > > int type, u64 cpu_addr, u64 pci_addr, u32 size) > > { > > + u32 val; > > + > > dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | index, > > PCIE_ATU_VIEWPORT); > > dw_pcie_writel_rc(pp, lower_32_bits(cpu_addr), PCIE_ATU_LOWER_BASE); > > @@ -164,6 +166,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index, > > dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET); > > dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1); > > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); > > + /* > > + * ensure that the ATU enable has been happaned before accessing > > + * pci configuration/io spaces through dw_pcie_cfg_[read|write]. > > + */ > > + dw_pcie_readl_rc(pp, PCIE_ATU_CR2, &val); > > This particular fix makes sense to me. > > But I have a larger question about how the ATU works. I see these > definitions: > > #define PCIE_ATU_TYPE_MEM > #define PCIE_ATU_TYPE_IO > #define PCIE_ATU_TYPE_CFG0 > #define PCIE_ATU_TYPE_CFG1 > > and these uses: > > - In dw_pcie_host_init(), set PCIE_ATU_TYPE_MEM for unit 1 > (but only if rd_other_conf is not overridden) > > - In dw_pcie_rd_other_conf() and dw_pcie_wr_other_conf(), > set PCIE_ATU_TYPE_CFG0 before config access to own bus; > set PCIE_ATU_TYPE_CFG1 before config access to other bus; > set PCIE_ATU_TYPE_IO after completion > > I'm confused: > > 1) I assume PCIE_ATU_TYPE_MEM is for access to PCI memory space. Why > is that initialization related to rd_other_conf? Shouldn't that be > set up always? A comment here would be nice, to clarify that this is > not related to the subsequent dw_pcie_wr_own_conf() calls.
Indeed, the comment is necessary. I forget the reason until read the code carefully again. The reason here is
If the platform provides ->rd_other_conf, it means the platform doesn't support ATU, it uses its own address translation component rather than ATU, so we should ignore ATU programming for this kind of platform.
I have sent out one patch to add this comment.
> > 2) Why doesn't dw_pcie_host_init() use dw_pcie_rd_conf() instead of > dw_pcie_rd_own_conf()? Using pci_read_config_dword() might be even > better. Using the internal interfaces piecemeal like we do today is > just an opportunity for doing it wrong.
IMHO, the reason is during host_init, the pci bus etc. are not initialized, from another side, the code is really accessing its own conf registers.
> > 3) The definitions and your comment about "accessing PCI > configuration/io spaces" suggest that the ATU must be programmed > differently for accesses to PCI config space vs. I/O space. If that's
Yes.
> the case, we'd need some kind of mutex to protect inl(), etc., during > config accesses. For example, in this scenario: > > thread A thread B > --------------------------------------------- ---------- > pci_read_config_dword() > dw_pcie_prog_outbound_atu(PCIE_ATU_TYPE_CFG0) > inl() > dw_pcie_cfg_read() > readl() > dw_pcie_prog_outbound_atu(PCIE_ATU_TYPE_IO) > inl() > > Do both inl() calls by thread B work correctly, even though the ATU > seems to be programmed for CFG0 for the first but IO for the second? >
Indeed, there's such race issue as you and RMK pointed out.
IMHO, we need support:
1. platforms which contain three or more iATUs, there's no need to mux iATU usage: one ATU for IO, one for cfg and another for MEM. But how to get the number of iATUs, DT? eg. "snps,atu_num = 3"
2. platforms which contain only two iATUs, add mechanism to protect the cfg vs IO race. Could you please give suggestions to how to achieve this goal?
Thanks, Jisheng
| |