| 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
| ||
|
Message-ID: <871bc3f2-d4c3-4c83-ad0c-04c65ed15598@linaro.org>
Date: Tue, 2 Apr 2024 10:25:52 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Abel Vesa <abel.vesa@...aro.org>, 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>
Cc: 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
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
> +
> 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
Powered by blists - more mailing lists