| 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: <9af9cfd1-38c7-4f38-a652-fd046f427f98@linaro.org>
Date: Thu, 28 Mar 2024 09:51:49 +0100
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 RESEND v6 3/5] spmi: pmic-arb: Make the APID init a
version operation
On 26/03/2024 17:28, Abel Vesa wrote:
> Rather than using conditionals in probe function, add the APID init
> as a version specific operation. Due to v7, which supports multiple
> buses, pass on the bus index to be used for sorting out the apid base
> and count.
>
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
> drivers/spmi/spmi-pmic-arb.c | 199 +++++++++++++++++++++++++++----------------
> 1 file changed, 124 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 9ed1180fe31f..38fed8a585fe 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -183,6 +183,7 @@ struct spmi_pmic_arb {
> * struct pmic_arb_ver_ops - version dependent functionality.
> *
> * @ver_str: version string.
> + * @init_apid: finds the apid base and count
> * @ppid_to_apid: finds the apid for a given ppid.
> * @non_data_cmd: on v1 issues an spmi non-data command.
> * on v2 no HW support, returns -EOPNOTSUPP.
> @@ -202,6 +203,7 @@ struct spmi_pmic_arb {
> */
> struct pmic_arb_ver_ops {
> const char *ver_str;
> + int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index);
> int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid);
> /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> int (*offset)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> @@ -942,6 +944,38 @@ static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
> return 0;
> }
>
> +static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb)
> +{
> + /*
> + * Initialize max_apid/min_apid to the opposite bounds, during
> + * the irq domain translation, we are sure to update these
> + */
> + pmic_arb->max_apid = 0;
> + pmic_arb->min_apid = pmic_arb->max_periphs - 1;
> +
> + return 0;
> +}
> +
> +static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index)
> +{
> + u32 *mapping_table;
> +
> + if (index) {
> + dev_err(&pmic_arb->spmic->dev, "Unsupported buses count %d detected\n",
> + index);
> + return -EINVAL;
> + }
> +
> + mapping_table = devm_kcalloc(&pmic_arb->spmic->dev, pmic_arb->max_periphs,
> + sizeof(*mapping_table), GFP_KERNEL);
> + if (!mapping_table)
> + return -ENOMEM;
> +
> + pmic_arb->mapping_table = mapping_table;
Can you specify in the spmi_pmic_arb->mapping_table struct documentation the mapping_table
is only used in v1 ? or even better rename it to mapping_table_v1
> +
> + return pmic_arb_init_apid_min_max(pmic_arb);
> +}
> +
> static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u16 ppid)
> {
> u32 *mapping_table = pmic_arb->mapping_table;
> @@ -1144,6 +1178,40 @@ static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> return 0x1000 * pmic_arb->ee + 0x8000 * apid;
> }
>
> +static int pmic_arb_init_apid_v5(struct spmi_pmic_arb *pmic_arb, int index)
> +{
> + int ret;
> +
> + if (index) {
> + dev_err(&pmic_arb->spmic->dev, "Unsupported buses count %d detected\n",
> + index);
> + return -EINVAL;
> + }
> +
> + pmic_arb->base_apid = 0;
> + pmic_arb->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) &
> + PMIC_ARB_FEATURES_PERIPH_MASK;
> +
> + if (pmic_arb->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) {
> + dev_err(&pmic_arb->spmic->dev, "Unsupported APID count %d detected\n",
> + pmic_arb->base_apid + pmic_arb->apid_count);
> + return -EINVAL;
> + }
> +
> + ret = pmic_arb_init_apid_min_max(pmic_arb);
> + if (ret)
> + return ret;
> +
> + ret = pmic_arb_read_apid_map_v5(pmic_arb);
> + if (ret) {
> + dev_err(&pmic_arb->spmic->dev, "could not read APID->PPID mapping table, rc= %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /*
> * v5 offset per ee and per apid for observer channels and per apid for
> * read/write channels.
> @@ -1178,6 +1246,49 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> return offset;
> }
>
> +/*
> + * 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 *pmic_arb, int index)
> +{
> + int ret;
> +
> + if (index == 0) {
> + pmic_arb->base_apid = 0;
> + pmic_arb->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) &
> + PMIC_ARB_FEATURES_PERIPH_MASK;
> + } else if (index == 1) {
> + pmic_arb->base_apid = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES) &
> + PMIC_ARB_FEATURES_PERIPH_MASK;
> + pmic_arb->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES1) &
> + PMIC_ARB_FEATURES_PERIPH_MASK;
> + } else {
> + dev_err(&pmic_arb->spmic->dev, "Unsupported buses count %d detected\n",
> + index);
> + return -EINVAL;
> + }
> +
> + if (pmic_arb->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) {
> + dev_err(&pmic_arb->spmic->dev, "Unsupported APID count %d detected\n",
> + pmic_arb->base_apid + pmic_arb->apid_count);
> + return -EINVAL;
> + }
> +
> + ret = pmic_arb_init_apid_min_max(pmic_arb);
> + if (ret)
> + return ret;
> +
> + ret = pmic_arb_read_apid_map_v5(pmic_arb);
> + if (ret) {
> + dev_err(&pmic_arb->spmic->dev, "could not read APID->PPID mapping table, rc= %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /*
> * v7 offset per ee and per apid for observer channels and per apid for
> * read/write channels.
> @@ -1358,6 +1469,7 @@ pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
>
> static const struct pmic_arb_ver_ops pmic_arb_v1 = {
> .ver_str = "v1",
> + .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v1,
> .non_data_cmd = pmic_arb_non_data_cmd_v1,
> .offset = pmic_arb_offset_v1,
> @@ -1372,6 +1484,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> .ver_str = "v2",
> + .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v2,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> .offset = pmic_arb_offset_v2,
> @@ -1386,6 +1499,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> .ver_str = "v3",
> + .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v2,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> .offset = pmic_arb_offset_v2,
> @@ -1400,6 +1514,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> .ver_str = "v5",
> + .init_apid = pmic_arb_init_apid_v5,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v5,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> .offset = pmic_arb_offset_v5,
> @@ -1414,6 +1529,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v7 = {
> .ver_str = "v7",
> + .init_apid = pmic_arb_init_apid_v7,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v5,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> .offset = pmic_arb_offset_v7,
> @@ -1439,7 +1555,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> struct spmi_controller *ctrl;
> struct resource *res;
> void __iomem *core;
> - u32 *mapping_table;
> u32 channel, ee, hw_ver;
> int err;
>
> @@ -1467,12 +1582,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>
> pmic_arb->core_size = resource_size(res);
>
> - pmic_arb->ppid_to_apid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PPID,
> - sizeof(*pmic_arb->ppid_to_apid),
> - GFP_KERNEL);
> - if (!pmic_arb->ppid_to_apid)
> - return -ENOMEM;
> -
> hw_ver = readl_relaxed(core + PMIC_ARB_VERSION);
>
> if (hw_ver < PMIC_ARB_VERSION_V2_MIN) {
> @@ -1506,58 +1615,17 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> return PTR_ERR(pmic_arb->wr_base);
> }
>
> - pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> + dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
> + pmic_arb->ver_ops->ver_str, hw_ver);
>
> - if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) {
> + if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> + else
> pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
> - /* Optional property for v7: */
> - of_property_read_u32(pdev->dev.of_node, "qcom,bus-id",
> - &pmic_arb->bus_instance);
> - if (pmic_arb->bus_instance > 1) {
> - dev_err(&pdev->dev, "invalid bus instance (%u) specified\n",
> - pmic_arb->bus_instance);
> - return -EINVAL;
> - }
> -
> - if (pmic_arb->bus_instance == 0) {
> - pmic_arb->base_apid = 0;
> - pmic_arb->apid_count =
> - readl_relaxed(core + PMIC_ARB_FEATURES) &
> - PMIC_ARB_FEATURES_PERIPH_MASK;
> - } else {
> - pmic_arb->base_apid =
> - readl_relaxed(core + PMIC_ARB_FEATURES) &
> - PMIC_ARB_FEATURES_PERIPH_MASK;
> - pmic_arb->apid_count =
> - readl_relaxed(core + PMIC_ARB_FEATURES1) &
> - PMIC_ARB_FEATURES_PERIPH_MASK;
> - }
>
> - if (pmic_arb->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) {
> - dev_err(&pdev->dev, "Unsupported APID count %d detected\n",
> - pmic_arb->base_apid + pmic_arb->apid_count);
> - return -EINVAL;
> - }
> - } else if (hw_ver >= PMIC_ARB_VERSION_V5_MIN) {
> - pmic_arb->base_apid = 0;
> - pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES) &
> - PMIC_ARB_FEATURES_PERIPH_MASK;
> -
> - if (pmic_arb->apid_count > pmic_arb->max_periphs) {
> - dev_err(&pdev->dev, "Unsupported APID count %d detected\n",
> - pmic_arb->apid_count);
> - return -EINVAL;
> - }
> - }
> -
> - pmic_arb->apid_data = devm_kcalloc(&ctrl->dev, pmic_arb->max_periphs,
> - sizeof(*pmic_arb->apid_data),
> - GFP_KERNEL);
> - if (!pmic_arb->apid_data)
> - return -ENOMEM;
> -
> - dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
> - pmic_arb->ver_ops->ver_str, hw_ver);
> + err = pmic_arb->ver_ops->init_apid(pmic_arb, 0);
> + if (err)
> + return err;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> pmic_arb->intr = devm_ioremap_resource(&ctrl->dev, res);
> @@ -1599,16 +1667,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> }
>
> pmic_arb->ee = ee;
> - mapping_table = devm_kcalloc(&ctrl->dev, pmic_arb->max_periphs,
> - sizeof(*mapping_table), GFP_KERNEL);
> - if (!mapping_table)
> - return -ENOMEM;
> -
> - pmic_arb->mapping_table = mapping_table;
> - /* Initialize max_apid/min_apid to the opposite bounds, during
> - * the irq domain translation, we are sure to update these */
> - pmic_arb->max_apid = 0;
> - pmic_arb->min_apid = pmic_arb->max_periphs - 1;
>
> platform_set_drvdata(pdev, ctrl);
> raw_spin_lock_init(&pmic_arb->lock);
> @@ -1617,15 +1675,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> ctrl->read_cmd = pmic_arb_read_cmd;
> ctrl->write_cmd = pmic_arb_write_cmd;
>
> - if (hw_ver >= PMIC_ARB_VERSION_V5_MIN) {
> - err = pmic_arb_read_apid_map_v5(pmic_arb);
> - if (err) {
> - dev_err(&pdev->dev, "could not read APID->PPID mapping table, rc= %d\n",
> - err);
> - return err;
> - }
> - }
> -
> dev_dbg(&pdev->dev, "adding irq domain\n");
> pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
> &pmic_arb_irq_domain_ops, pmic_arb);
>
With that clarified:
Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>
Powered by blists - more mailing lists