[PATCH] spmi-pmic-arb: support configurable number of peripherals

[PATCH] spmi-pmic-arb: support configurable number of peripherals

[Gilad Avidov]

The current driver implementation supports only 128 peripherals.
Adding support for configurable number of peripherals since the
spmi-pmic-arb v2 HW has sub-versions which support from 128 to 512
PMIC peripherals.

Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
Reviewed-by: Sagar Dharia <sdharia@codeaurora.org>
---
 .../bindings/spmi/qcom,spmi-pmic-arb.txt           | 34 ++++++++-
 drivers/spmi/spmi-pmic-arb.c                       | 88 ++++++++++++++--------
 2 files changed, 91 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
index e16b9b5..fba7915 100644
--- a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
+++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
@@ -42,7 +42,11 @@ Required properties:
     cell 4: interrupt flags indicating level-sense information, as defined in
             dt-bindings/interrupt-controller/irq.h
 
-Example:
+Optional properties:
+- qcom,max-peripherals : number of PMIC peripherals (same as maximum APID)
+			 supported by HW. Default (minimum supported) is 128.
+
+Example V1 PMIC-Arbiter:
 
 	spmi {
 		compatible = "qcom,spmi-pmic-arb";
@@ -62,4 +66,32 @@ Example:
 
 		interrupt-controller;
 		#interrupt-cells = <4>;
+
+		qcom,max-peripherals = <256>;
+	};
+
+Example V2 PMIC-Arbiter:
+
+	spmi_bus: qcom,spmi@200f000 {
+		compatible = "qcom,spmi-pmic-arb";
+		reg-names = "core", "chnls", "obsrvr", "intr", "cnfg";
+		reg = <0x200f000 0x1000>,
+			<0x2400000 0x400000>,
+			<0x2c00000 0x400000>,
+			<0x3800000 0x200000>,
+			<0x200a000 0x2100>;
+
+		interrupt-names = "periph_irq";
+		interrupts = <0 190 0>;
+
+		qcom,ee = <0>;
+		qcom,channel = <0>;
+
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		interrupt-controller;
+		#interrupt-cells = <4>;
+
+		qcom,max-peripherals = <256>;
 	};
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index d7119db..ae0f05d 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -47,9 +47,8 @@
 #define SPMI_MAPPING_BIT_IS_1_FLAG(X)	(((X) >> 8) & 0x1)
 #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
 
-#define SPMI_MAPPING_TABLE_LEN		255
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
-#define PPID_TO_CHAN_TABLE_SZ		BIT(12)	/* PPID is 12bit chan is 1byte*/
+#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
 
 /* Ownership Table */
 #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
@@ -84,9 +83,8 @@ enum pmic_arb_cmd_op_code {
 	PMIC_ARB_OP_ZERO_WRITE = 16,
 };
 
-/* Maximum number of support PMIC peripherals */
-#define PMIC_ARB_MAX_PERIPHS		256
-#define PMIC_ARB_MAX_CHNL		128
+/* Default (can override in config) max number of support PMIC peripherals */
+#define PMIC_ARB_MAX_APID_DEFAULT	128
 #define PMIC_ARB_PERIPH_ID_VALID	(1 << 15)
 #define PMIC_ARB_TIMEOUT_US		100
 #define PMIC_ARB_MAX_TRANS_BYTES	(8)
@@ -110,12 +108,16 @@ struct pmic_arb_ver_ops;
  * @channel:		execution environment channel to use for accesses.
  * @irq:		PMIC ARB interrupt.
  * @ee:			the current Execution Environment
- * @min_apid:		minimum APID (used for bounding IRQ search)
- * @max_apid:		maximum APID
+ * @min_irq_apid:	minimum APID with requested IRQ (used for bounding IRQ
+ *                      search).
+ * @max_irq_apid:	maximum APID with requested IRQ (used for bounding IRQ
+ *                      search).
+ * max_apid:		maximum APID supported by HW.
  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object
- * @apid_to_ppid:	in-memory copy of APID -> PPID mapping table.
+ * @irq_apid_to_ppid:	table which keeps track of APID -> PPID mapping for
+			peripherals which IRQ was requested for.
  * @ver_ops:		version dependent operations.
  * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping table.
  *			v2 only.
@@ -129,14 +131,15 @@ struct spmi_pmic_arb_dev {
 	u8			channel;
 	int			irq;
 	u8			ee;
-	u8			min_apid;
-	u8			max_apid;
-	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
+	u16			min_irq_apid;
+	u16			max_irq_apid;
+	u16			max_apid;
+	u32			*mapping_table;
 	struct irq_domain	*domain;
 	struct spmi_controller	*spmic;
-	u16			apid_to_ppid[256];
+	u16			*irq_apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
-	u8			*ppid_to_chan;
+	u16			*ppid_to_chan;
 };
 
 /**
@@ -444,7 +447,7 @@ static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
 		id = ffs(status) - 1;
 		status &= ~(1 << id);
 		irq = irq_find_mapping(pa->domain,
-				       pa->apid_to_ppid[apid] << 16
+				       pa->irq_apid_to_ppid[apid] << 16
 				     | id << 8
 				     | apid);
 		generic_handle_irq(irq);
@@ -456,8 +459,8 @@ static void pmic_arb_chained_irq(unsigned int irq, struct irq_desc *desc)
 	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
 	struct irq_chip *chip = irq_get_chip(irq);
 	void __iomem *intr = pa->intr;
-	int first = pa->min_apid >> 5;
-	int last = pa->max_apid >> 5;
+	int first = pa->min_irq_apid >> 5;
+	int last = pa->max_irq_apid >> 5;
 	u32 status;
 	int i, id;
 
@@ -655,13 +658,13 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	if (err)
 		return err;
 
-	pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
+	pa->irq_apid_to_ppid[apid] = spec.slave << 8 | spec.per;
 
 	/* Keep track of {max,min}_apid for bounding search during interrupt */
-	if (apid > pa->max_apid)
-		pa->max_apid = apid;
-	if (apid < pa->min_apid)
-		pa->min_apid = apid;
+	if (apid > pa->max_irq_apid)
+		pa->max_irq_apid = apid;
+	if (apid < pa->min_irq_apid)
+		pa->min_irq_apid = apid;
 
 	*out_hwirq = spec.slave << 24
 		   | spec.per   << 16
@@ -794,6 +797,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	pa = spmi_controller_get_drvdata(ctrl);
 	pa->spmic = ctrl;
+	pa->max_apid = PMIC_ARB_MAX_APID_DEFAULT;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
 	core = devm_ioremap_resource(&ctrl->dev, res);
@@ -813,7 +817,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		pa->wr_base = core;
 		pa->rd_base = core;
 	} else {
-		u8  chan;
+		u16 chan;
 		u16 ppid;
 		u32 regval;
 
@@ -836,16 +840,24 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		}
 
 		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
-					PPID_TO_CHAN_TABLE_SZ, GFP_KERNEL);
+						PMIC_ARB_MAX_PPID *
+						sizeof(*pa->ppid_to_chan),
+						GFP_KERNEL);
 		if (!pa->ppid_to_chan) {
 			err = -ENOMEM;
 			goto err_put_ctrl;
 		}
+
+		err = of_property_read_u32(pdev->dev.of_node,
+					   "qcom,max-peripherals", &regval);
+		if (!err)
+			pa->max_apid = regval;
+
 		/*
 		 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 		 * ppid_to_chan is an in-memory invert of that table.
 		 */
-		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
+		for (chan = 0; chan < pa->max_apid ; ++chan) {
 			regval = readl_relaxed(core + PMIC_ARB_REG_CHNL(chan));
 			if (!regval)
 				continue;
@@ -903,14 +915,30 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	pa->ee = ee;
 
-	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
-		pa->mapping_table[i] = readl_relaxed(
-				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
+	pa->irq_apid_to_ppid = devm_kzalloc(&ctrl->dev, pa->max_apid *
+					    sizeof(*pa->irq_apid_to_ppid),
+					    GFP_KERNEL);
+	if (!pa->irq_apid_to_ppid) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
+
+	pa->mapping_table = devm_kzalloc(&ctrl->dev,
+					(pa->max_apid - 1) * sizeof(u32),
+					GFP_KERNEL);
+	if (!pa->mapping_table) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
+
+	for (i = 0; i < (pa->max_apid - 1); ++i)
+		pa->mapping_table[i] = readl_relaxed(pa->cnfg +
+						  SPMI_MAPPING_TABLE_REG(i));
 
-	/* Initialize max_apid/min_apid to the opposite bounds, during
+	/* Initialize max_irq_apid/min_irq_apid to the opposite bounds, during
 	 * the irq domain translation, we are sure to update these */
-	pa->max_apid = 0;
-	pa->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
+	pa->max_irq_apid = 0;
+	pa->min_irq_apid = pa->max_apid - 1;
 
 	platform_set_drvdata(pdev, ctrl);
 	raw_spin_lock_init(&pa->lock);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/03, Gilad Avidov wrote:
> +			 supported by HW. Default (minimum supported) is 128.
> +
> +Example V1 PMIC-Arbiter:
>  
>  	spmi {
>  		compatible = "qcom,spmi-pmic-arb";
> @@ -62,4 +66,32 @@ Example:
>  
>  		interrupt-controller;
>  		#interrupt-cells = <4>;
> +
> +		qcom,max-peripherals = <256>;

If it's v1 isn't it always 128? So having 256 here is just
confusing.

> +	};
> +
> @@ -129,14 +131,15 @@ struct spmi_pmic_arb_dev {
>  	u8			channel;
>  	int			irq;
>  	u8			ee;
> -	u8			min_apid;
> -	u8			max_apid;
> -	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
> +	u16			min_irq_apid;
> +	u16			max_irq_apid;
> +	u16			max_apid;
> +	u32			*mapping_table;
>  	struct irq_domain	*domain;
>  	struct spmi_controller	*spmic;
> -	u16			apid_to_ppid[256];
> +	u16			*irq_apid_to_ppid;

Please drop all this renaming noise, or at the least, put it in a
different patch. More than half the patch is just changing the
names of these variables for what seems like no reason.

>  	const struct pmic_arb_ver_ops *ver_ops;
> -	u8			*ppid_to_chan;
> +	u16			*ppid_to_chan;
>  };
>  
>  	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
>  	struct irq_chip *chip = irq_get_chip(irq);
>  	void __iomem *intr = pa->intr;
> -	int first = pa->min_apid >> 5;
> -	int last = pa->max_apid >> 5;
> +	int first = pa->min_irq_apid >> 5;
> +	int last = pa->max_irq_apid >> 5;
>  	u32 status;
>  	int i, id;
>  
> @@ -903,14 +915,30 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  
>  	pa->ee = ee;
>  
> -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> -		pa->mapping_table[i] = readl_relaxed(
> -				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
> +	pa->irq_apid_to_ppid = devm_kzalloc(&ctrl->dev, pa->max_apid *
> +					    sizeof(*pa->irq_apid_to_ppid),
> +					    GFP_KERNEL);
> +	if (!pa->irq_apid_to_ppid) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
> +
> +	pa->mapping_table = devm_kzalloc(&ctrl->dev,
> +					(pa->max_apid - 1) * sizeof(u32),
> +					GFP_KERNEL);
> +	if (!pa->mapping_table) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
> +
> +	for (i = 0; i < (pa->max_apid - 1); ++i)
> +		pa->mapping_table[i] = readl_relaxed(pa->cnfg +
> +						  SPMI_MAPPING_TABLE_REG(i));

Maybe we should stop doing this during probe and always allocate
an empty cache of size 128 on v1 and 512 on v2 chips? So when
we're searching through the mapping table we can cache the value
from the register if the entry isn't 0. This delays the
processing to when we're mapping irqs, hopefully speeding up
probe for the case where you have a handful of irqs to map.

The DT property wouldn't be necessary then. Arguably it's being
added there to optimize the size of the mapping table and isn't
really necessary otherwise.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Gilad Avidov]

On Thu, 3 Sep 2015 17:16:30 -0700
Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:

> On 09/03, Gilad Avidov wrote:
> > +			 supported by HW. Default (minimum
> > supported) is 128. +
> > +Example V1 PMIC-Arbiter:
> >  
> >  	spmi {
> >  		compatible = "qcom,spmi-pmic-arb";
> > @@ -62,4 +66,32 @@ Example:
> >  
> >  		interrupt-controller;
> >  		#interrupt-cells = <4>;
> > +
> > +		qcom,max-peripherals = <256>;
> 
> If it's v1 isn't it always 128? So having 256 here is just
> confusing.

Hi Stephen,

Actually some v1 chipsets, such as Dragonboard APQ8074, support 256
peripherals.

> 
> > +	};
> > +
> > @@ -129,14 +131,15 @@ struct spmi_pmic_arb_dev {
> >  	u8			channel;
> >  	int			irq;
> >  	u8			ee;
> > -	u8			min_apid;
> > -	u8			max_apid;
> > -	u32
> > mapping_table[SPMI_MAPPING_TABLE_LEN];
> > +	u16			min_irq_apid;
> > +	u16			max_irq_apid;
> > +	u16			max_apid;
> > +	u32			*mapping_table;
> >  	struct irq_domain	*domain;
> >  	struct spmi_controller	*spmic;
> > -	u16			apid_to_ppid[256];
> > +	u16			*irq_apid_to_ppid;
> 
> Please drop all this renaming noise, or at the least, put it in a
> different patch. More than half the patch is just changing the
> names of these variables for what seems like no reason.
> 

I agree that changing apid_to_ppid to irq_apid_to_ppid is not required
and I'll undo this change on next patch.

Regarding the change of max/min_apid to max/min_irq_apid:
max_apid was already used but the name does not make good sense. Since
really it is not the max_apid supported. Instead it is the largest apid
which interrupt is currently requested for. But now we need a value that
is actually the maximum supported apid. This is why repurposed max_apid
and corrected the previous naming.

> >  	const struct pmic_arb_ver_ops *ver_ops;
> > -	u8			*ppid_to_chan;
> > +	u16			*ppid_to_chan;
> >  };
> >  
> >  	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
> >  	struct irq_chip *chip = irq_get_chip(irq);
> >  	void __iomem *intr = pa->intr;
> > -	int first = pa->min_apid >> 5;
> > -	int last = pa->max_apid >> 5;
> > +	int first = pa->min_irq_apid >> 5;
> > +	int last = pa->max_irq_apid >> 5;
> >  	u32 status;
> >  	int i, id;
> >  
> > @@ -903,14 +915,30 @@ static int spmi_pmic_arb_probe(struct
> > platform_device *pdev) 
> >  	pa->ee = ee;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> > -		pa->mapping_table[i] = readl_relaxed(
> > -				pa->cnfg +
> > SPMI_MAPPING_TABLE_REG(i));
> > +	pa->irq_apid_to_ppid = devm_kzalloc(&ctrl->dev,
> > pa->max_apid *
> > +
> > sizeof(*pa->irq_apid_to_ppid),
> > +					    GFP_KERNEL);
> > +	if (!pa->irq_apid_to_ppid) {
> > +		err = -ENOMEM;
> > +		goto err_put_ctrl;
> > +	}
> > +
> > +	pa->mapping_table = devm_kzalloc(&ctrl->dev,
> > +					(pa->max_apid - 1) *
> > sizeof(u32),
> > +					GFP_KERNEL);
> > +	if (!pa->mapping_table) {
> > +		err = -ENOMEM;
> > +		goto err_put_ctrl;
> > +	}
> > +
> > +	for (i = 0; i < (pa->max_apid - 1); ++i)
> > +		pa->mapping_table[i] = readl_relaxed(pa->cnfg +
> > +
> > SPMI_MAPPING_TABLE_REG(i));
> 
> Maybe we should stop doing this during probe and always allocate
> an empty cache of size 128 on v1 and 512 on v2 chips? So when

As mentioned above, both v1 and v2 can have different number of
peripherals. 

> we're searching through the mapping table we can cache the value
> from the register if the entry isn't 0. This delays the

Greedy approach (reading upto 512 registers from hw) have very small
upfront penalty since sequential reads take place very fast
and HW acceleration (prefetching) will improve it. A lazy approach will
required much more complex algorithm (for example any value
including zero is valid. So we will need to augment a marker for
unassigned entries) and prefetching will not help.

> processing to when we're mapping irqs, hopefully speeding up
> probe for the case where you have a handful of irqs to map.

Please note that we traverse this mapping table every time a PMIC
driver registers for an interrupt. This means that even if we speed up
the probe of pmic-arb driver we will not speed the system boot since
the probing of the PMIC drivers will register for interrupts.

> 
> The DT property wouldn't be necessary then. Arguably it's being
> added there to optimize the size of the mapping table and isn't
> really necessary otherwise.
> 

We need to know this value for both memory allocation
and for stopping before reading over the bounds of the mapping_table in
HW.

Thank you,
Gilad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/04, Gilad Avidov wrote:
> On Thu, 3 Sep 2015 17:16:30 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > On 09/03, Gilad Avidov wrote:
> > > +			 supported by HW. Default (minimum
> > > supported) is 128. +
> > > +Example V1 PMIC-Arbiter:
> > >  
> > >  	spmi {
> > >  		compatible = "qcom,spmi-pmic-arb";
> > > @@ -62,4 +66,32 @@ Example:
> > >  
> > >  		interrupt-controller;
> > >  		#interrupt-cells = <4>;
> > > +
> > > +		qcom,max-peripherals = <256>;
> > 
> > If it's v1 isn't it always 128? So having 256 here is just
> > confusing.
> 
> Hi Stephen,
> 
> Actually some v1 chipsets, such as Dragonboard APQ8074, support 256
> peripherals.

Then the commit text should be updated. It only mentions that v2
HW has sub-versions which support more than 128.

> 
> > 
> > > +	};
> > > +
> > > @@ -129,14 +131,15 @@ struct spmi_pmic_arb_dev {
> > >  	u8			channel;
> > >  	int			irq;
> > >  	u8			ee;
> > > -	u8			min_apid;
> > > -	u8			max_apid;
> > > -	u32
> > > mapping_table[SPMI_MAPPING_TABLE_LEN];
> > > +	u16			min_irq_apid;
> > > +	u16			max_irq_apid;
> > > +	u16			max_apid;
> > > +	u32			*mapping_table;
> > >  	struct irq_domain	*domain;
> > >  	struct spmi_controller	*spmic;
> > > -	u16			apid_to_ppid[256];
> > > +	u16			*irq_apid_to_ppid;
> > 
> > Please drop all this renaming noise, or at the least, put it in a
> > different patch. More than half the patch is just changing the
> > names of these variables for what seems like no reason.
> > 
> 
> Regarding the change of max/min_apid to max/min_irq_apid:
> max_apid was already used but the name does not make good sense. Since
> really it is not the max_apid supported. Instead it is the largest apid
> which interrupt is currently requested for. But now we need a value that
> is actually the maximum supported apid. This is why repurposed max_apid
> and corrected the previous naming.

max_apid in this patch is only used in probe, so there's no need
for that variable to be a member of the structure. Meaning we can
leave max_apid alone and call this new variable max_periphs or
something like that and leave it local to the probe function.

> 
> > >  	const struct pmic_arb_ver_ops *ver_ops;
> > > -	u8			*ppid_to_chan;
> > > +	u16			*ppid_to_chan;
> > >  };
> > >  
> > >  	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
> > >  	struct irq_chip *chip = irq_get_chip(irq);
> > >  	void __iomem *intr = pa->intr;
> > > -	int first = pa->min_apid >> 5;
> > > -	int last = pa->max_apid >> 5;
> > > +	int first = pa->min_irq_apid >> 5;
> > > +	int last = pa->max_irq_apid >> 5;
> > >  	u32 status;
> > >  	int i, id;
> > >  
> > > @@ -903,14 +915,30 @@ static int spmi_pmic_arb_probe(struct
> > > platform_device *pdev) 
> > >  	pa->ee = ee;
> > >  
> > > -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> > > -		pa->mapping_table[i] = readl_relaxed(
> > > -				pa->cnfg +
> > > SPMI_MAPPING_TABLE_REG(i));
> > > +	pa->irq_apid_to_ppid = devm_kzalloc(&ctrl->dev,
> > > pa->max_apid *
> > > +
> > > sizeof(*pa->irq_apid_to_ppid),
> > > +					    GFP_KERNEL);
> > > +	if (!pa->irq_apid_to_ppid) {
> > > +		err = -ENOMEM;
> > > +		goto err_put_ctrl;
> > > +	}
> > > +
> > > +	pa->mapping_table = devm_kzalloc(&ctrl->dev,
> > > +					(pa->max_apid - 1) *
> > > sizeof(u32),
> > > +					GFP_KERNEL);
> > > +	if (!pa->mapping_table) {
> > > +		err = -ENOMEM;
> > > +		goto err_put_ctrl;
> > > +	}
> > > +
> > > +	for (i = 0; i < (pa->max_apid - 1); ++i)
> > > +		pa->mapping_table[i] = readl_relaxed(pa->cnfg +
> > > +
> > > SPMI_MAPPING_TABLE_REG(i));
> 
> > we're searching through the mapping table we can cache the value
> > from the register if the entry isn't 0. This delays the
> 
> Greedy approach (reading upto 512 registers from hw) have very small
> upfront penalty since sequential reads take place very fast
> and HW acceleration (prefetching) will improve it. A lazy approach will
> required much more complex algorithm (for example any value
> including zero is valid. So we will need to augment a marker for
> unassigned entries) and prefetching will not help.

That's solvable with a bitmap of 512 entries, not too much of a
complicated algorithm.

We're sort of screwed now that we have to find the channel for a
ppid by reading X number of PMIC_ARB_REG_CHNL registers. We could
lazily do that too though, where we fill in the array linearly
with what we read until we find the ppid we're looking for. We
would need to do a bitmap trick again, but since we know ppid is
at most going to use 12 bits, we can set the highest bit to
indicate the ppid_to_chan entry is valid.

> 
> > processing to when we're mapping irqs, hopefully speeding up
> > probe for the case where you have a handful of irqs to map.
> 
> Please note that we traverse this mapping table every time a PMIC
> driver registers for an interrupt. This means that even if we speed up
> the probe of pmic-arb driver we will not speed the system boot since
> the probing of the PMIC drivers will register for interrupts.

Do we need to read all 512 entries for a typical system? If we do
then I agree reading it all upfront is probably better, but if we
only read a sparse amount of entries then we can defer it to when
it's going to be used. On upstream 8974 dts files with all the
devices we have in DT right now, I see 75 unique register reads
during boot on apq8074 dragonboard.

> 
> > 
> > The DT property wouldn't be necessary then. Arguably it's being
> > added there to optimize the size of the mapping table and isn't
> > really necessary otherwise.
> > 
> 
> We need to know this value for both memory allocation
> and for stopping before reading over the bounds of the mapping_table in
> HW.
> 

Presumably the hardware is correct and isn't telling us to read
entries in the mapping table that are beyond the bounds, so
stepping over the bounds of the mapping table cache isn't
possible if we always make it the size of the biggest table.

Here's the untested patch. I hope we don't ever have more than
512 periphs.

BTW, we shouldn't make qcom,channel required on v2 hardware. That
doesn't make any sense so it should be changed to required only
on v1 devices and we should remove it from the 8916 dtsi file.

----8<-----
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

 drivers/spmi/spmi-pmic-arb.c | 98 +++++++++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 32 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index c7aa6f1a898e..cbe963f829d7 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -10,6 +10,7 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
+#include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -47,9 +48,9 @@
 #define SPMI_MAPPING_BIT_IS_1_FLAG(X)	(((X) >> 8) & 0x1)
 #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
 
-#define SPMI_MAPPING_TABLE_LEN		255
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
-#define PPID_TO_CHAN_TABLE_SZ		BIT(12)	/* PPID is 12bit chan is 1byte*/
+#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
+#define PMIC_ARB_PPID_VALID		BIT(15)
 
 /* Ownership Table */
 #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
@@ -85,9 +86,7 @@ enum pmic_arb_cmd_op_code {
 };
 
 /* Maximum number of support PMIC peripherals */
-#define PMIC_ARB_MAX_PERIPHS		256
-#define PMIC_ARB_MAX_CHNL		128
-#define PMIC_ARB_PERIPH_ID_VALID	(1 << 15)
+#define PMIC_ARB_MAX_PERIPHS		512
 #define PMIC_ARB_TIMEOUT_US		100
 #define PMIC_ARB_MAX_TRANS_BYTES	(8)
 
@@ -125,18 +124,21 @@ struct spmi_pmic_arb_dev {
 	void __iomem		*wr_base;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
+	void __iomem		*core;
 	raw_spinlock_t		lock;
 	u8			channel;
 	int			irq;
 	u8			ee;
-	u8			min_apid;
-	u8			max_apid;
-	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
+	u16			min_apid;
+	u16			max_apid;
+	u32			*mapping_table;
+	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
 	struct irq_domain	*domain;
 	struct spmi_controller	*spmic;
-	u16			apid_to_ppid[256];
+	u16			*apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
-	u8			*ppid_to_chan;
+	u16			*ppid_to_chan;
+	u16			last_channel;
 };
 
 /**
@@ -614,6 +616,10 @@ static int search_mapping_table(struct spmi_pmic_arb_dev *pa,
 	u32 data;
 
 	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
+		if (!test_and_set_bit(index, pa->mapping_table_valid))
+			mapping_table[index] = readl_relaxed(pa->cnfg +
+						SPMI_MAPPING_TABLE_REG(index));
+
 		data = mapping_table[index];
 
 		if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
@@ -706,11 +712,37 @@ static u32 pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
 	return 0x800 + 0x80 * pa->channel;
 }
 
+static u16 find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
+{
+	u32 regval;
+	u16 chan;
+	u16 _ppid;
+
+	for (chan = pa->last_channel; ; chan++, pa->last_channel++) {
+		regval = readl_relaxed(pa->core + PMIC_ARB_REG_CHNL(chan));
+		if (!regval)
+			continue;
+
+		_ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
+		pa->ppid_to_chan[_ppid] = chan | PMIC_ARB_PPID_VALID;
+		if (_ppid == ppid)
+			break;
+	}
+
+	return chan;
+}
+
+
 /* v2 offset per ppid (chan) and per ee */
 static u32 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
 {
 	u16 ppid = (sid << 8) | (addr >> 8);
-	u8  chan = pa->ppid_to_chan[ppid];
+	u16 chan;
+
+	chan = pa->ppid_to_chan[ppid];
+	if (!(chan & PMIC_ARB_PPID_VALID))
+		chan = find_chan(pa, ppid);
+	chan &= ~PMIC_ARB_PPID_MASK;
 
 	return 0x1000 * pa->ee + 0x8000 * chan;
 }
@@ -797,7 +829,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *core;
 	u32 channel, ee, hw_ver;
-	int err, i;
+	int err;
 	bool is_v1;
 
 	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
@@ -825,10 +857,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		pa->wr_base = core;
 		pa->rd_base = core;
 	} else {
-		u8  chan;
-		u16 ppid;
-		u32 regval;
-
+		pa->core = core;
 		pa->ver_ops = &pmic_arb_v2;
 
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -847,23 +876,17 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 			goto err_put_ctrl;
 		}
 
-		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
-					PPID_TO_CHAN_TABLE_SZ, GFP_KERNEL);
-		if (!pa->ppid_to_chan) {
-			err = -ENOMEM;
-			goto err_put_ctrl;
-		}
 		/*
 		 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 		 * ppid_to_chan is an in-memory invert of that table.
 		 */
-		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
-			regval = readl_relaxed(core + PMIC_ARB_REG_CHNL(chan));
-			if (!regval)
-				continue;
-
-			ppid = (regval >> 8) & 0xFFF;
-			pa->ppid_to_chan[ppid] = chan;
+		pa->ppid_to_chan = devm_kcalloc(&ctrl->dev,
+						PMIC_ARB_MAX_PPID,
+						sizeof(*pa->ppid_to_chan),
+						GFP_KERNEL);
+		if (!pa->ppid_to_chan) {
+			err = -ENOMEM;
+			goto err_put_ctrl;
 		}
 	}
 
@@ -915,9 +938,20 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	pa->ee = ee;
 
-	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
-		pa->mapping_table[i] = readl_relaxed(
-				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
+	pa->apid_to_ppid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS,
+					    sizeof(*pa->apid_to_ppid),
+					    GFP_KERNEL);
+	if (!pa->apid_to_ppid) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
+
+	pa->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
+					sizeof(*pa->mapping_table), GFP_KERNEL);
+	if (!pa->mapping_table) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
 
 	/* Initialize max_apid/min_apid to the opposite bounds, during
 	 * the irq domain translation, we are sure to update these */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Gilad Avidov]

On Fri, 4 Sep 2015 17:50:11 -0700
Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 09/04, Gilad Avidov wrote:
> > On Thu, 3 Sep 2015 17:16:30 -0700
> > Stephen Boyd <sboyd@codeaurora.org> wrote:
> > 
> > > On 09/03, Gilad Avidov wrote:
> > > > +			 supported by HW. Default (minimum
> > > > supported) is 128. +
> > > > +Example V1 PMIC-Arbiter:
> > > >  
> > > >  	spmi {
> > > >  		compatible = "qcom,spmi-pmic-arb";
> > > > @@ -62,4 +66,32 @@ Example:
> > > >  
> > > >  		interrupt-controller;
> > > >  		#interrupt-cells = <4>;
> > > > +
> > > > +		qcom,max-peripherals = <256>;
> > > 
> > > If it's v1 isn't it always 128? So having 256 here is just
> > > confusing.
> > 
> > Hi Stephen,
> > 
> > Actually some v1 chipsets, such as Dragonboard APQ8074, support 256
> > peripherals.
> 
> Then the commit text should be updated. It only mentions that v2
> HW has sub-versions which support more than 128.
> 

Correct, I'll update the commit text.

> > 
> > > 
> > > > +	};
> > > > +
> > > > @@ -129,14 +131,15 @@ struct spmi_pmic_arb_dev {
> > > >  	u8			channel;
> > > >  	int			irq;
> > > >  	u8			ee;
> > > > -	u8			min_apid;
> > > > -	u8			max_apid;
> > > > -	u32
> > > > mapping_table[SPMI_MAPPING_TABLE_LEN];
> > > > +	u16			min_irq_apid;
> > > > +	u16			max_irq_apid;
> > > > +	u16			max_apid;
> > > > +	u32			*mapping_table;
> > > >  	struct irq_domain	*domain;
> > > >  	struct spmi_controller	*spmic;
> > > > -	u16			apid_to_ppid[256];
> > > > +	u16			*irq_apid_to_ppid;
> > > 
> > > Please drop all this renaming noise, or at the least, put it in a
> > > different patch. More than half the patch is just changing the
> > > names of these variables for what seems like no reason.
> > > 
> > 
> > Regarding the change of max/min_apid to max/min_irq_apid:
> > max_apid was already used but the name does not make good sense.
> > Since really it is not the max_apid supported. Instead it is the
> > largest apid which interrupt is currently requested for. But now we
> > need a value that is actually the maximum supported apid. This is
> > why repurposed max_apid and corrected the previous naming.
> 
> max_apid in this patch is only used in probe, so there's no need
> for that variable to be a member of the structure. Meaning we can
> leave max_apid alone and call this new variable max_periphs or
> something like that and leave it local to the probe function.
> 

I'll avoid the renaming altogether.

> > 
> > > >  	const struct pmic_arb_ver_ops *ver_ops;
> > > > -	u8			*ppid_to_chan;
> > > > +	u16			*ppid_to_chan;
> > > >  };
> > > >  
> > > >  	struct spmi_pmic_arb_dev *pa =
> > > > irq_get_handler_data(irq); struct irq_chip *chip =
> > > > irq_get_chip(irq); void __iomem *intr = pa->intr;
> > > > -	int first = pa->min_apid >> 5;
> > > > -	int last = pa->max_apid >> 5;
> > > > +	int first = pa->min_irq_apid >> 5;
> > > > +	int last = pa->max_irq_apid >> 5;
> > > >  	u32 status;
> > > >  	int i, id;
> > > >  
> > > > @@ -903,14 +915,30 @@ static int spmi_pmic_arb_probe(struct
> > > > platform_device *pdev) 
> > > >  	pa->ee = ee;
> > > >  
> > > > -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> > > > -		pa->mapping_table[i] = readl_relaxed(
> > > > -				pa->cnfg +
> > > > SPMI_MAPPING_TABLE_REG(i));
> > > > +	pa->irq_apid_to_ppid = devm_kzalloc(&ctrl->dev,
> > > > pa->max_apid *
> > > > +
> > > > sizeof(*pa->irq_apid_to_ppid),
> > > > +					    GFP_KERNEL);
> > > > +	if (!pa->irq_apid_to_ppid) {
> > > > +		err = -ENOMEM;
> > > > +		goto err_put_ctrl;
> > > > +	}
> > > > +
> > > > +	pa->mapping_table = devm_kzalloc(&ctrl->dev,
> > > > +					(pa->max_apid - 1) *
> > > > sizeof(u32),
> > > > +					GFP_KERNEL);
> > > > +	if (!pa->mapping_table) {
> > > > +		err = -ENOMEM;
> > > > +		goto err_put_ctrl;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < (pa->max_apid - 1); ++i)
> > > > +		pa->mapping_table[i] = readl_relaxed(pa->cnfg +
> > > > +
> > > > SPMI_MAPPING_TABLE_REG(i));
> > 
> > > we're searching through the mapping table we can cache the value
> > > from the register if the entry isn't 0. This delays the
> > 
> > Greedy approach (reading upto 512 registers from hw) have very small
> > upfront penalty since sequential reads take place very fast
> > and HW acceleration (prefetching) will improve it. A lazy approach
> > will required much more complex algorithm (for example any value
> > including zero is valid. So we will need to augment a marker for
> > unassigned entries) and prefetching will not help.
> 
> That's solvable with a bitmap of 512 entries, not too much of a
> complicated algorithm.
> 
> We're sort of screwed now that we have to find the channel for a
> ppid by reading X number of PMIC_ARB_REG_CHNL registers. We could
> lazily do that too though, where we fill in the array linearly
> with what we read until we find the ppid we're looking for. We
> would need to do a bitmap trick again, but since we know ppid is
> at most going to use 12 bits, we can set the highest bit to
> indicate the ppid_to_chan eI agree that the algorithm is not
> complicated. However, I still think

I agree that the algorithm is not complicated. However, I still think
that just reading max of 512 registers is simpler and overhead is small.

> 
> > 
> > > processing to when we're mapping irqs, hopefully speeding up
> > > probe for the case where you have a handful of irqs to map.
> > 
> > Please note that we traverse this mapping table every time a PMIC
> > driver registers for an interrupt. This means that even if we speed
> > up the probe of pmic-arb driver we will not speed the system boot
> > since the probing of the PMIC drivers will register for interrupts.
> 
> Do we need to read all 512 entries for a typical system? If we do
> then I agree reading it all upfront is probably better, but if we
> only read a sparse amount of entries then we can defer it to when

We need a many of the 512 entries since for each request_irq() we
traverse down the. Typically during boot pmic drivers request irqs.

> it's going to be used. On upstream 8974 dts files with all the
> devices we have in DT right now, I see 75 unique register reads
> during boot on apq8074 dragonboard.
> 

There are still very few of pmic driver upstreamed. However, many more
of them are likely to get upstreamed soon. Once all of them will be
active on upstream I am sure that the number of register access will
dramatically increase (just as it is today on Codeaurora).

> > 
> > > 
> > > The DT property wouldn't be necessary then. Arguably it's being
> > > added there to optimize the size of the mapping table and isn't
> > > really necessary otherwise.
> > > 
> > 
> > We need to know this value for both memory allocation
> > and for stopping before reading over the bounds of the
> > mapping_table in HW.
> > 
> 
> Presumably the hardware is correct and isn't telling us to read

HW tells us nothing about the size of the HW mapping table.

> entries in the mapping table that are beyond the bounds, so
> stepping over the bounds of the mapping table cache isn't
> possible if we always make it the size of the biggest table.
> 

The size of the table in RAM is the driver writer choice. It is
wasteful to have it larger then needed but no harm to stability.
On the flip side, reading registers beyond the HW bounds can have
undefined behavior. 

> Here's the untested patch. I hope we don't ever have more than
> 512 periphs.
> 
> BTW, we shouldn't make qcom,channel required on v2 hardware. That
> doesn't make any sense so it should be changed to required only
> on v1 devices and we should remove it from the 8916 dtsi file.
> 
> ----8<-----
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> 
>  drivers/spmi/spmi-pmic-arb.c | 98
> +++++++++++++++++++++++++++++--------------- 1 file changed, 66
> insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c
> b/drivers/spmi/spmi-pmic-arb.c index c7aa6f1a898e..cbe963f829d7 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -10,6 +10,7 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
>   */
> +#include <linux/bitmap.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> @@ -47,9 +48,9 @@
>  #define SPMI_MAPPING_BIT_IS_1_FLAG(X)	(((X) >> 8) & 0x1)
>  #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
>  
> -#define SPMI_MAPPING_TABLE_LEN		255
>  #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of
> 16-bits */ -#define PPID_TO_CHAN_TABLE_SZ
> BIT(12)	/* PPID is 12bit chan is 1byte*/ +#define
> PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */ +#define
> PMIC_ARB_PPID_VALID		BIT(15) 
>  /* Ownership Table */
>  #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
> @@ -85,9 +86,7 @@ enum pmic_arb_cmd_op_code {
>  };
>  
>  /* Maximum number of support PMIC peripherals */
> -#define PMIC_ARB_MAX_PERIPHS		256
> -#define PMIC_ARB_MAX_CHNL		128
> -#define PMIC_ARB_PERIPH_ID_VALID	(1 << 15)
> +#define PMIC_ARB_MAX_PERIPHS		512
>  #define PMIC_ARB_TIMEOUT_US		100
>  #define PMIC_ARB_MAX_TRANS_BYTES	(8)
>  
> @@ -125,18 +124,21 @@ struct spmi_pmic_arb_dev {
>  	void __iomem		*wr_base;
>  	void __iomem		*intr;
>  	void __iomem		*cnfg;
> +	void __iomem		*core;
>  	raw_spinlock_t		lock;
>  	u8			channel;
>  	int			irq;
>  	u8			ee;
> -	u8			min_apid;
> -	u8			max_apid;
> -	u32
> mapping_table[SPMI_MAPPING_TABLE_LEN];
> +	u16			min_apid;
> +	u16			max_apid;
> +	u32			*mapping_table;
> +	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>  	struct irq_domain	*domain;
>  	struct spmi_controller	*spmic;
> -	u16			apid_to_ppid[256];
> +	u16			*apid_to_ppid;
>  	const struct pmic_arb_ver_ops *ver_ops;
> -	u8			*ppid_to_chan;
> +	u16			*ppid_to_chan;
> +	u16			last_channel;
>  };
>  
>  /**
> @@ -614,6 +616,10 @@ static int search_mapping_table(struct
> spmi_pmic_arb_dev *pa, u32 data;
>  
>  	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
> +		if (!test_and_set_bit(index,
> pa->mapping_table_valid))
> +			mapping_table[index] =
> readl_relaxed(pa->cnfg +
> +
> SPMI_MAPPING_TABLE_REG(index)); +
>  		data = mapping_table[index];
>  
>  		if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
> @@ -706,11 +712,37 @@ static u32 pmic_arb_offset_v1(struct
> spmi_pmic_arb_dev *pa, u8 sid, u16 addr) return 0x800 + 0x80 *
> pa->channel; }
>  
> +static u16 find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
> +{
> +	u32 regval;
> +	u16 chan;
> +	u16 _ppid;
> +
> +	for (chan = pa->last_channel; ; chan++, pa->last_channel++) {
> +		regval = readl_relaxed(pa->core +
> PMIC_ARB_REG_CHNL(chan));
> +		if (!regval)
> +			continue;
> +

I'm pretty sure that caching this table is way faster then searching it
by reading from hw per any newly used apid. If we use 512 peripherals
then we will read this table 512 times.

> +		_ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> +		pa->ppid_to_chan[_ppid] = chan | PMIC_ARB_PPID_VALID;
> +		if (_ppid == ppid)
> +			break;
> +	}
> +
> +	return chan;
> +}
> +
> +
>  /* v2 offset per ppid (chan) and per ee */
>  static u32 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid,
> u16 addr) {
>  	u16 ppid = (sid << 8) | (addr >> 8);
> -	u8  chan = pa->ppid_to_chan[ppid];
> +	u16 chan;
> +

I would use PMIC_ARB_PPID_VALID here to reject access to invalid ppids.

> +	chan = pa->ppid_to_chan[ppid];
> +	if (!(chan & PMIC_ARB_PPID_VALID))
> +		chan = find_chan(pa, ppid);
> +	chan &= ~PMIC_ARB_PPID_MASK;
>  
>  	return 0x1000 * pa->ee + 0x8000 * chan;
>  }
> @@ -797,7 +829,7 @@ static int spmi_pmic_arb_probe(struct
> platform_device *pdev) struct resource *res;
>  	void __iomem *core;
>  	u32 channel, ee, hw_ver;
> -	int err, i;
> +	int err;
>  	bool is_v1;
>  
>  	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
> @@ -825,10 +857,7 @@ static int spmi_pmic_arb_probe(struct
> platform_device *pdev) pa->wr_base = core;
>  		pa->rd_base = core;
>  	} else {
> -		u8  chan;
> -		u16 ppid;
> -		u32 regval;
> -
> +		pa->core = core;
>  		pa->ver_ops = &pmic_arb_v2;
>  
>  		res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM, @@ -847,23 +876,17 @@ static int
> spmi_pmic_arb_probe(struct platform_device *pdev) goto err_put_ctrl;
>  		}
>  
> -		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
> -					PPID_TO_CHAN_TABLE_SZ,
> GFP_KERNEL);
> -		if (!pa->ppid_to_chan) {
> -			err = -ENOMEM;
> -			goto err_put_ctrl;
> -		}
>  		/*
>  		 * PMIC_ARB_REG_CHNL is a table in HW mapping
> channel to ppid.
>  		 * ppid_to_chan is an in-memory invert of that table.
>  		 */
> -		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
> -			regval = readl_relaxed(core +
> PMIC_ARB_REG_CHNL(chan));
> -			if (!regval)
> -				continue;
> -
> -			ppid = (regval >> 8) & 0xFFF;
> -			pa->ppid_to_chan[ppid] = chan;
> +		pa->ppid_to_chan = devm_kcalloc(&ctrl->dev,
> +						PMIC_ARB_MAX_PPID,
> +
> sizeof(*pa->ppid_to_chan),
> +						GFP_KERNEL);
> +		if (!pa->ppid_to_chan) {
> +			err = -ENOMEM;
> +			goto err_put_ctrl;
>  		}
>  	}
>  
> @@ -915,9 +938,20 @@ static int spmi_pmic_arb_probe(struct
> platform_device *pdev) 
>  	pa->ee = ee;
>  
> -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> -		pa->mapping_table[i] = readl_relaxed(
> -				pa->cnfg +
> SPMI_MAPPING_TABLE_REG(i));
> +	pa->apid_to_ppid = devm_kcalloc(&ctrl->dev,
> PMIC_ARB_MAX_PERIPHS,
> +
> sizeof(*pa->apid_to_ppid),
> +					    GFP_KERNEL);
> +	if (!pa->apid_to_ppid) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
> +
> +	pa->mapping_table = devm_kcalloc(&ctrl->dev,
> PMIC_ARB_MAX_PERIPHS - 1,
> +					sizeof(*pa->mapping_table),
> GFP_KERNEL);
> +	if (!pa->mapping_table) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
>  
>  	/* Initialize max_apid/min_apid to the opposite bounds,
> during
>  	 * the irq domain translation, we are sure to update these */

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/09, Gilad Avidov wrote:
> On Fri, 4 Sep 2015 17:50:11 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > On 09/04, Gilad Avidov wrote:
> > > On Thu, 3 Sep 2015 17:16:30 -0700
> > > Stephen Boyd <sboyd@codeaurora.org> wrote:
> > > 
> > > > On 09/03, Gilad Avidov wrote:
> 
> I agree that the algorithm is not complicated. However, I still think
> that just reading max of 512 registers is simpler and overhead is small.
> 
> > 
> > > 
> > > > processing to when we're mapping irqs, hopefully speeding up
> > > > probe for the case where you have a handful of irqs to map.
> > > 
> > > Please note that we traverse this mapping table every time a PMIC
> > > driver registers for an interrupt. This means that even if we speed
> > > up the probe of pmic-arb driver we will not speed the system boot
> > > since the probing of the PMIC drivers will register for interrupts.
> > 
> > Do we need to read all 512 entries for a typical system? If we do
> > then I agree reading it all upfront is probably better, but if we
> > only read a sparse amount of entries then we can defer it to when
> 
> We need a many of the 512 entries since for each request_irq() we
> traverse down the. Typically during boot pmic drivers request irqs.
> 
> > it's going to be used. On upstream 8974 dts files with all the
> > devices we have in DT right now, I see 75 unique register reads
> > during boot on apq8074 dragonboard.
> > 
> 
> There are still very few of pmic driver upstreamed. However, many more
> of them are likely to get upstreamed soon. Once all of them will be
> active on upstream I am sure that the number of register access will
> dramatically increase (just as it is today on Codeaurora).

So on the latest hardware I see 33 unique register reads. That's
on the codeaurora android tree. It seems to be less than on 8974.
I'll have to boot up the android tree on 8974 to see how much it
is. The patch is simple:

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index d7119db49cfe..8aea9f57f7af 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -603,6 +603,7 @@ static int search_mapping_table(struct spmi_pmic_arb_dev *pa,
 
 	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
 		data = mapping_table[index];
+		pr_info("mapping table %d\n", index);
 
 		if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
 			if (SPMI_MAPPING_BIT_IS_1_FLAG(data)) {


And then

	$ dmesg | grep "mapping table " | sort -u | wc -l

> 
> > > 
> > > > 
> > > > The DT property wouldn't be necessary then. Arguably it's being
> > > > added there to optimize the size of the mapping table and isn't
> > > > really necessary otherwise.
> > > > 
> > > 
> > > We need to know this value for both memory allocation
> > > and for stopping before reading over the bounds of the
> > > mapping_table in HW.
> > > 
> > 
> > Presumably the hardware is correct and isn't telling us to read
> 
> HW tells us nothing about the size of the HW mapping table.
> 
> > entries in the mapping table that are beyond the bounds, so
> > stepping over the bounds of the mapping table cache isn't
> > possible if we always make it the size of the biggest table.
> > 
> 
> The size of the table in RAM is the driver writer choice. It is
> wasteful to have it larger then needed but no harm to stability.
> On the flip side, reading registers beyond the HW bounds can have
> undefined behavior. 

Sure. We could realloc the table at runtime if we really care
about saving the space.

> 
> > @@ -706,11 +712,37 @@ static u32 pmic_arb_offset_v1(struct
> > spmi_pmic_arb_dev *pa, u8 sid, u16 addr) return 0x800 + 0x80 *
> > pa->channel; }
> >  
> > +static u16 find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
> > +{
> > +	u32 regval;
> > +	u16 chan;
> > +	u16 _ppid;
> > +
> > +	for (chan = pa->last_channel; ; chan++, pa->last_channel++) {
> > +		regval = readl_relaxed(pa->core +
> > PMIC_ARB_REG_CHNL(chan));
> > +		if (!regval)
> > +			continue;
> > +
> 
> I'm pretty sure that caching this table is way faster then searching it
> by reading from hw per any newly used apid. If we use 512 peripherals
> then we will read this table 512 times.

You missed the part where we don't read this table from hardware
more than once. We read the hw table linearly until we find the
ppid that we were looking for, caching other values along the
way. If that happens to be the whole table then I guess we don't
win anything by doing it here vs. in probe, but hopefully we
don't have to search the entire table on the first try to find
the ppid.

> 
> > +		_ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> > +		pa->ppid_to_chan[_ppid] = chan | PMIC_ARB_PPID_VALID;
> > +		if (_ppid == ppid)
> > +			break;
> > +	}
> > +
> > +	return chan;
> > +}
> > +
> > +
> >  /* v2 offset per ppid (chan) and per ee */
> >  static u32 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid,
> > u16 addr) {
> >  	u16 ppid = (sid << 8) | (addr >> 8);
> > -	u8  chan = pa->ppid_to_chan[ppid];
> > +	u16 chan;
> > +
> 
> I would use PMIC_ARB_PPID_VALID here to reject access to invalid ppids.

Do you mean PMIC_ARB_MAX_PPID? We don't have that check now. I
would guess that's because we assume the ppid is always valid at
this point.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

The current driver implementation supports only 128 peripherals.
Add support for more than 128 peripherals by taking a lazy
caching approach to the mapping tables. Instead of reading the
tables at boot given some fixed size, read them on an as needed
basis and cache the results. We still assume a max number of 512
peripherals, trading off some space for simplicity.

Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
Sagar Dharia <sdharia@codeaurora.org>.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

This replaces a previous patch from Gilad and Sagar. I've 
added some commit text and made it into a proper patch.

 drivers/spmi/spmi-pmic-arb.c | 101 +++++++++++++++++++++++++++++--------------
 1 file changed, 68 insertions(+), 33 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index c7aa6f1a898e..0cd86d96bb7d 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -10,6 +10,7 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
+#include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -47,9 +48,9 @@
 #define SPMI_MAPPING_BIT_IS_1_FLAG(X)	(((X) >> 8) & 0x1)
 #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
 
-#define SPMI_MAPPING_TABLE_LEN		255
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
-#define PPID_TO_CHAN_TABLE_SZ		BIT(12)	/* PPID is 12bit chan is 1byte*/
+#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
+#define PMIC_ARB_CHAN_VALID		BIT(15)
 
 /* Ownership Table */
 #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
@@ -85,9 +86,7 @@ enum pmic_arb_cmd_op_code {
 };
 
 /* Maximum number of support PMIC peripherals */
-#define PMIC_ARB_MAX_PERIPHS		256
-#define PMIC_ARB_MAX_CHNL		128
-#define PMIC_ARB_PERIPH_ID_VALID	(1 << 15)
+#define PMIC_ARB_MAX_PERIPHS		512
 #define PMIC_ARB_TIMEOUT_US		100
 #define PMIC_ARB_MAX_TRANS_BYTES	(8)
 
@@ -125,18 +124,21 @@ struct spmi_pmic_arb_dev {
 	void __iomem		*wr_base;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
+	void __iomem		*core;
 	raw_spinlock_t		lock;
 	u8			channel;
 	int			irq;
 	u8			ee;
-	u8			min_apid;
-	u8			max_apid;
-	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
+	u16			min_apid;
+	u16			max_apid;
+	u32			*mapping_table;
+	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
 	struct irq_domain	*domain;
 	struct spmi_controller	*spmic;
-	u16			apid_to_ppid[256];
+	u16			*apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
-	u8			*ppid_to_chan;
+	u16			*ppid_to_chan;
+	u16			last_channel;
 };
 
 /**
@@ -614,6 +616,10 @@ static int search_mapping_table(struct spmi_pmic_arb_dev *pa,
 	u32 data;
 
 	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
+		if (!test_and_set_bit(index, pa->mapping_table_valid))
+			mapping_table[index] = readl_relaxed(pa->cnfg +
+						SPMI_MAPPING_TABLE_REG(index));
+
 		data = mapping_table[index];
 
 		if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
@@ -706,11 +712,42 @@ static u32 pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
 	return 0x800 + 0x80 * pa->channel;
 }
 
+static u16 pmic_arb_find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
+{
+	u32 regval;
+	u16 chan;
+	u16 id;
+
+	/*
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
+	 * ppid_to_chan is an in-memory invert of that table.
+	 */
+	for (chan = pa->last_channel; ; chan++) {
+		regval = readl_relaxed(pa->core + PMIC_ARB_REG_CHNL(chan));
+		if (!regval)
+			continue;
+
+		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
+		pa->ppid_to_chan[id] = chan | PMIC_ARB_CHAN_VALID;
+		if (id == ppid)
+			break;
+	}
+	pa->last_channel = chan;
+
+	return chan;
+}
+
+
 /* v2 offset per ppid (chan) and per ee */
 static u32 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
 {
 	u16 ppid = (sid << 8) | (addr >> 8);
-	u8  chan = pa->ppid_to_chan[ppid];
+	u16 chan;
+
+	chan = pa->ppid_to_chan[ppid];
+	if (!(chan & PMIC_ARB_CHAN_VALID))
+		chan = pmic_arb_find_chan(pa, ppid);
+	chan &= ~PMIC_ARB_CHAN_VALID;
 
 	return 0x1000 * pa->ee + 0x8000 * chan;
 }
@@ -797,7 +834,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *core;
 	u32 channel, ee, hw_ver;
-	int err, i;
+	int err;
 	bool is_v1;
 
 	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
@@ -825,10 +862,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		pa->wr_base = core;
 		pa->rd_base = core;
 	} else {
-		u8  chan;
-		u16 ppid;
-		u32 regval;
-
+		pa->core = core;
 		pa->ver_ops = &pmic_arb_v2;
 
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
@@ -847,24 +881,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 			goto err_put_ctrl;
 		}
 
-		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
-					PPID_TO_CHAN_TABLE_SZ, GFP_KERNEL);
+		pa->ppid_to_chan = devm_kcalloc(&ctrl->dev,
+						PMIC_ARB_MAX_PPID,
+						sizeof(*pa->ppid_to_chan),
+						GFP_KERNEL);
 		if (!pa->ppid_to_chan) {
 			err = -ENOMEM;
 			goto err_put_ctrl;
 		}
-		/*
-		 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
-		 * ppid_to_chan is an in-memory invert of that table.
-		 */
-		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
-			regval = readl_relaxed(core + PMIC_ARB_REG_CHNL(chan));
-			if (!regval)
-				continue;
-
-			ppid = (regval >> 8) & 0xFFF;
-			pa->ppid_to_chan[ppid] = chan;
-		}
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
@@ -915,9 +939,20 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	pa->ee = ee;
 
-	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
-		pa->mapping_table[i] = readl_relaxed(
-				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
+	pa->apid_to_ppid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS,
+					    sizeof(*pa->apid_to_ppid),
+					    GFP_KERNEL);
+	if (!pa->apid_to_ppid) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
+
+	pa->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
+					sizeof(*pa->mapping_table), GFP_KERNEL);
+	if (!pa->mapping_table) {
+		err = -ENOMEM;
+		goto err_put_ctrl;
+	}
 
 	/* Initialize max_apid/min_apid to the opposite bounds, during
 	 * the irq domain translation, we are sure to update these */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/14/2015 02:54 PM, Stephen Boyd wrote:
> The current driver implementation supports only 128 peripherals.
> Add support for more than 128 peripherals by taking a lazy
> caching approach to the mapping tables. Instead of reading the
> tables at boot given some fixed size, read them on an as needed
> basis and cache the results. We still assume a max number of 512
> peripherals, trading off some space for simplicity.
>
> Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
> Sagar Dharia <sdharia@codeaurora.org>.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---

Hi Ivan,

This patch causes 8916 to crash, because there isn't a mapping for ppid
257 in the ppid to channel table. It seems that we're reading the revid
from the slave id 1 pmic by going through channel 0, which seems to be
setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can
we stop reading the revid registers from non-zero slave id pmic devices?
That would be one solution to fix this problem. Or maybe we need to
special case this in the pmic arbiter code to fold ppid 0xN01 (slave id
N and address 0x100) onto channel 0 all the time?

-Stephen

> This replaces a previous patch from Gilad and Sagar. I've 
> added some commit text and made it into a proper patch.
>
>  drivers/spmi/spmi-pmic-arb.c | 101 +++++++++++++++++++++++++++++--------------
>  1 file changed, 68 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index c7aa6f1a898e..0cd86d96bb7d 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -10,6 +10,7 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
>   */
> +#include <linux/bitmap.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> @@ -47,9 +48,9 @@
>  #define SPMI_MAPPING_BIT_IS_1_FLAG(X)	(((X) >> 8) & 0x1)
>  #define SPMI_MAPPING_BIT_IS_1_RESULT(X)	(((X) >> 0) & 0xFF)
>  
> -#define SPMI_MAPPING_TABLE_LEN		255
>  #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
> -#define PPID_TO_CHAN_TABLE_SZ		BIT(12)	/* PPID is 12bit chan is 1byte*/
> +#define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
> +#define PMIC_ARB_CHAN_VALID		BIT(15)
>  
>  /* Ownership Table */
>  #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
> @@ -85,9 +86,7 @@ enum pmic_arb_cmd_op_code {
>  };
>  
>  /* Maximum number of support PMIC peripherals */
> -#define PMIC_ARB_MAX_PERIPHS		256
> -#define PMIC_ARB_MAX_CHNL		128
> -#define PMIC_ARB_PERIPH_ID_VALID	(1 << 15)
> +#define PMIC_ARB_MAX_PERIPHS		512
>  #define PMIC_ARB_TIMEOUT_US		100
>  #define PMIC_ARB_MAX_TRANS_BYTES	(8)
>  
> @@ -125,18 +124,21 @@ struct spmi_pmic_arb_dev {
>  	void __iomem		*wr_base;
>  	void __iomem		*intr;
>  	void __iomem		*cnfg;
> +	void __iomem		*core;
>  	raw_spinlock_t		lock;
>  	u8			channel;
>  	int			irq;
>  	u8			ee;
> -	u8			min_apid;
> -	u8			max_apid;
> -	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
> +	u16			min_apid;
> +	u16			max_apid;
> +	u32			*mapping_table;
> +	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>  	struct irq_domain	*domain;
>  	struct spmi_controller	*spmic;
> -	u16			apid_to_ppid[256];
> +	u16			*apid_to_ppid;
>  	const struct pmic_arb_ver_ops *ver_ops;
> -	u8			*ppid_to_chan;
> +	u16			*ppid_to_chan;
> +	u16			last_channel;
>  };
>  
>  /**
> @@ -614,6 +616,10 @@ static int search_mapping_table(struct spmi_pmic_arb_dev *pa,
>  	u32 data;
>  
>  	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
> +		if (!test_and_set_bit(index, pa->mapping_table_valid))
> +			mapping_table[index] = readl_relaxed(pa->cnfg +
> +						SPMI_MAPPING_TABLE_REG(index));
> +
>  		data = mapping_table[index];
>  
>  		if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
> @@ -706,11 +712,42 @@ static u32 pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
>  	return 0x800 + 0x80 * pa->channel;
>  }
>  
> +static u16 pmic_arb_find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
> +{
> +	u32 regval;
> +	u16 chan;
> +	u16 id;
> +
> +	/*
> +	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
> +	 * ppid_to_chan is an in-memory invert of that table.
> +	 */
> +	for (chan = pa->last_channel; ; chan++) {
> +		regval = readl_relaxed(pa->core + PMIC_ARB_REG_CHNL(chan));
> +		if (!regval)
> +			continue;
> +
> +		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
> +		pa->ppid_to_chan[id] = chan | PMIC_ARB_CHAN_VALID;
> +		if (id == ppid)
> +			break;
> +	}
> +	pa->last_channel = chan;
> +
> +	return chan;
> +}
> +
> +
>  /* v2 offset per ppid (chan) and per ee */
>  static u32 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr)
>  {
>  	u16 ppid = (sid << 8) | (addr >> 8);
> -	u8  chan = pa->ppid_to_chan[ppid];
> +	u16 chan;
> +
> +	chan = pa->ppid_to_chan[ppid];
> +	if (!(chan & PMIC_ARB_CHAN_VALID))
> +		chan = pmic_arb_find_chan(pa, ppid);
> +	chan &= ~PMIC_ARB_CHAN_VALID;
>  
>  	return 0x1000 * pa->ee + 0x8000 * chan;
>  }
> @@ -797,7 +834,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	void __iomem *core;
>  	u32 channel, ee, hw_ver;
> -	int err, i;
> +	int err;
>  	bool is_v1;
>  
>  	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
> @@ -825,10 +862,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  		pa->wr_base = core;
>  		pa->rd_base = core;
>  	} else {
> -		u8  chan;
> -		u16 ppid;
> -		u32 regval;
> -
> +		pa->core = core;
>  		pa->ver_ops = &pmic_arb_v2;
>  
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> @@ -847,24 +881,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  			goto err_put_ctrl;
>  		}
>  
> -		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
> -					PPID_TO_CHAN_TABLE_SZ, GFP_KERNEL);
> +		pa->ppid_to_chan = devm_kcalloc(&ctrl->dev,
> +						PMIC_ARB_MAX_PPID,
> +						sizeof(*pa->ppid_to_chan),
> +						GFP_KERNEL);
>  		if (!pa->ppid_to_chan) {
>  			err = -ENOMEM;
>  			goto err_put_ctrl;
>  		}
> -		/*
> -		 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
> -		 * ppid_to_chan is an in-memory invert of that table.
> -		 */
> -		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
> -			regval = readl_relaxed(core + PMIC_ARB_REG_CHNL(chan));
> -			if (!regval)
> -				continue;
> -
> -			ppid = (regval >> 8) & 0xFFF;
> -			pa->ppid_to_chan[ppid] = chan;
> -		}
>  	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> @@ -915,9 +939,20 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  
>  	pa->ee = ee;
>  
> -	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> -		pa->mapping_table[i] = readl_relaxed(
> -				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
> +	pa->apid_to_ppid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS,
> +					    sizeof(*pa->apid_to_ppid),
> +					    GFP_KERNEL);
> +	if (!pa->apid_to_ppid) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
> +
> +	pa->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
> +					sizeof(*pa->mapping_table), GFP_KERNEL);
> +	if (!pa->mapping_table) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
>  
>  	/* Initialize max_apid/min_apid to the opposite bounds, during
>  	 * the irq domain translation, we are sure to update these */


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Ivan T. Ivanov]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]


On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote:
> On 09/14/2015 02:54 PM, Stephen Boyd wrote:
> > The current driver implementation supports only 128 peripherals.
> > Add support for more than 128 peripherals by taking a lazy
> > caching approach to the mapping tables. Instead of reading the
> > tables at boot given some fixed size, read them on an as needed
> > basis and cache the results. We still assume a max number of 512
> > peripherals, trading off some space for simplicity.
> > 
> > Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
> > Sagar Dharia <sdharia@codeaurora.org>.
> > 
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> 
> Hi Ivan,
> 
> This patch causes 8916 to crash, because there isn't a mapping for ppid
> 257 in the ppid to channel table. It seems that we're reading the revid
> from the slave id 1 pmic by going through channel 0, which seems to be
> setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can
> we stop reading the revid registers from non-zero slave id pmic devices?
> That would be one solution to fix this problem. Or maybe we need to
> special case this in the pmic arbiter code to fold ppid 0xN01 (slave id
> N and address 0x100) onto channel 0 all the time?
> 

Yes, we can. We are not using this information at the moment.
Right now, revision read is more or less for debug purposes.

Would following patch work for you? Of course it will be difficult
to guaranties that some other driver misbehave and touch non-existing
register, right?

Regards,
Ivan

[-- Attachment #2: 0001-mfd-qcom-spmi-pmic-Do-not-access-non-existing-regist.patch --]
[-- Type: text/x-patch, Size: 1392 bytes --]

From d7c9c59b7134f093cf3f829832f4f7771a65664e Mon Sep 17 00:00:00 2001
From: "Ivan T. Ivanov" <ivan.ivanov@linaro.org>
Date: Tue, 15 Sep 2015 09:43:10 +0300
Subject: [PATCH] mfd: qcom-spmi-pmic: Do not access non existing registers
Cc: linux-kernel@vger.kernel.org

Revision ID registers are available only on devices with
Slave ID Zero, so don't make access to unavailable registers.

Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org>
---
 drivers/mfd/qcom-spmi-pmic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c
index af6ac1c4b45c..d7ad72af5682 100644
--- a/drivers/mfd/qcom-spmi-pmic.c
+++ b/drivers/mfd/qcom-spmi-pmic.c
@@ -122,12 +122,22 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
 {
 	struct device_node *root = sdev->dev.of_node;
 	struct regmap *regmap;
+	u32 sid;
+	int ret;
+
+	ret = of_property_read_u32(root, "reg", &sid);
+	if (ret < 0) {
+		dev_err(&sdev->dev, "Missing SID\n");
+		return ret;
+	}
 
 	regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-	pmic_spmi_show_revid(regmap, &sdev->dev);
+	/* Only devices with Slave ID Zero contain this information */
+	if (sid == 0)
+		pmic_spmi_show_revid(regmap, &sdev->dev);
 
 	return of_platform_populate(root, NULL, NULL, &sdev->dev);
 }
-- 
1.9.1

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/15, Ivan T. Ivanov wrote:
> 
> On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote:
> > On 09/14/2015 02:54 PM, Stephen Boyd wrote:
> > > The current driver implementation supports only 128 peripherals.
> > > Add support for more than 128 peripherals by taking a lazy
> > > caching approach to the mapping tables. Instead of reading the
> > > tables at boot given some fixed size, read them on an as needed
> > > basis and cache the results. We still assume a max number of 512
> > > peripherals, trading off some space for simplicity.
> > > 
> > > Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
> > > Sagar Dharia <sdharia@codeaurora.org>.
> > > 
> > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > > ---
> > 
> > Hi Ivan,
> > 
> > This patch causes 8916 to crash, because there isn't a mapping for ppid
> > 257 in the ppid to channel table. It seems that we're reading the revid
> > from the slave id 1 pmic by going through channel 0, which seems to be
> > setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can
> > we stop reading the revid registers from non-zero slave id pmic devices?
> > That would be one solution to fix this problem. Or maybe we need to
> > special case this in the pmic arbiter code to fold ppid 0xN01 (slave id
> > N and address 0x100) onto channel 0 all the time?
> > 
> 
> Yes, we can. We are not using this information at the moment.
> Right now, revision read is more or less for debug purposes.
> 
> Would following patch work for you? Of course it will be difficult

Yes the patch works fine. Feel free to add a

Tested-by: Stephen Boyd <sboyd@codeaurora.org>

> to guaranties that some other driver misbehave and touch non-existing
> register, right?

Yeah some driver could misbehave and try to read/write something
that doesn't exist. As in this case, the device will reset. Given
that all this stuff is in the kernel though, it seems fine to
blow up instead of add a bunch of checks that aren't ever going
to matter in practice.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 09/15/2015 11:27 AM, Stephen Boyd wrote:
> On 09/15, Ivan T. Ivanov wrote:
>> On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote:
>>> On 09/14/2015 02:54 PM, Stephen Boyd wrote:
>>>> The current driver implementation supports only 128 peripherals.
>>>> Add support for more than 128 peripherals by taking a lazy
>>>> caching approach to the mapping tables. Instead of reading the
>>>> tables at boot given some fixed size, read them on an as needed
>>>> basis and cache the results. We still assume a max number of 512
>>>> peripherals, trading off some space for simplicity.
>>>>
>>>> Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
>>>> Sagar Dharia <sdharia@codeaurora.org>.
>>>>
>>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>>> ---
>>> Hi Ivan,
>>>
>>> This patch causes 8916 to crash, because there isn't a mapping for ppid
>>> 257 in the ppid to channel table. It seems that we're reading the revid
>>> from the slave id 1 pmic by going through channel 0, which seems to be
>>> setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can
>>> we stop reading the revid registers from non-zero slave id pmic devices?
>>> That would be one solution to fix this problem. Or maybe we need to
>>> special case this in the pmic arbiter code to fold ppid 0xN01 (slave id
>>> N and address 0x100) onto channel 0 all the time?
>>>
>> Yes, we can. We are not using this information at the moment.
>> Right now, revision read is more or less for debug purposes.
>>
>> Would following patch work for you? Of course it will be difficult
> Yes the patch works fine. Feel free to add a
>
> Tested-by: Stephen Boyd <sboyd@codeaurora.org>
>
>

I have to take this back. I missed the part where some pmics are on
slave id 2 or slave id 4, so this check isn't going to work. I've
adjusted it to use sid % 2 instead and I'll resend these two patches,
but I imagine to be more robust we're going to need to add a revid node
to the DT under the SID that actually has it. Then we can search the
child nodes for a revid compatible node and do the rev probing stuff.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Ivan T. Ivanov]

> On Oct 15, 2015, at 1:43 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> On 09/15/2015 11:27 AM, Stephen Boyd wrote:
>> On 09/15, Ivan T. Ivanov wrote:
>>> On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote:
>>>> On 09/14/2015 02:54 PM, Stephen Boyd wrote:
>>>>> The current driver implementation supports only 128 peripherals.
>>>>> Add support for more than 128 peripherals by taking a lazy
>>>>> caching approach to the mapping tables. Instead of reading the
>>>>> tables at boot given some fixed size, read them on an as needed
>>>>> basis and cache the results. We still assume a max number of 512
>>>>> peripherals, trading off some space for simplicity.
>>>>> 
>>>>> Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and
>>>>> Sagar Dharia <sdharia@codeaurora.org>.
>>>>> 
>>>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>>>> ---
>>>> Hi Ivan,
>>>> 
>>>> This patch causes 8916 to crash, because there isn't a mapping for ppid
>>>> 257 in the ppid to channel table. It seems that we're reading the revid
>>>> from the slave id 1 pmic by going through channel 0, which seems to be
>>>> setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can
>>>> we stop reading the revid registers from non-zero slave id pmic devices?
>>>> That would be one solution to fix this problem. Or maybe we need to
>>>> special case this in the pmic arbiter code to fold ppid 0xN01 (slave id
>>>> N and address 0x100) onto channel 0 all the time?
>>>> 
>>> Yes, we can. We are not using this information at the moment.
>>> Right now, revision read is more or less for debug purposes.
>>> 
>>> Would following patch work for you? Of course it will be difficult
>> Yes the patch works fine. Feel free to add a
>> 
>> Tested-by: Stephen Boyd <sboyd@codeaurora.org>
>> 
>> 
> 
> I have to take this back. I missed the part where some pmics are on
> slave id 2 or slave id 4, so this check isn't going to work. I've
> adjusted it to use sid % 2 instead and I'll resend these two patches,
> but I imagine to be more robust we're going to need to add a revid node
> to the DT under the SID that actually has it. Then we can search the
> child nodes for a revid compatible node and do the rev probing stuff.

Ah, yes. We don’t use revision information for now. 
I suppose we can just remove these reads until we need
this information?

Regards,
Ivan

Re: [PATCH] spmi-pmic-arb: support configurable number of peripherals

[Stephen Boyd]

On 10/15, Ivan T. Ivanov wrote:
> 
> > On Oct 15, 2015, at 1:43 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > 
> > I have to take this back. I missed the part where some pmics are on
> > slave id 2 or slave id 4, so this check isn't going to work. I've
> > adjusted it to use sid % 2 instead and I'll resend these two patches,
> > but I imagine to be more robust we're going to need to add a revid node
> > to the DT under the SID that actually has it. Then we can search the
> > child nodes for a revid compatible node and do the rev probing stuff.
> 
> Ah, yes. We don’t use revision information for now. 
> I suppose we can just remove these reads until we need
> this information?
> 

True, we could just remove all the code and make it look for a
revid node at some later time. But later would be soon because
I'm working on patches to add the read/write/volatile regmap
tables to this driver. I guess I'll just go all the way and do
the revid node part.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project