Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
> Hi Leon,
>
> Thanks for your reply.
>
> 在 2021/1/25 13:48, Leon Romanovsky 写道:
> > On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> > > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> > > is Rockchip designed IP which is only used for RK3399. So all the following
> > > non-RK3399 SoCs should use this driver.
> > >
> > > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > ---
> > >   drivers/pci/controller/dwc/Kconfig            |   9 +
> > >   drivers/pci/controller/dwc/Makefile           |   1 +
> > >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
> > >   3 files changed, 296 insertions(+)
> > >   create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 22c5529e9a65..aee408fe9283 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
> > >   	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> > >   	  endpoint mode. This uses the DesignWare core.
> > >
> > > +config PCIE_ROCKCHIP_DW_HOST
> > > +	bool "Rockchip DesignWare PCIe controller"
> > > +	select PCIE_DW
> > > +	select PCIE_DW_HOST
> > > +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > +	depends on OF
> > > +	help
> > > +	  Enables support for the DW PCIe controller in the Rockchip SoC.
> > > +
> > >   config PCIE_INTEL_GW
> > >   	bool "Intel Gateway PCIe host controller support"
> > >   	depends on OF && (X86 || COMPILE_TEST)
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index a751553fa0db..30eef8e9ee8a 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > >   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > >   obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > >   obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > > +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> > >   obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > >   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> > >   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > new file mode 100644
> > > index 000000000000..07f6d1cd5853
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > @@ -0,0 +1,286 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PCIe host controller driver for Rockchip SoCs
> > > + *
> > > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> > > + *		http://www.rock-chips.com
> > > + *
> > > + * Author: Simon Xue <xxm@rock-chips.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/phy/phy.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#include "pcie-designware.h"
> > > +
> > > +/*
> > > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> > > + * mask for the lower 16 bits.  This allows atomic updates
> > > + * of the register without locking.
> > > + */
> > This is correct only for the variables that naturally aligned, I imagine
> > that this is the case here and in the Linux, but better do not write comments
> > in the code that are not accurate.
>
> Ok, will remove.
> I wonder what it would be when outside the Linux? Could you share some information?

The C standard says nothing about atomicity, integer assignment maybe atomic,
maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
by definition.

The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
here (Linux) it is ok and you are not alone in expecting lockless write.

Thanks

>
> > Thanks
> >
> >
> >
>
>