| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221114083903.r2vyuyotwkf52jk7@mobilestation>
Date: Mon, 14 Nov 2022 11:39:03 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Serge Semin <Sergey.Semin@...kalelectronics.ru>,
Rob Herring <robh+dt@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Cai Huoqing <cai.huoqing@...ux.dev>,
Robin Murphy <robin.murphy@....com>,
Jingoo Han <jingoohan1@...il.com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
Frank Li <Frank.Li@....com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
caihuoqing <caihuoqing@...du.com>, Vinod Koul <vkoul@...nel.org>,
linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 17/20] PCI: dwc: Introduce generic resources getter
On Mon, Nov 14, 2022 at 12:16:54PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Nov 13, 2022 at 10:12:58PM +0300, Serge Semin wrote:
> > 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.
> >
>
> You have clubbed both generic resource API and introducing CDM_CHECK flag.
> Please split them into separate patches.
This modification is a part of the new method dw_pcie_get_resources().
Without that method there is no point in adding the new flag. So no.
It's better to have all of it in a single patch as a part of creating
a coherent resources getter method.
-Sergey
>
> Thanks,
> Mani
>
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > Reviewed-by: Rob Herring <robh@...nel.org>
> >
> > ---
> >
> > 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)
> >
> > Changelog v7:
> > - Get back device.of_node pointer to the dw_pcie_ep_init() method.
> > (@Yoshihiro)
> > ---
> > .../pci/controller/dwc/pcie-designware-ep.c | 25 +------
> > .../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(+), 53 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 237bb01d7852..f68d1ab83bb3 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;
> > @@ -694,23 +692,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >
> > 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 +723,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.1
> >
> >
>
> --
> மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists