2022-11-07 22:01:23

by Serge Semin

[permalink] [raw]
Subject: [PATCH v6 00/20] PCI: dwc: Add generic resources and Baikal-T1 support

This patchset is a third one in the series created in the framework of
my Baikal-T1 PCIe/eDMA-related work:

[1: Done v5] PCI: dwc: Various fixes and cleanups
Link: https://lore.kernel.org/linux-pci/[email protected]/
Merged: kernel 6.0-rc1
[2: Done v4] PCI: dwc: Add hw version and dma-ranges support
Link: https://lore.kernel.org/linux-pci/[email protected]
Merged: kernel 6.0-rc1
[3: In-review v5] PCI: dwc: Add generic resources and Baikal-T1 support
Link: ---you are looking at it---
[4: Done v4] dmaengine: dw-edma: Add RP/EP local DMA support
Link: https://lore.kernel.org/linux-pci/[email protected]/

Note it is very recommended to merge the patchsets in the same order as
they are listed in the set above in order to have them applied smoothly.
Nothing prevents them from being reviewed synchronously though.

Originally the patches submitted in this patchset were a part of the series:
Link: https://lore.kernel.org/linux-pci/[email protected]/
but due to the reviewers requests the series was expanded to about 30
patches which made it too bulky for a comfortable review. So I decided to
split it up into two patchsets: 2. and 3. in the table above.

Regarding the series content. This patchset is mainly about adding new DW
PCIe platform support - Baikal-T1 PCIe of DW PCIe v4.60a IP-core. But a
set of feature-reach preparations are done first. It starts from
converting the currently available DT-schema into a more flexible schemas
hierarchy with separately defined regs, clocks, resets and interrupts
properties. As a result the common schema can be easily re-used by all the
currently available platforms while the named properties above can be
either re-defined or used as is if the platforms support they. In the
framework of that modification we also suggest to add a set of generic
regs, clocks, resets and interrupts resource names in accordance with what
the DW PCIe hardware reference manual describes and what the DW PCIe core
driver already expects to be specified. Thus the new platform driver will
be able to re-use the common resources infrastructure.

Link: https://lore.kernel.org/linux-pci/[email protected]/
Changelog v2:
- Rename 'syscon' property to 'baikal,bt1-syscon'. (@Rob)
- Move the iATU region selection procedure into a helper function (@Rob).
- Rebase from kernel v5.17 onto v5.18-rc3 since the later kernel has
already DT bindings converted. (@Rob)
- Use 'definitions' property instead of the '$defs' one. It fixes the
dt-validate error: 'X is not of type array.'
- Drop 'interrupts' and 'interrupt-names' property from being required
for the native DW PCIe host.
- Evaluate the 'snps,dw-pcie-common.yaml' schema in the
'socionext,uniphier-pcie-ep.yaml' DT-bindings since the later has
platform-specific names defined.

Link: https://lore.kernel.org/linux-pci/[email protected]
Changelog v3:
- Split up the patch "dt-bindings: PCI: dwc: Define common and native DT
bindings" into a series of modifications. (@Rob)
- Detach this series of the patches into a dedicated patchset.
- Add a new feature patch: "PCI: dwc: Introduce generic controller
capabilities interface".
- Add a new feature patch: "PCI: dwc: Introduce generic resources getter".
- Add a new cleanup patch: "PCI: dwc: Combine iATU detection procedures".
- Add a method to at least request the generic clocks and resets. (@Rob)
- Add GPIO-based PERST# signal support to the core module.
- Redefine Baikal-T1 PCIe host bridge config space accessors with the
pci_generic_config_read32() and pci_generic_config_write32() methods.
(@Rob)
- Drop synonymous from the names list in the common DT-schema since the
device sub-schemas create their own enumerations anyway.
- Rebase onto kernel v5.18.

Link: https://lore.kernel.org/linux-pci/[email protected]/
Changelog v4:
- Drop PCIBIOS_* macros usage. (@Rob)
- Add "static const" to the dw_pcie_ops and dw_pcie_host_ops structure
instances. (@Bjorn)
- Rename bt1_pcie_dw_ops to bt1_pcie_ops. (@Bjorn)
- Rename bt1_pcie_ops to bt1_pci_ops. (@Bjorn)
- Use start_link/stop_link suffixes in the Baikal-T1 PCIe
start/stop link callbacks. (@Bjorn)
- Change the get_res() method suffix to being get_resources(). (@Bjorn)
- Change *_{add,del}_dw_port() method to *_{add,del}_port(). (@Bjorn)
- Drop dma_coerce_mask_and_coherent() applied to the PCI host bridge
kernel device instance. (@Bjorn)
- Add the comment above the dma_set_mask_and_coherent() method usage
regarding the controller eDMA feature. (@Bjorn)
- Fix the comment above the core reset controls assertion. (@Bjorn)
- Replace delays and timeout numeric literals with macros. (@Bjorn)
- Convert the method name from dw_pcie_get_res() to
dw_pcie_get_resources(). (@Bjorn)
- Rebase onto the kernel v5.19-rcX.

Link: https://lore.kernel.org/linux-pci/[email protected]/
Changelog v5:
- Add a note about having line-based PHY phandles order. (@Rob)
- Prefer 'pcie[0-9]+' PHY-names over the rest of the cases. (@Rob)
- Drop generic fallback names from the Baikal-T1 compatible property
constraints. (@Rob)
- Define ordered {reg,interrupt,clock,reset}-names Baikal-T1 PCIe
properties. (@Rob)
- Drop minItems from the Baikal-T1 PCIe clocks and reset properties,
since it equals to the maxItems for them.
- Drop num-ob-windows and num-ib-windows properties constraint from
Baikal-T1 PCIe bindings. (@Rob)
- Add a note about having line-based PHY phandles order. (@Rob)
- Prefer 'pcie[0-9]+' PHY-names over the rest of the cases. (@Rob)
- Add platform-specific reg/interrupt/clock/reset names to the generic
schema, but mark them as deprecated.
- Add new patches:
dt-bindings: visconti-pcie: Fix interrupts array max constraints
dt-bindings: imx6q-pcie: Fix clock names for imx6sx and imx8mq
- Move the patch:
PCI: dwc: Introduce dma-ranges property support for RC-host
from the previous patchset in here. (@Bjorn)
- Rebase onto the kernel v6.0-rc2.

Link: https://lore.kernel.org/linux-pci/[email protected]/
Changelog v6:
- Add the Nvidia Tegra194-specific "p2u-[0-7]" phy-names too. (@DT-tbot)
- Drop 'deprecated' keywords from the vendor-specific names. (@Rob)
- Move the common *-names definitions to the RP/EP schemas. Thus drop
the 'definitions' property usage from the DT-schemas. (@Rob)
- Move the DMA-mask setup to the eDMA driver. (@Robin)
- Rebase onto the kernel v6.1-rc3.

Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: "Krzysztof WilczyƄski" <[email protected]>
Cc: Frank Li <[email protected]>
Cc: Manivannan Sadhasivam <[email protected]>
Cc: caihuoqing <[email protected]>
Cc: Vinod Koul <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]

Serge Semin (20):
dt-bindings: imx6q-pcie: Fix clock names for imx6sx and imx8mq
dt-bindings: visconti-pcie: Fix interrupts array max constraints
dt-bindings: PCI: dwc: Detach common RP/EP DT bindings
dt-bindings: PCI: dwc: Remove bus node from the examples
dt-bindings: PCI: dwc: Add phys/phy-names common properties
dt-bindings: PCI: dwc: Add max-link-speed common property
dt-bindings: PCI: dwc: Apply generic schema for generic device only
dt-bindings: PCI: dwc: Add max-functions EP property
dt-bindings: PCI: dwc: Add interrupts/interrupt-names common
properties
dt-bindings: PCI: dwc: Add reg/reg-names common properties
dt-bindings: PCI: dwc: Add clocks/resets common properties
dt-bindings: PCI: dwc: Add dma-coherent property
dt-bindings: PCI: dwc: Apply common schema to Rockchip DW PCIe nodes
dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings
PCI: dwc: Introduce dma-ranges property support for RC-host
PCI: dwc: Introduce generic controller capabilities interface
PCI: dwc: Introduce generic resources getter
PCI: dwc: Combine iATU detection procedures
PCI: dwc: Introduce generic platform clocks and resets
PCI: dwc: Add Baikal-T1 PCIe controller support

.../bindings/pci/baikal,bt1-pcie.yaml | 168 +++++
.../bindings/pci/fsl,imx6q-pcie.yaml | 47 +-
.../bindings/pci/rockchip-dw-pcie.yaml | 4 +-
.../bindings/pci/snps,dw-pcie-common.yaml | 266 ++++++++
.../bindings/pci/snps,dw-pcie-ep.yaml | 212 ++++--
.../devicetree/bindings/pci/snps,dw-pcie.yaml | 260 +++++--
.../bindings/pci/toshiba,visconti-pcie.yaml | 7 +-
drivers/pci/controller/dwc/Kconfig | 9 +
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-bt1.c | 645 ++++++++++++++++++
.../pci/controller/dwc/pcie-designware-ep.c | 30 +-
.../pci/controller/dwc/pcie-designware-host.c | 47 +-
drivers/pci/controller/dwc/pcie-designware.c | 262 +++++--
drivers/pci/controller/dwc/pcie-designware.h | 63 +-
14 files changed, 1798 insertions(+), 223 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
create mode 100644 Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
create mode 100644 drivers/pci/controller/dwc/pcie-bt1.c

--
2.38.0




2022-11-07 22:01:28

by Serge Semin

[permalink] [raw]
Subject: [PATCH v6 01/20] dt-bindings: imx6q-pcie: Fix clock names for imx6sx and imx8mq

Originally as it was defined the legacy bindings the pcie_inbound_axi and
pcie_aux clock names were supposed to be used in the fsl,imx6sx-pcie and
fsl,imx8mq-pcie devices respectively. But the bindings conversion has been
incorrectly so now the fourth clock name is defined as "pcie_inbound_axi
for imx6sx-pcie, pcie_aux for imx8mq-pcie", which is completely wrong.
Let's fix that by conditionally apply the clock-names constraints based on
the compatible string content.

Fixes: 751ca492f131 ("dt-bindings: PCI: imx6: convert the imx pcie controller to dtschema")
Signed-off-by: Serge Semin <[email protected]>
Acked-by: Alexander Stein <[email protected]>

---

Changelog v5:
- This is a new patch added on the v5 release of the patchset.
---
.../bindings/pci/fsl,imx6q-pcie.yaml | 47 +++++++++++++++++--
1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index 376e739bcad4..ebfe75f1576e 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -16,6 +16,47 @@ description: |+

allOf:
- $ref: /schemas/pci/snps,dw-pcie.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: fsl,imx6sx-pcie
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: pcie
+ - const: pcie_bus
+ - const: pcie_phy
+ - const: pcie_inbound_axi
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: fsl,imx8mq-pcie
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: pcie
+ - const: pcie_bus
+ - const: pcie_phy
+ - const: pcie_aux
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ enum:
+ - fsl,imx6sx-pcie
+ - fsl,imx8mq-pcie
+ then:
+ properties:
+ clock-names:
+ items:
+ - const: pcie
+ - const: pcie_bus
+ - const: pcie_phy

properties:
compatible:
@@ -57,11 +98,7 @@ properties:

clock-names:
minItems: 3
- items:
- - const: pcie
- - const: pcie_bus
- - const: pcie_phy
- - const: pcie_inbound_axi for imx6sx-pcie, pcie_aux for imx8mq-pcie
+ maxItems: 4

num-lanes:
const: 1
--
2.38.0



2022-11-07 22:01:36

by Serge Semin

[permalink] [raw]
Subject: [PATCH v6 17/20] PCI: dwc: Introduce generic resources getter

Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
the separate parts of the DW PCIe core driver. It doesn't really make
sense since the both controller types have identical set of the core CSR
regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
and EP initialization methods by moving the platform-specific registers
space getting and mapping into a common method. It gets to be even more
justified seeing the CSRs base address pointers are preserved in the
common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
initialization will be moved to the new method too in order to have a
single function for all the generic platform properties handling in single
place.

A nice side-effect of this change is that the pcie-designware-host.c and
pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
storage modification, which makes the DW PCIe core, Root Port and Endpoint
modules more coherent.

Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Rob Herring <[email protected]>

---

Changelog v3:
- This is a new patch created on v3 lap of the series.

Changelog v4:
- Convert the method name from dw_pcie_get_res() to
dw_pcie_get_resources(). (@Bjorn)
---
.../pci/controller/dwc/pcie-designware-ep.c | 26 +------
.../pci/controller/dwc/pcie-designware-host.c | 15 +---
drivers/pci/controller/dwc/pcie-designware.c | 75 ++++++++++++++-----
drivers/pci/controller/dwc/pcie-designware.h | 3 +
4 files changed, 65 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 237bb01d7852..80a64b63c055 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -13,8 +13,6 @@
#include <linux/pci-epc.h>
#include <linux/pci-epf.h>

-#include "../../pci.h"
-
void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
{
struct pci_epc *epc = ep->epc;
@@ -688,29 +686,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
struct device *dev = pci->dev;
struct platform_device *pdev = to_platform_device(dev);
- struct device_node *np = dev->of_node;
const struct pci_epc_features *epc_features;
struct dw_pcie_ep_func *ep_func;

INIT_LIST_HEAD(&ep->func_list);

- if (!pci->dbi_base) {
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
- pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
- if (IS_ERR(pci->dbi_base))
- return PTR_ERR(pci->dbi_base);
- }
-
- if (!pci->dbi_base2) {
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
- if (!res) {
- pci->dbi_base2 = pci->dbi_base + SZ_4K;
- } else {
- pci->dbi_base2 = devm_pci_remap_cfg_resource(dev, res);
- if (IS_ERR(pci->dbi_base2))
- return PTR_ERR(pci->dbi_base2);
- }
- }
+ ret = dw_pcie_get_resources(pci);
+ if (ret)
+ return ret;

res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
if (!res)
@@ -739,9 +722,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
return -ENOMEM;
ep->outbound_addr = addr;

- if (pci->link_gen < 1)
- pci->link_gen = of_pci_get_max_link_speed(np);
-
epc = devm_pci_epc_create(dev, &epc_ops);
if (IS_ERR(epc)) {
dev_err(dev, "Failed to create epc device\n");
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ea923c25e12d..3ab6ae3712c4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -16,7 +16,6 @@
#include <linux/pci_regs.h>
#include <linux/platform_device.h>

-#include "../../pci.h"
#include "pcie-designware.h"

static struct pci_ops dw_pcie_ops;
@@ -395,6 +394,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)

raw_spin_lock_init(&pp->lock);

+ ret = dw_pcie_get_resources(pci);
+ if (ret)
+ return ret;
+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
if (res) {
pp->cfg0_size = resource_size(res);
@@ -408,13 +411,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
return -ENODEV;
}

- if (!pci->dbi_base) {
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
- pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
- if (IS_ERR(pci->dbi_base))
- return PTR_ERR(pci->dbi_base);
- }
-
bridge = devm_pci_alloc_host_bridge(dev, 0);
if (!bridge)
return -ENOMEM;
@@ -429,9 +425,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
pp->io_base = pci_pio_to_address(win->res->start);
}

- if (pci->link_gen < 1)
- pci->link_gen = of_pci_get_max_link_speed(np);
-
/* Set default bus ops */
bridge->ops = &dw_pcie_ops;
bridge->child_ops = &dw_child_pcie_ops;
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9d78e7ca61e1..a8436027434d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -11,6 +11,7 @@
#include <linux/align.h>
#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/ioport.h>
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/sizes.h>
@@ -19,6 +20,59 @@
#include "../../pci.h"
#include "pcie-designware.h"

+int dw_pcie_get_resources(struct dw_pcie *pci)
+{
+ struct platform_device *pdev = to_platform_device(pci->dev);
+ struct device_node *np = dev_of_node(pci->dev);
+ struct resource *res;
+
+ if (!pci->dbi_base) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+ pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
+ if (IS_ERR(pci->dbi_base))
+ return PTR_ERR(pci->dbi_base);
+ }
+
+ /* DBI2 is mainly useful for the endpoint controller */
+ if (!pci->dbi_base2) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
+ if (res) {
+ pci->dbi_base2 = devm_pci_remap_cfg_resource(pci->dev, res);
+ if (IS_ERR(pci->dbi_base2))
+ return PTR_ERR(pci->dbi_base2);
+ } else {
+ pci->dbi_base2 = pci->dbi_base + SZ_4K;
+ }
+ }
+
+ /* For non-unrolled iATU/eDMA platforms this range will be ignored */
+ if (!pci->atu_base) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
+ if (res) {
+ pci->atu_size = resource_size(res);
+ pci->atu_base = devm_ioremap_resource(pci->dev, res);
+ if (IS_ERR(pci->atu_base))
+ return PTR_ERR(pci->atu_base);
+ } else {
+ pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
+ }
+ }
+
+ /* Set a default value suitable for at most 8 in and 8 out windows */
+ if (!pci->atu_size)
+ pci->atu_size = SZ_4K;
+
+ if (pci->link_gen < 1)
+ pci->link_gen = of_pci_get_max_link_speed(np);
+
+ of_property_read_u32(np, "num-lanes", &pci->num_lanes);
+
+ if (of_property_read_bool(np, "snps,enable-cdm-check"))
+ dw_pcie_cap_set(pci, CDM_CHECK);
+
+ return 0;
+}
+
void dw_pcie_version_detect(struct dw_pcie *pci)
{
u32 ver;
@@ -639,25 +693,8 @@ static void dw_pcie_iatu_detect_regions(struct dw_pcie *pci)

void dw_pcie_iatu_detect(struct dw_pcie *pci)
{
- struct platform_device *pdev = to_platform_device(pci->dev);
-
if (dw_pcie_iatu_unroll_enabled(pci)) {
dw_pcie_cap_set(pci, IATU_UNROLL);
-
- if (!pci->atu_base) {
- struct resource *res =
- platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
- if (res) {
- pci->atu_size = resource_size(res);
- pci->atu_base = devm_ioremap_resource(pci->dev, res);
- }
- if (!pci->atu_base || IS_ERR(pci->atu_base))
- pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
- }
-
- if (!pci->atu_size)
- /* Pick a minimal default, enough for 8 in and 8 out windows */
- pci->atu_size = SZ_4K;
} else {
pci->atu_base = pci->dbi_base + PCIE_ATU_VIEWPORT_BASE;
pci->atu_size = PCIE_ATU_VIEWPORT_SIZE;
@@ -675,7 +712,6 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)

void dw_pcie_setup(struct dw_pcie *pci)
{
- struct device_node *np = pci->dev->of_node;
u32 val;

if (pci->link_gen > 0)
@@ -703,14 +739,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
val |= PORT_LINK_DLL_LINK_EN;
dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);

- if (of_property_read_bool(np, "snps,enable-cdm-check")) {
+ if (dw_pcie_cap_is(pci, CDM_CHECK)) {
val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
PCIE_PL_CHK_REG_CHK_REG_START;
dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
}

- of_property_read_u32(np, "num-lanes", &pci->num_lanes);
if (!pci->num_lanes) {
dev_dbg(pci->dev, "Using h/w default number of lanes\n");
return;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index c6dddacee3b1..081f169e6021 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -46,6 +46,7 @@

/* DWC PCIe controller capabilities */
#define DW_PCIE_CAP_IATU_UNROLL 1
+#define DW_PCIE_CAP_CDM_CHECK 2

#define dw_pcie_cap_is(_pci, _cap) \
test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
@@ -338,6 +339,8 @@ struct dw_pcie {
#define to_dw_pcie_from_ep(endpoint) \
container_of((endpoint), struct dw_pcie, ep)

+int dw_pcie_get_resources(struct dw_pcie *pci);
+
void dw_pcie_version_detect(struct dw_pcie *pci);

u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
--
2.38.0



2022-11-07 22:01:39

by Serge Semin

[permalink] [raw]
Subject: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

Currently the 'interrupts' and 'interrupt-names' properties are defined
being too generic to really describe any actual IRQ interface. Moreover
the DW PCIe End-point devices are left with no IRQ signals. All of that
can be fixed by adding the IRQ-related properties to the common DW PCIe
DT-schemas in accordance with the hardware reference manual. The DW PCIe
common DT-schema will contain the generic properties definitions with just
a number of entries per property, while the DW PCIe RP/EP-specific schemas
will have the particular number of items and the generic resource names
listed.

Note since there are DW PCI-based vendor-specific DT-bindings with the
custom names assigned to the same IRQ resources we have no much choice but
to add them to the generic DT-schemas in order to have the schemas being
applicable for such devices. These names are marked as vendor-specific and
should be avoided being used in new bindings in favor of the generic
names.

Signed-off-by: Serge Semin <[email protected]>

---

Note without the next dtschema tool fix

--- a/lib.py 2022-09-29 15:17:13.100033810 +0300
+++ b/lib.py 2022-09-29 15:19:54.886172794 +0300
@@ -1307,7 +1307,7 @@
def format_error(filename, error, prefix="", nodename=None, verbose=False):
src = prefix + os.path.abspath(filename) + ':'

- if error.linecol[0] >= 0:
+ if hasattr(error, 'linecol') and error.linecol[0] >= 0:
src = src + '%i:%i: ' % (error.linecol[0]+1, error.linecol[1]+1)
else:
src += ' '
@@ -1342,10 +1342,10 @@
else:
msg = error.message

- if error.note:
+ if hasattr(error, 'note') and error.note:
msg += '\n\t' + prefix + 'hint: ' + error.note

- if error.schema_file:
+ if hasattr(error, 'schema_file') and error.schema_file:
msg += '\n\t' + prefix + 'from schema $id: ' + error.schema_file

return src + msg

any DT-bindings error will cause the dt-schema script crash:

Traceback (most recent call last):
File "/home/fancer/.local/bin/dt-validate", line 175, in <module>
sg.check_trees(filename, testtree)
File "/home/fancer/.local/bin/dt-validate", line 122, in check_trees
self.check_subtree(dt, subtree, False, "/", "/", filename)
File "/home/fancer/.local/bin/dt-validate", line 111, in check_subtree
self.check_subtree(tree, value, disabled, name, fullname + name, filename)
File "/home/fancer/.local/bin/dt-validate", line 111, in check_subtree
self.check_subtree(tree, value, disabled, name, fullname + name, filename)
File "/home/fancer/.local/bin/dt-validate", line 106, in check_subtree
self.check_node(tree, subtree, disabled, nodename, fullname, filename)
File "/home/fancer/.local/bin/dt-validate", line 84, in check_node
print(dtschema.format_error(filename, error, nodename=nodename, verbose=verbose) +
File "/home/fancer/.local/lib/python3.8/site-packages/dtschema/lib.py", line 1332, in format_error
msg += '\n' + format_error(filename, suberror, prefix=prefix+"\t", nodename=nodename, verbose=verbose)
File "/home/fancer/.local/lib/python3.8/site-packages/dtschema/lib.py", line 1310, in format_error
if error.linecol[0] >= 0:
AttributeError: 'ValidationError' object has no attribute 'linecol'

Changelog v3:
- This is a new patch unpinned from the next one:
https://lore.kernel.org/linux-pci/[email protected]/
by the Rob' request. (@Rob)

Changelog v5:
- Add platform-specific interrupt names, but mark them as deprecated.

Changelog v6:
- Move the common interrupt-names definitions to the RP/EP schemas.
Thus drop the 'definitions' property. (@Rob)
- Drop the 'deprecated' keywords from the vendor-specific names. (@Rob)
---
.../bindings/pci/snps,dw-pcie-common.yaml | 19 ++++
.../bindings/pci/snps,dw-pcie-ep.yaml | 52 +++++++++++
.../devicetree/bindings/pci/snps,dw-pcie.yaml | 90 ++++++++++++++++++-
3 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
index e63c21783fc1..4646fb14e817 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
@@ -17,6 +17,25 @@ description:
select: false

properties:
+ interrupts:
+ description:
+ There are two main sub-blocks which are normally capable of
+ generating interrupts. It's System Information Interface and MSI
+ interface. While the former one has some common for the Host and
+ Endpoint controllers IRQ-signals, the later interface is obviously
+ Root Complex specific since it's responsible for the incoming MSI
+ messages signalling. The System Information IRQ signals are mainly
+ responsible for reporting the generic PCIe hierarchy and Root
+ Complex events like VPD IO request, general AER, PME, Hot-plug, link
+ bandwidth change, link equalization request, INTx asserted/deasserted
+ Message detection, embedded DMA Tx/Rx/Error.
+ minItems: 1
+ maxItems: 26
+
+ interrupt-names:
+ minItems: 1
+ maxItems: 26
+
phys:
description:
There can be up to the number of possible lanes PHYs specified placed in
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index 71dd19ae1060..7d3f8fc8b7b4 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -41,6 +41,55 @@ properties:
items:
enum: [dbi, dbi2, config, atu, addr_space, link, atu_dma, appl]

+ interrupts:
+ description:
+ There is no mandatory IRQ signals for the normal controller functioning,
+ but in addition to the native set the platforms may have a link- or
+ PM-related IRQs specified.
+ minItems: 1
+ maxItems: 20
+
+ interrupt-names:
+ minItems: 1
+ maxItems: 20
+ items:
+ oneOf:
+ - description:
+ Controller request to read or write virtual product data
+ from/to the VPD capability registers.
+ const: vpd
+ - description:
+ Link Equalization Request flag is set in the Link Status 2
+ register (applicable if the corresponding IRQ is enabled in
+ the Link Control 3 register).
+ const: l_eq
+ - description:
+ Indicates that the eDMA Tx/Rx transfer is complete or that an
+ error has occurred on the corresponding channel. eDMA can have
+ eight Tx (Write) and Rx (Read) eDMA channels thus supporting up
+ to 16 IRQ signals all together. Write eDMA channels shall go
+ first in the ordered row as per default edma_int[*] bus setup.
+ pattern: '^dma([0-9]|1[0-5])?$'
+ - description:
+ PCIe protocol correctable error or a Data Path protection
+ correctable error is detected by the automotive/safety
+ feature.
+ const: sft_ce
+ - description:
+ Indicates that the internal safety mechanism has detected an
+ uncorrectable error.
+ const: sft_ue
+ - description:
+ Application-specific IRQ raised depending on the vendor-specific
+ events basis.
+ const: app
+ - description:
+ Vendor-specific IRQ names. Consider using the generic names above
+ for new bindings.
+ oneOf:
+ - description: See native "app" IRQ for details
+ enum: [ intr ]
+
max-functions:
maximum: 32

@@ -60,6 +109,9 @@ examples:
<0xd0000000 0x2000000>; /* Configuration space */
reg-names = "dbi", "dbi2", "addr_space";

+ interrupts = <23>, <24>;
+ interrupt-names = "dma0", "dma1";
+
phys = <&pcie_phy0>, <&pcie_phy1>, <&pcie_phy2>, <&pcie_phy3>;
phy-names = "pcie0", "pcie1", "pcie2", "pcie3";

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index 85861b71d9ff..fa1db57b2b97 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -42,9 +42,92 @@ properties:
enum: [ dbi, dbi2, config, atu, atu_dma, app, appl, elbi, mgmt, ctrl,
parf, cfg, link, ulreg, smu, mpu, apb, phy ]

- interrupts: true
-
- interrupt-names: true
+ interrupts:
+ description:
+ DWC PCIe Root Port/Complex specific IRQ signals. At least MSI interrupt
+ signal is supposed to be specified for the host controller.
+ minItems: 1
+ maxItems: 26
+
+ interrupt-names:
+ minItems: 1
+ maxItems: 26
+ items:
+ oneOf:
+ - description:
+ Controller request to read or write virtual product data
+ from/to the VPD capability registers.
+ const: vpd
+ - description:
+ Link Equalization Request flag is set in the Link Status 2
+ register (applicable if the corresponding IRQ is enabled in
+ the Link Control 3 register).
+ const: l_eq
+ - description:
+ Indicates that the eDMA Tx/Rx transfer is complete or that an
+ error has occurred on the corresponding channel. eDMA can have
+ eight Tx (Write) and Rx (Read) eDMA channels thus supporting up
+ to 16 IRQ signals all together. Write eDMA channels shall go
+ first in the ordered row as per default edma_int[*] bus setup.
+ pattern: '^dma([0-9]|1[0-5])?$'
+ - description:
+ PCIe protocol correctable error or a Data Path protection
+ correctable error is detected by the automotive/safety
+ feature.
+ const: sft_ce
+ - description:
+ Indicates that the internal safety mechanism has detected an
+ uncorrectable error.
+ const: sft_ue
+ - description:
+ Application-specific IRQ raised depending on the vendor-specific
+ events basis.
+ const: app
+ - description:
+ DSP AXI MSI Interrupt detected. It gets de-asserted when there is
+ no more MSI interrupt pending. The interrupt is relevant to the
+ iMSI-RX - Integrated MSI Receiver (AXI bridge).
+ const: msi
+ - description:
+ Legacy A/B/C/D interrupt signal. Basically it's triggered by
+ receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message
+ from the downstream device.
+ pattern: "^int(a|b|c|d)$"
+ - description:
+ Error condition detected and a flag is set in the Root Error Status
+ register of the AER capability. It's asserted when the RC
+ internally generated an error or an error message is received by
+ the RC.
+ const: aer
+ - description:
+ PME message is received by the port. That means having the PME
+ status bit set in the Root Status register (the event is
+ supposed to be unmasked in the Root Control register).
+ const: pme
+ - description:
+ Hot-plug event is detected. That is a bit has been set in the
+ Slot Status register and the corresponding event is enabled in
+ the Slot Control register.
+ const: hp
+ - description:
+ Link Autonomous Bandwidth Status flag has been set in the Link
+ Status register (the event is supposed to be unmasked in the
+ Link Control register).
+ const: bw_au
+ - description:
+ Bandwidth Management Status flag has been set in the Link
+ Status register (the event is supposed to be unmasked in the
+ Link Control register).
+ const: bw_mg
+ - description:
+ Vendor-specific IRQ names. Consider using the generic names above
+ for new bindings.
+ oneOf:
+ - description: See native "app" IRQ for details
+ enum: [ intr ]
+ allOf:
+ - contains:
+ const: msi

clocks: true

@@ -70,6 +153,7 @@ examples:
bus-range = <0x0 0xff>;

interrupts = <25>, <24>;
+ interrupt-names = "msi", "hp";
#interrupt-cells = <1>;

reset-gpios = <&port0 0 1>;
--
2.38.0



2022-11-07 22:02:33

by Serge Semin

[permalink] [raw]
Subject: [PATCH v6 14/20] dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings

Baikal-T1 SoC is equipped with DWC PCIe v4.60a Root Port controller, which
link can be trained to work on up to Gen.3 speed over up to x4 lanes. The
controller is supposed to be fed up with four clock sources: DBI
peripheral clock, AXI application Tx/Rx clocks and external PHY/core
reference clock generating the 100MHz signal. In addition to that the
platform provide a way to reset each part of the controller:
sticky/non-sticky bits, host controller core, PIPE interface, PCS/PHY and
Hot/Power reset signal. The Root Port controller is equipped with multiple
IRQ lines like MSI, system AER, PME, HP, Bandwidth change, Link
equalization request and eDMA ones. The registers space is accessed over
the DBI interface. There can be no more than four inbound or outbound iATU
windows configured.

Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Rob Herring <[email protected]>

---

Changelog v2:
- Rename 'syscon' property to 'baikal,bt1-syscon'.
- Fix the 'compatible' property definition to being more specific about
what strings are supposed to be used. Due to that we had to add the
select property to evaluate the schema against the Baikal-T1 PCIe DT
nodes only.

Changelog v5:
- Drop generic fallback names from the compatible property constraints.
(@Rob)
- Define ordered {reg,interrupt,clock,reset}-names properties. (@Rob)
- Drop minItems from the clocks and reset properties, since it equals
to the maxItems for them.
- Drop num-ob-windows and num-ib-windows properties constraint. (@Rob)
---
.../bindings/pci/baikal,bt1-pcie.yaml | 168 ++++++++++++++++++
1 file changed, 168 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
new file mode 100644
index 000000000000..8eaa07ae9774
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
@@ -0,0 +1,168 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/baikal,bt1-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Baikal-T1 PCIe Root Port Controller
+
+maintainers:
+ - Serge Semin <[email protected]>
+
+description:
+ Embedded into Baikal-T1 SoC Root Complex controller with a single port
+ activated. It's based on the DWC RC PCIe v4.60a IP-core, which is configured
+ to have just a single Root Port function and is capable of establishing the
+ link up to Gen.3 speed on x4 lanes. It doesn't have embedded clock and reset
+ control module, so the proper interface initialization is supposed to be
+ performed by software. There four in- and four outbound iATU regions
+ which can be used to emit all required TLP types on the PCIe bus.
+
+allOf:
+ - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+ compatible:
+ const: baikal,bt1-pcie
+
+ reg:
+ description:
+ DBI, DBI2 and at least 4KB outbound iATU-capable region for the
+ peripheral devices CFG-space access.
+ maxItems: 3
+
+ reg-names:
+ items:
+ - const: dbi
+ - const: dbi2
+ - const: config
+
+ interrupts:
+ description:
+ MSI, AER, PME, Hot-plug, Link Bandwidth Management, Link Equalization
+ request and eight Read/Write eDMA IRQ lines are available.
+ maxItems: 14
+
+ interrupt-names:
+ items:
+ - const: dma0
+ - const: dma1
+ - const: dma2
+ - const: dma3
+ - const: dma4
+ - const: dma5
+ - const: dma6
+ - const: dma7
+ - const: msi
+ - const: aer
+ - const: pme
+ - const: hp
+ - const: bw_mg
+ - const: l_eq
+
+ clocks:
+ description:
+ DBI (attached to the APB bus), AXI-bus master and slave interfaces
+ are fed up by the dedicated application clocks. A common reference
+ clock signal is supposed to be attached to the corresponding Ref-pad
+ of the SoC. It will be redistributed amongst the controller core
+ sub-modules (pipe, core, aux, etc).
+ maxItems: 4
+
+ clock-names:
+ items:
+ - const: dbi
+ - const: mstr
+ - const: slv
+ - const: ref
+
+ resets:
+ description:
+ A comprehensive controller reset logic is supposed to be implemented
+ by software, so almost all the possible application and core reset
+ signals are exposed via the system CCU module.
+ maxItems: 9
+
+ reset-names:
+ items:
+ - const: mstr
+ - const: slv
+ - const: pwr
+ - const: hot
+ - const: phy
+ - const: core
+ - const: pipe
+ - const: sticky
+ - const: non-sticky
+
+ baikal,bt1-syscon:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ Phandle to the Baikal-T1 System Controller DT node. It's required to
+ access some additional PM, Reset-related and LTSSM signals.
+
+ num-lanes:
+ maximum: 4
+
+ max-link-speed:
+ maximum: 3
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - interrupts
+ - interrupt-names
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/mips-gic.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ pcie@1f052000 {
+ compatible = "baikal,bt1-pcie";
+ device_type = "pci";
+ reg = <0x1f052000 0x1000>, <0x1f053000 0x1000>, <0x1bdbf000 0x1000>;
+ reg-names = "dbi", "dbi2", "config";
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges = <0x81000000 0 0x00000000 0x1bdb0000 0 0x00008000>,
+ <0x82000000 0 0x20000000 0x08000000 0 0x13db0000>;
+ bus-range = <0x0 0xff>;
+
+ interrupts = <GIC_SHARED 80 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 81 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 82 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 83 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 84 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 85 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 86 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 87 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 88 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 89 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 90 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 91 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 92 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 93 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "dma0", "dma1", "dma2", "dma3",
+ "dma4", "dma5", "dma6", "dma7",
+ "msi", "aer", "pme", "hp", "bw_mg",
+ "l_eq";
+
+ clocks = <&ccu_sys 1>, <&ccu_axi 6>, <&ccu_axi 7>, <&clk_pcie>;
+ clock-names = "dbi", "mstr", "slv", "ref";
+
+ resets = <&ccu_axi 6>, <&ccu_axi 7>, <&ccu_sys 7>, <&ccu_sys 10>,
+ <&ccu_sys 4>, <&ccu_sys 6>, <&ccu_sys 5>, <&ccu_sys 8>,
+ <&ccu_sys 9>;
+ reset-names = "mstr", "slv", "pwr", "hot", "phy", "core", "pipe",
+ "sticky", "non-sticky";
+
+ reset-gpios = <&port0 0 GPIO_ACTIVE_LOW>;
+
+ num-lanes = <4>;
+ max-link-speed = <3>;
+ };
+...
--
2.38.0



2022-11-08 13:02:46

by Yoshihiro Shimoda

[permalink] [raw]
Subject: RE: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

Hi Serge,

> From: Serge Semin, Sent: Tuesday, November 8, 2022 5:49 AM
>
> Currently the 'interrupts' and 'interrupt-names' properties are defined
> being too generic to really describe any actual IRQ interface. Moreover
> the DW PCIe End-point devices are left with no IRQ signals. All of that
> can be fixed by adding the IRQ-related properties to the common DW PCIe
> DT-schemas in accordance with the hardware reference manual. The DW PCIe
> common DT-schema will contain the generic properties definitions with just
> a number of entries per property, while the DW PCIe RP/EP-specific schemas
> will have the particular number of items and the generic resource names
> listed.
>
> Note since there are DW PCI-based vendor-specific DT-bindings with the
> custom names assigned to the same IRQ resources we have no much choice but
> to add them to the generic DT-schemas in order to have the schemas being
> applicable for such devices. These names are marked as vendor-specific and
> should be avoided being used in new bindings in favor of the generic
> names.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Note without the next dtschema tool fix
>
> --- a/lib.py 2022-09-29 15:17:13.100033810 +0300
> +++ b/lib.py 2022-09-29 15:19:54.886172794 +0300

JFYI.

git am command could not work correctly by this lib.py file:
---
Applying: dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
error: lib.py: does not exist in index
Patch failed at 0001 dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
---

If I used patch command and skipped the lib.py, it could apply this patch correctly.

Best regards,
Yoshihiro Shimoda


2022-11-08 14:28:31

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

Hi Yoshihiro

On Tue, Nov 08, 2022 at 12:40:54PM +0000, Yoshihiro Shimoda wrote:
> Hi Serge,
>
> > From: Serge Semin, Sent: Tuesday, November 8, 2022 5:49 AM
> >
> > Currently the 'interrupts' and 'interrupt-names' properties are defined
> > being too generic to really describe any actual IRQ interface. Moreover
> > the DW PCIe End-point devices are left with no IRQ signals. All of that
> > can be fixed by adding the IRQ-related properties to the common DW PCIe
> > DT-schemas in accordance with the hardware reference manual. The DW PCIe
> > common DT-schema will contain the generic properties definitions with just
> > a number of entries per property, while the DW PCIe RP/EP-specific schemas
> > will have the particular number of items and the generic resource names
> > listed.
> >
> > Note since there are DW PCI-based vendor-specific DT-bindings with the
> > custom names assigned to the same IRQ resources we have no much choice but
> > to add them to the generic DT-schemas in order to have the schemas being
> > applicable for such devices. These names are marked as vendor-specific and
> > should be avoided being used in new bindings in favor of the generic
> > names.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> >
> > ---
> >
> > Note without the next dtschema tool fix
> >
> > --- a/lib.py 2022-09-29 15:17:13.100033810 +0300
> > +++ b/lib.py 2022-09-29 15:19:54.886172794 +0300
>

> JFYI.
>
> git am command could not work correctly by this lib.py file:
> ---
> Applying: dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> error: lib.py: does not exist in index
> Patch failed at 0001 dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> ---
>
> If I used patch command and skipped the lib.py, it could apply this patch correctly.

Got it. Thanks for the note. I'll either drop this part on the next
patchset revision (hopefully Rob will do something about that by then)
or make it less looking like a patch so git am wouldn't be confused.

-Sergey

>
> Best regards,
> Yoshihiro Shimoda
>

2022-11-08 23:05:20

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

On Tue, Nov 8, 2022 at 7:52 AM Serge Semin <[email protected]> wrote:
>
> Hi Yoshihiro
>
> On Tue, Nov 08, 2022 at 12:40:54PM +0000, Yoshihiro Shimoda wrote:
> > Hi Serge,
> >
> > > From: Serge Semin, Sent: Tuesday, November 8, 2022 5:49 AM
> > >
> > > Currently the 'interrupts' and 'interrupt-names' properties are defined
> > > being too generic to really describe any actual IRQ interface. Moreover
> > > the DW PCIe End-point devices are left with no IRQ signals. All of that
> > > can be fixed by adding the IRQ-related properties to the common DW PCIe
> > > DT-schemas in accordance with the hardware reference manual. The DW PCIe
> > > common DT-schema will contain the generic properties definitions with just
> > > a number of entries per property, while the DW PCIe RP/EP-specific schemas
> > > will have the particular number of items and the generic resource names
> > > listed.
> > >
> > > Note since there are DW PCI-based vendor-specific DT-bindings with the
> > > custom names assigned to the same IRQ resources we have no much choice but
> > > to add them to the generic DT-schemas in order to have the schemas being
> > > applicable for such devices. These names are marked as vendor-specific and
> > > should be avoided being used in new bindings in favor of the generic
> > > names.
> > >
> > > Signed-off-by: Serge Semin <[email protected]>
> > >
> > > ---
> > >
> > > Note without the next dtschema tool fix
> > >
> > > --- a/lib.py 2022-09-29 15:17:13.100033810 +0300
> > > +++ b/lib.py 2022-09-29 15:19:54.886172794 +0300
> >
>
> > JFYI.
> >
> > git am command could not work correctly by this lib.py file:
> > ---
> > Applying: dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> > error: lib.py: does not exist in index
> > Patch failed at 0001 dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> > ---
> >
> > If I used patch command and skipped the lib.py, it could apply this patch correctly.
>
> Got it. Thanks for the note. I'll either drop this part on the next
> patchset revision (hopefully Rob will do something about that by then)
> or make it less looking like a patch so git am wouldn't be confused.

Now fixed in main branch. Thanks for the report.

Rob

2022-11-09 02:58:53

by Yoshihiro Shimoda

[permalink] [raw]
Subject: RE: [PATCH v6 17/20] PCI: dwc: Introduce generic resources getter

Hi Serge,

> From: Serge Semin, Sent: Tuesday, November 8, 2022 5:50 AM
>
> Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> the separate parts of the DW PCIe core driver. It doesn't really make
> sense since the both controller types have identical set of the core CSR
> regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> and EP initialization methods by moving the platform-specific registers
> space getting and mapping into a common method. It gets to be even more
> justified seeing the CSRs base address pointers are preserved in the
> common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> initialization will be moved to the new method too in order to have a
> single function for all the generic platform properties handling in single
> place.
>
> A nice side-effect of this change is that the pcie-designware-host.c and
> pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> storage modification, which makes the DW PCIe core, Root Port and Endpoint
> modules more coherent.
>
> Signed-off-by: Serge Semin <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
>
> ---
>
> Changelog v3:
> - This is a new patch created on v3 lap of the series.
>
> Changelog v4:
> - Convert the method name from dw_pcie_get_res() to
> dw_pcie_get_resources(). (@Bjorn)
> ---
> .../pci/controller/dwc/pcie-designware-ep.c | 26 +------
> .../pci/controller/dwc/pcie-designware-host.c | 15 +---
> drivers/pci/controller/dwc/pcie-designware.c | 75 ++++++++++++++-----
> drivers/pci/controller/dwc/pcie-designware.h | 3 +
> 4 files changed, 65 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 237bb01d7852..80a64b63c055 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -13,8 +13,6 @@
> #include <linux/pci-epc.h>
> #include <linux/pci-epf.h>
>
> -#include "../../pci.h"
> -
> void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> {
> struct pci_epc *epc = ep->epc;
> @@ -688,29 +686,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct device *dev = pci->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - struct device_node *np = dev->of_node;

Removing this np causes the following build error if CONFIG_PCIE_DW_EP is enabled:
---
CC drivers/pci/controller/dwc/pcie-designware-ep.o
drivers/pci/controller/dwc/pcie-designware-ep.c: In function 'dw_pcie_ep_init':
drivers/pci/controller/dwc/pcie-designware-ep.c:751:35: error: 'np' undeclared (first use in this function); did you mean 'ep'?
751 | ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
| ^~
| ep
drivers/pci/controller/dwc/pcie-designware-ep.c:751:35: note: each undeclared identifier is reported only once for each function it appears in
---

So, we should keep the np or use "dev->of_node" to the of_property_read_u8().

Best regards,
Yoshihiro Shimoda


2022-11-10 12:37:11

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties

On Tue, Nov 08, 2022 at 04:32:22PM -0600, Rob Herring wrote:
> On Tue, Nov 8, 2022 at 7:52 AM Serge Semin <[email protected]> wrote:
> >
> > Hi Yoshihiro
> >
> > On Tue, Nov 08, 2022 at 12:40:54PM +0000, Yoshihiro Shimoda wrote:
> > > Hi Serge,
> > >
> > > > From: Serge Semin, Sent: Tuesday, November 8, 2022 5:49 AM
> > > >
> > > > Currently the 'interrupts' and 'interrupt-names' properties are defined
> > > > being too generic to really describe any actual IRQ interface. Moreover
> > > > the DW PCIe End-point devices are left with no IRQ signals. All of that
> > > > can be fixed by adding the IRQ-related properties to the common DW PCIe
> > > > DT-schemas in accordance with the hardware reference manual. The DW PCIe
> > > > common DT-schema will contain the generic properties definitions with just
> > > > a number of entries per property, while the DW PCIe RP/EP-specific schemas
> > > > will have the particular number of items and the generic resource names
> > > > listed.
> > > >
> > > > Note since there are DW PCI-based vendor-specific DT-bindings with the
> > > > custom names assigned to the same IRQ resources we have no much choice but
> > > > to add them to the generic DT-schemas in order to have the schemas being
> > > > applicable for such devices. These names are marked as vendor-specific and
> > > > should be avoided being used in new bindings in favor of the generic
> > > > names.
> > > >
> > > > Signed-off-by: Serge Semin <[email protected]>
> > > >
> > > > ---
> > > >
> > > > Note without the next dtschema tool fix
> > > >
> > > > --- a/lib.py 2022-09-29 15:17:13.100033810 +0300
> > > > +++ b/lib.py 2022-09-29 15:19:54.886172794 +0300
> > >
> >
> > > JFYI.
> > >
> > > git am command could not work correctly by this lib.py file:
> > > ---
> > > Applying: dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> > > error: lib.py: does not exist in index
> > > Patch failed at 0001 dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties
> > > ---
> > >
> > > If I used patch command and skipped the lib.py, it could apply this patch correctly.
> >
> > Got it. Thanks for the note. I'll either drop this part on the next
> > patchset revision (hopefully Rob will do something about that by then)
> > or make it less looking like a patch so git am wouldn't be confused.
>
> Now fixed in main branch. Thanks for the report.

Ok. I'll drop that chunk from v7 then.

-Sergey

>
> Rob

2022-11-10 13:55:40

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH v6 17/20] PCI: dwc: Introduce generic resources getter

Hi Yoshihiro

On Wed, Nov 09, 2022 at 02:17:45AM +0000, Yoshihiro Shimoda wrote:
> Hi Serge,
>
> > From: Serge Semin, Sent: Tuesday, November 8, 2022 5:50 AM
> >
> > Currently the DW PCIe Root Port and Endpoint CSR spaces are retrieved in
> > the separate parts of the DW PCIe core driver. It doesn't really make
> > sense since the both controller types have identical set of the core CSR
> > regions: DBI, DBI CS2 and iATU/eDMA. Thus we can simplify the DW PCIe Host
> > and EP initialization methods by moving the platform-specific registers
> > space getting and mapping into a common method. It gets to be even more
> > justified seeing the CSRs base address pointers are preserved in the
> > common DW PCIe descriptor. Note all the OF-based common DW PCIe settings
> > initialization will be moved to the new method too in order to have a
> > single function for all the generic platform properties handling in single
> > place.
> >
> > A nice side-effect of this change is that the pcie-designware-host.c and
> > pcie-designware-ep.c drivers are cleaned up from all the direct dw_pcie
> > storage modification, which makes the DW PCIe core, Root Port and Endpoint
> > modules more coherent.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> >
> > ---
> >
> > Changelog v3:
> > - This is a new patch created on v3 lap of the series.
> >
> > Changelog v4:
> > - Convert the method name from dw_pcie_get_res() to
> > dw_pcie_get_resources(). (@Bjorn)
> > ---
> > .../pci/controller/dwc/pcie-designware-ep.c | 26 +------
> > .../pci/controller/dwc/pcie-designware-host.c | 15 +---
> > drivers/pci/controller/dwc/pcie-designware.c | 75 ++++++++++++++-----
> > drivers/pci/controller/dwc/pcie-designware.h | 3 +
> > 4 files changed, 65 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 237bb01d7852..80a64b63c055 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -13,8 +13,6 @@
> > #include <linux/pci-epc.h>
> > #include <linux/pci-epf.h>
> >
> > -#include "../../pci.h"
> > -
> > void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > {
> > struct pci_epc *epc = ep->epc;
> > @@ -688,29 +686,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > struct device *dev = pci->dev;
> > struct platform_device *pdev = to_platform_device(dev);
> > - struct device_node *np = dev->of_node;
>
> Removing this np causes the following build error if CONFIG_PCIE_DW_EP is enabled:
> ---
> CC drivers/pci/controller/dwc/pcie-designware-ep.o
> drivers/pci/controller/dwc/pcie-designware-ep.c: In function 'dw_pcie_ep_init':
> drivers/pci/controller/dwc/pcie-designware-ep.c:751:35: error: 'np' undeclared (first use in this function); did you mean 'ep'?
> 751 | ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
> | ^~
> | ep
> drivers/pci/controller/dwc/pcie-designware-ep.c:751:35: note: each undeclared identifier is reported only once for each function it appears in
> ---
>
> So, we should keep the np or use "dev->of_node" to the of_property_read_u8().

Indeed. Thanks for noticing that. I'll fix it in v7.

-Sergey

>
> Best regards,
> Yoshihiro Shimoda
>

2022-11-10 21:45:54

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v6 09/20] dt-bindings: PCI: dwc: Add interrupts/interrupt-names common properties


On Mon, 07 Nov 2022 23:49:23 +0300, Serge Semin wrote:
> Currently the 'interrupts' and 'interrupt-names' properties are defined
> being too generic to really describe any actual IRQ interface. Moreover
> the DW PCIe End-point devices are left with no IRQ signals. All of that
> can be fixed by adding the IRQ-related properties to the common DW PCIe
> DT-schemas in accordance with the hardware reference manual. The DW PCIe
> common DT-schema will contain the generic properties definitions with just
> a number of entries per property, while the DW PCIe RP/EP-specific schemas
> will have the particular number of items and the generic resource names
> listed.
>
> Note since there are DW PCI-based vendor-specific DT-bindings with the
> custom names assigned to the same IRQ resources we have no much choice but
> to add them to the generic DT-schemas in order to have the schemas being
> applicable for such devices. These names are marked as vendor-specific and
> should be avoided being used in new bindings in favor of the generic
> names.
>
> Signed-off-by: Serge Semin <[email protected]>
>
> ---
>
> Note without the next dtschema tool fix
>
> --- a/lib.py 2022-09-29 15:17:13.100033810 +0300
> +++ b/lib.py 2022-09-29 15:19:54.886172794 +0300
> @@ -1307,7 +1307,7 @@
> def format_error(filename, error, prefix="", nodename=None, verbose=False):
> src = prefix + os.path.abspath(filename) + ':'
>
> - if error.linecol[0] >= 0:
> + if hasattr(error, 'linecol') and error.linecol[0] >= 0:
> src = src + '%i:%i: ' % (error.linecol[0]+1, error.linecol[1]+1)
> else:
> src += ' '
> @@ -1342,10 +1342,10 @@
> else:
> msg = error.message
>
> - if error.note:
> + if hasattr(error, 'note') and error.note:
> msg += '\n\t' + prefix + 'hint: ' + error.note
>
> - if error.schema_file:
> + if hasattr(error, 'schema_file') and error.schema_file:
> msg += '\n\t' + prefix + 'from schema $id: ' + error.schema_file
>
> return src + msg
>
> any DT-bindings error will cause the dt-schema script crash:
>
> Traceback (most recent call last):
> File "/home/fancer/.local/bin/dt-validate", line 175, in <module>
> sg.check_trees(filename, testtree)
> File "/home/fancer/.local/bin/dt-validate", line 122, in check_trees
> self.check_subtree(dt, subtree, False, "/", "/", filename)
> File "/home/fancer/.local/bin/dt-validate", line 111, in check_subtree
> self.check_subtree(tree, value, disabled, name, fullname + name, filename)
> File "/home/fancer/.local/bin/dt-validate", line 111, in check_subtree
> self.check_subtree(tree, value, disabled, name, fullname + name, filename)
> File "/home/fancer/.local/bin/dt-validate", line 106, in check_subtree
> self.check_node(tree, subtree, disabled, nodename, fullname, filename)
> File "/home/fancer/.local/bin/dt-validate", line 84, in check_node
> print(dtschema.format_error(filename, error, nodename=nodename, verbose=verbose) +
> File "/home/fancer/.local/lib/python3.8/site-packages/dtschema/lib.py", line 1332, in format_error
> msg += '\n' + format_error(filename, suberror, prefix=prefix+"\t", nodename=nodename, verbose=verbose)
> File "/home/fancer/.local/lib/python3.8/site-packages/dtschema/lib.py", line 1310, in format_error
> if error.linecol[0] >= 0:
> AttributeError: 'ValidationError' object has no attribute 'linecol'
>
> Changelog v3:
> - This is a new patch unpinned from the next one:
> https://lore.kernel.org/linux-pci/[email protected]/
> by the Rob' request. (@Rob)
>
> Changelog v5:
> - Add platform-specific interrupt names, but mark them as deprecated.
>
> Changelog v6:
> - Move the common interrupt-names definitions to the RP/EP schemas.
> Thus drop the 'definitions' property. (@Rob)
> - Drop the 'deprecated' keywords from the vendor-specific names. (@Rob)
> ---
> .../bindings/pci/snps,dw-pcie-common.yaml | 19 ++++
> .../bindings/pci/snps,dw-pcie-ep.yaml | 52 +++++++++++
> .../devicetree/bindings/pci/snps,dw-pcie.yaml | 90 ++++++++++++++++++-
> 3 files changed, 158 insertions(+), 3 deletions(-)
>

Reviewed-by: Rob Herring <[email protected]>

2022-11-10 21:51:00

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v6 01/20] dt-bindings: imx6q-pcie: Fix clock names for imx6sx and imx8mq

On Mon, Nov 07, 2022 at 11:49:15PM +0300, Serge Semin wrote:
> Originally as it was defined the legacy bindings the pcie_inbound_axi and
> pcie_aux clock names were supposed to be used in the fsl,imx6sx-pcie and
> fsl,imx8mq-pcie devices respectively. But the bindings conversion has been
> incorrectly so now the fourth clock name is defined as "pcie_inbound_axi
> for imx6sx-pcie, pcie_aux for imx8mq-pcie", which is completely wrong.
> Let's fix that by conditionally apply the clock-names constraints based on
> the compatible string content.
>
> Fixes: 751ca492f131 ("dt-bindings: PCI: imx6: convert the imx pcie controller to dtschema")
> Signed-off-by: Serge Semin <[email protected]>
> Acked-by: Alexander Stein <[email protected]>
>
> ---
>
> Changelog v5:
> - This is a new patch added on the v5 release of the patchset.
> ---
> .../bindings/pci/fsl,imx6q-pcie.yaml | 47 +++++++++++++++++--
> 1 file changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index 376e739bcad4..ebfe75f1576e 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -16,6 +16,47 @@ description: |+
>
> allOf:
> - $ref: /schemas/pci/snps,dw-pcie.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: fsl,imx6sx-pcie
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: pcie
> + - const: pcie_bus
> + - const: pcie_phy
> + - const: pcie_inbound_axi
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: fsl,imx8mq-pcie
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: pcie
> + - const: pcie_bus
> + - const: pcie_phy
> + - const: pcie_aux
> + - if:
> + properties:
> + compatible:
> + not:
> + contains:
> + enum:
> + - fsl,imx6sx-pcie
> + - fsl,imx8mq-pcie
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: pcie
> + - const: pcie_bus
> + - const: pcie_phy
>
> properties:
> compatible:
> @@ -57,11 +98,7 @@ properties:
>
> clock-names:
> minItems: 3
> - items:
> - - const: pcie
> - - const: pcie_bus
> - - const: pcie_phy
> - - const: pcie_inbound_axi for imx6sx-pcie, pcie_aux for imx8mq-pcie

This should have been just 'enum: [ pcie_inbound_axi, pcie_aux ]'

And then do:

- if:
properties:
compatible:
contains:
const: fsl,imx8mq-pcie
then:
properties:
clock-names:
items:
- {}
- {}
- {}
- const: pcie_aux


And then another if/then with 'maxItems: 3'

2022-11-11 11:29:44

by Serge Semin

[permalink] [raw]
Subject: Re: [PATCH v6 01/20] dt-bindings: imx6q-pcie: Fix clock names for imx6sx and imx8mq

On Thu, Nov 10, 2022 at 03:01:04PM -0600, Rob Herring wrote:
> On Mon, Nov 07, 2022 at 11:49:15PM +0300, Serge Semin wrote:
> > Originally as it was defined the legacy bindings the pcie_inbound_axi and
> > pcie_aux clock names were supposed to be used in the fsl,imx6sx-pcie and
> > fsl,imx8mq-pcie devices respectively. But the bindings conversion has been
> > incorrectly so now the fourth clock name is defined as "pcie_inbound_axi
> > for imx6sx-pcie, pcie_aux for imx8mq-pcie", which is completely wrong.
> > Let's fix that by conditionally apply the clock-names constraints based on
> > the compatible string content.
> >
> > Fixes: 751ca492f131 ("dt-bindings: PCI: imx6: convert the imx pcie controller to dtschema")
> > Signed-off-by: Serge Semin <[email protected]>
> > Acked-by: Alexander Stein <[email protected]>
> >
> > ---
> >
> > Changelog v5:
> > - This is a new patch added on the v5 release of the patchset.
> > ---
> > .../bindings/pci/fsl,imx6q-pcie.yaml | 47 +++++++++++++++++--
> > 1 file changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > index 376e739bcad4..ebfe75f1576e 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > @@ -16,6 +16,47 @@ description: |+
> >
> > allOf:
> > - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: fsl,imx6sx-pcie
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: pcie
> > + - const: pcie_bus
> > + - const: pcie_phy
> > + - const: pcie_inbound_axi
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: fsl,imx8mq-pcie
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: pcie
> > + - const: pcie_bus
> > + - const: pcie_phy
> > + - const: pcie_aux
> > + - if:
> > + properties:
> > + compatible:
> > + not:
> > + contains:
> > + enum:
> > + - fsl,imx6sx-pcie
> > + - fsl,imx8mq-pcie
> > + then:
> > + properties:
> > + clock-names:
> > + items:
> > + - const: pcie
> > + - const: pcie_bus
> > + - const: pcie_phy
> >
> > properties:
> > compatible:
> > @@ -57,11 +98,7 @@ properties:
> >
> > clock-names:
> > minItems: 3
> > - items:
> > - - const: pcie
> > - - const: pcie_bus
> > - - const: pcie_phy
> > - - const: pcie_inbound_axi for imx6sx-pcie, pcie_aux for imx8mq-pcie
>

> This should have been just 'enum: [ pcie_inbound_axi, pcie_aux ]'
>
> And then do:
>
> - if:
> properties:
> compatible:
> contains:
> const: fsl,imx8mq-pcie
> then:
> properties:
> clock-names:
> items:
> - {}
> - {}
> - {}
> - const: pcie_aux
>
>
> And then another if/then with 'maxItems: 3'

Ok. Will fix it in v7. But IMO it looks a bit less descriptive
especially with the '{}' pattern and a need to look in two different
places to comprehend the whole constraint. I understand though what is an
intention of such construction. It's to place as much info into the
schema body and isolate the platform-specific constraints in the allOf
clause. Pretty neat anyway.

-Sergey