| Message ID | 1441322451-30149-1-git-send-email-gavidov@codeaurora.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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. > > > + }; > > + > > @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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", ®val); + 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);