| Message ID | 1429180540-5692-4-git-send-email-keita.kobayashi.ym@renesas.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
Hi Kobayashi-san, On Thu, Apr 16, 2015 at 7:35 PM, Keita Kobayashi <keita.kobayashi.ym@renesas.com> wrote: > This patch add the ARM CPUs Device Tree binding to document a new > enable method of Renesas cpuidle. > > Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > --- > Documentation/devicetree/bindings/arm/cpus.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > index 8b9e0a9..663ee11 100644 > --- a/Documentation/devicetree/bindings/arm/cpus.txt > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. > "qcom,gcc-msm8660" > "qcom,kpss-acc-v1" > "qcom,kpss-acc-v2" > + "renesas,rcar-idle" > "rockchip,rk3066-smp" > > - cpu-release-addr I don't mind this portion so much and I can see other vendors are using this format. And with risk of upsetting the DT police I have to mention my doubts about why we need this particular piece of binding. The proposed string by this patch does not seem to describe any hardware but it is aligned to other existing stings referring to software such as "spin-table" and "psci". From a C code implementation point of view we would get enough information to make a good decision by just checking SoC compatible string. At the same time, this seems to be the standard way of doing things and standard is good. So if we should turn this into something that describes actual hardware, how about pointing out the APMU hardware that is actually managing the Core Standby mode that this CPUIdle implementation is invoking? May I suggest using "renesas,rcar-apmu"? Or perhaps to be safe we should do per-SoC string like in other cases like "renesas,r8a7791-apmu"? Or is there some way we can make this work without adding a new binding? Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote: > This patch add the ARM CPUs Device Tree binding to document a new > enable method of Renesas cpuidle. > > Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > --- > Documentation/devicetree/bindings/arm/cpus.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > index 8b9e0a9..663ee11 100644 > --- a/Documentation/devicetree/bindings/arm/cpus.txt > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. > "qcom,gcc-msm8660" > "qcom,kpss-acc-v1" > "qcom,kpss-acc-v2" > + "renesas,rcar-idle" The enable-method is about how to bring the CPU up (and potentially implies other things, like how to take it down again). These is no code in this series to that effect, no semantics are provided, and the name implies this is idle-specific. So this doesn't look right, and makes no sense to me. Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote: >> This patch add the ARM CPUs Device Tree binding to document a new >> enable method of Renesas cpuidle. >> >> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> >> --- >> Documentation/devicetree/bindings/arm/cpus.txt | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt >> index 8b9e0a9..663ee11 100644 >> --- a/Documentation/devicetree/bindings/arm/cpus.txt >> +++ b/Documentation/devicetree/bindings/arm/cpus.txt >> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. >> "qcom,gcc-msm8660" >> "qcom,kpss-acc-v1" >> "qcom,kpss-acc-v2" >> + "renesas,rcar-idle" > > The enable-method is about how to bring the CPU up (and potentially > implies other things, like how to take it down again). Thanks for your clarification. I now understand that this is related to SMP boot and CPU Hotplug. > These is no code in this series to that effect, no semantics are > provided, and the name implies this is idle-specific. > > So this doesn't look right, and makes no sense to me. Ok, I somehow (incorrectly?) assumed that the following line in patch 2/5* tied into this: +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops); [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle But if it is unrelated it should of course be dropped or reworked. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote: > Hi Mark, > > On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote: > >> This patch add the ARM CPUs Device Tree binding to document a new > >> enable method of Renesas cpuidle. > >> > >> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> > >> --- > >> Documentation/devicetree/bindings/arm/cpus.txt | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > >> index 8b9e0a9..663ee11 100644 > >> --- a/Documentation/devicetree/bindings/arm/cpus.txt > >> +++ b/Documentation/devicetree/bindings/arm/cpus.txt > >> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. > >> "qcom,gcc-msm8660" > >> "qcom,kpss-acc-v1" > >> "qcom,kpss-acc-v2" > >> + "renesas,rcar-idle" > > > > The enable-method is about how to bring the CPU up (and potentially > > implies other things, like how to take it down again). > > Thanks for your clarification. I now understand that this is related > to SMP boot and CPU Hotplug. > > > These is no code in this series to that effect, no semantics are > > provided, and the name implies this is idle-specific. > > > > So this doesn't look right, and makes no sense to me. > > Ok, I somehow (incorrectly?) assumed that the following line in patch > 2/5* tied into this: > +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops); > > [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle > > But if it is unrelated it should of course be dropped or reworked. It is related, but Mark's remark is correct. We have to keep in mind that an enable-method defines how a CPU is powered {up/down} and also suspended (quiesced through idle). It has to be defined through proper bindings and related code, adding a random compatible string for the sake of matching does not cut it, that is not acceptable and I stated it from the beginning. So, to make it clear, an enable-method defines CPU operations as a whole, cpu init, power{up/down} and suspend. It must be documented with proper bindings that defines compatible string and related properties. The sooner we incorporate the CPUidle ops into SMP ops the better to prevent this abuse from happening, an enable-method encompasses SMP ops and CPUidle ops, actually they must be merged because they represent the enable-method implementation as a whole. I hope this helps. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, Mark, Lorenzo Thank you for your comment. (2015/04/17 23:37), Lorenzo Pieralisi wrote: > On Fri, Apr 17, 2015 at 02:11:24PM +0100, Magnus Damm wrote: >> Hi Mark, >> >> On Fri, Apr 17, 2015 at 9:46 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Thu, Apr 16, 2015 at 11:35:38AM +0100, Keita Kobayashi wrote: >>>> This patch add the ARM CPUs Device Tree binding to document a new >>>> enable method of Renesas cpuidle. >>>> >>>> Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> >>>> --- >>>> Documentation/devicetree/bindings/arm/cpus.txt | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt >>>> index 8b9e0a9..663ee11 100644 >>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt >>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt >>>> @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. >>>> "qcom,gcc-msm8660" >>>> "qcom,kpss-acc-v1" >>>> "qcom,kpss-acc-v2" >>>> + "renesas,rcar-idle" >>> >>> The enable-method is about how to bring the CPU up (and potentially >>> implies other things, like how to take it down again). >> >> Thanks for your clarification. I now understand that this is related >> to SMP boot and CPU Hotplug. >> >>> These is no code in this series to that effect, no semantics are >>> provided, and the name implies this is idle-specific. >>> >>> So this doesn't look right, and makes no sense to me. >> >> Ok, I somehow (incorrectly?) assumed that the following line in patch >> 2/5* tied into this: >> +CPUIDLE_METHOD_OF_DECLARE(rcar, "renesas,rcar-idle", &rcar_cpuidle_ops); >> >> [*] [RFC/PATCH v2 2/5] ARM: shmobile: Add cpuidle_ops for R-Car cpuidle >> >> But if it is unrelated it should of course be dropped or reworked. > > It is related, but Mark's remark is correct. We have to keep in mind > that an enable-method defines how a CPU is powered {up/down} and > also suspended (quiesced through idle). > > It has to be defined through proper bindings and related code, adding a > random compatible string for the sake of matching does not cut it, > that is not acceptable and I stated it from the beginning. > > So, to make it clear, an enable-method defines CPU operations as a > whole, cpu init, power{up/down} and suspend. > > It must be documented with proper bindings that defines compatible string and > related properties. > > The sooner we incorporate the CPUidle ops into SMP ops the better to > prevent this abuse from happening, an enable-method encompasses SMP > ops and CPUidle ops, actually they must be merged because they represent > the enable-method implementation as a whole. > > I hope this helps. > > Lorenzo I will rename "renesas,rcar-idle" in the next patch. Regards. Keita Kobayashi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 08:39:35AM +0100, keita kobayashi wrote: [...] > > It is related, but Mark's remark is correct. We have to keep in mind > > that an enable-method defines how a CPU is powered {up/down} and > > also suspended (quiesced through idle). > > > > It has to be defined through proper bindings and related code, adding a > > random compatible string for the sake of matching does not cut it, > > that is not acceptable and I stated it from the beginning. > > > > So, to make it clear, an enable-method defines CPU operations as a > > whole, cpu init, power{up/down} and suspend. > > > > It must be documented with proper bindings that defines compatible string and > > related properties. > > > > The sooner we incorporate the CPUidle ops into SMP ops the better to > > prevent this abuse from happening, an enable-method encompasses SMP > > ops and CPUidle ops, actually they must be merged because they represent > > the enable-method implementation as a whole. > > > > I hope this helps. > > > > Lorenzo > > > I will rename "renesas,rcar-idle" in the next patch. It is not just about renaming a compatible string, it is about defining a DT binding and related enable-method as a whole. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt index 8b9e0a9..663ee11 100644 --- a/Documentation/devicetree/bindings/arm/cpus.txt +++ b/Documentation/devicetree/bindings/arm/cpus.txt @@ -196,6 +196,7 @@ nodes to be present and contain the properties described below. "qcom,gcc-msm8660" "qcom,kpss-acc-v1" "qcom,kpss-acc-v2" + "renesas,rcar-idle" "rockchip,rk3066-smp" - cpu-release-addr
This patch add the ARM CPUs Device Tree binding to document a new enable method of Renesas cpuidle. Signed-off-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com> --- Documentation/devicetree/bindings/arm/cpus.txt | 1 + 1 file changed, 1 insertion(+)