| 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: <Zn57sVccaBog2KeV@daire-X570>
Date: Fri, 28 Jun 2024 10:02:31 +0100
From: Daire McNamara <daire.mcnamara@...rochip.com>
To: Conor Dooley <conor@...nel.org>
CC: <linux-pci@...r.kernel.org>, Conor Dooley <conor.dooley@...rochip.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof
Wilczyński <kw@...ux.com>, Rob Herring <robh@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH v2 2/2] PCI: microchip: rework reg region handing to
support using either instance 1 or 2
On Wed, Jun 12, 2024 at 11:57:56AM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@...rochip.com>
>
> The PCI host controller on PolarFire SoC has multiple "instances", each
> with their own bridge and ctrl address spaces. The original binding has
> an "apb" register region, and it is expected to be set to the base
> address of the host controllers register space. Defines in the driver
> were used to compute the addresses of the bridge and ctrl address ranges
> corresponding to instance1. Some customers want to use instance0 however
> and that requires changing the defines in the driver, which is clearly
> not a portable solution.
>
> The binding has been changed from a single register region to a pair,
> corresponding to the bridge and ctrl regions respectively, so modify the
> driver to read these regions directly from the devicetree rather than
> compute them from the base address of the abp region.
>
> To maintain backwards compatibility with the existing binding, the
> driver retains code to handle the "abp" reg and computes the base
> address of the bridge and ctrl regions using the defines if it is
> present. reg-names has always been a required property, so this is
> safe to do.
>
> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> ---
> drivers/pci/controller/pcie-microchip-host.c | 155 +++++++++----------
> 1 file changed, 73 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 137fb8570ba2..2580a5bd2743 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -27,9 +27,6 @@
> #define MC_PCIE1_BRIDGE_ADDR 0x00008000u
> #define MC_PCIE1_CTRL_ADDR 0x0000a000u
>
> -#define MC_PCIE_BRIDGE_ADDR (MC_PCIE1_BRIDGE_ADDR)
> -#define MC_PCIE_CTRL_ADDR (MC_PCIE1_CTRL_ADDR)
> -
> /* PCIe Bridge Phy Regs */
> #define PCIE_PCI_IRQ_DW0 0xa8
> #define MSIX_CAP_MASK BIT(31)
> @@ -207,7 +204,6 @@
> [EVENT_LOCAL_ ## x] = { __stringify(x), s }
>
> #define PCIE_EVENT(x) \
> - .base = MC_PCIE_CTRL_ADDR, \
> .offset = PCIE_EVENT_INT, \
> .mask_offset = PCIE_EVENT_INT, \
> .mask_high = 1, \
> @@ -215,7 +211,6 @@
> .enb_mask = PCIE_EVENT_INT_ENB_MASK
>
> #define SEC_EVENT(x) \
> - .base = MC_PCIE_CTRL_ADDR, \
> .offset = SEC_ERROR_INT, \
> .mask_offset = SEC_ERROR_INT_MASK, \
> .mask = SEC_ERROR_INT_ ## x ## _INT, \
> @@ -223,7 +218,6 @@
> .enb_mask = 0
>
> #define DED_EVENT(x) \
> - .base = MC_PCIE_CTRL_ADDR, \
> .offset = DED_ERROR_INT, \
> .mask_offset = DED_ERROR_INT_MASK, \
> .mask_high = 1, \
> @@ -231,7 +225,6 @@
> .enb_mask = 0
>
> #define LOCAL_EVENT(x) \
> - .base = MC_PCIE_BRIDGE_ADDR, \
> .offset = ISTATUS_LOCAL, \
> .mask_offset = IMASK_LOCAL, \
> .mask_high = 0, \
> @@ -265,7 +258,8 @@ struct mc_msi {
> };
>
> struct mc_pcie {
> - void __iomem *axi_base_addr;
> + void __iomem *bridge_base_addr;
> + void __iomem *ctrl_base_addr;
> struct device *dev;
> struct irq_domain *intx_domain;
> struct irq_domain *event_domain;
> @@ -344,7 +338,6 @@ static struct event_map local_status_to_event[] = {
> };
>
> static struct {
> - u32 base;
> u32 offset;
> u32 mask;
> u32 shift;
> @@ -415,18 +408,17 @@ static void mc_handle_msi(struct irq_desc *desc)
> struct irq_chip *chip = irq_desc_get_chip(desc);
> struct device *dev = port->dev;
> struct mc_msi *msi = &port->msi;
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> unsigned long status;
> u32 bit;
> int ret;
>
> chained_irq_enter(chip, desc);
>
> - status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> + status = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
> if (status & PM_MSI_INT_MSI_MASK) {
> - writel_relaxed(status & PM_MSI_INT_MSI_MASK, bridge_base_addr + ISTATUS_LOCAL);
> - status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> + writel_relaxed(status & PM_MSI_INT_MSI_MASK,
> + port->bridge_base_addr + ISTATUS_LOCAL);
> + status = readl_relaxed(port->bridge_base_addr + ISTATUS_MSI);
> for_each_set_bit(bit, &status, msi->num_vectors) {
> ret = generic_handle_domain_irq(msi->dev_domain, bit);
> if (ret)
> @@ -441,11 +433,9 @@ static void mc_handle_msi(struct irq_desc *desc)
> static void mc_msi_bottom_irq_ack(struct irq_data *data)
> {
> struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> u32 bitpos = data->hwirq;
>
> - writel_relaxed(BIT(bitpos), bridge_base_addr + ISTATUS_MSI);
> + writel_relaxed(BIT(bitpos), port->bridge_base_addr + ISTATUS_MSI);
> }
>
> static void mc_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> @@ -564,15 +554,13 @@ static void mc_handle_intx(struct irq_desc *desc)
> struct mc_pcie *port = irq_desc_get_handler_data(desc);
> struct irq_chip *chip = irq_desc_get_chip(desc);
> struct device *dev = port->dev;
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> unsigned long status;
> u32 bit;
> int ret;
>
> chained_irq_enter(chip, desc);
>
> - status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> + status = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
> if (status & PM_MSI_INT_INTX_MASK) {
> status &= PM_MSI_INT_INTX_MASK;
> status >>= PM_MSI_INT_INTX_SHIFT;
> @@ -590,42 +578,36 @@ static void mc_handle_intx(struct irq_desc *desc)
> static void mc_ack_intx_irq(struct irq_data *data)
> {
> struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
>
> - writel_relaxed(mask, bridge_base_addr + ISTATUS_LOCAL);
> + writel_relaxed(mask, port->bridge_base_addr + ISTATUS_LOCAL);
> }
>
> static void mc_mask_intx_irq(struct irq_data *data)
> {
> struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> unsigned long flags;
> u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
> u32 val;
>
> raw_spin_lock_irqsave(&port->lock, flags);
> - val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> + val = readl_relaxed(port->bridge_base_addr + IMASK_LOCAL);
> val &= ~mask;
> - writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> + writel_relaxed(val, port->bridge_base_addr + IMASK_LOCAL);
> raw_spin_unlock_irqrestore(&port->lock, flags);
> }
>
> static void mc_unmask_intx_irq(struct irq_data *data)
> {
> struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> unsigned long flags;
> u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
> u32 val;
>
> raw_spin_lock_irqsave(&port->lock, flags);
> - val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> + val = readl_relaxed(port->bridge_base_addr + IMASK_LOCAL);
> val |= mask;
> - writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> + writel_relaxed(val, port->bridge_base_addr + IMASK_LOCAL);
> raw_spin_unlock_irqrestore(&port->lock, flags);
> }
>
> @@ -656,8 +638,7 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
>
> static u32 pcie_events(struct mc_pcie *port)
> {
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> - u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
> + u32 reg = readl_relaxed(port->ctrl_base_addr + PCIE_EVENT_INT);
> u32 val = 0;
> int i;
>
> @@ -669,8 +650,7 @@ static u32 pcie_events(struct mc_pcie *port)
>
> static u32 sec_errors(struct mc_pcie *port)
> {
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> - u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
> + u32 reg = readl_relaxed(port->ctrl_base_addr + SEC_ERROR_INT);
> u32 val = 0;
> int i;
>
> @@ -682,8 +662,7 @@ static u32 sec_errors(struct mc_pcie *port)
>
> static u32 ded_errors(struct mc_pcie *port)
> {
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> - u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
> + u32 reg = readl_relaxed(port->ctrl_base_addr + DED_ERROR_INT);
> u32 val = 0;
> int i;
>
> @@ -695,8 +674,7 @@ static u32 ded_errors(struct mc_pcie *port)
>
> static u32 local_events(struct mc_pcie *port)
> {
> - void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> - u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> + u32 reg = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
> u32 val = 0;
> int i;
>
> @@ -758,8 +736,12 @@ static void mc_ack_event_irq(struct irq_data *data)
> void __iomem *addr;
> u32 mask;
>
> - addr = port->axi_base_addr + event_descs[event].base +
> - event_descs[event].offset;
> + if (event_descs[event].offset == ISTATUS_LOCAL)
> + addr = port->bridge_base_addr;
> + else
> + addr = port->ctrl_base_addr;
> +
> + addr += event_descs[event].offset;
> mask = event_descs[event].mask;
> mask |= event_descs[event].enb_mask;
>
> @@ -774,8 +756,12 @@ static void mc_mask_event_irq(struct irq_data *data)
> u32 mask;
> u32 val;
>
> - addr = port->axi_base_addr + event_descs[event].base +
> - event_descs[event].mask_offset;
> + if (event_descs[event].offset == ISTATUS_LOCAL)
> + addr = port->bridge_base_addr;
> + else
> + addr = port->ctrl_base_addr;
> +
> + addr += event_descs[event].mask_offset;
> mask = event_descs[event].mask;
> if (event_descs[event].enb_mask) {
> mask <<= PCIE_EVENT_INT_ENB_SHIFT;
> @@ -804,8 +790,12 @@ static void mc_unmask_event_irq(struct irq_data *data)
> u32 mask;
> u32 val;
>
> - addr = port->axi_base_addr + event_descs[event].base +
> - event_descs[event].mask_offset;
> + if (event_descs[event].offset == ISTATUS_LOCAL)
> + addr = port->bridge_base_addr;
> + else
> + addr = port->ctrl_base_addr;
> +
> + addr += event_descs[event].mask_offset;
> mask = event_descs[event].mask;
>
> if (event_descs[event].enb_mask)
> @@ -972,8 +962,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
> static int mc_pcie_setup_windows(struct platform_device *pdev,
> struct mc_pcie *port)
> {
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> struct resource_entry *entry;
> u64 pci_addr;
> @@ -982,7 +970,7 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
> resource_list_for_each_entry(entry, &bridge->windows) {
> if (resource_type(entry->res) == IORESOURCE_MEM) {
> pci_addr = entry->res->start - entry->offset;
> - mc_pcie_setup_window(bridge_base_addr, index,
> + mc_pcie_setup_window(port->bridge_base_addr, index,
> entry->res->start, pci_addr,
> resource_size(entry->res));
> index++;
> @@ -994,26 +982,20 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>
> static inline void mc_clear_secs(struct mc_pcie *port)
> {
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -
> - writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> + writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
> SEC_ERROR_INT);
> - writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> + writel_relaxed(0, port->ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> }
>
> static inline void mc_clear_deds(struct mc_pcie *port)
> {
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -
> - writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> + writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
> DED_ERROR_INT);
> - writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> + writel_relaxed(0, port->ctrl_base_addr + DED_ERROR_EVENT_CNT);
> }
>
> static void mc_disable_interrupts(struct mc_pcie *port)
> {
> - void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> - void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> u32 val;
>
> /* Ensure ECC bypass is enabled */
> @@ -1021,22 +1003,22 @@ static void mc_disable_interrupts(struct mc_pcie *port)
> ECC_CONTROL_RX_RAM_ECC_BYPASS |
> ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS |
> ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
> - writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
> + writel_relaxed(val, port->ctrl_base_addr + ECC_CONTROL);
>
> /* Disable SEC errors and clear any outstanding */
> - writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> + writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
> SEC_ERROR_INT_MASK);
> mc_clear_secs(port);
>
> /* Disable DED errors and clear any outstanding */
> - writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> + writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
> DED_ERROR_INT_MASK);
> mc_clear_deds(port);
>
> /* Disable local interrupts and clear any outstanding */
> - writel_relaxed(0, bridge_base_addr + IMASK_LOCAL);
> - writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_LOCAL);
> - writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_MSI);
> + writel_relaxed(0, port->bridge_base_addr + IMASK_LOCAL);
> + writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_LOCAL);
> + writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_MSI);
>
> /* Disable PCIe events and clear any outstanding */
> val = PCIE_EVENT_INT_L2_EXIT_INT |
> @@ -1045,11 +1027,11 @@ static void mc_disable_interrupts(struct mc_pcie *port)
> PCIE_EVENT_INT_L2_EXIT_INT_MASK |
> PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK |
> PCIE_EVENT_INT_DLUP_EXIT_INT_MASK;
> - writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> + writel_relaxed(val, port->ctrl_base_addr + PCIE_EVENT_INT);
>
> /* Disable host interrupts and clear any outstanding */
> - writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> - writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> + writel_relaxed(0, port->bridge_base_addr + IMASK_HOST);
> + writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_HOST);
> }
>
> static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port)
> @@ -1112,14 +1094,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
> {
> struct device *dev = cfg->parent;
> struct platform_device *pdev = to_platform_device(dev);
> - void __iomem *bridge_base_addr =
> - port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> int ret;
>
> /* Configure address translation table 0 for PCIe config space */
> - mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> - cfg->res.start,
> - resource_size(&cfg->res));
> + mc_pcie_setup_window(port->bridge_base_addr, 0, cfg->res.start,
> + cfg->res.start, resource_size(&cfg->res));
>
> /* Need some fixups in config space */
> mc_pcie_enable_msi(port, cfg->win);
> @@ -1140,7 +1119,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
> static int mc_host_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - void __iomem *bridge_base_addr;
> + void __iomem *axi_base_addr;
This is an apb bus. The variable name should be apb_base_addr, not axi_base_addr
> int ret;
> u32 val;
>
> @@ -1150,28 +1129,40 @@ static int mc_host_probe(struct platform_device *pdev)
>
> port->dev = dev;
>
> - port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> - if (IS_ERR(port->axi_base_addr))
> - return PTR_ERR(port->axi_base_addr);
> + port->bridge_base_addr = devm_platform_ioremap_resource_byname(pdev, "bridge");
> + port->ctrl_base_addr = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> + if(!IS_ERR(port->bridge_base_addr) && !IS_ERR(port->ctrl_base_addr))
> + goto addrs_set;
>
> + /*
> + * The original, incorrect, binding that lumped the control and
> + * bridge addresses together still needs to be handled by the driver.
> + */
> + axi_base_addr = devm_platform_ioremap_resource_byname(pdev, "apb");
> + if (IS_ERR(axi_base_addr))
> + return dev_err_probe(dev, PTR_ERR(port->bridge_base_addr),
> + "both legacy apb register and ctrl/bridge regions missing");
> +
> + port->bridge_base_addr = axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> + port->ctrl_base_addr = axi_base_addr + MC_PCIE1_CTRL_ADDR;
> +
> +addrs_set:
> mc_disable_interrupts(port);
>
> - bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -
> /* Allow enabling MSI by disabling MSI-X */
> - val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> + val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
> val &= ~MSIX_CAP_MASK;
> - writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
> + writel(val, port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>
> /* Pick num vectors from bitfile programmed onto FPGA fabric */
> - val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> + val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
> val &= NUM_MSI_MSGS_MASK;
> val >>= NUM_MSI_MSGS_SHIFT;
>
> port->msi.num_vectors = 1 << val;
>
> /* Pick vector address from design */
> - port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
> + port->msi.vector_phy = readl_relaxed(port->bridge_base_addr + IMSI_ADDR);
>
> ret = mc_pcie_init_clks(dev);
> if (ret) {
> --
> 2.43.0
>
>
Powered by blists - more mailing lists