spmi: pmic-arb: Add support for PMIC v7

Message ID	20211201072718.3969011-1-vkoul@kernel.org (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-arm-msm-owner@kernel.org> From: Vinod Koul <vkoul@kernel.org> To: Linus Walleij <linus.walleij@linaro.org> Cc: linux-arm-msm@vger.kernel.org, Bjorn Andersson <bjorn.andersson@linaro.org>, David Dai <daidavid1@codeaurora.org>, Stephen Boyd <sboyd@kernel.org>, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Vinod Koul <vkoul@kernel.org> Subject: [PATCH] spmi: pmic-arb: Add support for PMIC v7 Date: Wed, 1 Dec 2021 12:57:18 +0530 Message-Id: <20211201072718.3969011-1-vkoul@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	spmi: pmic-arb: Add support for PMIC v7 \| expand spmi: pmic-arb: Add support for PMIC v7

Vinod Koul Dec. 1, 2021, 7:27 a.m. UTC

From: David Dai <daidavid1@codeaurora.org>

PMIC v7 has different offset values and seqeunces, so add support for
this new version of PMIC

Signed-off-by: David Dai <daidavid1@codeaurora.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spmi/spmi-pmic-arb.c | 188 +++++++++++++++++++++++++++++++----
 1 file changed, 169 insertions(+), 19 deletions(-)

Stephen Boyd Dec. 2, 2021, 11:06 p.m. UTC | #1

Quoting Vinod Koul (2021-11-30 23:27:18)
> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>         pmic_arb = spmi_controller_get_drvdata(ctrl);
>         pmic_arb->spmic = ctrl;
>  
> +       /*
> +        * Don't use devm_ioremap_resource() as the resources are shared in
> +        * PMIC v7 onwards, so causing failure when mapping
> +        */
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> -       core = devm_ioremap_resource(&ctrl->dev, res);
> +       core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));

What does this mean? We have two nodes in DT that have the same reg
properties? How does that work?

>         if (IS_ERR(core)) {
>                 err = PTR_ERR(core);
>                 goto err_put_ctrl;

Subbaraman Narayanamurthy Dec. 2, 2021, 11:09 p.m. UTC | #2

On 11/30/21 11:27 PM, Vinod Koul wrote:
> From: David Dai <daidavid1@codeaurora.org>

This change was made internally by David Collins.Can you please fix it?

commit ee7581a559720dd5a45f2f4c3eb7f8b4bde36118
Author: David Collins <collinsd@codeaurora.org>
Date:   Thu Oct 8 15:36:46 2020 -0700

    spmi: pmic-arb: add support for HW version 7
   
    Add support for version 7 of the SPMI PMIC arbiter.  It provides
    two independent SPMI bus interfaces which share some common PMIC
    arbiter registers.
   
    Change-Id: I7a2f816c9cd6898ada28967b47c8192f4529bc04
    Signed-off-by: David Collins <collinsd@codeaurora.org>

We wanted to send this patch once the current spmi-pmic-arb patch series [1] got reviewed and accepted.

[1] https://lore.kernel.org/linux-arm-msm/1638403212-29265-1-git-send-email-quic_fenglinw@quicinc.com/T/#t
>
> PMIC v7 has different offset values and seqeunces, so add support for
> this new version of PMIC
>
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 188 +++++++++++++++++++++++++++++++----
>  1 file changed, 169 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index bbbd311eda03..28418a10ee5c 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -22,8 +22,14 @@
>  #define PMIC_ARB_VERSION_V2_MIN		0x20010000
>  #define PMIC_ARB_VERSION_V3_MIN		0x30000000
>  #define PMIC_ARB_VERSION_V5_MIN		0x50000000
> +#define PMIC_ARB_VERSION_V7_MIN		0x70000000
>  #define PMIC_ARB_INT_EN			0x0004
>  
> +#define PMIC_ARB_FEATURES		0x0004
> +#define PMIC_ARB_FEATURES_PERIPH_MASK	GENMASK(10, 0)
> +
> +#define PMIC_ARB_FEATURES1		0x008
> +
>  /* PMIC Arbiter channel registers offsets */
>  #define PMIC_ARB_CMD			0x00
>  #define PMIC_ARB_CONFIG			0x04
> @@ -48,7 +54,6 @@
>  #define INVALID_EE				0xFF
>  
>  /* Ownership Table */
> -#define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
>  #define SPMI_OWNERSHIP_PERIPH2OWNER(X)	((X) & 0x7)
>  
>  /* Channel Status fields */
> @@ -91,6 +96,7 @@ enum pmic_arb_channel {
>  
>  /* Maximum number of support PMIC peripherals */
>  #define PMIC_ARB_MAX_PERIPHS		512
> +#define PMIC_ARB_MAX_PERIPHS_V7		1024
>  #define PMIC_ARB_TIMEOUT_US		100
>  #define PMIC_ARB_MAX_TRANS_BYTES	(8)
>  
> @@ -104,12 +110,12 @@ enum pmic_arb_channel {
>  	((((slave_id) & 0xF)   << 28) | \
>  	(((periph_id) & 0xFF)  << 20) | \
>  	(((irq_id)    & 0x7)   << 16) | \
> -	(((apid)      & 0x1FF) << 0))
> +	(((apid)      & 0x3FF) << 0))
>  
>  #define hwirq_to_sid(hwirq)  (((hwirq) >> 28) & 0xF)
>  #define hwirq_to_per(hwirq)  (((hwirq) >> 20) & 0xFF)
>  #define hwirq_to_irq(hwirq)  (((hwirq) >> 16) & 0x7)
> -#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x1FF)
> +#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x3FF)
>  
>  struct pmic_arb_ver_ops;
>  
> @@ -149,8 +155,11 @@ struct spmi_pmic_arb {
>  	u8			channel;
>  	int			irq;
>  	u8			ee;
> +	u32			bus_instance;
>  	u16			min_apid;
>  	u16			max_apid;
> +	u16			base_apid;
> +	int			apid_count;
>  	u32			*mapping_table;
>  	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>  	struct irq_domain	*domain;
> @@ -158,7 +167,8 @@ struct spmi_pmic_arb {
>  	const struct pmic_arb_ver_ops *ver_ops;
>  	u16			*ppid_to_apid;
>  	u16			last_apid;
> -	struct apid_data	apid_data[PMIC_ARB_MAX_PERIPHS];
> +	struct apid_data	*apid_data;
> +	int			max_periphs;
>  };
>  
>  /**
> @@ -196,6 +206,7 @@ struct pmic_arb_ver_ops {
>  	void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
>  	void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
>  	u32 (*apid_map_offset)(u16 n);
> +	void __iomem *(*apid_owner)(struct spmi_pmic_arb *pmic_arb, u16 n);
>  };
>  
>  static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,
> @@ -530,6 +541,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	int first = pmic_arb->min_apid >> 5;
>  	int last = pmic_arb->max_apid >> 5;
> +	int acc_offsets = pmic_arb->base_apid >> 5;
>  	u8 ee = pmic_arb->ee;
>  	u32 status, enable;
>  	int i, id, apid;
> @@ -538,7 +550,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>  
>  	for (i = first; i <= last; ++i) {
>  		status = readl_relaxed(
> -				ver_ops->owner_acc_status(pmic_arb, ee, i));
> +				ver_ops->owner_acc_status(pmic_arb, ee, i - acc_offsets));
>  		while (status) {
>  			id = ffs(status) - 1;
>  			status &= ~BIT(id);
> @@ -839,8 +851,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
>  		if (offset >= pmic_arb->core_size)
>  			break;
>  
> -		regval = readl_relaxed(pmic_arb->cnfg +
> -				      SPMI_OWNERSHIP_TABLE_REG(apid));
> +		regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, apid));
>  		apidd->irq_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>  		apidd->write_ee = apidd->irq_ee;
>  
> @@ -876,9 +887,9 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
>  
>  static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>  {
> -	struct apid_data *apidd = pmic_arb->apid_data;
> +	struct apid_data *apidd;
>  	struct apid_data *prev_apidd;
> -	u16 i, apid, ppid;
> +	u16 i, apid, ppid, apid_max;
>  	bool valid, is_irq_ee;
>  	u32 regval, offset;
>  
> @@ -889,7 +900,9 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>  	 * allowed to write to the APID.  The owner of the last (highest) APID
>  	 * for a given PPID will receive interrupts from the PPID.
>  	 */
> -	for (i = 0; ; i++, apidd++) {
> +	apidd = &pmic_arb->apid_data[pmic_arb->base_apid];
> +	apid_max = pmic_arb->base_apid + pmic_arb->apid_count;
> +	for (i = pmic_arb->base_apid; i < apid_max; i++, apidd++) {
>  		offset = pmic_arb->ver_ops->apid_map_offset(i);
>  		if (offset >= pmic_arb->core_size)
>  			break;
> @@ -900,8 +913,7 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>  		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
>  		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
>  
> -		regval = readl_relaxed(pmic_arb->cnfg +
> -				      SPMI_OWNERSHIP_TABLE_REG(i));
> +		regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, i));
>  		apidd->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>  
>  		apidd->irq_ee = is_irq_ee ? apidd->write_ee : INVALID_EE;
> @@ -995,6 +1007,36 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>  	return offset;
>  }
>  
> +static int pmic_arb_offset_v7(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> +			      enum pmic_arb_channel ch_type)
> +{
> +	u16 apid;
> +	int rc;
> +	u32 offset = 0;
> +	u16 ppid = (sid << 8) | (addr >> 8);
> +
> +	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
> +	if (rc < 0)
> +		return rc;
> +
> +	apid = rc;
> +	switch (ch_type) {
> +	case PMIC_ARB_CHANNEL_OBS:
> +		offset = 0x8000 * pmic_arb->ee + 0x20 * apid;
> +		break;
> +	case PMIC_ARB_CHANNEL_RW:
> +		if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) {
> +			dev_err(&pmic_arb->spmic->dev, "disallow spmi write to sid=%u, add: %x\n",
> +				sid, addr);
> +			return -EPERM;
> +		}
> +		offset = 0x10000 * apid;
> +		break;
> +	}
> +
> +	return offset;
> +}
> +
>  static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
>  {
>  	return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
> @@ -1029,6 +1071,12 @@ pmic_arb_owner_acc_status_v5(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
>  	return pmic_arb->intr + 0x10000 * m + 0x4 * n;
>  }
>  
> +static void __iomem *
> +pmic_arb_owner_acc_status_v7(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
> +{
> +	return pmic_arb->intr + 0x1000 * m + 0x4 * n;
> +}
> +
>  static void __iomem *
>  pmic_arb_acc_enable_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
>  {
> @@ -1047,6 +1095,12 @@ pmic_arb_acc_enable_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
>  	return pmic_arb->wr_base + 0x100 + 0x10000 * n;
>  }
>  
> +static void __iomem *
> +pmic_arb_acc_enable_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> +{
> +	return pmic_arb->wr_base + 0x100 + 0x1000 * n;
> +}
> +
>  static void __iomem *
>  pmic_arb_irq_status_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
>  {
> @@ -1065,6 +1119,12 @@ pmic_arb_irq_status_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
>  	return pmic_arb->wr_base + 0x104 + 0x10000 * n;
>  }
>  
> +static void __iomem *
> +pmic_arb_irq_status_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> +{
> +	return pmic_arb->wr_base + 0x104 + 0x1000 * n;
> +}
> +
>  static void __iomem *
>  pmic_arb_irq_clear_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
>  {
> @@ -1079,6 +1139,12 @@ pmic_arb_irq_clear_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
>  
>  static void __iomem *
>  pmic_arb_irq_clear_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
> +{
> +	return pmic_arb->wr_base + 0x108 + 0x1000 * n;
> +}
> +
> +static void __iomem *
> +pmic_arb_irq_clear_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
>  {
>  	return pmic_arb->wr_base + 0x108 + 0x10000 * n;
>  }
> @@ -1093,6 +1159,23 @@ static u32 pmic_arb_apid_map_offset_v5(u16 n)
>  	return 0x900 + 0x4 * n;
>  }
>  
> +static u32 pmic_arb_apid_map_offset_v7(u16 n)
> +{
> +	return 0x2000 + 0x4 * n;
> +}
> +
> +static void __iomem *
> +pmic_arb_apid_owner_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
> +{
> +	return pmic_arb->cnfg + 0x700 + 0x4 * n;
> +}
> +
> +static void __iomem *
> +pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> +{
> +	return pmic_arb->cnfg + 0x4 * (n - pmic_arb->base_apid);
> +}
> +
>  static const struct pmic_arb_ver_ops pmic_arb_v1 = {
>  	.ver_str		= "v1",
>  	.ppid_to_apid		= pmic_arb_ppid_to_apid_v1,
> @@ -1104,6 +1187,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
>  	.irq_status		= pmic_arb_irq_status_v1,
>  	.irq_clear		= pmic_arb_irq_clear_v1,
>  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> +	.apid_owner		= pmic_arb_apid_owner_v2,
>  };
>  
>  static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> @@ -1117,6 +1201,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
>  	.irq_status		= pmic_arb_irq_status_v2,
>  	.irq_clear		= pmic_arb_irq_clear_v2,
>  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> +	.apid_owner		= pmic_arb_apid_owner_v2,
>  };
>  
>  static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> @@ -1130,6 +1215,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
>  	.irq_status		= pmic_arb_irq_status_v2,
>  	.irq_clear		= pmic_arb_irq_clear_v2,
>  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> +	.apid_owner		= pmic_arb_apid_owner_v2,
>  };
>  
>  static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> @@ -1143,6 +1229,21 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
>  	.irq_status		= pmic_arb_irq_status_v5,
>  	.irq_clear		= pmic_arb_irq_clear_v5,
>  	.apid_map_offset	= pmic_arb_apid_map_offset_v5,
> +	.apid_owner		= pmic_arb_apid_owner_v2,
> +};
> +
> +static const struct pmic_arb_ver_ops pmic_arb_v7 = {
> +	.ver_str		= "v7",
> +	.ppid_to_apid		= pmic_arb_ppid_to_apid_v5,
> +	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
> +	.offset			= pmic_arb_offset_v7,
> +	.fmt_cmd		= pmic_arb_fmt_cmd_v2,
> +	.owner_acc_status	= pmic_arb_owner_acc_status_v7,
> +	.acc_enable		= pmic_arb_acc_enable_v7,
> +	.irq_status		= pmic_arb_irq_status_v7,
> +	.irq_clear		= pmic_arb_irq_clear_v7,
> +	.apid_map_offset	= pmic_arb_apid_map_offset_v7,
> +	.apid_owner		= pmic_arb_apid_owner_v7,
>  };
>  
>  static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  	pmic_arb = spmi_controller_get_drvdata(ctrl);
>  	pmic_arb->spmic = ctrl;
>  
> +	/*
> +	 * Don't use devm_ioremap_resource() as the resources are shared in
> +	 * PMIC v7 onwards, so causing failure when mapping
> +	 */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> -	core = devm_ioremap_resource(&ctrl->dev, res);
> +	core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>  	if (IS_ERR(core)) {
>  		err = PTR_ERR(core);
>  		goto err_put_ctrl;
> @@ -1199,12 +1304,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  			pmic_arb->ver_ops = &pmic_arb_v2;
>  		else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
>  			pmic_arb->ver_ops = &pmic_arb_v3;
> -		else
> +		else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
>  			pmic_arb->ver_ops = &pmic_arb_v5;
> +		else
> +			pmic_arb->ver_ops = &pmic_arb_v7;
>  
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  						   "obsrvr");
> -		pmic_arb->rd_base = devm_ioremap_resource(&ctrl->dev, res);
> +		pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>  		if (IS_ERR(pmic_arb->rd_base)) {
>  			err = PTR_ERR(pmic_arb->rd_base);
>  			goto err_put_ctrl;
> @@ -1212,25 +1319,68 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  						   "chnls");
> -		pmic_arb->wr_base = devm_ioremap_resource(&ctrl->dev, res);
> +		pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>  		if (IS_ERR(pmic_arb->wr_base)) {
>  			err = PTR_ERR(pmic_arb->wr_base);
>  			goto err_put_ctrl;
>  		}
>  	}
>  
> +	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> +
> +	if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) {
> +		pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
> +
> +		of_property_read_u32(pdev->dev.of_node, "qcom,bus-id", &pmic_arb->bus_instance);
> +		if (pmic_arb->bus_instance > 1) {
> +			err = -EINVAL;
> +			dev_err(&ctrl->dev, "invalid bus instance: %d\n", pmic_arb->bus_instance);
> +			goto err_put_ctrl;
> +		}
> +
> +		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;
> +		}
> +
> +	} 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->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) {
> +		err = -EINVAL;
> +		dev_err(&ctrl->dev, "Unsupported APID count: %d\n",
> +			pmic_arb->base_apid + pmic_arb->apid_count);
> +		goto err_put_ctrl;
> +	}
> +
> +	pmic_arb->apid_data = devm_kcalloc(&ctrl->dev, pmic_arb->max_periphs,
> +					   sizeof(*pmic_arb->apid_data), GFP_KERNEL);
> +	if (!pmic_arb->apid_data) {
> +		err = -ENOMEM;
> +		goto err_put_ctrl;
> +	}
> +
>  	dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
>  		 pmic_arb->ver_ops->ver_str, hw_ver);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> -	pmic_arb->intr = devm_ioremap_resource(&ctrl->dev, res);
> +	pmic_arb->intr = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>  	if (IS_ERR(pmic_arb->intr)) {
>  		err = PTR_ERR(pmic_arb->intr);
>  		goto err_put_ctrl;
>  	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg");
> -	pmic_arb->cnfg = devm_ioremap_resource(&ctrl->dev, res);
> +	pmic_arb->cnfg = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>  	if (IS_ERR(pmic_arb->cnfg)) {
>  		err = PTR_ERR(pmic_arb->cnfg);
>  		goto err_put_ctrl;
> @@ -1281,7 +1431,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  	/* 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;
> +	pmic_arb->min_apid = pmic_arb->max_periphs - 1;
>  
>  	platform_set_drvdata(pdev, ctrl);
>  	raw_spin_lock_init(&pmic_arb->lock);

David Collins Dec. 2, 2021, 11:51 p.m. UTC | #3

On 12/2/21 3:06 PM, Stephen Boyd wrote:
> Quoting Vinod Koul (2021-11-30 23:27:18)
>> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>>         pmic_arb = spmi_controller_get_drvdata(ctrl);
>>         pmic_arb->spmic = ctrl;
>>  
>> +       /*
>> +        * Don't use devm_ioremap_resource() as the resources are shared in
>> +        * PMIC v7 onwards, so causing failure when mapping
>> +        */
>>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
>> -       core = devm_ioremap_resource(&ctrl->dev, res);
>> +       core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> 
> What does this mean? We have two nodes in DT that have the same reg
> properties? How does that work?

PMIC Arbiter v7 has two SPMI bus master interfaces.  These are used to
communicate with two sets of PMICs.  The SPMI interfaces operate
independently; however, they share some register address ranges (e.g.
one common one is used for APID->PPID mapping).  The most
straightforward way to handle this is to treat them as two independent
top-level DT devices.

In this case the "cnfg" address is used in the DT node name as that is
unique between the two instances.

Here are the DT nodes used downstream on a target with PMIC Arbiter v7:

spmi0_bus: qcom,spmi@c42d000 {
	compatible = "qcom,spmi-pmic-arb";
	reg = <0xc42d000 0x4000>,
	      <0xc400000 0x3000>,
	      <0xc500000 0x400000>,
	      <0xc440000 0x80000>,
	      <0xc4c0000 0x10000>;
	reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
	interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
	interrupt-names = "periph_irq";
	interrupt-controller;
	#interrupt-cells = <4>;
	#address-cells = <2>;
	#size-cells = <0>;
	cell-index = <0>;
	qcom,channel = <0>;
	qcom,ee = <0>;
	qcom,bus-id = <0>;
};

spmi1_bus: qcom,spmi@c432000 {
	compatible = "qcom,spmi-pmic-arb";
	reg = <0xc432000 0x4000>,
	      <0xc400000 0x3000>,
	      <0xc500000 0x400000>,
	      <0xc440000 0x80000>,
	      <0xc4d0000 0x10000>;
	reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
	interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>;
	interrupt-names = "periph_irq";
	interrupt-controller;
	#interrupt-cells = <4>;
	#address-cells = <2>;
	#size-cells = <0>;
	cell-index = <0>;
	qcom,channel = <0>;
	qcom,ee = <0>;
	qcom,bus-id = <1>;
};

Note the inclusion of a new DT property: "qcom,bus-id".  This was
defined in a DT binding patch that isn't present in Vinod's submission.
Here is its definition:

- qcom,bus-id : Specifies which SPMI bus instance to use.  This property
		is only applicable for PMIC arbiter version 7 and
		beyond.
		Support values: 0 = primary bus, 1 = secondary bus
		Assumed to be 0 if unspecified.

Take care,
David

Stephen Boyd Dec. 3, 2021, 3:13 a.m. UTC | #4

Quoting David Collins (2021-12-02 15:51:18)
> On 12/2/21 3:06 PM, Stephen Boyd wrote:
> > Quoting Vinod Koul (2021-11-30 23:27:18)
> >> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >>         pmic_arb = spmi_controller_get_drvdata(ctrl);
> >>         pmic_arb->spmic = ctrl;
> >>  
> >> +       /*
> >> +        * Don't use devm_ioremap_resource() as the resources are shared in
> >> +        * PMIC v7 onwards, so causing failure when mapping
> >> +        */
> >>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> >> -       core = devm_ioremap_resource(&ctrl->dev, res);
> >> +       core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> > 
> > What does this mean? We have two nodes in DT that have the same reg
> > properties? How does that work?
> 
> PMIC Arbiter v7 has two SPMI bus master interfaces.  These are used to
> communicate with two sets of PMICs.  The SPMI interfaces operate
> independently; however, they share some register address ranges (e.g.
> one common one is used for APID->PPID mapping).

It looks like the two SPMI busses share the mapping table which is OK
because it's read-only but then the 'chnls' and 'obsrvr' regions are
also shared. There must be some global lock in place to make sure the
two controllers aren't writing to the same register in there? Or are we
saved because the mapping table splits the regions between the two
busses?

> The most
> straightforward way to handle this is to treat them as two independent
> top-level DT devices.
> 
> In this case the "cnfg" address is used in the DT node name as that is
> unique between the two instances.

Nice trick. The reg properties aren't supposed to change order though so
we shouldn't do that.

> 
> Here are the DT nodes used downstream on a target with PMIC Arbiter v7:
> 
> spmi0_bus: qcom,spmi@c42d000 {
>         compatible = "qcom,spmi-pmic-arb";
>         reg = <0xc42d000 0x4000>,
>               <0xc400000 0x3000>,
>               <0xc500000 0x400000>,
>               <0xc440000 0x80000>,
>               <0xc4c0000 0x10000>;
>         reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
>         interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
>         interrupt-names = "periph_irq";
>         interrupt-controller;
>         #interrupt-cells = <4>;
>         #address-cells = <2>;
>         #size-cells = <0>;
>         cell-index = <0>;
>         qcom,channel = <0>;
>         qcom,ee = <0>;
>         qcom,bus-id = <0>;
> };
> 
> spmi1_bus: qcom,spmi@c432000 {
>         compatible = "qcom,spmi-pmic-arb";
>         reg = <0xc432000 0x4000>,
>               <0xc400000 0x3000>,
>               <0xc500000 0x400000>,
>               <0xc440000 0x80000>,
>               <0xc4d0000 0x10000>;
>         reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
>         interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>;
>         interrupt-names = "periph_irq";
>         interrupt-controller;
>         #interrupt-cells = <4>;
>         #address-cells = <2>;
>         #size-cells = <0>;
>         cell-index = <0>;
>         qcom,channel = <0>;
>         qcom,ee = <0>;
>         qcom,bus-id = <1>;
> };

It feels like we should make a parent node that holds the core, chnls,
obsrvr reg properties with a new compatible string and then have two
child nodes for each bus. Then the various PMICs can hang off those two
different bus nodes. Of course, it needs DT review to make sure nothing
else goes wrong.

> 
> Note the inclusion of a new DT property: "qcom,bus-id".  This was
> defined in a DT binding patch that isn't present in Vinod's submission.
> Here is its definition:
> 
> - qcom,bus-id : Specifies which SPMI bus instance to use.  This property
>                 is only applicable for PMIC arbiter version 7 and
>                 beyond.
>                 Support values: 0 = primary bus, 1 = secondary bus
>                 Assumed to be 0 if unspecified.
> 

How is it used in the driver? Does something care what is primary and
what is secondary?

David Collins Dec. 4, 2021, 12:45 a.m. UTC | #5

On 12/2/21 7:13 PM, Stephen Boyd wrote:
> Quoting David Collins (2021-12-02 15:51:18)
>> On 12/2/21 3:06 PM, Stephen Boyd wrote:
>>> Quoting Vinod Koul (2021-11-30 23:27:18)
>>>> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>>>>         pmic_arb = spmi_controller_get_drvdata(ctrl);
>>>>         pmic_arb->spmic = ctrl;
>>>>  
>>>> +       /*
>>>> +        * Don't use devm_ioremap_resource() as the resources are shared in
>>>> +        * PMIC v7 onwards, so causing failure when mapping
>>>> +        */
>>>>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
>>>> -       core = devm_ioremap_resource(&ctrl->dev, res);
>>>> +       core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
>>>
>>> What does this mean? We have two nodes in DT that have the same reg
>>> properties? How does that work?
>>
>> PMIC Arbiter v7 has two SPMI bus master interfaces.  These are used to
>> communicate with two sets of PMICs.  The SPMI interfaces operate
>> independently; however, they share some register address ranges (e.g.
>> one common one is used for APID->PPID mapping).
> 
> It looks like the two SPMI busses share the mapping table which is OK
> because it's read-only but then the 'chnls' and 'obsrvr' regions are
> also shared. There must be some global lock in place to make sure the
> two controllers aren't writing to the same register in there? Or are we
> saved because the mapping table splits the regions between the two
> busses?

No global lock is present or needed.  The 'chnls' and 'obsvr' region
registers are indexed by APID.  Both SPMI buses use the same APID
numbering space.  However, that numbering space is segmented, e.g. APID
0-767 corresponds to SPMI0 bus and APID 768-1023 corresponds to SPMI1
bus.  Therefore, there are no circumstances where both buses could be
writing to the same set of registers.

The base and size of these segments is read from registers based upon
the "qcom,bus-id" property values:

>>>> +	if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) {
>>>> +		pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
>>>> +
>>>> +		of_property_read_u32(pdev->dev.of_node, "qcom,bus-id", &pmic_arb->bus_instance);
>>>> +		if (pmic_arb->bus_instance > 1) {
>>>> +			err = -EINVAL;
>>>> +			dev_err(&ctrl->dev, "invalid bus instance: %d\n", pmic_arb->bus_instance);
>>>> +			goto err_put_ctrl;
>>>> +		}
>>>> +
>>>> +		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;
>>>> +		}
>>>> +
>>>> +	}

Note that the specific register(s) that need to be read differ based on
the bus instance (0 or 1).  The 'base_apid' and 'apid_count' elements
are then used in various callback functions when indexing into regions
with a common numbering space.

>> The most
>> straightforward way to handle this is to treat them as two independent
>> top-level DT devices.
>>
>> In this case the "cnfg" address is used in the DT node name as that is
>> unique between the two instances.
> 
> Nice trick. The reg properties aren't supposed to change order though so
> we shouldn't do that.

Why does the ordering of "reg" entries matter?  "reg-names" defines what
each of the entries corresponds to (independent of the ordering).  Is
there a policy/requirement specified somewhere about the ordering of
"reg" entries when "reg-names" is a required property?

>> Here are the DT nodes used downstream on a target with PMIC Arbiter v7:
>>
>> spmi0_bus: qcom,spmi@c42d000 {
>>         compatible = "qcom,spmi-pmic-arb";
>>         reg = <0xc42d000 0x4000>,
>>               <0xc400000 0x3000>,
>>               <0xc500000 0x400000>,
>>               <0xc440000 0x80000>,
>>               <0xc4c0000 0x10000>;
>>         reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
>>         interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
>>         interrupt-names = "periph_irq";
>>         interrupt-controller;
>>         #interrupt-cells = <4>;
>>         #address-cells = <2>;
>>         #size-cells = <0>;
>>         cell-index = <0>;
>>         qcom,channel = <0>;
>>         qcom,ee = <0>;
>>         qcom,bus-id = <0>;
>> };
>>
>> spmi1_bus: qcom,spmi@c432000 {
>>         compatible = "qcom,spmi-pmic-arb";
>>         reg = <0xc432000 0x4000>,
>>               <0xc400000 0x3000>,
>>               <0xc500000 0x400000>,
>>               <0xc440000 0x80000>,
>>               <0xc4d0000 0x10000>;
>>         reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
>>         interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>;
>>         interrupt-names = "periph_irq";
>>         interrupt-controller;
>>         #interrupt-cells = <4>;
>>         #address-cells = <2>;
>>         #size-cells = <0>;
>>         cell-index = <0>;
>>         qcom,channel = <0>;
>>         qcom,ee = <0>;
>>         qcom,bus-id = <1>;
>> };
> 
> It feels like we should make a parent node that holds the core, chnls,
> obsrvr reg properties with a new compatible string and then have two
> child nodes for each bus. Then the various PMICs can hang off those two
> different bus nodes. Of course, it needs DT review to make sure nothing
> else goes wrong.

We considered this alternative DT layout when implementing PMIC arbiter
v7 support downstream.  The benefit is allowing common register ranges
to be specified once instead of in both SPMI bus nodes.  However, this
approach has several downsides.

It results in substantially more complex device tree binding
documentation that requires a different layout for SPMI buses for PMIC
arbiter v7 (and above) vs early versions even though support can be
provided with only a minimal modification (i.e. "qcom,bus-id").
Complexity is also increased inside of the spmi-pmic-arb driver.  There,
special data structures and logic would be needed to handle the shared
vs independent register addresses and data.

The SPMI framework currently uses a one-to-one mapping between SPMI
buses and struct device pointers.  This would not work if the new
top-level node represents the true struct device and the per-bus
subnodes are not true devices.  Also, platform_get_resource_byname()
(used in the spmi-pmic-arb probe function) needs a struct
platform_device pointer to read address ranges using "reg" +
"reg-names".  That wouldn't work for the subnodes.

I suppose that "compatible" could be specified for the top-level node
and the per bus subnodes so that all three get probed as struct devices.
 However, that makes things even more complicated and convoluted in the
driver (and DT).

I would prefer to stay with the approach of the two bus instances being
specified independently in DT.

Note that the clk-imx8qxp-lpcg driver has a similar situation where
multiple drivers need to map addresses in the same region.  Commit [1]
documents the requirement.  The details of the problem where
devm_platform_ioremap_resource() cannot be used in place of
devm_ioremap() were discussed in this thread [2].

>> Note the inclusion of a new DT property: "qcom,bus-id".  This was
>> defined in a DT binding patch that isn't present in Vinod's submission.
>> Here is its definition:
>>
>> - qcom,bus-id : Specifies which SPMI bus instance to use.  This property
>>                 is only applicable for PMIC arbiter version 7 and
>>                 beyond.
>>                 Support values: 0 = primary bus, 1 = secondary bus
>>                 Assumed to be 0 if unspecified.
>>
> 
> How is it used in the driver? Does something care what is primary and
> what is secondary?

See above for the code snippet where "qcom,bus-id" is read.

Thanks,
David

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=249fce6f3ab0dbd98caa9bc8ea4b50fa709e786a
[2]: https://lkml.org/lkml/2019/12/4/260

Vinod Koul Dec. 7, 2021, 11:47 a.m. UTC | #6

On 02-12-21, 15:09, Subbaraman Narayanamurthy wrote:
> On 11/30/21 11:27 PM, Vinod Koul wrote:
> > From: David Dai <daidavid1@codeaurora.org>
> 
> This change was made internally by David Collins.Can you please fix it?
> 
> commit ee7581a559720dd5a45f2f4c3eb7f8b4bde36118
> Author: David Collins <collinsd@codeaurora.org>
> Date:   Thu Oct 8 15:36:46 2020 -0700
> 
>     spmi: pmic-arb: add support for HW version 7
>    
>     Add support for version 7 of the SPMI PMIC arbiter.  It provides
>     two independent SPMI bus interfaces which share some common PMIC
>     arbiter registers.
>    
>     Change-Id: I7a2f816c9cd6898ada28967b47c8192f4529bc04
>     Signed-off-by: David Collins <collinsd@codeaurora.org>
> 
> We wanted to send this patch once the current spmi-pmic-arb patch series [1] got reviewed and accepted.

I do not mind doing that. I need this for SM8450 to work

> 
> [1] https://lore.kernel.org/linux-arm-msm/1638403212-29265-1-git-send-email-quic_fenglinw@quicinc.com/T/#t
> >
> > PMIC v7 has different offset values and seqeunces, so add support for
> > this new version of PMIC
> >
> > Signed-off-by: David Dai <daidavid1@codeaurora.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/spmi/spmi-pmic-arb.c | 188 +++++++++++++++++++++++++++++++----
> >  1 file changed, 169 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> > index bbbd311eda03..28418a10ee5c 100644
> > --- a/drivers/spmi/spmi-pmic-arb.c
> > +++ b/drivers/spmi/spmi-pmic-arb.c
> > @@ -22,8 +22,14 @@
> >  #define PMIC_ARB_VERSION_V2_MIN		0x20010000
> >  #define PMIC_ARB_VERSION_V3_MIN		0x30000000
> >  #define PMIC_ARB_VERSION_V5_MIN		0x50000000
> > +#define PMIC_ARB_VERSION_V7_MIN		0x70000000
> >  #define PMIC_ARB_INT_EN			0x0004
> >  
> > +#define PMIC_ARB_FEATURES		0x0004
> > +#define PMIC_ARB_FEATURES_PERIPH_MASK	GENMASK(10, 0)
> > +
> > +#define PMIC_ARB_FEATURES1		0x008
> > +
> >  /* PMIC Arbiter channel registers offsets */
> >  #define PMIC_ARB_CMD			0x00
> >  #define PMIC_ARB_CONFIG			0x04
> > @@ -48,7 +54,6 @@
> >  #define INVALID_EE				0xFF
> >  
> >  /* Ownership Table */
> > -#define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
> >  #define SPMI_OWNERSHIP_PERIPH2OWNER(X)	((X) & 0x7)
> >  
> >  /* Channel Status fields */
> > @@ -91,6 +96,7 @@ enum pmic_arb_channel {
> >  
> >  /* Maximum number of support PMIC peripherals */
> >  #define PMIC_ARB_MAX_PERIPHS		512
> > +#define PMIC_ARB_MAX_PERIPHS_V7		1024
> >  #define PMIC_ARB_TIMEOUT_US		100
> >  #define PMIC_ARB_MAX_TRANS_BYTES	(8)
> >  
> > @@ -104,12 +110,12 @@ enum pmic_arb_channel {
> >  	((((slave_id) & 0xF)   << 28) | \
> >  	(((periph_id) & 0xFF)  << 20) | \
> >  	(((irq_id)    & 0x7)   << 16) | \
> > -	(((apid)      & 0x1FF) << 0))
> > +	(((apid)      & 0x3FF) << 0))
> >  
> >  #define hwirq_to_sid(hwirq)  (((hwirq) >> 28) & 0xF)
> >  #define hwirq_to_per(hwirq)  (((hwirq) >> 20) & 0xFF)
> >  #define hwirq_to_irq(hwirq)  (((hwirq) >> 16) & 0x7)
> > -#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x1FF)
> > +#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x3FF)
> >  
> >  struct pmic_arb_ver_ops;
> >  
> > @@ -149,8 +155,11 @@ struct spmi_pmic_arb {
> >  	u8			channel;
> >  	int			irq;
> >  	u8			ee;
> > +	u32			bus_instance;
> >  	u16			min_apid;
> >  	u16			max_apid;
> > +	u16			base_apid;
> > +	int			apid_count;
> >  	u32			*mapping_table;
> >  	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
> >  	struct irq_domain	*domain;
> > @@ -158,7 +167,8 @@ struct spmi_pmic_arb {
> >  	const struct pmic_arb_ver_ops *ver_ops;
> >  	u16			*ppid_to_apid;
> >  	u16			last_apid;
> > -	struct apid_data	apid_data[PMIC_ARB_MAX_PERIPHS];
> > +	struct apid_data	*apid_data;
> > +	int			max_periphs;
> >  };
> >  
> >  /**
> > @@ -196,6 +206,7 @@ struct pmic_arb_ver_ops {
> >  	void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
> >  	void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
> >  	u32 (*apid_map_offset)(u16 n);
> > +	void __iomem *(*apid_owner)(struct spmi_pmic_arb *pmic_arb, u16 n);
> >  };
> >  
> >  static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,
> > @@ -530,6 +541,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> >  	int first = pmic_arb->min_apid >> 5;
> >  	int last = pmic_arb->max_apid >> 5;
> > +	int acc_offsets = pmic_arb->base_apid >> 5;
> >  	u8 ee = pmic_arb->ee;
> >  	u32 status, enable;
> >  	int i, id, apid;
> > @@ -538,7 +550,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
> >  
> >  	for (i = first; i <= last; ++i) {
> >  		status = readl_relaxed(
> > -				ver_ops->owner_acc_status(pmic_arb, ee, i));
> > +				ver_ops->owner_acc_status(pmic_arb, ee, i - acc_offsets));
> >  		while (status) {
> >  			id = ffs(status) - 1;
> >  			status &= ~BIT(id);
> > @@ -839,8 +851,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
> >  		if (offset >= pmic_arb->core_size)
> >  			break;
> >  
> > -		regval = readl_relaxed(pmic_arb->cnfg +
> > -				      SPMI_OWNERSHIP_TABLE_REG(apid));
> > +		regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, apid));
> >  		apidd->irq_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> >  		apidd->write_ee = apidd->irq_ee;
> >  
> > @@ -876,9 +887,9 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
> >  
> >  static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
> >  {
> > -	struct apid_data *apidd = pmic_arb->apid_data;
> > +	struct apid_data *apidd;
> >  	struct apid_data *prev_apidd;
> > -	u16 i, apid, ppid;
> > +	u16 i, apid, ppid, apid_max;
> >  	bool valid, is_irq_ee;
> >  	u32 regval, offset;
> >  
> > @@ -889,7 +900,9 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
> >  	 * allowed to write to the APID.  The owner of the last (highest) APID
> >  	 * for a given PPID will receive interrupts from the PPID.
> >  	 */
> > -	for (i = 0; ; i++, apidd++) {
> > +	apidd = &pmic_arb->apid_data[pmic_arb->base_apid];
> > +	apid_max = pmic_arb->base_apid + pmic_arb->apid_count;
> > +	for (i = pmic_arb->base_apid; i < apid_max; i++, apidd++) {
> >  		offset = pmic_arb->ver_ops->apid_map_offset(i);
> >  		if (offset >= pmic_arb->core_size)
> >  			break;
> > @@ -900,8 +913,7 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
> >  		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> >  		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
> >  
> > -		regval = readl_relaxed(pmic_arb->cnfg +
> > -				      SPMI_OWNERSHIP_TABLE_REG(i));
> > +		regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, i));
> >  		apidd->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> >  
> >  		apidd->irq_ee = is_irq_ee ? apidd->write_ee : INVALID_EE;
> > @@ -995,6 +1007,36 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> >  	return offset;
> >  }
> >  
> > +static int pmic_arb_offset_v7(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> > +			      enum pmic_arb_channel ch_type)
> > +{
> > +	u16 apid;
> > +	int rc;
> > +	u32 offset = 0;
> > +	u16 ppid = (sid << 8) | (addr >> 8);
> > +
> > +	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	apid = rc;
> > +	switch (ch_type) {
> > +	case PMIC_ARB_CHANNEL_OBS:
> > +		offset = 0x8000 * pmic_arb->ee + 0x20 * apid;
> > +		break;
> > +	case PMIC_ARB_CHANNEL_RW:
> > +		if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) {
> > +			dev_err(&pmic_arb->spmic->dev, "disallow spmi write to sid=%u, add: %x\n",
> > +				sid, addr);
> > +			return -EPERM;
> > +		}
> > +		offset = 0x10000 * apid;
> > +		break;
> > +	}
> > +
> > +	return offset;
> > +}
> > +
> >  static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
> >  {
> >  	return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
> > @@ -1029,6 +1071,12 @@ pmic_arb_owner_acc_status_v5(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
> >  	return pmic_arb->intr + 0x10000 * m + 0x4 * n;
> >  }
> >  
> > +static void __iomem *
> > +pmic_arb_owner_acc_status_v7(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n)
> > +{
> > +	return pmic_arb->intr + 0x1000 * m + 0x4 * n;
> > +}
> > +
> >  static void __iomem *
> >  pmic_arb_acc_enable_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  {
> > @@ -1047,6 +1095,12 @@ pmic_arb_acc_enable_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  	return pmic_arb->wr_base + 0x100 + 0x10000 * n;
> >  }
> >  
> > +static void __iomem *
> > +pmic_arb_acc_enable_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> > +{
> > +	return pmic_arb->wr_base + 0x100 + 0x1000 * n;
> > +}
> > +
> >  static void __iomem *
> >  pmic_arb_irq_status_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  {
> > @@ -1065,6 +1119,12 @@ pmic_arb_irq_status_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  	return pmic_arb->wr_base + 0x104 + 0x10000 * n;
> >  }
> >  
> > +static void __iomem *
> > +pmic_arb_irq_status_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> > +{
> > +	return pmic_arb->wr_base + 0x104 + 0x1000 * n;
> > +}
> > +
> >  static void __iomem *
> >  pmic_arb_irq_clear_v1(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  {
> > @@ -1079,6 +1139,12 @@ pmic_arb_irq_clear_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  
> >  static void __iomem *
> >  pmic_arb_irq_clear_v5(struct spmi_pmic_arb *pmic_arb, u16 n)
> > +{
> > +	return pmic_arb->wr_base + 0x108 + 0x1000 * n;
> > +}
> > +
> > +static void __iomem *
> > +pmic_arb_irq_clear_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> >  {
> >  	return pmic_arb->wr_base + 0x108 + 0x10000 * n;
> >  }
> > @@ -1093,6 +1159,23 @@ static u32 pmic_arb_apid_map_offset_v5(u16 n)
> >  	return 0x900 + 0x4 * n;
> >  }
> >  
> > +static u32 pmic_arb_apid_map_offset_v7(u16 n)
> > +{
> > +	return 0x2000 + 0x4 * n;
> > +}
> > +
> > +static void __iomem *
> > +pmic_arb_apid_owner_v2(struct spmi_pmic_arb *pmic_arb, u16 n)
> > +{
> > +	return pmic_arb->cnfg + 0x700 + 0x4 * n;
> > +}
> > +
> > +static void __iomem *
> > +pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
> > +{
> > +	return pmic_arb->cnfg + 0x4 * (n - pmic_arb->base_apid);
> > +}
> > +
> >  static const struct pmic_arb_ver_ops pmic_arb_v1 = {
> >  	.ver_str		= "v1",
> >  	.ppid_to_apid		= pmic_arb_ppid_to_apid_v1,
> > @@ -1104,6 +1187,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
> >  	.irq_status		= pmic_arb_irq_status_v1,
> >  	.irq_clear		= pmic_arb_irq_clear_v1,
> >  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> > +	.apid_owner		= pmic_arb_apid_owner_v2,
> >  };
> >  
> >  static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> > @@ -1117,6 +1201,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> >  	.irq_status		= pmic_arb_irq_status_v2,
> >  	.irq_clear		= pmic_arb_irq_clear_v2,
> >  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> > +	.apid_owner		= pmic_arb_apid_owner_v2,
> >  };
> >  
> >  static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> > @@ -1130,6 +1215,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> >  	.irq_status		= pmic_arb_irq_status_v2,
> >  	.irq_clear		= pmic_arb_irq_clear_v2,
> >  	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
> > +	.apid_owner		= pmic_arb_apid_owner_v2,
> >  };
> >  
> >  static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> > @@ -1143,6 +1229,21 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> >  	.irq_status		= pmic_arb_irq_status_v5,
> >  	.irq_clear		= pmic_arb_irq_clear_v5,
> >  	.apid_map_offset	= pmic_arb_apid_map_offset_v5,
> > +	.apid_owner		= pmic_arb_apid_owner_v2,
> > +};
> > +
> > +static const struct pmic_arb_ver_ops pmic_arb_v7 = {
> > +	.ver_str		= "v7",
> > +	.ppid_to_apid		= pmic_arb_ppid_to_apid_v5,
> > +	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
> > +	.offset			= pmic_arb_offset_v7,
> > +	.fmt_cmd		= pmic_arb_fmt_cmd_v2,
> > +	.owner_acc_status	= pmic_arb_owner_acc_status_v7,
> > +	.acc_enable		= pmic_arb_acc_enable_v7,
> > +	.irq_status		= pmic_arb_irq_status_v7,
> > +	.irq_clear		= pmic_arb_irq_clear_v7,
> > +	.apid_map_offset	= pmic_arb_apid_map_offset_v7,
> > +	.apid_owner		= pmic_arb_apid_owner_v7,
> >  };
> >  
> >  static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
> > @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >  	pmic_arb = spmi_controller_get_drvdata(ctrl);
> >  	pmic_arb->spmic = ctrl;
> >  
> > +	/*
> > +	 * Don't use devm_ioremap_resource() as the resources are shared in
> > +	 * PMIC v7 onwards, so causing failure when mapping
> > +	 */
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> > -	core = devm_ioremap_resource(&ctrl->dev, res);
> > +	core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> >  	if (IS_ERR(core)) {
> >  		err = PTR_ERR(core);
> >  		goto err_put_ctrl;
> > @@ -1199,12 +1304,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >  			pmic_arb->ver_ops = &pmic_arb_v2;
> >  		else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
> >  			pmic_arb->ver_ops = &pmic_arb_v3;
> > -		else
> > +		else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
> >  			pmic_arb->ver_ops = &pmic_arb_v5;
> > +		else
> > +			pmic_arb->ver_ops = &pmic_arb_v7;
> >  
> >  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >  						   "obsrvr");
> > -		pmic_arb->rd_base = devm_ioremap_resource(&ctrl->dev, res);
> > +		pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> >  		if (IS_ERR(pmic_arb->rd_base)) {
> >  			err = PTR_ERR(pmic_arb->rd_base);
> >  			goto err_put_ctrl;
> > @@ -1212,25 +1319,68 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >  
> >  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >  						   "chnls");
> > -		pmic_arb->wr_base = devm_ioremap_resource(&ctrl->dev, res);
> > +		pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> >  		if (IS_ERR(pmic_arb->wr_base)) {
> >  			err = PTR_ERR(pmic_arb->wr_base);
> >  			goto err_put_ctrl;
> >  		}
> >  	}
> >  
> > +	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> > +
> > +	if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) {
> > +		pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
> > +
> > +		of_property_read_u32(pdev->dev.of_node, "qcom,bus-id", &pmic_arb->bus_instance);
> > +		if (pmic_arb->bus_instance > 1) {
> > +			err = -EINVAL;
> > +			dev_err(&ctrl->dev, "invalid bus instance: %d\n", pmic_arb->bus_instance);
> > +			goto err_put_ctrl;
> > +		}
> > +
> > +		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;
> > +		}
> > +
> > +	} 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->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) {
> > +		err = -EINVAL;
> > +		dev_err(&ctrl->dev, "Unsupported APID count: %d\n",
> > +			pmic_arb->base_apid + pmic_arb->apid_count);
> > +		goto err_put_ctrl;
> > +	}
> > +
> > +	pmic_arb->apid_data = devm_kcalloc(&ctrl->dev, pmic_arb->max_periphs,
> > +					   sizeof(*pmic_arb->apid_data), GFP_KERNEL);
> > +	if (!pmic_arb->apid_data) {
> > +		err = -ENOMEM;
> > +		goto err_put_ctrl;
> > +	}
> > +
> >  	dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
> >  		 pmic_arb->ver_ops->ver_str, hw_ver);
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> > -	pmic_arb->intr = devm_ioremap_resource(&ctrl->dev, res);
> > +	pmic_arb->intr = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> >  	if (IS_ERR(pmic_arb->intr)) {
> >  		err = PTR_ERR(pmic_arb->intr);
> >  		goto err_put_ctrl;
> >  	}
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg");
> > -	pmic_arb->cnfg = devm_ioremap_resource(&ctrl->dev, res);
> > +	pmic_arb->cnfg = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> >  	if (IS_ERR(pmic_arb->cnfg)) {
> >  		err = PTR_ERR(pmic_arb->cnfg);
> >  		goto err_put_ctrl;
> > @@ -1281,7 +1431,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >  	/* 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;
> > +	pmic_arb->min_apid = pmic_arb->max_periphs - 1;
> >  
> >  	platform_set_drvdata(pdev, ctrl);
> >  	raw_spin_lock_init(&pmic_arb->lock);

Vinod Koul Dec. 7, 2021, 11:49 a.m. UTC | #7

On 02-12-21, 15:51, David Collins wrote:
> On 12/2/21 3:06 PM, Stephen Boyd wrote:
> > Quoting Vinod Koul (2021-11-30 23:27:18)
> >> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> >>         pmic_arb = spmi_controller_get_drvdata(ctrl);
> >>         pmic_arb->spmic = ctrl;
> >>  
> >> +       /*
> >> +        * Don't use devm_ioremap_resource() as the resources are shared in
> >> +        * PMIC v7 onwards, so causing failure when mapping
> >> +        */
> >>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> >> -       core = devm_ioremap_resource(&ctrl->dev, res);
> >> +       core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> > 
> > What does this mean? We have two nodes in DT that have the same reg
> > properties? How does that work?
> 
> PMIC Arbiter v7 has two SPMI bus master interfaces.  These are used to
> communicate with two sets of PMICs.  The SPMI interfaces operate
> independently; however, they share some register address ranges (e.g.
> one common one is used for APID->PPID mapping).  The most
> straightforward way to handle this is to treat them as two independent
> top-level DT devices.
> 
> In this case the "cnfg" address is used in the DT node name as that is
> unique between the two instances.
> 
> Here are the DT nodes used downstream on a target with PMIC Arbiter v7:
> 
> spmi0_bus: qcom,spmi@c42d000 {
> 	compatible = "qcom,spmi-pmic-arb";
> 	reg = <0xc42d000 0x4000>,
> 	      <0xc400000 0x3000>,
> 	      <0xc500000 0x400000>,
> 	      <0xc440000 0x80000>,
> 	      <0xc4c0000 0x10000>;
> 	reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
> 	interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
> 	interrupt-names = "periph_irq";
> 	interrupt-controller;
> 	#interrupt-cells = <4>;
> 	#address-cells = <2>;
> 	#size-cells = <0>;
> 	cell-index = <0>;
> 	qcom,channel = <0>;
> 	qcom,ee = <0>;
> 	qcom,bus-id = <0>;
> };
> 
> spmi1_bus: qcom,spmi@c432000 {
> 	compatible = "qcom,spmi-pmic-arb";
> 	reg = <0xc432000 0x4000>,
> 	      <0xc400000 0x3000>,
> 	      <0xc500000 0x400000>,
> 	      <0xc440000 0x80000>,
> 	      <0xc4d0000 0x10000>;
> 	reg-names = "cnfg", "core", "chnls", "obsrvr", "intr";
> 	interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>;
> 	interrupt-names = "periph_irq";
> 	interrupt-controller;
> 	#interrupt-cells = <4>;
> 	#address-cells = <2>;
> 	#size-cells = <0>;
> 	cell-index = <0>;
> 	qcom,channel = <0>;
> 	qcom,ee = <0>;
> 	qcom,bus-id = <1>;
> };
> 
> Note the inclusion of a new DT property: "qcom,bus-id".  This was
> defined in a DT binding patch that isn't present in Vinod's submission.
> Here is its definition:
> 
> - qcom,bus-id : Specifies which SPMI bus instance to use.  This property
> 		is only applicable for PMIC arbiter version 7 and
> 		beyond.
> 		Support values: 0 = primary bus, 1 = secondary bus
> 		Assumed to be 0 if unspecified.

I havent picked that part yet. This was not needed for base stuff to
work yet. Feel free to send that as additional change

Vinod Koul Dec. 7, 2021, 11:52 a.m. UTC | #8

On 03-12-21, 16:45, David Collins wrote:
> On 12/2/21 7:13 PM, Stephen Boyd wrote:
> > Quoting David Collins (2021-12-02 15:51:18)
> >> On 12/2/21 3:06 PM, Stephen Boyd wrote:
> >>> Quoting Vinod Koul (2021-11-30 23:27:18)

> > It feels like we should make a parent node that holds the core, chnls,
> > obsrvr reg properties with a new compatible string and then have two
> > child nodes for each bus. Then the various PMICs can hang off those two
> > different bus nodes. Of course, it needs DT review to make sure nothing
> > else goes wrong.
> 
> We considered this alternative DT layout when implementing PMIC arbiter
> v7 support downstream.  The benefit is allowing common register ranges
> to be specified once instead of in both SPMI bus nodes.  However, this
> approach has several downsides.
> 
> It results in substantially more complex device tree binding
> documentation that requires a different layout for SPMI buses for PMIC
> arbiter v7 (and above) vs early versions even though support can be
> provided with only a minimal modification (i.e. "qcom,bus-id").
> Complexity is also increased inside of the spmi-pmic-arb driver.  There,
> special data structures and logic would be needed to handle the shared
> vs independent register addresses and data.
> 
> The SPMI framework currently uses a one-to-one mapping between SPMI
> buses and struct device pointers.  This would not work if the new
> top-level node represents the true struct device and the per-bus
> subnodes are not true devices.  Also, platform_get_resource_byname()
> (used in the spmi-pmic-arb probe function) needs a struct
> platform_device pointer to read address ranges using "reg" +
> "reg-names".  That wouldn't work for the subnodes.
> 
> I suppose that "compatible" could be specified for the top-level node
> and the per bus subnodes so that all three get probed as struct devices.
>  However, that makes things even more complicated and convoluted in the
> driver (and DT).
> 
> I would prefer to stay with the approach of the two bus instances being
> specified independently in DT.
> 
> Note that the clk-imx8qxp-lpcg driver has a similar situation where
> multiple drivers need to map addresses in the same region.  Commit [1]
> documents the requirement.  The details of the problem where
> devm_platform_ioremap_resource() cannot be used in place of
> devm_ioremap() were discussed in this thread [2].

Steve, David,

Is this the way we are recommending for this to be move forward with?

Stephen Boyd Dec. 10, 2021, 2:01 a.m. UTC | #9

Quoting Vinod Koul (2021-12-07 03:52:30)
> On 03-12-21, 16:45, David Collins wrote:
> > On 12/2/21 7:13 PM, Stephen Boyd wrote:
> > > Quoting David Collins (2021-12-02 15:51:18)
> > >> On 12/2/21 3:06 PM, Stephen Boyd wrote:
> > >>> Quoting Vinod Koul (2021-11-30 23:27:18)
> 
> > > It feels like we should make a parent node that holds the core, chnls,
> > > obsrvr reg properties with a new compatible string and then have two
> > > child nodes for each bus. Then the various PMICs can hang off those two
> > > different bus nodes. Of course, it needs DT review to make sure nothing
> > > else goes wrong.
> > 
> > We considered this alternative DT layout when implementing PMIC arbiter
> > v7 support downstream.  The benefit is allowing common register ranges
> > to be specified once instead of in both SPMI bus nodes.  However, this
> > approach has several downsides.
> > 
> > It results in substantially more complex device tree binding
> > documentation that requires a different layout for SPMI buses for PMIC
> > arbiter v7 (and above) vs early versions even though support can be
> > provided with only a minimal modification (i.e. "qcom,bus-id").
> > Complexity is also increased inside of the spmi-pmic-arb driver.  There,
> > special data structures and logic would be needed to handle the shared
> > vs independent register addresses and data.
> > 
> > The SPMI framework currently uses a one-to-one mapping between SPMI
> > buses and struct device pointers.  This would not work if the new
> > top-level node represents the true struct device and the per-bus
> > subnodes are not true devices.  Also, platform_get_resource_byname()
> > (used in the spmi-pmic-arb probe function) needs a struct
> > platform_device pointer to read address ranges using "reg" +
> > "reg-names".  That wouldn't work for the subnodes.
> > 
> > I suppose that "compatible" could be specified for the top-level node
> > and the per bus subnodes so that all three get probed as struct devices.
> >  However, that makes things even more complicated and convoluted in the
> > driver (and DT).
> > 
> > I would prefer to stay with the approach of the two bus instances being
> > specified independently in DT.

The driver is already pretty hard to read because it combines so many
generations of spmi arbiter hardware from qcom into one file. It would
probably be better to start over and simplify the new version of the
driver, possibly sharing code between the two files if possible, but
otherwise dropping lots of cruft along the way and simplifying review
burden.

> 
> Steve, David,
> 
> Is this the way we are recommending for this to be move forward with?
> 

Please send the yamlification or update to the yamlification of the
binding.

spmi: pmic-arb: Add support for PMIC v7

Commit Message

Comments

Patch