On 24-02-13 15:55:56, Neil Armstrong wrote:

On 13/02/2024 15:41, Abel Vesa wrote:

quoted

The v7 HW supports currently 2 buses. So register each bus as a separate
spmi controller and adapt all ops to use the bus instead of the
arbitrator. Legacy mode is still supported as long as there is no child
node that represents a bus, instead all nodes are expected to be actual
slave devices.

Signed-off-by: Abel Vesa <redacted>
---
Changes in v2:
- Reworked it so that it registers a spmi controller for each bus
   rather than relying on the generic framework to pass on the bus
   (master) id.

Thanks, I think this is better.

quoted

- Link to v1: https://lore.kernel.org/r/20240207-spmi-multi-master-support-v1-0-ce57f301c7fd@linaro.org (local)
---
  drivers/spmi/spmi-pmic-arb.c | 950 ++++++++++++++++++++++++++-----------------
  1 file changed, 585 insertions(+), 365 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 9ed1180fe31f..eced35b712b4 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c

<snip>

quoted

+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 = &pmic_arb->buses[bus_index];
+	struct device *dev = &pdev->dev;
+	struct spmi_controller *ctrl;
+	void __iomem *intr;
+	void __iomem *cnfg;
+	int index, ret;
+	u32 irq;
+
+	ctrl = devm_spmi_controller_alloc(dev, sizeof(*ctrl));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	ctrl->cmd = pmic_arb_cmd;
+	ctrl->read_cmd = pmic_arb_read_cmd;
+	ctrl->write_cmd = pmic_arb_write_cmd;
+
+	bus = spmi_controller_get_drvdata(ctrl);
+	bus->spmic = ctrl;
+
+	bus->ppid_to_apid = devm_kcalloc(dev, PMIC_ARB_MAX_PPID,
+					 sizeof(*bus->ppid_to_apid),
+					 GFP_KERNEL);
+	if (!bus->ppid_to_apid)
+		return -ENOMEM;
+
+	bus->apid_data = devm_kcalloc(dev, pmic_arb->max_periphs,
+				      sizeof(*bus->apid_data),
+				      GFP_KERNEL);
+	if (!bus->apid_data)
+		return -ENOMEM;
+
+	/* Optional property for v7: */
+	of_property_read_u32(node, "qcom,bus-id", &bus_index);

Not sure what bindings you plan to use, but this should be the reg property.

This is already part of the old bindings (and implementation) and it's
only used for cases where some platforms would have two separate DT
nodes (one for each bus, v7 only) as it would specify which bus id it
is.

None of the upstream platforms use the qcom,bus-id with any other value
than 0, currently.

TBH, I think it should be dropped entirely.

Also, just realized that if any platforms is using two separate
controller nodes for each bus, this new approach will break the second
instance as it will expect the qcom,bus-id value to still be 0.

quoted

+	if (bus_index != pmic_arb->buses_available) {
+		dev_err(dev, "wrong bus-id value");
+		return -EINVAL;
+	}
+
+	index = of_property_match_string(node, "reg-names", "cnfg");
+	if (index < 0) {
+		dev_err(dev, "cnfg reg region missing");
+		return -EINVAL;
+	}
+
+	cnfg = devm_of_iomap(dev, node, index, NULL);
+	if (IS_ERR(cnfg))
+		return PTR_ERR(cnfg);
+
+	index = of_property_match_string(node, "reg-names", "intr");
+	if (index < 0) {
+		dev_err(dev, "intr reg region missing");
+		return -EINVAL;
+	}
+
+	intr = devm_of_iomap(dev, node, index, NULL);
+	if (IS_ERR(intr))
+		return PTR_ERR(intr);
+
+	irq = of_irq_get_byname(node, "periph_irq");
+	if (irq < 0)
+		return irq;
+
+	bus->pmic_arb = pmic_arb;
+	bus->intr = intr;
+	bus->cnfg = cnfg;
+	bus->irq = irq;
+	bus->id = bus_index;
+
+	ret = pmic_arb->ver_ops->init_apid(bus, index);
+	if (ret)
+		return ret;
+
+	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);
+	if (!bus->domain) {
+		dev_err(&pdev->dev, "unable to create irq_domain\n");
+		return -ENOMEM;
+	}
+
+	irq_set_chained_handler_and_data(bus->irq,
+					 pmic_arb_chained_irq, bus);
+
+	bus->spmic->dev.of_node = node;
+	dev_set_name(&bus->spmic->dev, "spmi-%d", bus_index);
+
+	ret = devm_spmi_controller_add(dev, bus->spmic);
+	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;
+
+	for_each_available_child_of_node(node, child)
+		if (of_node_name_eq(child, "bus")) {

It seems you use "bus" subnodes, it seems you should also submit a new
bindings scheme for v7 controllers with subnodes

Yep, will do that.

quoted

+			ret = spmi_pmic_arb_bus_init(pdev, child, pmic_arb);
+			if (ret)
+				return ret;
+		}
+
+	if (!pmic_arb->buses_available)
+		ret = spmi_pmic_arb_bus_init(pdev, node, pmic_arb);
+
+	return ret;
+}
+
+static void spmi_pmic_arb_deregister_buses(struct spmi_pmic_arb *pmic_arb)
+{
+	int i;
+
+	for (i = 0; i < pmic_arb->buses_available; 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);
+	}
+}
+

<snip>

Overall the patch is __huge__, could you split it ? Like move the bus handling
then add the multi-bus support in separate patches ?

Sure thing.

But first please add new bindings first so we know what you expect from DT.

Yep, will add another example to the existing bindings schema.

Thanks,
Neil

quoted

---
base-commit: 445a555e0623387fa9b94e68e61681717e70200a
change-id: 20240207-spmi-multi-master-support-832a704b779b

Best regards,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel