lkml.org 
[lkml]   [2016]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2016-01-07 08:21    [W:3.279 / U:0.904 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site