linux-kernel - Re: [PATCH] spmi: pmic-arb: Add support for PMIC v7

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20211203031323.211E5C00446@smtp.kernel.org>
Date:   Thu, 02 Dec 2021 19:13:21 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     David Collins <quic_collinsd@...cinc.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Cc:     linux-arm-msm@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        David Dai <daidavid1@...eaurora.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Collins <quic_collinsd@...cinc.com>
Subject: Re: [PATCH] spmi: pmic-arb: Add support for PMIC v7

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@...d000 {
>         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@...2000 {
>         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?