From: Stephen Boyd <swboyd@chromium.org> To: andy.gross@linaro.org, david.brown@linaro.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org Cc: rnayak@codeaurora.org, bjorn.andersson@linaro.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, evgreen@chromium.org, dianders@chromium.org, mka@chromium.org, ilina@codeaurora.org, "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>, devicetree@vger.kernel.org Subject: Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs Date: Fri, 21 Dec 2018 23:39:49 -0800 [thread overview] Message-ID: <154546438942.179992.14851496143150245966@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <20181221115946.10095-4-rplsssn@codeaurora.org> Quoting Raju P.L.S.S.S.N (2018-12-21 03:59:44) > diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > index 9b86d1eff219..f24afb45d0d9 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > +++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > @@ -30,6 +30,12 @@ will be an aggregate of the sleep votes from each of those subsystems. Clients > may request a sleep value for their shared resources in addition to the active > mode requests. > > +When the processor enters deepest low power mode, the next wake-up timer value > +needs to be programmed to PDC (Power Domain Controller) through RSC registers > +dedicated for this purpose. PDC timer is specified as child node of RSC with > +register offets to program timer match value. That's great info, but I have no idea why it's in the DT binding document. > + > + > Properties: > > - compatible: > @@ -86,6 +92,20 @@ Properties: > Drivers that want to use the RSC to communicate with RPMH must specify their > bindings as child nodes of the RSC controllers they wish to communicate with. > > +If an RSC needs to program next wake-up in the PDC timer, it must specify the > +binding as child node with the following properties: > + > +Properties: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be "qcom,pdc-timer". > + > +- reg: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: Specifies the offset of the control register. > + > Example 1: > > For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the > @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00 > <0x179d0000 0x10000>, > <0x179e0000 0x10000>; > reg-names = "drv-0", "drv-1", "drv-2"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>; > @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00 > <SLEEP_TCS 3>, > <WAKE_TCS 3>, > <CONTROL_TCS 1>; > + > + pdc_timer@38 { > + compatible = "qcom,pdc-timer"; > + reg = <0x38 0x1>, > + <0x40 0x1>; I don't understand this whole binding. Why can't the pdc timer be programmed within the rpmh driver? This looks like a node is being added as a child just to make a platform driver and device match up in the linux kernel. And that in turn causes a regmap to need to be created? Sorry, it just looks really bad. At least for the regulators we have a semi-good reason to have a subnode because of the pmic-id property that we need to match up to the right pmic. The argument for the rpmh clock node is unclear to me so we should probably get rid of that node entirely. I can't figure out why that wasn't just a #clock-cells at the toplevel rsc node. The same goes for the interconnect and rpmhpd bindings. It's all sub-nodes to placate linux device driver model. And now that I've had to look at the rpmh binding again I'm disappointed that we have SoC data stored in there with the qcom,tcs-offset and qcom,tcs-config properties. Just make an SoC compatible like qcom,rpmh-rsc-sdm845" and figure these things out that way. And please tell hardware folks to stop moving the register regions around in the future so that there doesn't have to be so many variants in the driver and compatible string space.
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org> To: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>, andy.gross@linaro.org, david.brown@linaro.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org Cc: rnayak@codeaurora.org, bjorn.andersson@linaro.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, evgreen@chromium.org, dianders@chromium.org, mka@chromium.org, ilina@codeaurora.org, "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>, devicetree@vger.kernel.org Subject: Re: [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs Date: Fri, 21 Dec 2018 23:39:49 -0800 [thread overview] Message-ID: <154546438942.179992.14851496143150245966@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <20181221115946.10095-4-rplsssn@codeaurora.org> Quoting Raju P.L.S.S.S.N (2018-12-21 03:59:44) > diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > index 9b86d1eff219..f24afb45d0d9 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > +++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt > @@ -30,6 +30,12 @@ will be an aggregate of the sleep votes from each of those subsystems. Clients > may request a sleep value for their shared resources in addition to the active > mode requests. > > +When the processor enters deepest low power mode, the next wake-up timer value > +needs to be programmed to PDC (Power Domain Controller) through RSC registers > +dedicated for this purpose. PDC timer is specified as child node of RSC with > +register offets to program timer match value. That's great info, but I have no idea why it's in the DT binding document. > + > + > Properties: > > - compatible: > @@ -86,6 +92,20 @@ Properties: > Drivers that want to use the RSC to communicate with RPMH must specify their > bindings as child nodes of the RSC controllers they wish to communicate with. > > +If an RSC needs to program next wake-up in the PDC timer, it must specify the > +binding as child node with the following properties: > + > +Properties: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be "qcom,pdc-timer". > + > +- reg: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: Specifies the offset of the control register. > + > Example 1: > > For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the > @@ -103,6 +123,9 @@ TCS-OFFSET: 0xD00 > <0x179d0000 0x10000>, > <0x179e0000 0x10000>; > reg-names = "drv-0", "drv-1", "drv-2"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>; > @@ -112,6 +135,12 @@ TCS-OFFSET: 0xD00 > <SLEEP_TCS 3>, > <WAKE_TCS 3>, > <CONTROL_TCS 1>; > + > + pdc_timer@38 { > + compatible = "qcom,pdc-timer"; > + reg = <0x38 0x1>, > + <0x40 0x1>; I don't understand this whole binding. Why can't the pdc timer be programmed within the rpmh driver? This looks like a node is being added as a child just to make a platform driver and device match up in the linux kernel. And that in turn causes a regmap to need to be created? Sorry, it just looks really bad. At least for the regulators we have a semi-good reason to have a subnode because of the pmic-id property that we need to match up to the right pmic. The argument for the rpmh clock node is unclear to me so we should probably get rid of that node entirely. I can't figure out why that wasn't just a #clock-cells at the toplevel rsc node. The same goes for the interconnect and rpmhpd bindings. It's all sub-nodes to placate linux device driver model. And now that I've had to look at the rpmh binding again I'm disappointed that we have SoC data stored in there with the qcom,tcs-offset and qcom,tcs-config properties. Just make an SoC compatible like qcom,rpmh-rsc-sdm845" and figure these things out that way. And please tell hardware folks to stop moving the register regions around in the future so that there doesn't have to be so many variants in the driver and compatible string space.
next prev parent reply other threads:[~2018-12-22 7:39 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-21 11:59 [PATCH RFC 0/5] Add support for PDC timer for wake-ups Raju P.L.S.S.S.N 2018-12-21 11:59 ` [PATCH RFC 1/5] drivers: qcom: rpmh-rsc: Add regmap for RSC controller Raju P.L.S.S.S.N 2018-12-21 11:59 ` [PATCH RFC 2/5] drivers: qcom: rpmh-pdc-timer: add PDC timer support for RPMH based SoCs Raju P.L.S.S.S.N 2018-12-21 11:59 ` [PATCH RFC 3/5] dt-bindings: Add PDC timer bindings for Qualcomm SoCs Raju P.L.S.S.S.N 2018-12-22 7:39 ` Stephen Boyd [this message] 2018-12-22 7:39 ` Stephen Boyd 2018-12-26 9:44 ` Raju P L S S S N 2018-12-28 21:38 ` Stephen Boyd 2018-12-28 21:38 ` Stephen Boyd 2019-01-03 12:22 ` Raju P L S S S N 2019-01-03 12:22 ` Raju P L S S S N 2019-01-03 12:22 ` Raju P L S S S N 2019-01-03 21:19 ` Stephen Boyd 2019-01-03 21:19 ` Stephen Boyd 2019-01-03 21:19 ` Stephen Boyd 2019-01-07 16:17 ` Raju P L S S S N 2019-01-07 16:17 ` Raju P L S S S N 2019-01-09 5:34 ` Raju P L S S S N 2019-01-09 5:34 ` Raju P L S S S N 2019-01-09 17:46 ` Stephen Boyd 2019-01-09 17:46 ` Stephen Boyd 2019-01-10 16:58 ` Raju P L S S S N 2019-01-10 16:58 ` Raju P L S S S N 2019-01-10 21:27 ` Stephen Boyd 2019-01-10 21:27 ` Stephen Boyd 2018-12-21 11:59 ` [PATCH RFC 4/5] drivers: qcom: rpmh-pdc-timer: Add power management ops Raju P.L.S.S.S.N 2018-12-21 11:59 ` [PATCH RFC 5/5] arm64: dts: msm: add PDC timer for apps_rsc for SDM845 Raju P.L.S.S.S.N
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=154546438942.179992.14851496143150245966@swboyd.mtv.corp.google.com \ --to=swboyd@chromium.org \ --cc=andy.gross@linaro.org \ --cc=bjorn.andersson@linaro.org \ --cc=david.brown@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=evgreen@chromium.org \ --cc=ilina@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-soc@vger.kernel.org \ --cc=mka@chromium.org \ --cc=rnayak@codeaurora.org \ --cc=rplsssn@codeaurora.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.