Messages in this thread | | | Date | Tue, 2 Apr 2024 10:25:52 +0200 | From | Neil Armstrong <> | Subject | Re: [PATCH v7 6/6] spmi: pmic-arb: Add multi bus support |
| |
Hi Abel,
On 29/03/2024 19:54, Abel Vesa wrote: > Starting with HW version 7, there are actually two separate buses > (with two separate sets of wires). So add support for the second bus. > The first platform that needs this support for the second bus is the > Qualcomm X1 Elite, so add the compatible for it as well. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/spmi/spmi-pmic-arb.c | 138 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 120 insertions(+), 18 deletions(-) > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index 19ff8665f3d9..56f2b3190d82 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -13,6 +13,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > @@ -95,6 +96,8 @@ enum pmic_arb_channel { > PMIC_ARB_CHANNEL_OBS, > }; > > +#define PMIC_ARB_MAX_BUSES 2 > + > /* Maximum number of support PMIC peripherals */ > #define PMIC_ARB_MAX_PERIPHS 512 > #define PMIC_ARB_MAX_PERIPHS_V7 1024 > @@ -148,6 +151,7 @@ struct spmi_pmic_arb; > * @min_apid: minimum APID (used for bounding IRQ search) > * @max_apid: maximum APID > * @irq: PMIC ARB interrupt. > + * @id: unique ID of the bus > */ > struct spmi_pmic_arb_bus { > struct spmi_pmic_arb *pmic_arb; > @@ -165,6 +169,7 @@ struct spmi_pmic_arb_bus { > u16 min_apid; > u16 max_apid; > int irq; > + u8 id; > }; > > /** > @@ -179,7 +184,8 @@ struct spmi_pmic_arb_bus { > * @ee: the current Execution Environment > * @ver_ops: version dependent operations. > * @max_periphs: Number of elements in apid_data[] > - * @bus: per arbiter bus instance > + * @buses: per arbiter buses instances > + * @buses_available: number of buses registered > */ > struct spmi_pmic_arb { > void __iomem *rd_base; > @@ -191,7 +197,8 @@ struct spmi_pmic_arb { > u8 ee; > const struct pmic_arb_ver_ops *ver_ops; > int max_periphs; > - struct spmi_pmic_arb_bus *bus; > + struct spmi_pmic_arb_bus *buses[PMIC_ARB_MAX_BUSES]; > + int buses_available; > }; > > /** > @@ -219,7 +226,7 @@ struct spmi_pmic_arb { > struct pmic_arb_ver_ops { > const char *ver_str; > int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > - int (*init_apid)(struct spmi_pmic_arb_bus *bus); > + int (*init_apid)(struct spmi_pmic_arb_bus *bus, int index); > int (*ppid_to_apid)(struct spmi_pmic_arb_bus *bus, u16 ppid); > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > int (*offset)(struct spmi_pmic_arb_bus *bus, u8 sid, u16 addr, > @@ -308,8 +315,8 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl, > } > > if (status & PMIC_ARB_STATUS_FAILURE) { > - dev_err(&ctrl->dev, "%s: %#x %#x: transaction failed (%#x)\n", > - __func__, sid, addr, status); > + dev_err(&ctrl->dev, "%s: %#x %#x: transaction failed (%#x) reg: 0x%x\n", > + __func__, sid, addr, status, offset); > WARN_ON(1); > return -EIO; > } > @@ -325,8 +332,8 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl, > udelay(1); > } > > - dev_err(&ctrl->dev, "%s: %#x %#x: timeout, status %#x\n", > - __func__, sid, addr, status); > + dev_err(&ctrl->dev, "%s: %#x %#x %#x: timeout, status %#x\n", > + __func__, bus->id, sid, addr, status); > return -ETIMEDOUT; > } > > @@ -1005,11 +1012,17 @@ static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > return 0; > } > > -static int pmic_arb_init_apid_v1(struct spmi_pmic_arb_bus *bus) > +static int pmic_arb_init_apid_v1(struct spmi_pmic_arb_bus *bus, int index) > { > struct spmi_pmic_arb *pmic_arb = bus->pmic_arb; > u32 *mapping_table; > > + if (index) { > + dev_err(&bus->spmic->dev, "Unsupported buses count %d detected\n", > + index); > + return -EINVAL; > + }
Shouldn't be here
> + > mapping_table = devm_kcalloc(&bus->spmic->dev, pmic_arb->max_periphs, > sizeof(*mapping_table), GFP_KERNEL); > if (!mapping_table) > @@ -1252,11 +1265,17 @@ static int pmic_arb_offset_v2(struct spmi_pmic_arb_bus *bus, u8 sid, u16 addr, > return 0x1000 * pmic_arb->ee + 0x8000 * apid; > } > > -static int pmic_arb_init_apid_v5(struct spmi_pmic_arb_bus *bus) > +static int pmic_arb_init_apid_v5(struct spmi_pmic_arb_bus *bus, int index) > { > struct spmi_pmic_arb *pmic_arb = bus->pmic_arb; > int ret; > > + if (index) { > + dev_err(&bus->spmic->dev, "Unsupported buses count %d detected\n", > + index); > + return -EINVAL; > + }
Shouldn't be here
> + > bus->base_apid = 0; > bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) & > PMIC_ARB_FEATURES_PERIPH_MASK; > @@ -1328,6 +1347,50 @@ static int pmic_arb_get_core_resources_v7(struct platform_device *pdev, > return pmic_arb_get_obsrvr_chnls_v2(pdev); > } > > +/* > + * Only v7 supports 2 buses. Each bus will get a different apid count, read > + * from different registers. > + */ > +static int pmic_arb_init_apid_v7(struct spmi_pmic_arb_bus *bus, int index) > +{ > + struct spmi_pmic_arb *pmic_arb = bus->pmic_arb; > + int ret; > + > + if (index == 0) { > + bus->base_apid = 0; > + bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) & > + PMIC_ARB_FEATURES_PERIPH_MASK; > + } else if (index == 1) { > + bus->base_apid = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) & > + PMIC_ARB_FEATURES_PERIPH_MASK; > + bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES1) & > + PMIC_ARB_FEATURES_PERIPH_MASK; > + } else { > + dev_err(&bus->spmic->dev, "Unsupported buses count %d detected\n", > + bus->id); > + return -EINVAL; > + } > + > + if (bus->base_apid + bus->apid_count > pmic_arb->max_periphs) { > + dev_err(&bus->spmic->dev, "Unsupported APID count %d detected\n", > + bus->base_apid + bus->apid_count); > + return -EINVAL; > + } > + > + ret = pmic_arb_init_apid_min_max(bus); > + if (ret) > + return ret; > + > + ret = pmic_arb_read_apid_map_v5(bus); > + if (ret) { > + dev_err(&bus->spmic->dev, "could not read APID->PPID mapping table, rc= %d\n", > + ret); > + return ret; > + } > + > + return 0; > +}
Shouldn't be here
> + > /* > * v7 offset per ee and per apid for observer channels and per apid for > * read/write channels. > @@ -1580,7 +1643,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = { > static const struct pmic_arb_ver_ops pmic_arb_v7 = { > .ver_str = "v7", > .get_core_resources = pmic_arb_get_core_resources_v7, > - .init_apid = pmic_arb_init_apid_v5, > + .init_apid = pmic_arb_init_apid_v7,
Shouldn't be here
> .ppid_to_apid = pmic_arb_ppid_to_apid_v5, > .non_data_cmd = pmic_arb_non_data_cmd_v2, > .offset = pmic_arb_offset_v7, > @@ -1604,6 +1667,7 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev, > struct device_node *node, > struct spmi_pmic_arb *pmic_arb) > { > + int bus_index = pmic_arb->buses_available; > struct spmi_pmic_arb_bus *bus; > struct device *dev = &pdev->dev; > struct spmi_controller *ctrl; > @@ -1622,7 +1686,7 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev, > > bus = spmi_controller_get_drvdata(ctrl); > > - pmic_arb->bus = bus; > + pmic_arb->buses[bus_index] = bus; > > bus->ppid_to_apid = devm_kcalloc(dev, PMIC_ARB_MAX_PPID, > sizeof(*bus->ppid_to_apid), > @@ -1665,12 +1729,13 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev, > bus->cnfg = cnfg; > bus->irq = irq; > bus->spmic = ctrl; > + bus->id = bus_index; > > - ret = pmic_arb->ver_ops->init_apid(bus); > + ret = pmic_arb->ver_ops->init_apid(bus, bus_index); > if (ret) > return ret; > > - dev_dbg(&pdev->dev, "adding irq domain\n"); > + dev_dbg(&pdev->dev, "adding irq domain for bus %d\n", bus_index); > > bus->domain = irq_domain_add_tree(dev->of_node, > &pmic_arb_irq_domain_ops, bus); > @@ -1683,14 +1748,53 @@ static int spmi_pmic_arb_bus_init(struct platform_device *pdev, > pmic_arb_chained_irq, bus); > > ctrl->dev.of_node = node; > + dev_set_name(&ctrl->dev, "spmi-%d", bus_index); > > ret = devm_spmi_controller_add(dev, ctrl); > if (ret) > return ret; > > + pmic_arb->buses_available++; > + > return 0; > } > > +static int spmi_pmic_arb_register_buses(struct spmi_pmic_arb *pmic_arb, > + struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct device_node *child; > + int ret; > + > + /* legacy mode doesn't provide child node for the bus */ > + if (of_device_is_compatible(node, "qcom,spmi-pmic-arb")) > + return spmi_pmic_arb_bus_init(pdev, node, pmic_arb); > + > + for_each_available_child_of_node(node, child) { > + if (of_node_name_eq(child, "spmi")) { > + ret = spmi_pmic_arb_bus_init(pdev, child, pmic_arb); > + if (ret) > + return ret; > + } > + } > + > + return ret; > +} > + > +static void spmi_pmic_arb_deregister_buses(struct spmi_pmic_arb *pmic_arb) > +{ > + int i; > + > + for (i = 0; i < PMIC_ARB_MAX_BUSES; i++) { > + struct spmi_pmic_arb_bus *bus = pmic_arb->buses[i]; > + > + irq_set_chained_handler_and_data(bus->irq, > + NULL, NULL); > + irq_domain_remove(bus->domain); > + } > +} > + > static int spmi_pmic_arb_probe(struct platform_device *pdev) > { > struct spmi_pmic_arb *pmic_arb; > @@ -1761,21 +1865,19 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) > > pmic_arb->ee = ee; > > - return spmi_pmic_arb_bus_init(pdev, dev->of_node, pmic_arb); > + return spmi_pmic_arb_register_buses(pmic_arb, pdev); > } > > static void spmi_pmic_arb_remove(struct platform_device *pdev) > { > struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > - struct spmi_pmic_arb_bus *bus = pmic_arb->bus; > > - irq_set_chained_handler_and_data(bus->irq, > - NULL, NULL); > - irq_domain_remove(bus->domain); > + spmi_pmic_arb_deregister_buses(pmic_arb); > } > > static const struct of_device_id spmi_pmic_arb_match_table[] = { > { .compatible = "qcom,spmi-pmic-arb", }, > + { .compatible = "qcom,x1e80100-spmi-pmic-arb", }, > {}, > }; > MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table); >
With issues fixed, it looks fine.
Thanks, Neil
| |