Received-SPF: pass (google.com: domain of linux-kernel+bounces-63820-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 13 Feb 2024 17:38:16 +0200
From: Abel Vesa <abel.vesa@linaro.org>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	Srini Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-mediatek@lists.infradead.org
Subject: Re: [PATCH RFC v2] spmi: pmic-arb: Add support for multiple buses
Message-ID: <ZcuM6ChFFN6pIlrf@linaro.org>
References: <20240213-spmi-multi-master-support-v2-1-b3b102326906@linaro.org>
 <3fcb9f16-61f8-4544-a319-20496d5eb106@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3fcb9f16-61f8-4544-a319-20496d5eb106@linaro.org>

On 24-02-13 15:55:56, Neil Armstrong wrote:
> On 13/02/2024 15:41, Abel Vesa wrote:
> > 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 <abel.vesa@linaro.org>
> > ---
> > 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.
> 
> > - Link to v1: https://lore.kernel.org/r/20240207-spmi-multi-master-support-v1-0-ce57f301c7fd@linaro.org
> > ---
> >   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>
> 
> 
> > +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.

> > +	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.

> 
> > +			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
> 
> > 
> > ---
> > base-commit: 445a555e0623387fa9b94e68e61681717e70200a
> > change-id: 20240207-spmi-multi-master-support-832a704b779b
> > 
> > Best regards,
>