[2/5] drivers/bus: Split Arm CCI driver

Commit Message

Robin Murphy Feb. 14, 2018, 2:28 p.m. UTC

On 14/02/18 11:51, Will Deacon wrote:
> On Wed, Feb 07, 2018 at 01:07:53PM +0000, Robin Murphy wrote:
>> Hi Punit,
>>
>> On 07/02/18 12:22, Punit Agrawal wrote:
>> [...]
>>>> -static void __iomem *cci_ctrl_base;
>>>> +void __iomem *cci_ctrl_base __ro_after_init;
>>>
>>> Initially I wondered if cci_ctrl_base gets used in the pmu driver before
>>> it's initialised. But as it gets set in early_initcall() that looks to
>>> be fine.
>>
>> In fact it's even more robust than initcall ordering, since the PMU device
>> will only be created at all via cci_platform_probe(), thus cci_init() is
>> guaranteed to have run successfully before the PMU driver probe can ever
>> touch anything.
> 
> Could you hijack the platform data at this point by passing an of_dev_auxdata
> to of_platform_populate and then use that to pass the __iomem address to the
> PMU driver?

OK, I've cooked up the patch below (on top of this series), but I'm
currently somewhat on the fence about how nice it really is :/

If it helps, I have an updated branch pushed out here:

   git://linux-arm.org/linux-rm.git perf

Robin.

----->8-----
From: Robin Murphy <[e-mail]>
Subject: [PATCH] perf/arm-cci: Untangle global cci_ctrl_base

Depending on the bus driver's global cci_ctrl_base variable is a little
unpleasant, and exporting it to allow the PMU driver to be modular would
be even more so. Let's make things a little better abstracted by adding
the control registers to the cci_pmu instance data and passing their
location from the bus driver via platform data.

It's not practical to try the same thing for the bus driver itself,
given that it's mostly hairy assembly code for port control, so we leave
the globals in place there. It does however make sense to be prudent and
move them to __ro_after_init, since the addresses really should never be
changing once mapped.

Signed-off-by: Robin Murphy <[e-mail]>
---
  drivers/bus/arm-cci.c  | 17 +++++++++++++----
  drivers/perf/arm-cci.c | 47 ++++++++++++++++++++++++-----------------------
  2 files changed, 37 insertions(+), 27 deletions(-)

Comments

Will Deacon Feb. 15, 2018, 1:01 p.m. UTC | #1

On Wed, Feb 14, 2018 at 02:28:15PM +0000, Robin Murphy wrote:
> On 14/02/18 11:51, Will Deacon wrote:
> >On Wed, Feb 07, 2018 at 01:07:53PM +0000, Robin Murphy wrote:
> >>Hi Punit,
> >>
> >>On 07/02/18 12:22, Punit Agrawal wrote:
> >>[...]
> >>>>-static void __iomem *cci_ctrl_base;
> >>>>+void __iomem *cci_ctrl_base __ro_after_init;
> >>>
> >>>Initially I wondered if cci_ctrl_base gets used in the pmu driver before
> >>>it's initialised. But as it gets set in early_initcall() that looks to
> >>>be fine.
> >>
> >>In fact it's even more robust than initcall ordering, since the PMU device
> >>will only be created at all via cci_platform_probe(), thus cci_init() is
> >>guaranteed to have run successfully before the PMU driver probe can ever
> >>touch anything.
> >
> >Could you hijack the platform data at this point by passing an of_dev_auxdata
> >to of_platform_populate and then use that to pass the __iomem address to the
> >PMU driver?
> 
> OK, I've cooked up the patch below (on top of this series), but I'm
> currently somewhat on the fence about how nice it really is :/

Yeah, the ugly part is:

> +const struct of_dev_auxdata arm_cci_auxdata[] = {
> +	OF_DEV_AUXDATA("arm,cci-400-pmu", 0, NULL, &cci_ctrl_base),
> +	OF_DEV_AUXDATA("arm,cci-400-pmu,r0", 0, NULL, &cci_ctrl_base),
> +	OF_DEV_AUXDATA("arm,cci-400-pmu,r1", 0, NULL, &cci_ctrl_base),
> +	OF_DEV_AUXDATA("arm,cci-500-pmu,r0", 0, NULL, &cci_ctrl_base),
> +	OF_DEV_AUXDATA("arm,cci-550-pmu,r0", 0, NULL, &cci_ctrl_base),
> +	{}
> +};

I suppose you could macro-ise this table to hide the redundancy, but it's up
to you.

Will

Robin Murphy Feb. 15, 2018, 4:38 p.m. UTC | #2

On 15/02/18 13:01, Will Deacon wrote:
> On Wed, Feb 14, 2018 at 02:28:15PM +0000, Robin Murphy wrote:
>> On 14/02/18 11:51, Will Deacon wrote:
>>> On Wed, Feb 07, 2018 at 01:07:53PM +0000, Robin Murphy wrote:
>>>> Hi Punit,
>>>>
>>>> On 07/02/18 12:22, Punit Agrawal wrote:
>>>> [...]
>>>>>> -static void __iomem *cci_ctrl_base;
>>>>>> +void __iomem *cci_ctrl_base __ro_after_init;
>>>>>
>>>>> Initially I wondered if cci_ctrl_base gets used in the pmu driver before
>>>>> it's initialised. But as it gets set in early_initcall() that looks to
>>>>> be fine.
>>>>
>>>> In fact it's even more robust than initcall ordering, since the PMU device
>>>> will only be created at all via cci_platform_probe(), thus cci_init() is
>>>> guaranteed to have run successfully before the PMU driver probe can ever
>>>> touch anything.
>>>
>>> Could you hijack the platform data at this point by passing an of_dev_auxdata
>>> to of_platform_populate and then use that to pass the __iomem address to the
>>> PMU driver?
>>
>> OK, I've cooked up the patch below (on top of this series), but I'm
>> currently somewhat on the fence about how nice it really is :/
> 
> Yeah, the ugly part is:
> 
>> +const struct of_dev_auxdata arm_cci_auxdata[] = {
>> +	OF_DEV_AUXDATA("arm,cci-400-pmu", 0, NULL, &cci_ctrl_base),
>> +	OF_DEV_AUXDATA("arm,cci-400-pmu,r0", 0, NULL, &cci_ctrl_base),
>> +	OF_DEV_AUXDATA("arm,cci-400-pmu,r1", 0, NULL, &cci_ctrl_base),
>> +	OF_DEV_AUXDATA("arm,cci-500-pmu,r0", 0, NULL, &cci_ctrl_base),
>> +	OF_DEV_AUXDATA("arm,cci-550-pmu,r0", 0, NULL, &cci_ctrl_base),
>> +	{}
>> +};
> 
> I suppose you could macro-ise this table to hide the redundancy, but it's up
> to you.

I did ponder various alternatives, but whatever the approach we still 
end up needing a copy of all the PMU compatible strings in this driver; 
in that context, I think the auxdata table is probably in fact the least 
invasive. And given that it's surrounded by MCPM code, trying to hide 
things behind more macros just brings to mind cosmetics and swine ;)

On reflection, I'm feeling a bit happier with this one now, so I'll add 
it on the end and resubmit the lot to arm-soc as you suggested (ta for 
the ack too).

Robin.

Patch

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 23dc0b890d0c..d603fa8a0b92 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -25,9 +25,8 @@ 
  #include <asm/cacheflush.h>
  #include <asm/smp_plat.h>
  
-/* Referenced read-only by the PMU driver; see drivers/perf/arm-cci.c */
-void __iomem *cci_ctrl_base;
-static unsigned long cci_ctrl_phys;
+static void __iomem *cci_ctrl_base __ro_after_init;
+static unsigned long cci_ctrl_phys __ro_after_init;
  
  #ifdef CONFIG_ARM_CCI400_PORT_CTRL
  struct cci_nb_ports {
@@ -56,6 +55,15 @@  static const struct of_device_id arm_cci_matches[] = {
  	{},
  };
  
+const struct of_dev_auxdata arm_cci_auxdata[] = {
+	OF_DEV_AUXDATA("arm,cci-400-pmu", 0, NULL, &cci_ctrl_base),
+	OF_DEV_AUXDATA("arm,cci-400-pmu,r0", 0, NULL, &cci_ctrl_base),
+	OF_DEV_AUXDATA("arm,cci-400-pmu,r1", 0, NULL, &cci_ctrl_base),
+	OF_DEV_AUXDATA("arm,cci-500-pmu,r0", 0, NULL, &cci_ctrl_base),
+	OF_DEV_AUXDATA("arm,cci-550-pmu,r0", 0, NULL, &cci_ctrl_base),
+	{}
+};
+
  #define DRIVER_NAME		"ARM-CCI"
  
  static int cci_platform_probe(struct platform_device *pdev)
@@ -63,7 +71,8 @@  static int cci_platform_probe(struct platform_device *pdev)
  	if (!cci_probed())
  		return -ENODEV;
  
-	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	return of_platform_populate(pdev->dev.of_node, NULL,
+				    arm_cci_auxdata, &pdev->dev);
  }
  
  static struct platform_driver cci_platform_driver = {
diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c
index 336f1455cf96..67a74c48c7c2 100644
--- a/drivers/perf/arm-cci.c
+++ b/drivers/perf/arm-cci.c
@@ -16,8 +16,6 @@ 
  #include <linux/slab.h>
  #include <linux/spinlock.h>
  
-extern void __iomem *const cci_ctrl_base;
-
  #define DRIVER_NAME		"ARM-CCI PMU"
  
  #define CCI_PMCR		0x0100
@@ -90,6 +88,7 @@  static struct cci_pmu_model cci_pmu_models[];
  
  struct cci_pmu {
  	void __iomem *base;
+	void __iomem *ctrl_base;
  	struct pmu pmu;
  	int cpu;
  	int nr_irqs;
@@ -360,10 +359,10 @@  static int cci400_validate_hw_event(struct cci_pmu *cci_pmu, unsigned long hw_ev
  	return -ENOENT;
  }
  
-static int probe_cci400_revision(void)
+static int probe_cci400_revision(struct cci_pmu *cci_pmu)
  {
  	int rev;
-	rev = readl_relaxed(cci_ctrl_base + CCI_PID2) & CCI_PID2_REV_MASK;
+	rev = readl_relaxed(cci_pmu->ctrl_base + CCI_PID2) & CCI_PID2_REV_MASK;
  	rev >>= CCI_PID2_REV_SHIFT;
  
  	if (rev < CCI400_R1_PX)
@@ -372,14 +371,14 @@  static int probe_cci400_revision(void)
  		return CCI400_R1;
  }
  
-static const struct cci_pmu_model *probe_cci_model(void)
+static const struct cci_pmu_model *probe_cci_model(struct cci_pmu *cci_pmu)
  {
  	if (platform_has_secure_cci_access())
-		return &cci_pmu_models[probe_cci400_revision()];
+		return &cci_pmu_models[probe_cci400_revision(cci_pmu)];
  	return NULL;
  }
  #else	/* !CONFIG_ARM_CCI400_PMU */
-static inline struct cci_pmu_model *probe_cci_model(void)
+static inline struct cci_pmu_model *probe_cci_model(struct cci_pmu *cci_pmu)
  {
  	return NULL;
  }
@@ -662,8 +661,8 @@  static void __cci_pmu_enable_nosync(struct cci_pmu *cci_pmu)
  	u32 val;
  
  	/* Enable all the PMU counters. */
-	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
-	writel(val, cci_ctrl_base + CCI_PMCR);
+	val = readl_relaxed(cci_pmu->ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
+	writel(val, cci_pmu->ctrl_base + CCI_PMCR);
  }
  
  /* Should be called with cci_pmu->hw_events->pmu_lock held */
@@ -674,13 +673,13 @@  static void __cci_pmu_enable_sync(struct cci_pmu *cci_pmu)
  }
  
  /* Should be called with cci_pmu->hw_events->pmu_lock held */
-static void __cci_pmu_disable(void)
+static void __cci_pmu_disable(struct cci_pmu *cci_pmu)
  {
  	u32 val;
  
  	/* Disable all the PMU counters. */
-	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
-	writel(val, cci_ctrl_base + CCI_PMCR);
+	val = readl_relaxed(cci_pmu->ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
+	writel(val, cci_pmu->ctrl_base + CCI_PMCR);
  }
  
  static ssize_t cci_pmu_format_show(struct device *dev,
@@ -782,9 +781,9 @@  pmu_restore_counters(struct cci_pmu *cci_pmu, unsigned long *mask)
   * Returns the number of programmable counters actually implemented
   * by the cci
   */
-static u32 pmu_get_max_counters(void)
+static u32 pmu_get_max_counters(struct cci_pmu *cci_pmu)
  {
-	return (readl_relaxed(cci_ctrl_base + CCI_PMCR) &
+	return (readl_relaxed(cci_pmu->ctrl_base + CCI_PMCR) &
  		CCI_PMCR_NCNT_MASK) >> CCI_PMCR_NCNT_SHIFT;
  }
  
@@ -965,7 +964,7 @@  static void cci5xx_pmu_write_counters(struct cci_pmu *cci_pmu, unsigned long *ma
  		pmu_set_event(cci_pmu, i, event->hw.config_base);
  	}
  
-	__cci_pmu_disable();
+	__cci_pmu_disable(cci_pmu);
  
  	pmu_restore_counters(cci_pmu, saved_mask);
  }
@@ -1026,7 +1025,7 @@  static irqreturn_t pmu_handle_irq(int irq_num, void *dev)
  	raw_spin_lock_irqsave(&events->pmu_lock, flags);
  
  	/* Disable the PMU while we walk through the counters */
-	__cci_pmu_disable();
+	__cci_pmu_disable(cci_pmu);
  	/*
  	 * Iterate over counters and update the corresponding perf events.
  	 * This should work regardless of whether we have per-counter overflow
@@ -1108,7 +1107,7 @@  static void cci_pmu_disable(struct pmu *pmu)
  	unsigned long flags;
  
  	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
-	__cci_pmu_disable();
+	__cci_pmu_disable(cci_pmu);
  	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
  }
  
@@ -1438,7 +1437,7 @@  static int cci_pmu_init(struct cci_pmu *cci_pmu, struct platform_device *pdev)
  	};
  
  	cci_pmu->plat_device = pdev;
-	num_cntrs = pmu_get_max_counters();
+	num_cntrs = pmu_get_max_counters(cci_pmu);
  	if (num_cntrs > cci_pmu->model->num_hw_cntrs) {
  		dev_warn(&pdev->dev,
  			"PMU implements more counters(%d) than supported by"
@@ -1611,21 +1610,23 @@  static struct cci_pmu *cci_pmu_alloc(struct device *dev)
  	 * them explicitly on an error, as it would end up in driver
  	 * detach.
  	 */
+	cci_pmu = devm_kzalloc(dev, sizeof(*cci_pmu), GFP_KERNEL);
+	if (!cci_pmu)
+		return ERR_PTR(-ENOMEM);
+
+	cci_pmu->ctrl_base = *(void __iomem **)dev->platform_data;
+
  	model = of_device_get_match_data(dev);
  	if (!model) {
  		dev_warn(dev,
  			 "DEPRECATED compatible property, requires secure access to CCI registers");
-		model = probe_cci_model();
+		model = probe_cci_model(cci_pmu);
  	}
  	if (!model) {
  		dev_warn(dev, "CCI PMU version not supported\n");
  		return ERR_PTR(-ENODEV);
  	}
  
-	cci_pmu = devm_kzalloc(dev, sizeof(*cci_pmu), GFP_KERNEL);
-	if (!cci_pmu)
-		return ERR_PTR(-ENOMEM);
-
  	cci_pmu->model = model;
  	cci_pmu->irqs = devm_kcalloc(dev, CCI_PMU_MAX_HW_CNTRS(model),
  					sizeof(*cci_pmu->irqs), GFP_KERNEL);