| Message ID | 20220705133917.8405-4-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ipq806x missing bindings | expand |
On 05/07/2022 15:39, Christian Marangi wrote: > Add cells definition for rpm node and add missing regulators for the 4 > regulator present on ipq8064. There regulators are controlled by rpm and > to correctly works gsbi4_i2c require to be NEVER disabled or rpm will > reject any regulator change request. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Tested-by: Jonathan McDowell <noodles@earth.li> > --- > arch/arm/boot/dts/qcom-ipq8064.dtsi | 36 +++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi > index 1b4b72723ead..c0b05d2a2d6d 100644 > --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi > +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi > @@ -844,10 +844,46 @@ rpm: rpm@108000 { > clocks = <&gcc RPM_MSG_RAM_H_CLK>; > clock-names = "ram"; > > + #address-cells = <1>; > + #size-cells = <0>; Why adding these? > + > rpmcc: clock-controller { > compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; > #clock-cells = <1>; > }; > + > + smb208_regulators: regulators { > + compatible = "qcom,rpm-smb208-regulators"; > + status = "okay"; Was the node disabled? > + > + smb208_s1a: s1a { > + regulator-min-microvolt = <1050000>; > + regulator-max-microvolt = <1150000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s1b: s1b { > + regulator-min-microvolt = <1050000>; > + regulator-max-microvolt = <1150000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s2a: s2a { > + regulator-min-microvolt = < 800000>; > + regulator-max-microvolt = <1250000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s2b: s2b { > + regulator-min-microvolt = < 800000>; > + regulator-max-microvolt = <1250000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + }; > }; > > tcsr: syscon@1a400000 { Best regards, Krzysztof
On Wed, Jul 06, 2022 at 10:34:16AM +0200, Krzysztof Kozlowski wrote: > On 05/07/2022 15:39, Christian Marangi wrote: > > Add cells definition for rpm node and add missing regulators for the 4 > > regulator present on ipq8064. There regulators are controlled by rpm and > > to correctly works gsbi4_i2c require to be NEVER disabled or rpm will > > reject any regulator change request. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Tested-by: Jonathan McDowell <noodles@earth.li> > > --- > > arch/arm/boot/dts/qcom-ipq8064.dtsi | 36 +++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi > > index 1b4b72723ead..c0b05d2a2d6d 100644 > > --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi > > +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi > > @@ -844,10 +844,46 @@ rpm: rpm@108000 { > > clocks = <&gcc RPM_MSG_RAM_H_CLK>; > > clock-names = "ram"; > > > > + #address-cells = <1>; > > + #size-cells = <0>; > > Why adding these? > Fix dt warning, will split and put it in a separate commit. > > + > > rpmcc: clock-controller { > > compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; > > #clock-cells = <1>; > > }; > > + > > + smb208_regulators: regulators { > > + compatible = "qcom,rpm-smb208-regulators"; > > + status = "okay"; > > Was the node disabled? > smb208 is the normal and advised way to handle regulators on this platform. Some device may want to not follow that and implement their own regulator bypassing rpm so we add a status and on the current device present upstream we set it disabled as it does use different regulators implementation. > > + > > + smb208_s1a: s1a { > > + regulator-min-microvolt = <1050000>; > > + regulator-max-microvolt = <1150000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s1b: s1b { > > + regulator-min-microvolt = <1050000>; > > + regulator-max-microvolt = <1150000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s2a: s2a { > > + regulator-min-microvolt = < 800000>; > > + regulator-max-microvolt = <1250000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s2b: s2b { > > + regulator-min-microvolt = < 800000>; > > + regulator-max-microvolt = <1250000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + }; > > }; > > > > tcsr: syscon@1a400000 { > > > Best regards, > Krzysztof
On Wed, 6 Jul 2022 at 13:26, Christian Marangi <ansuelsmth@gmail.com> wrote: > On Wed, Jul 06, 2022 at 10:34:16AM +0200, Krzysztof Kozlowski wrote: > > On 05/07/2022 15:39, Christian Marangi wrote: > > > Add cells definition for rpm node and add missing regulators for the 4 > > > regulator present on ipq8064. There regulators are controlled by rpm and > > > to correctly works gsbi4_i2c require to be NEVER disabled or rpm will > > > reject any regulator change request. > > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > Tested-by: Jonathan McDowell <noodles@earth.li> [...] > > > > + > > > rpmcc: clock-controller { > > > compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; > > > #clock-cells = <1>; > > > }; > > > + > > > + smb208_regulators: regulators { > > > + compatible = "qcom,rpm-smb208-regulators"; > > > + status = "okay"; > > > > Was the node disabled? > > > > smb208 is the normal and advised way to handle regulators on this > platform. Some device may want to not follow that and implement their > own regulator bypassing rpm so we add a status and on the current device > present upstream we set it disabled as it does use different regulators > implementation. Yep, this is correct. But you don't have to define status = 'okay'. It is the default. There are two typical patterns: 1) Disable by default foo.dtsi: abc: def { status = "disabled"; }; foo-bar.dtsi: &abc { status = "okay"; } 2) Enable by default foo.dtsi: abc: def { /* usual properties */ }; foo-bar.dtsi: &abc { status = "disabled"; }
On Wed, Jul 06, 2022 at 03:02:44PM +0200, Konrad Dybcio wrote: > > > On 5.07.2022 15:39, Christian Marangi wrote: > > Add cells definition for rpm node and add missing regulators for the 4 > > regulator present on ipq8064. There regulators are controlled by rpm and > > to correctly works gsbi4_i2c require to be NEVER disabled or rpm will > > reject any regulator change request. > That sounds.. very weird for a RPM regulator.. > > I know but it's like that. In theory the smb208 can be controlled via the gsbi4_i2c and bypass rpm completely but we have no ducmentation of smb208 so rpm is the only way to scale voltage on this SoC. If gsbi4_i2c is disabled any RPM voltage request fails as RPM in it's firmware use the same line and assume it's always enabled. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Tested-by: Jonathan McDowell <noodles@earth.li> > > --- > > arch/arm/boot/dts/qcom-ipq8064.dtsi | 36 +++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi > > index 1b4b72723ead..c0b05d2a2d6d 100644 > > --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi > > +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi > > @@ -844,10 +844,46 @@ rpm: rpm@108000 { > > clocks = <&gcc RPM_MSG_RAM_H_CLK>; > > clock-names = "ram"; > > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > rpmcc: clock-controller { > > compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; > > #clock-cells = <1>; > > }; > > + > > + smb208_regulators: regulators { > Are you sure it is used on all ipq8064 boards? And with the same > voltage settings? > Out of 28 device we have on openwrt only the one present upstream doesn't use the smb208 regulator and use it's own way. The voltage are all the same, it does change with other revision of the SoC (ipq8065) but they will pushed in another series later. > > + compatible = "qcom,rpm-smb208-regulators"; > > + status = "okay"; > They are enabled by default, as you are defining them here and > the status property is not overwritten anywhere else. > > Konrad > > + > > + smb208_s1a: s1a { > > + regulator-min-microvolt = <1050000>; > > + regulator-max-microvolt = <1150000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s1b: s1b { > > + regulator-min-microvolt = <1050000>; > > + regulator-max-microvolt = <1150000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s2a: s2a { > > + regulator-min-microvolt = < 800000>; > > + regulator-max-microvolt = <1250000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + > > + smb208_s2b: s2b { > > + regulator-min-microvolt = < 800000>; > > + regulator-max-microvolt = <1250000>; > > + > > + qcom,switch-mode-frequency = <1200000>; > > + }; > > + }; > > }; > > > > tcsr: syscon@1a400000 {
On 5.07.2022 15:39, Christian Marangi wrote: > Add cells definition for rpm node and add missing regulators for the 4 > regulator present on ipq8064. There regulators are controlled by rpm and > to correctly works gsbi4_i2c require to be NEVER disabled or rpm will > reject any regulator change request. That sounds.. very weird for a RPM regulator.. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Tested-by: Jonathan McDowell <noodles@earth.li> > --- > arch/arm/boot/dts/qcom-ipq8064.dtsi | 36 +++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi > index 1b4b72723ead..c0b05d2a2d6d 100644 > --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi > +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi > @@ -844,10 +844,46 @@ rpm: rpm@108000 { > clocks = <&gcc RPM_MSG_RAM_H_CLK>; > clock-names = "ram"; > > + #address-cells = <1>; > + #size-cells = <0>; > + > rpmcc: clock-controller { > compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; > #clock-cells = <1>; > }; > + > + smb208_regulators: regulators { Are you sure it is used on all ipq8064 boards? And with the same voltage settings? > + compatible = "qcom,rpm-smb208-regulators"; > + status = "okay"; They are enabled by default, as you are defining them here and the status property is not overwritten anywhere else. Konrad > + > + smb208_s1a: s1a { > + regulator-min-microvolt = <1050000>; > + regulator-max-microvolt = <1150000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s1b: s1b { > + regulator-min-microvolt = <1050000>; > + regulator-max-microvolt = <1150000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s2a: s2a { > + regulator-min-microvolt = < 800000>; > + regulator-max-microvolt = <1250000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + > + smb208_s2b: s2b { > + regulator-min-microvolt = < 800000>; > + regulator-max-microvolt = <1250000>; > + > + qcom,switch-mode-frequency = <1200000>; > + }; > + }; > }; > > tcsr: syscon@1a400000 {
On 06/07/2022 12:09, Christian Marangi wrote: > On Wed, Jul 06, 2022 at 10:34:16AM +0200, Krzysztof Kozlowski wrote: >> On 05/07/2022 15:39, Christian Marangi wrote: >>> Add cells definition for rpm node and add missing regulators for the 4 >>> regulator present on ipq8064. There regulators are controlled by rpm and >>> to correctly works gsbi4_i2c require to be NEVER disabled or rpm will >>> reject any regulator change request. >>> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >>> Tested-by: Jonathan McDowell <noodles@earth.li> >>> --- >>> arch/arm/boot/dts/qcom-ipq8064.dtsi | 36 +++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi >>> index 1b4b72723ead..c0b05d2a2d6d 100644 >>> --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi >>> +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi >>> @@ -844,10 +844,46 @@ rpm: rpm@108000 { >>> clocks = <&gcc RPM_MSG_RAM_H_CLK>; >>> clock-names = "ram"; >>> >>> + #address-cells = <1>; >>> + #size-cells = <0>; >> >> Why adding these? >> > > Fix dt warning, will split and put it in a separate commit. > >>> + >>> rpmcc: clock-controller { >>> compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; >>> #clock-cells = <1>; >>> }; >>> + >>> + smb208_regulators: regulators { >>> + compatible = "qcom,rpm-smb208-regulators"; >>> + status = "okay"; >> >> Was the node disabled? >> > > smb208 is the normal and advised way to handle regulators on this > platform. Some device may want to not follow that and implement their > own regulator bypassing rpm so we add a status and on the current device > present upstream we set it disabled as it does use different regulators > implementation. You just added a new node and say we set it as disabled... so the code is not correct, because you enabled it. So again my question is valid - was the node already existing and was it disabled? > Best regards, Krzysztof
diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi index 1b4b72723ead..c0b05d2a2d6d 100644 --- a/arch/arm/boot/dts/qcom-ipq8064.dtsi +++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi @@ -844,10 +844,46 @@ rpm: rpm@108000 { clocks = <&gcc RPM_MSG_RAM_H_CLK>; clock-names = "ram"; + #address-cells = <1>; + #size-cells = <0>; + rpmcc: clock-controller { compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc"; #clock-cells = <1>; }; + + smb208_regulators: regulators { + compatible = "qcom,rpm-smb208-regulators"; + status = "okay"; + + smb208_s1a: s1a { + regulator-min-microvolt = <1050000>; + regulator-max-microvolt = <1150000>; + + qcom,switch-mode-frequency = <1200000>; + }; + + smb208_s1b: s1b { + regulator-min-microvolt = <1050000>; + regulator-max-microvolt = <1150000>; + + qcom,switch-mode-frequency = <1200000>; + }; + + smb208_s2a: s2a { + regulator-min-microvolt = < 800000>; + regulator-max-microvolt = <1250000>; + + qcom,switch-mode-frequency = <1200000>; + }; + + smb208_s2b: s2b { + regulator-min-microvolt = < 800000>; + regulator-max-microvolt = <1250000>; + + qcom,switch-mode-frequency = <1200000>; + }; + }; }; tcsr: syscon@1a400000 {