Date:   Tue, 7 Dec 2021 17:19:10 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     David Collins <quic_collinsd@quicinc.com>
Cc:     Stephen Boyd <sboyd@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        David Dai <daidavid1@codeaurora.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] spmi: pmic-arb: Add support for PMIC v7
Message-ID: <Ya9KNkw1mPpZtwDK@matsya>
References: <20211201072718.3969011-1-vkoul@kernel.org>
 <20211202230624.C05F3C00446@smtp.kernel.org>
 <9161450a-40e0-c84f-f529-c903d6f1d722@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9161450a-40e0-c84f-f529-c903d6f1d722@quicinc.com>
Precedence: bulk

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