| Message ID | 1411982092-7922-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/29/14 02:14, Srinivas Kandagatla wrote: > @@ -246,6 +247,24 @@ > #reset-cells = <1>; > }; > > + apcs: syscon@2011000 { > + compatible = "syscon"; > + reg = <0x2011000 0x1000>; > + }; This is actually a clock controller block that hw designers decided was good place to shove the ipc bits (because there's room!). Can we call it l2cc: clock-controller@2011000 { compatible = "syscon"; reg = <0x2011000 0x1000>; }; Eventually I'll add the specific krait compatible when we merge krait clock support: l2cc: clock-controller@2011000 { compatible = "qcom,kpss-gcc", "syscon"; reg = <0x2011000 0x1000>; clock-output-names = "acpu_l2_aux"; }; > + > + rpm@108000 { > + compatible = "qcom,rpm-apq8064"; > + reg = <0x108000 0x1000>; > + qcom,ipc = <&apcs 0x8 2>; There are actually 3 ipc bits. I guess if we ever have to use the other two we'll extend this binding to have the other bits specified some other way?
On Mon 29 Sep 15:17 PDT 2014, Stephen Boyd wrote: > On 09/29/14 02:14, Srinivas Kandagatla wrote: > > @@ -246,6 +247,24 @@ > > #reset-cells = <1>; > > }; > > > > + apcs: syscon@2011000 { > > + compatible = "syscon"; > > + reg = <0x2011000 0x1000>; > > + }; > > This is actually a clock controller block that hw designers decided was > good place to shove the ipc bits (because there's room!). Can we call it > > l2cc: clock-controller@2011000 { > compatible = "syscon"; > reg = <0x2011000 0x1000>; > }; > > Eventually I'll add the specific krait compatible when we merge krait > clock support: > > l2cc: clock-controller@2011000 { > compatible = "qcom,kpss-gcc", "syscon"; > reg = <0x2011000 0x1000>; > clock-output-names = "acpu_l2_aux"; > }; > As long as we can get hold of the regmap that would be fine. I pressume the idea is to have the kpss-gcc using syscon, just like the rpm. But hopefully the syscon patches that are floating around (merged?) will allow any driver to expose a "syscon regmap". > > + > > + rpm@108000 { > > + compatible = "qcom,rpm-apq8064"; > > + reg = <0x108000 0x1000>; > > + qcom,ipc = <&apcs 0x8 2>; > > There are actually 3 ipc bits. I guess if we ever have to use the other > two we'll extend this binding to have the other bits specified some > other way? I haven't seen any indications of us using more than this bit. If we want to do that, we could simply make it <&apcs 8 2 &apcs 8 3 &apcs 8 4> (or whatever those indices are). That way this should be easy it keep compatible. Regards, Bjorn
On Mon 29 Sep 02:14 PDT 2014, Srinivas Kandagatla wrote: > This patch adds rpm node to apq8064 dt as rpm would be used by other > devices for regulator support. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > arch/arm/boot/dts/qcom-apq8064.dtsi | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi > index b3154c0..9c2c8e6 100644 > --- a/arch/arm/boot/dts/qcom-apq8064.dtsi > +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi > @@ -3,6 +3,7 @@ > #include "skeleton.dtsi" > #include <dt-bindings/clock/qcom,gcc-msm8960.h> > #include <dt-bindings/clock/qcom,mmcc-msm8960.h> > +#include <dt-bindings/mfd/qcom-rpm.h> > #include <dt-bindings/soc/qcom,gsbi.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > > @@ -246,6 +247,24 @@ > #reset-cells = <1>; > }; > > + apcs: syscon@2011000 { > + compatible = "syscon"; > + reg = <0x2011000 0x1000>; > + }; > + > + rpm@108000 { > + compatible = "qcom,rpm-apq8064"; > + reg = <0x108000 0x1000>; > + qcom,ipc = <&apcs 0x8 2>; I don't like your indentation, but please stick with one way of doing it :) > + > + interrupts = <0 19 0>, <0 21 0>, <0 22 0>; > + interrupt-names = "ack", "err", "wakeup"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + This part looks good. But how about adding all the regulators here as well? Like: pm8921_l5: pm8921-l5 { compatible = "qcom,rpm-pm8921-pldo"; reg = <QCOM_RPM_PM8921_LDO5>; }; ... That way we can update the references from this file (while still allowing the dts to override it if needed). I'm not sure if we should add some sane defaults or just completely deferr specifying the voltage ranges to the dts. The benefit of the latter is that the regulators not configured by the dts author will not be accessible. But simply listing all the nodes here would be nice and I dont see much reason to postpone this work. > + }; > + > /* Temporary fixed regulator */ > vsdcc_fixed: vsdcc-regulator { > compatible = "regulator-fixed"; Regards, Bjorn
On 29/09/14 23:17, Stephen Boyd wrote: > On 09/29/14 02:14, Srinivas Kandagatla wrote: >> @@ -246,6 +247,24 @@ >> #reset-cells = <1>; >> }; >> >> + apcs: syscon@2011000 { >> + compatible = "syscon"; >> + reg = <0x2011000 0x1000>; >> + }; > > This is actually a clock controller block that hw designers decided was > good place to shove the ipc bits (because there's room!). Can we call it > > l2cc: clock-controller@2011000 { > compatible = "syscon"; > reg = <0x2011000 0x1000>; > }; > yep, I will rename this in next version. > Eventually I'll add the specific krait compatible when we merge krait > clock support: sounds good. --srini > > l2cc: clock-controller@2011000 { > compatible = "qcom,kpss-gcc", "syscon"; > reg = <0x2011000 0x1000>; > clock-output-names = "acpu_l2_aux"; > }; > >> + >> + rpm@108000 { >> + compatible = "qcom,rpm-apq8064"; >> + reg = <0x108000 0x1000>; >> + qcom,ipc = <&apcs 0x8 2>; > > There are actually 3 ipc bits. I guess if we ever have to use the other > two we'll extend this binding to have the other bits specified some > other way? >
On 30/09/14 06:11, Bjorn Andersson wrote: >> >> + apcs: syscon@2011000 { >> + compatible = "syscon"; >> + reg = <0x2011000 0x1000>; >> + }; >> + >> + rpm@108000 { >> + compatible = "qcom,rpm-apq8064"; >> + reg = <0x108000 0x1000>; >> + qcom,ipc = <&apcs 0x8 2>; > > I don't like your indentation, but please stick with one way of doing it :) > I agree, Will fix this in next version. >> + >> + interrupts = <0 19 0>, <0 21 0>, <0 22 0>; >> + interrupt-names = "ack", "err", "wakeup"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + > > This part looks good. > > But how about adding all the regulators here as well? Yes we can add for the sake of completeness but w.r.t testing only some of them will be tested > Like: > > pm8921_l5: pm8921-l5 { > compatible = "qcom,rpm-pm8921-pldo"; > reg = <QCOM_RPM_PM8921_LDO5>; > }; > Adding nodes like this should be trival. > ... > > That way we can update the references from this file (while still allowing the > dts to override it if needed). I'm not sure if we should add some sane defaults > or just completely deferr specifying the voltage ranges to the dts. The benefit > of the latter is that the regulators not configured by the dts author will not > be accessible. > > But simply listing all the nodes here would be nice and I dont see much reason > to postpone this work. Ok.. will be done in next version. --srini > >> + }; >> + >> /* Temporary fixed regulator */ >> vsdcc_fixed: vsdcc-regulator { >> compatible = "regulator-fixed"; > > Regards, > Bjorn >
On 09/29/14 22:02, Bjorn Andersson wrote: > On Mon 29 Sep 15:17 PDT 2014, Stephen Boyd wrote: > >> On 09/29/14 02:14, Srinivas Kandagatla wrote: >>> @@ -246,6 +247,24 @@ >>> #reset-cells = <1>; >>> }; >>> >>> + apcs: syscon@2011000 { >>> + compatible = "syscon"; >>> + reg = <0x2011000 0x1000>; >>> + }; >> This is actually a clock controller block that hw designers decided was >> good place to shove the ipc bits (because there's room!). Can we call it >> >> l2cc: clock-controller@2011000 { >> compatible = "syscon"; >> reg = <0x2011000 0x1000>; >> }; >> >> Eventually I'll add the specific krait compatible when we merge krait >> clock support: >> >> l2cc: clock-controller@2011000 { >> compatible = "qcom,kpss-gcc", "syscon"; >> reg = <0x2011000 0x1000>; >> clock-output-names = "acpu_l2_aux"; >> }; >> > As long as we can get hold of the regmap that would be fine. I pressume the > idea is to have the kpss-gcc using syscon, just like the rpm. But hopefully the > syscon patches that are floating around (merged?) will allow any driver to > expose a "syscon regmap". Agreed. I'm not sure if kpss-gcc will use syscon. With the floating patches I don't see a reason why we need to use it. > >>> + >>> + rpm@108000 { >>> + compatible = "qcom,rpm-apq8064"; >>> + reg = <0x108000 0x1000>; >>> + qcom,ipc = <&apcs 0x8 2>; >> There are actually 3 ipc bits. I guess if we ever have to use the other >> two we'll extend this binding to have the other bits specified some >> other way? > I haven't seen any indications of us using more than this bit. If we want to do > that, we could simply make it <&apcs 8 2 &apcs 8 3 &apcs 8 4> (or whatever > those indices are). That way this should be easy it keep compatible. > Good to hear we're not forced to use a new property. The indices have always been 2, 1, and 0 as far as I know. We use bit 1 to ack the RPM if it ever crashes. We know that it crashes because we receive the error interrupt. Instead of calling panic like we do downstream we can gracefully fail any new rpm requests. There really isn't any way to recover from this scenario though besides rebooting. Looking at the code I see this is all wrong and we use bit 2 to ack the error.
On Tue 30 Sep 11:27 PDT 2014, Stephen Boyd wrote: > On 09/29/14 22:02, Bjorn Andersson wrote: > > On Mon 29 Sep 15:17 PDT 2014, Stephen Boyd wrote: > > > >> On 09/29/14 02:14, Srinivas Kandagatla wrote: > >>> @@ -246,6 +247,24 @@ > >>> #reset-cells = <1>; > >>> }; > >>> > >>> + apcs: syscon@2011000 { > >>> + compatible = "syscon"; > >>> + reg = <0x2011000 0x1000>; > >>> + }; > >> This is actually a clock controller block that hw designers decided was > >> good place to shove the ipc bits (because there's room!). Can we call it > >> > >> l2cc: clock-controller@2011000 { > >> compatible = "syscon"; > >> reg = <0x2011000 0x1000>; > >> }; > >> > >> Eventually I'll add the specific krait compatible when we merge krait > >> clock support: > >> > >> l2cc: clock-controller@2011000 { > >> compatible = "qcom,kpss-gcc", "syscon"; > >> reg = <0x2011000 0x1000>; > >> clock-output-names = "acpu_l2_aux"; > >> }; > >> > > As long as we can get hold of the regmap that would be fine. I pressume the > > idea is to have the kpss-gcc using syscon, just like the rpm. But hopefully the > > syscon patches that are floating around (merged?) will allow any driver to > > expose a "syscon regmap". > > Agreed. I'm not sure if kpss-gcc will use syscon. With the floating > patches I don't see a reason why we need to use it. > All we need is a regmap for the kpss-gcc, but the restriction that it has to be a syscon is more of a implementation detail. But you probably don't want to implement the phandle->regmap lookup code in kpss-gcc (and all the other places) and if I understand the syscon improvements being suggested they are supposed to help with exactly that (help any device expose regmaps). Need to look into that again though. > > > >>> + > >>> + rpm@108000 { > >>> + compatible = "qcom,rpm-apq8064"; > >>> + reg = <0x108000 0x1000>; > >>> + qcom,ipc = <&apcs 0x8 2>; > >> There are actually 3 ipc bits. I guess if we ever have to use the other > >> two we'll extend this binding to have the other bits specified some > >> other way? > > I haven't seen any indications of us using more than this bit. If we want to do > > that, we could simply make it <&apcs 8 2 &apcs 8 3 &apcs 8 4> (or whatever > > those indices are). That way this should be easy it keep compatible. > > > > Good to hear we're not forced to use a new property. The indices have > always been 2, 1, and 0 as far as I know. > > We use bit 1 to ack the RPM if it ever crashes. We know that it crashes > because we receive the error interrupt. Instead of calling panic like we > do downstream we can gracefully fail any new rpm requests. There really > isn't any way to recover from this scenario though besides rebooting. > Looking at the code I see this is all wrong and we use bit 2 to ack the > error. > Hmm, seems I got that wrong, sorry about that. But do you mean "all wrong" as in that I use the wrong bit or to some greater extent? Currently all following requests should timeout, but maybe we should have a faster fail-path when we've hit this point? From a practical pov I guess that once the rpm starts returning errors on updates to regulators, root clocks and bus scaling then most of the system is becoming useless. Regards, Bjorn
On 09/30/14 12:00, Bjorn Andersson wrote: > > Hmm, seems I got that wrong, sorry about that. > > But do you mean "all wrong" as in that I use the wrong bit or to some greater > extent? Currently all following requests should timeout, but maybe we should > have a faster fail-path when we've hit this point? Yes. We just ack the interrupt and go on happy to keep allowing clients to request things. I'd rather see us blow up or start failing requests, or maybe both. > > From a practical pov I guess that once the rpm starts returning errors on > updates to regulators, root clocks and bus scaling then most of the system is > becoming useless. Right. Rebooting will be required fairly soon.
On Tue 30 Sep 12:13 PDT 2014, Stephen Boyd wrote: > On 09/30/14 12:00, Bjorn Andersson wrote: > > > > Hmm, seems I got that wrong, sorry about that. > > > > But do you mean "all wrong" as in that I use the wrong bit or to some greater > > extent? Currently all following requests should timeout, but maybe we should > > have a faster fail-path when we've hit this point? > > Yes. We just ack the interrupt and go on happy to keep allowing clients > to request things. I'd rather see us blow up or start failing requests, > or maybe both. > Okay, I naively figured that the RPM would be dead after this happened and from this point we would start getting timeouts. Let's ack the right bit and fail early after the RPM have rejected things. > > > > From a practical pov I guess that once the rpm starts returning errors on > > updates to regulators, root clocks and bus scaling then most of the system is > > becoming useless. > > Right. Rebooting will be required fairly soon. > Yes, but we want something else to trigger that right? Regards, Bjorn
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi index b3154c0..9c2c8e6 100644 --- a/arch/arm/boot/dts/qcom-apq8064.dtsi +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi @@ -3,6 +3,7 @@ #include "skeleton.dtsi" #include <dt-bindings/clock/qcom,gcc-msm8960.h> #include <dt-bindings/clock/qcom,mmcc-msm8960.h> +#include <dt-bindings/mfd/qcom-rpm.h> #include <dt-bindings/soc/qcom,gsbi.h> #include <dt-bindings/interrupt-controller/arm-gic.h> @@ -246,6 +247,24 @@ #reset-cells = <1>; }; + apcs: syscon@2011000 { + compatible = "syscon"; + reg = <0x2011000 0x1000>; + }; + + rpm@108000 { + compatible = "qcom,rpm-apq8064"; + reg = <0x108000 0x1000>; + qcom,ipc = <&apcs 0x8 2>; + + interrupts = <0 19 0>, <0 21 0>, <0 22 0>; + interrupt-names = "ack", "err", "wakeup"; + + #address-cells = <1>; + #size-cells = <0>; + + }; + /* Temporary fixed regulator */ vsdcc_fixed: vsdcc-regulator { compatible = "regulator-fixed";
This patch adds rpm node to apq8064 dt as rpm would be used by other devices for regulator support. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- arch/arm/boot/dts/qcom-apq8064.dtsi | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)