linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <fancer.lancer@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: "Serge Semin" <Sergey.Semin@baikalelectronics.ru>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rahul Tanwar" <rtanwar@maxlinear.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Jonathan Hunter" <jonathanh@nvidia.com>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH 04/16] PCI: dwc: Convert to using native IP-core versions representation
Date: Sun, 17 Apr 2022 17:42:28 +0300	[thread overview]
Message-ID: <20220417144228.pqufj4li45qj4vyq@mobilestation> (raw)
In-Reply-To: <YkMfFrKzz5eHPMrJ@robh.at.kernel.org>

On Tue, Mar 29, 2022 at 10:00:38AM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:22AM +0300, Serge Semin wrote:
> > Since DWC PCIe v4.70a the controller version can be read from the
> > PORT_LOGIC.PCIE_VERSION_OFF register. Version is represented in the FourCC
> > format [1]. It's standard versioning approach for the Synopsys DWC
> > IP-cores. Moreover some of the DWC kernel drivers already make use of it
> > to fixup version-dependent functionality (See DWC USB3, Stmicro STMMAC or
> > recent DW SPI driver). In order to preserve the standard version
> > representation and prevent the data conversion back and forth, we suggest
> > to preserve the native version representation in the DWC PCIe driver too
> > in the same way as it has already been done in the rest of the DWC
> > drivers. IP-core version reading from the CSR will be introduced in the
> > next commit together with a simple macro-based API to use it.
> > 
> > [1] https://en.wikipedia.org/wiki/FourCC
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/pci/controller/dwc/pci-keystone.c    | 12 ++++++------
> >  drivers/pci/controller/dwc/pcie-designware.c |  8 ++++----
> >  drivers/pci/controller/dwc/pcie-designware.h | 10 +++++++++-
> >  drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
> >  drivers/pci/controller/dwc/pcie-tegra194.c   |  2 +-
> >  5 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > index 1c2ee4e13f1c..ec65355dd29b 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > @@ -109,7 +109,7 @@ struct ks_pcie_of_data {
> >  	enum dw_pcie_device_mode mode;
> >  	const struct dw_pcie_host_ops *host_ops;
> >  	const struct dw_pcie_ep_ops *ep_ops;
> > -	unsigned int version;
> > +	u32 version;
> >  };
> >  
> >  struct keystone_pcie {
> > @@ -1069,19 +1069,19 @@ static int ks_pcie_am654_set_mode(struct device *dev,
> >  
> >  static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
> >  	.host_ops = &ks_pcie_host_ops,
> > -	.version = 0x365A,
> > +	.version = DW_PCIE_VER_365A,
> >  };
> >  
> >  static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
> >  	.host_ops = &ks_pcie_am654_host_ops,
> >  	.mode = DW_PCIE_RC_TYPE,

> > -	.version = 0x490A,
> > +	.version = DW_PCIE_VER_490A,
> 
> This version should be readable, so get rid of hardcoding it here.

Sorry but what do you mean by 'readable'? Do you mean that since the
version CSR has been available since 4.70a, it is supposed to be read
from the h/w registers for the newer IPs and there is no point in
having that field initialized here? If so do you suggest to drop the
initialization from the both pci-keystone.c:ks_pcie_am654_rc_of_data
and pci-keystone.c:ks_pcie_am654_rc_of_data structure instances? If
so then the same can be done in the pcie-tegra194.c and
pcie-intel-gw.c drivers. Shall I drop it from there too?

Anyway if that's what you mean it needs to be done in a dedicated
patch, not here.

> 
> >  };
> >  
> >  static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
> >  	.ep_ops = &ks_pcie_am654_ep_ops,
> >  	.mode = DW_PCIE_EP_TYPE,
> > -	.version = 0x490A,
> > +	.version = DW_PCIE_VER_490A,
> >  };
> >  
> >  static const struct of_device_id ks_pcie_of_match[] = {
> > @@ -1114,12 +1114,12 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
> >  	struct device_link **link;
> >  	struct gpio_desc *gpiod;
> >  	struct resource *res;
> > -	unsigned int version;
> >  	void __iomem *base;
> >  	u32 num_viewport;
> >  	struct phy **phy;
> >  	u32 num_lanes;
> >  	char name[10];
> > +	u32 version;
> >  	int ret;
> >  	int irq;
> >  	int i;
> > @@ -1233,7 +1233,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
> >  		goto err_get_sync;
> >  	}
> >  
> > -	if (pci->version >= 0x480A)
> > +	if (pci->version >= DW_PCIE_VER_480A)
> >  		ret = ks_pcie_am654_set_mode(dev, mode);
> >  	else
> >  		ret = ks_pcie_set_mode(dev);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index f1693e25afcb..c21373c6cb51 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -289,7 +289,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, u8 func_no,
> >  	val = type | PCIE_ATU_FUNC_NUM(func_no);
> >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr))
> >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > -	if (pci->version == 0x490A)
> > +	if (pci->version == DW_PCIE_VER_490A)
> >  		val = dw_pcie_enable_ecrc(val);
> >  	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1, val);
> >  	dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> > @@ -336,7 +336,7 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  			   upper_32_bits(cpu_addr));
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_LIMIT,
> >  			   lower_32_bits(limit_addr));
> > -	if (pci->version >= 0x460A)
> > +	if (pci->version >= DW_PCIE_VER_460A)
> >  		dw_pcie_writel_dbi(pci, PCIE_ATU_UPPER_LIMIT,
> >  				   upper_32_bits(limit_addr));
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_LOWER_TARGET,
> > @@ -345,9 +345,9 @@ static void __dw_pcie_prog_outbound_atu(struct dw_pcie *pci, u8 func_no,
> >  			   upper_32_bits(pci_addr));
> >  	val = type | PCIE_ATU_FUNC_NUM(func_no);
> >  	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
> > -	    pci->version >= 0x460A)
> > +	    pci->version >= DW_PCIE_VER_460A)
> >  		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > -	if (pci->version == 0x490A)
> > +	if (pci->version == DW_PCIE_VER_490A)
> >  		val = dw_pcie_enable_ecrc(val);
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_CR1, val);
> >  	dw_pcie_writel_dbi(pci, PCIE_ATU_CR2, PCIE_ATU_ENABLE);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 7d6e9b7576be..5be43c662176 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -20,6 +20,14 @@
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> >  

> > +/* DWC PCIe IP-core versions (native support since v4.70a) */
> > +#define DW_PCIE_VER_365A		0x3336352a
> > +#define DW_PCIE_VER_460A		0x3436302a
> > +#define DW_PCIE_VER_470A		0x3437302a
> > +#define DW_PCIE_VER_480A		0x3438302a
> > +#define DW_PCIE_VER_490A		0x3439302a
> > +#define DW_PCIE_VER_520A		0x3532302a
> 
> Create a macro to form these so we can do:
> 
> DW_PCIE_VER('5', '2', '0', 'A')

I am ok with that as long as you suggest to use it in the definition
of the macros above. Like this:

+#define DW_PCIE_VER_365A	__DW_PCIE_VER('3', '5', '6', '*')

otherwise if we used the macro suggested by you right in the code or
in the introduced further dw_pcie_ver_is() macros, the version
checking conditional statements turn into the much longer constructions.
I'd rather avoid that. So could you clarify what is the usage scenario
of the macro DW_PCIE_VER() suggested by you?

Please also note that mainly the DW PCIe (USB, SPI, *MAC, etc) FourCC
versions don't contain the letter symbols, but are enclosed with the
'*' symbol (see 0x2a ASCII code). So the 3.65a version isn't encoded
as 0x333635_61_, but as 0x333635_2a_. I don't know a real reasoning of
that, but can guess that the same version value might be used for the
series of releases like 3.65a, 4.65b, 4.65c, etc as some kind of
wildcard. Alas I don't have any non-'a' implementation of the Synopsys
IP-core to check that out.

-Sergey

> 
> > +
> >  /* Parameters for the waiting for link up routine */
> >  #define LINK_WAIT_MAX_RETRIES		10
> >  #define LINK_WAIT_USLEEP_MIN		90000
> > @@ -269,7 +277,7 @@ struct dw_pcie {
> >  	struct pcie_port	pp;
> >  	struct dw_pcie_ep	ep;
> >  	const struct dw_pcie_ops *ops;
> > -	unsigned int		version;
> > +	u32			version;
> >  	int			num_lanes;
> >  	int			link_gen;
> >  	u8			n_fts[2];
> > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > index 5ba144924ff8..786af2ba379f 100644
> > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > @@ -59,7 +59,7 @@
> >  #define RESET_INTERVAL_MS		100
> >  
> >  struct intel_pcie_soc {
> > -	unsigned int	pcie_ver;
> > +	u32	pcie_ver;
> >  };
> >  
> >  struct intel_pcie {
> > @@ -395,7 +395,7 @@ static const struct dw_pcie_host_ops intel_pcie_dw_ops = {
> >  };
> >  
> >  static const struct intel_pcie_soc pcie_data = {
> > -	.pcie_ver =		0x520A,
> > +	.pcie_ver =		DW_PCIE_VER_520A,
> >  };
> >  
> >  static int intel_pcie_probe(struct platform_device *pdev)
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index b1b5f836a806..6f1330ed63e5 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -1981,7 +1981,7 @@ static int tegra194_pcie_probe(struct platform_device *pdev)
> >  	pci->ops = &tegra_dw_pcie_ops;
> >  	pci->n_fts[0] = N_FTS_VAL;
> >  	pci->n_fts[1] = FTS_VAL;
> > -	pci->version = 0x490A;
> > +	pci->version = DW_PCIE_VER_490A;
> >  
> >  	pp = &pci->pp;
> >  	pp->num_vectors = MAX_MSI_IRQS;
> > -- 
> > 2.35.1
> > 

  reply	other threads:[~2022-04-17 14:42 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-24  1:37 [PATCH 00/16] PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support Serge Semin
2022-03-24  1:37 ` [PATCH 01/16] dt-bindings: PCI: dwc: Define generic and native DT bindings Serge Semin
2022-03-28 20:46   ` Rob Herring
2022-04-15 16:14     ` Serge Semin
2022-03-24  1:37 ` [PATCH 02/16] dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings Serge Semin
2022-03-28 23:20   ` Rob Herring
2022-04-15 20:22     ` Serge Semin
2022-03-24  1:37 ` [PATCH 03/16] PCI: dwc: Add more verbose link-up message Serge Semin
2022-03-28 23:31   ` Joe Perches
2022-04-16  8:01     ` Serge Semin
2022-03-29 14:47   ` Rob Herring
2022-04-17 10:10     ` Serge Semin
2022-03-24  1:37 ` [PATCH 04/16] PCI: dwc: Convert to using native IP-core versions representation Serge Semin
2022-03-29 15:00   ` Rob Herring
2022-04-17 14:42     ` Serge Semin [this message]
2022-03-24  1:37 ` [PATCH 05/16] PCI: dwc: Add IP-core version detection procedure Serge Semin
2022-03-29 15:08   ` Rob Herring
2022-04-17 15:13     ` Serge Semin
2022-03-24  1:37 ` [PATCH 06/16] PCI: dwc: Introduce Synopsys IP-core versions/types interface Serge Semin
2022-03-24  1:37 ` [PATCH 07/16] PCI: dwc: Add host de-initialization callback Serge Semin
2022-03-24  1:37 ` [PATCH 08/16] PCI: dwc: Drop inbound iATU types enumeration - dw_pcie_as_type Serge Semin
2022-03-29 15:16   ` Rob Herring
2022-03-24  1:37 ` [PATCH 09/16] PCI: dwc: Simplify in/outbound iATU setup methods Serge Semin
2022-03-29 15:28   ` Rob Herring
2022-04-17 20:08     ` Serge Semin
2022-03-24  1:37 ` [PATCH 10/16] PCI: dwc: Drop iATU regions enumeration - dw_pcie_region_type Serge Semin
2022-03-29 15:31   ` Rob Herring
2022-04-17 17:52     ` Serge Semin
2022-03-24  1:37 ` [PATCH 11/16] PCI: dwc: Add iATU regions size detection procedure Serge Semin
2022-03-24  1:37 ` [PATCH 12/16] PCI: dwc: Verify in/out regions against iATU constraints Serge Semin
2022-03-24  1:37 ` [PATCH 13/16] PCI: dwc: Check iATU in/outbound ranges setup methods status Serge Semin
2022-03-24  1:37 ` [PATCH 14/16] PCI: dwc: Introduce dma-ranges property support for RC-host Serge Semin
2022-03-24  1:37 ` [PATCH 15/16] PCI: dwc: Introduce generic platform clocks and resets sets Serge Semin
2022-03-24  1:37 ` [PATCH 16/16] PCI: dwc: Add Baikal-T1 PCIe controller support Serge Semin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220417144228.pqufj4li45qj4vyq@mobilestation \
    --to=fancer.lancer@gmail.com \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Frank.Li@nxp.com \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=Sergey.Semin@baikalelectronics.ru \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=robh@kernel.org \
    --cc=rtanwar@maxlinear.com \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).