lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d04dd893-7fd4-48a3-9b6b-19bd3b5a78f6@linaro.org>
Date: Tue, 2 Apr 2024 10:55:14 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Stephen Boyd <sboyd@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Srini Kandagatla <srinivas.kandagatla@...aro.org>,
 Johan Hovold <johan@...nel.org>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
 linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v7 6/6] spmi: pmic-arb: Add multi bus support

On 02/04/2024 10:50, Abel Vesa wrote:
> On 24-04-02 10:25:52, Neil Armstrong wrote:
>> 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@...aro.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
>>
> 
> You're right. Since the DT bindings for HW < v7 doesn't allow multi bus
> support, this check is not needed. Will drop.
> 
>>> +
>>>    	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
>>
> 
> Ditto.
> 
>>> +
>>>    	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
>>
> 
> Since the apid base and count are different between buses and since v7
> supports 2 buses, we need a v7 specific init_apid. So this one is
> needed.


I know, all those were wrongly removed in patch 5, just let them in place.

Neil

> 
>>> +
>>>    /*
>>>     * 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
>>
> 
> See above.
> 
>>>    	.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ