| 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: <006501cf70b0$41dd2240$c59766c0$%han@samsung.com>
Date: Fri, 16 May 2014 11:40:53 +0900
From: Jingoo Han <jg1.han@...sung.com>
To: 'Murali Karicheri' <m-karicheri2@...com>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
'Mohit Kumar' <mohit.kumar@...com>,
'Bjorn Helgaas' <bhelgaas@...gle.com>,
'Santosh Shilimkar' <santosh.shilimkar@...com>,
'Jingoo Han' <jg1.han@...sung.com>,
'Pratyush Anand' <pratyush.anand@...com>,
'Richard Zhu' <r65037@...escale.com>,
'Kishon Vijay Abraham I' <kishon@...com>,
'Marek Vasut' <marex@...x.de>
Subject: Re: [PATCH v1 2/5] pci: designware: enhancements to support keystone
pcie
On Friday, May 16, 2014 1:01 AM, Murali Karicheri wrote:
>
> keystone pcie hardware is based on designware hw version 3.65.
> There is no support for ATU port and has registers in
> application space to configure inbound/outbound access. Also
> doesn't support PCI PVM option. The MSI IRQ registers available
> in application space is used to mask/unmask/enable the MSI IRQs.
>
> DW core driver is a set of common functions that are abstracted
> to support DW pci drivers. To allow re-use of these functions for
> keystone pci driver, core driver is to be enhanced.
>
> Following are done to allow re-use of the functions on keystone pci
> driver.
>
> 1. Some of the variables in pcie_port struct is folded inside
> a union that now contains both new DW hw related variables as well
> as old hardware related variables such as application reg base.
> 2. Added a dw_pcie_common_host_init() function that holds common
> host initialization code for old and new hw.
> 3. dw_pcie_parse_resource() is used for parsing resource related
> information from DT bindings.
> 4. dw_pcie_host_init() is called by new DW hw drivers as before.
> Added dw_old_pcie_host_init() is it's counter part on old dw hw.
> Both these functions now call dw_pcie_common_host_init().
> 5. Some of the static functions are made global to allow use from
> dw old pci drivers such as pci-keystone.
>
> CC: Mohit Kumar <mohit.kumar@...com>
> CC: Jingoo Han <jg1.han@...sung.com>
> CC: Bjorn Helgaas <bhelgaas@...gle.com>
> CC: Santosh Shilimkar <santosh.shilimkar@...com>
>
> Signed-off-by: Murali Karicheri <m-karicheri2@...com>
> ---
> drivers/pci/host/pcie-designware.c | 101 ++++++++++++++++++++++++------------
> drivers/pci/host/pcie-designware.h | 42 ++++++++++++---
> 2 files changed, 103 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index c4e3732..9ea8e79 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -277,11 +277,15 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
> }
> set_bit(pos0 + i, pp->msi_irq_in_use);
> /*Enable corresponding interrupt in MSI interrupt controller */
> - res = ((pos0 + i) / 32) * 12;
> - bit = (pos0 + i) % 32;
> - dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
> - val |= 1 << bit;
> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
> + if (!(pp->version & DW_VERSION_OLD)) {
(+cc Pratyush Anand, Richard Zhu, Kishon Vijay Abraham I, Marek Vasut)
DW_VERSION_OLD?
I don't this name. What is OLD? It is too general.
There should be the proper feature name between your 3.65 version
and other newer versions. This feature name can be defined as DT
property. Also, this code should be selected by DT property.
> + res = ((pos0 + i) / 32) * 12;
> + bit = (pos0 + i) % 32;
> + dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res,
> + 4, &val);
> + val |= 1 << bit;
> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res,
> + 4, val);
> + }
> }
>
> *pos = pos0;
> @@ -349,7 +353,10 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
> */
> desc->msi_attrib.multiple = msgvec;
>
> - msg.address_lo = virt_to_phys((void *)pp->msi_data);
> + if (pp->ops->get_msi_data)
> + msg.address_lo = pp->ops->get_msi_data(pp);
> + else
> + msg.address_lo = virt_to_phys((void *)pp->msi_data);
> msg.address_hi = 0x0;
> msg.data = pos;
> write_msi_msg(irq, &msg);
> @@ -389,13 +396,11 @@ static const struct irq_domain_ops msi_domain_ops = {
> .map = dw_pcie_msi_map,
> };
>
> -int __init dw_pcie_host_init(struct pcie_port *pp)
> +int __init dw_pcie_parse_resource(struct pcie_port *pp)
> {
> struct device_node *np = pp->dev->of_node;
> - struct of_pci_range range;
> struct of_pci_range_parser parser;
> - u32 val;
> - int i;
> + struct of_pci_range range;
>
> if (of_pci_range_parser_init(&parser, np)) {
> dev_err(pp->dev, "missing ranges property\n");
> @@ -440,23 +445,17 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> return -ENOMEM;
> }
> }
> -
> - pp->cfg0_base = pp->cfg.start;
> - pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> pp->mem_base = pp->mem.start;
>
> - pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> - pp->config.cfg0_size);
> - if (!pp->va_cfg0_base) {
> - dev_err(pp->dev, "error with ioremap in function\n");
> - return -ENOMEM;
> - }
> - pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
> - pp->config.cfg1_size);
> - if (!pp->va_cfg1_base) {
> - dev_err(pp->dev, "error with ioremap\n");
> - return -ENOMEM;
> - }
> + return 0;
> +}
> +
> +int __init dw_pcie_common_host_init(struct pcie_port *pp, struct hw_pci *hw,
> + const struct irq_domain_ops *irq_msi_ops)
> +{
> + struct device_node *np = pp->dev->of_node;
> + u32 val;
> + int i;
>
> if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
> dev_err(pp->dev, "Failed to parse the number of lanes\n");
> @@ -465,7 +464,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
> - MAX_MSI_IRQS, &msi_domain_ops,
> + MAX_MSI_IRQS, irq_msi_ops,
> &dw_pcie_msi_chip);
> if (!pp->irq_domain) {
> dev_err(pp->dev, "irq domain init failed\n");
> @@ -488,10 +487,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> val |= PORT_LOGIC_SPEED_CHANGE;
> dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
>
> - dw_pci.nr_controllers = 1;
> - dw_pci.private_data = (void **)&pp;
> + hw->nr_controllers = 1;
> + hw->private_data = (void **)&pp;
>
> - pci_common_init_dev(pp->dev, &dw_pci);
> + pci_common_init_dev(pp->dev, hw);
> pci_assign_unassigned_resources();
> #ifdef CONFIG_PCI_DOMAINS
> dw_pci.domain++;
> @@ -500,6 +499,32 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> return 0;
> }
>
> +int __init dw_pcie_host_init(struct pcie_port *pp)
> +{
> + int ret;
> +
> + ret = dw_pcie_parse_resource(pp);
> + if (ret)
> + return ret;
> +
> + pp->cfg0_base = pp->cfg.start;
> + pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> + pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> + pp->config.cfg0_size);
> + if (!pp->va_cfg0_base) {
> + dev_err(pp->dev, "error with ioremap in function\n");
> + return -ENOMEM;
> + }
> + pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
> + pp->config.cfg1_size);
> + if (!pp->va_cfg1_base) {
> + dev_err(pp->dev, "error with ioremap\n");
> + return -ENOMEM;
> + }
> +
> + return dw_pcie_common_host_init(pp, &dw_pci, &msi_domain_ops);
> +}
> +
> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
> {
> /* Program viewport 0 : OUTBOUND : CFG0 */
> @@ -654,7 +679,11 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>
> spin_lock_irqsave(&pp->conf_lock, flags);
> if (bus->number != pp->root_bus_nr)
> - ret = dw_pcie_rd_other_conf(pp, bus, devfn,
> + if (pp->ops->rd_other_conf)
> + ret = pp->ops->rd_other_conf(pp, bus, devfn,
> + where, size, val);
> + else
> + ret = dw_pcie_rd_other_conf(pp, bus, devfn,
> where, size, val);
> else
> ret = dw_pcie_rd_own_conf(pp, where, size, val);
> @@ -680,7 +709,11 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>
> spin_lock_irqsave(&pp->conf_lock, flags);
> if (bus->number != pp->root_bus_nr)
> - ret = dw_pcie_wr_other_conf(pp, bus, devfn,
> + if (pp->ops->wr_other_conf)
> + ret = pp->ops->wr_other_conf(pp, bus, devfn,
> + where, size, val);
> + else
> + ret = dw_pcie_wr_other_conf(pp, bus, devfn,
> where, size, val);
> else
> ret = dw_pcie_wr_own_conf(pp, where, size, val);
> @@ -694,7 +727,7 @@ static struct pci_ops dw_pcie_ops = {
> .write = dw_pcie_wr_conf,
> };
>
> -static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> +int dw_pcie_setup(int nr, struct pci_sys_data *sys)
You removed 'static'; however, this modification is related
to another patch. ([PATCH v1 4/5] pci: dw: add common functions
to support old hw based pci driver)
Please don't mix the modification. So, move this fix to the 4th
patch.
> {
> struct pcie_port *pp;
>
> @@ -717,7 +750,7 @@ static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> return 1;
> }
>
> -static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> +struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> {
> struct pci_bus *bus;
> struct pcie_port *pp = sys_to_pcie(sys);
> @@ -746,7 +779,7 @@ static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> return irq;
> }
>
> -static void dw_pcie_add_bus(struct pci_bus *bus)
> +void dw_pcie_add_bus(struct pci_bus *bus)
> {
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> struct pcie_port *pp = sys_to_pcie(bus->sysdata);
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index 3063b35..e97f4d7 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -35,21 +35,39 @@ struct pcie_port {
> struct device *dev;
> u8 root_bus_nr;
> void __iomem *dbi_base;
> - u64 cfg0_base;
> - void __iomem *va_cfg0_base;
> - u64 cfg1_base;
> - void __iomem *va_cfg1_base;
> + /*
> + * Old DW version implement application register space for
> + * MSI and has no ATU view port
> + */
ATU can be defined as DT property. It can be selected by
this DT property.
> +#define DW_VERSION_OLD BIT(0)
> + u32 version;
> + union {
> + /* new dw core specific */
> + struct {
> + u64 cfg0_base;
> + void __iomem *va_cfg0_base;
> + u64 cfg1_base;
> + void __iomem *va_cfg1_base;
> + int msi_irq;
> + };
> +
> + /* old dw core specific */
> + struct {
> + struct irq_domain *legacy_irq_domain;
> + void __iomem *va_app_base;
> + u64 app_base;
> + };
> + };
> u64 io_base;
> u64 mem_base;
> spinlock_t conf_lock;
> - struct resource cfg;
> struct resource io;
> struct resource mem;
> + struct resource cfg;
Please don't move this variable, without the reason.
Keep in mind that this code is shared with other SoC
drivers. So, without special reason, please don't modify
it.
> struct pcie_port_info config;
> int irq;
> u32 lanes;
> struct pcie_host_ops *ops;
> - int msi_irq;
> struct irq_domain *irq_domain;
> unsigned long msi_data;
> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> @@ -62,8 +80,13 @@ struct pcie_host_ops {
> u32 val, void __iomem *dbi_base);
> int (*rd_own_conf)(struct pcie_port *pp, int where, int size, u32 *val);
> int (*wr_own_conf)(struct pcie_port *pp, int where, int size, u32 val);
> + int (*rd_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 *val);
> + int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 val);
> int (*link_up)(struct pcie_port *pp);
> void (*host_init)(struct pcie_port *pp);
> + u32 (*get_msi_data)(struct pcie_port *pp);
> };
>
> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
> @@ -73,5 +96,12 @@ void dw_pcie_msi_init(struct pcie_port *pp);
> int dw_pcie_link_up(struct pcie_port *pp);
> void dw_pcie_setup_rc(struct pcie_port *pp);
> int dw_pcie_host_init(struct pcie_port *pp);
> +int dw_pcie_setup(int nr, struct pci_sys_data *sys);
> +struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys);
> +void dw_pcie_add_bus(struct pci_bus *bus);
> +int dw_pcie_parse_resource(struct pcie_port *pp);
>
> +/* internal to dw core */
> +int dw_pcie_common_host_init(struct pcie_port *pp, struct hw_pci *hw,
> + const struct irq_domain_ops *irq_ops);
> #endif /* _PCIE_DESIGNWARE_H */
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists