Re: [PATCH v5 1/5] PCI: designware: ensure ATU is enabled before IO/conf space accesses

[Jisheng Zhang]

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@xxxxxxxxxx>
> > ---
> >  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/