Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1283046 > unrolled thread

[PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-03 14:40 +0100
    Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling  region Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-08 10:10 +0100
      Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region Pratyush Anand <pratyush.anand@gmail.com> - 2015-12-09 05:50 +0100
        Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region Arnd Bergmann <arnd@arndb.de> - 2015-12-09 11:00 +0100
          Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling  region Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-09 11:30 +0100
        Re: [PATCH v4 1/5] PCI: designware: add memory barrier after  enabling region Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-12-09 11:30 +0100
          Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region Pratyush Anand <pratyush.anand@gmail.com> - 2015-12-11 05:10 +0100
            Re: [PATCH v4 1/5] PCI: designware: add memory barrier after  enabling region Jisheng Zhang <jszhang@marvell.com> - 2015-12-11 07:00 +0100

#1283046 — [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

Add 'write memory' barrier after enable region in PCIE_ATU_CR2
register. The barrier is needed to ensure that the region enable
request has been reached it's destination at time when we
read/write to PCI configuration space.

Without this barrier 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 |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 02a7452bdf23..ed4dc2e2553b 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -164,6 +164,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].
+	 */
+	wmb();
 }
 
 static struct irq_chip dw_msi_irq_chip = {
-- 
1.7.9.5

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

[toc] | [next] | [standalone]

#1286335 — Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> register. The barrier is needed to ensure that the region enable
> request has been reached it's destination at time when we
> read/write to PCI configuration space.
> 
> Without this barrier 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.

Anand, Jingoo, what is your opinion?

> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/pci/host/pcie-designware.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 02a7452bdf23..ed4dc2e2553b 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -164,6 +164,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].
> +	 */
> +	wmb();
>  }
>  
>  static struct irq_chip dw_msi_irq_chip = {
> 


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

[toc] | [prev] | [next] | [standalone]

#1287103

On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> > Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> > register. The barrier is needed to ensure that the region enable
> > request has been reached it's destination at time when we
> > read/write to PCI configuration space.
> >
> > Without this barrier 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.
>
> Anand, Jingoo, what is your opinion?
>
> >
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > ---
> >  drivers/pci/host/pcie-designware.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 02a7452bdf23..ed4dc2e2553b 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -164,6 +164,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].
> > +      */
> > +     wmb();
> >  }
> >


My understnading is that since writel() of dw_pcie_writel_rc() in
above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
will follow) goes through same device (ie PCIe host here). So, it is
guaranteed that 1st writel() will be executed before later
readl()/writel(). If that is true then we do not need any explicit
barrier here.

Arnd, Russel: whats your opinion here.

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

[toc] | [prev] | [next] | [standalone]

#1287307

On Wednesday 09 December 2015 10:10:05 Pratyush Anand wrote:
> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  drivers/pci/host/pcie-designware.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 02a7452bdf23..ed4dc2e2553b 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -164,6 +164,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].
> > > +      */
> > > +     wmb();
> > >  }
> > >
> 
> 
> My understnading is that since writel() of dw_pcie_writel_rc() in
> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> will follow) goes through same device (ie PCIe host here). So, it is
> guaranteed that 1st writel() will be executed before later
> readl()/writel(). If that is true then we do not need any explicit
> barrier here.
> 
> Arnd, Russel: whats your opinion here.

I think the ordering is only enforced if the two register accesses are
on the same device as seen from the bus, and it's possible that the
RC registers and the config space registers are not considered the
same thing here.

For config write, this is not a problem, because the config space write
has a wmb() that enforces ordering, but it's possible that the config
space read may hit the device in parallel with the PCIE_ATU_ENABLE
write.

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

[toc] | [prev] | [next] | [standalone]

#1287374 — Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

On 12/09/2015 11:52 AM, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 10:10:05 Pratyush Anand wrote:
>> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  drivers/pci/host/pcie-designware.c |    5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>>> index 02a7452bdf23..ed4dc2e2553b 100644
>>>> --- a/drivers/pci/host/pcie-designware.c
>>>> +++ b/drivers/pci/host/pcie-designware.c
>>>> @@ -164,6 +164,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].
>>>> +      */
>>>> +     wmb();
>>>>  }
>>>>
>>
>>
>> My understnading is that since writel() of dw_pcie_writel_rc() in
>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>> will follow) goes through same device (ie PCIe host here). So, it is
>> guaranteed that 1st writel() will be executed before later
>> readl()/writel(). If that is true then we do not need any explicit
>> barrier here.
>>
>> Arnd, Russel: whats your opinion here.
> 
> I think the ordering is only enforced if the two register accesses are
> on the same device as seen from the bus, and it's possible that the
> RC registers and the config space registers are not considered the
> same thing here.
> 
> For config write, this is not a problem, because the config space write
> has a wmb() that enforces ordering, but it's possible that the config
> space read may hit the device in parallel with the PCIE_ATU_ENABLE
> write.

Hmm, just a matter of fact - as I described in the patch description
this wmb() fixed an issue with pcie device enumeration (I came down to
pci_bus_read_dev_vendor_id() returns zero) i.e. exactly a pci
configuration space read.

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

[toc] | [prev] | [next] | [standalone]

#1287360 — Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

On Wed, Dec 09, 2015 at 10:10:05AM +0530, Pratyush Anand wrote:
> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
> >
> > On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> > > Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> > > register. The barrier is needed to ensure that the region enable
> > > request has been reached it's destination at time when we
> > > read/write to PCI configuration space.
> > >
> > > Without this barrier 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.
> >
> > Anand, Jingoo, what is your opinion?
> >
> > >
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  drivers/pci/host/pcie-designware.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 02a7452bdf23..ed4dc2e2553b 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -164,6 +164,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].
> > > +      */
> > > +     wmb();
> > >  }
> > >
> 
> 
> My understnading is that since writel() of dw_pcie_writel_rc() in
> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> will follow) goes through same device (ie PCIe host here). So, it is
> guaranteed that 1st writel() will be executed before later
> readl()/writel(). If that is true then we do not need any explicit
> barrier here.
> 
> Arnd, Russel: whats your opinion here.
              ^l

writel() has a barrier _before_ the access but not after.

The fact is that there's nothing which guarantees that the write will hit
the hardware in a timely manner (forget any rules about PCI config space,
the PCI ordering rules apply to the PCI bus, not to the ARM buses.)

If you need this write to have hit the hardware before continuing, you
need to read back from the same register.

I'm just looking at this driver, trying to decipher what it's doing.  It
_looks_ to me like it's reprogramming one of the outbound windows (IO?)
so that configuration space can be accessed.  Doesn't this have the
effect of disabling access to the IO segment of the PCI bus from the
host CPU?

What protections are there against other CPUs in the system issuing a
PCI I/O read/write while this outbound window is programmed as
configuration space?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1289122

On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

[...]

>> > >       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].
>> > > +      */
>> > > +     wmb();
>> > >  }
>> > >
>>
>>
>> My understnading is that since writel() of dw_pcie_writel_rc() in
>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>> will follow) goes through same device (ie PCIe host here). So, it is
>> guaranteed that 1st writel() will be executed before later
>> readl()/writel(). If that is true then we do not need any explicit
>> barrier here.
>>
>> Arnd, Russel: whats your opinion here.
>               ^l

Sorry :(

>
> writel() has a barrier _before_ the access but not after.
>
> The fact is that there's nothing which guarantees that the write will hit
> the hardware in a timely manner (forget any rules about PCI config space,
> the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
>
> If you need this write to have hit the hardware before continuing, you
> need to read back from the same register.

OK, so better to replace wmb() with read back of control register.

>
> I'm just looking at this driver, trying to decipher what it's doing.  It
> _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> so that configuration space can be accessed.  Doesn't this have the
> effect of disabling access to the IO segment of the PCI bus from the
> host CPU?
>
> What protections are there against other CPUs in the system issuing a
> PCI I/O read/write while this outbound window is programmed as
> configuration space?


Yes, that is an issue with this driver. Most of the host controller
has 4 or more viewpoints, and it is very easy to handle for them. But
there are few which has only two viewpoints. Do not know how to solve
it, so that it works for all.

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

[toc] | [prev] | [next] | [standalone]

#1289231 — Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

On Fri, 11 Dec 2015 09:35:10 +0530 Pratyush Anand wrote:

> On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux wrote:
> 
> [...]
> 
> >> > >       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].
> >> > > +      */
> >> > > +     wmb();
> >> > >  }
> >> > >  
> >>
> >>
> >> My understnading is that since writel() of dw_pcie_writel_rc() in
> >> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> >> will follow) goes through same device (ie PCIe host here). So, it is
> >> guaranteed that 1st writel() will be executed before later
> >> readl()/writel(). If that is true then we do not need any explicit
> >> barrier here.
> >>
> >> Arnd, Russel: whats your opinion here.  
> >               ^l  
> 
> Sorry :(
> 
> >
> > writel() has a barrier _before_ the access but not after.
> >
> > The fact is that there's nothing which guarantees that the write will hit
> > the hardware in a timely manner (forget any rules about PCI config space,
> > the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
> >
> > If you need this write to have hit the hardware before continuing, you
> > need to read back from the same register.  
> 
> OK, so better to replace wmb() with read back of control register.
> 
> >
> > I'm just looking at this driver, trying to decipher what it's doing.  It
> > _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> > so that configuration space can be accessed.  Doesn't this have the
> > effect of disabling access to the IO segment of the PCI bus from the
> > host CPU?
> >
> > What protections are there against other CPUs in the system issuing a
> > PCI I/O read/write while this outbound window is programmed as
> > configuration space?  
> 
> 
> Yes, that is an issue with this driver. Most of the host controller
> has 4 or more viewpoints, and it is very easy to handle for them. But
> there are few which has only two viewpoints. Do not know how to solve
> it, so that it works for all.
> 

The default outbound iATU number is two, this may be the reason why the driver
is written in current style. And two outbound iATUs may be common for pcie dw
users because ASIC people just follow the default configuration ;).

In our case, Marvell Berlin SoCs have two outbound iATUs.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web