From: krzk@kernel.org (Krzysztof Kozlowski)
To: linux-arm-kernel@lists.infradead.org
Subject: [RESEND-PATCH] ARM: EXYNOS: remove smp hook from machine descriptor
Date: Fri, 9 Dec 2016 16:24:33 +0200 [thread overview]
Message-ID: <20161209142433.GA10080@kozik-lap> (raw)
In-Reply-To: <fbc36736-ce7c-3eed-2833-c2e45ea02ce5@samsung.com>
On Fri, Dec 09, 2016 at 02:42:36PM +0530, pankaj.dubey wrote:
> Hi Krzysztof,
>
> On Thursday 08 December 2016 11:03 PM, Krzysztof Kozlowski wrote:
> > On Thu, Dec 08, 2016 at 08:32:15AM +0530, Pankaj Dubey wrote:
> >> Use CPU_METHOD_OF_DECLARE() for smp_ops instead of using it
> >> via machine descriptor.
> >>
> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> >> ---
> >>
> >> Resending as I missed to include samsung mailing list.
> >>
> >> arch/arm/boot/dts/exynos3250.dtsi | 1 +
> >> arch/arm/boot/dts/exynos4210.dtsi | 1 +
> >> arch/arm/boot/dts/exynos4212.dtsi | 1 +
> >> arch/arm/boot/dts/exynos4412.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5250.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5260.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5410.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5420-cpus.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5422-cpus.dtsi | 1 +
> >> arch/arm/boot/dts/exynos5440.dtsi | 1 +
> >> arch/arm/mach-exynos/common.h | 2 --
> >> arch/arm/mach-exynos/exynos.c | 1 -
> >> arch/arm/mach-exynos/platsmp.c | 2 ++
> >> 13 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
> >> index ba17ee1..f28f669 100644
> >> --- a/arch/arm/boot/dts/exynos3250.dtsi
> >> +++ b/arch/arm/boot/dts/exynos3250.dtsi
> >> @@ -53,6 +53,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> >> index 7f3a18c..6dfd98d 100644
> >> --- a/arch/arm/boot/dts/exynos4210.dtsi
> >> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> >> @@ -35,6 +35,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 900 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos4212.dtsi b/arch/arm/boot/dts/exynos4212.dtsi
> >> index 5389011..3e8982e 100644
> >> --- a/arch/arm/boot/dts/exynos4212.dtsi
> >> +++ b/arch/arm/boot/dts/exynos4212.dtsi
> >> @@ -25,6 +25,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at A00 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> >> index 40beede..faf2fb8 100644
> >> --- a/arch/arm/boot/dts/exynos4412.dtsi
> >> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> >> @@ -25,6 +25,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at A00 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> >> index b6d7444..580897c 100644
> >> --- a/arch/arm/boot/dts/exynos5250.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> >> @@ -52,6 +52,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5260.dtsi b/arch/arm/boot/dts/exynos5260.dtsi
> >> index 5818718..1af6e76 100644
> >> --- a/arch/arm/boot/dts/exynos5260.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5260.dtsi
> >> @@ -32,6 +32,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5410.dtsi b/arch/arm/boot/dts/exynos5410.dtsi
> >> index 2b6adaf..b092cdc 100644
> >> --- a/arch/arm/boot/dts/exynos5410.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5410.dtsi
> >> @@ -33,6 +33,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5420-cpus.dtsi b/arch/arm/boot/dts/exynos5420-cpus.dtsi
> >> index 5c052d7..a587704 100644
> >> --- a/arch/arm/boot/dts/exynos5420-cpus.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5420-cpus.dtsi
> >> @@ -24,6 +24,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5422-cpus.dtsi b/arch/arm/boot/dts/exynos5422-cpus.dtsi
> >> index bf3c6f1..7fcdfd0 100644
> >> --- a/arch/arm/boot/dts/exynos5422-cpus.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5422-cpus.dtsi
> >> @@ -23,6 +23,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu0: cpu at 100 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/boot/dts/exynos5440.dtsi b/arch/arm/boot/dts/exynos5440.dtsi
> >> index 2a2e570..0a958e8 100644
> >> --- a/arch/arm/boot/dts/exynos5440.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5440.dtsi
> >> @@ -50,6 +50,7 @@
> >> cpus {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> + enable-method = "samsung,exynos-smp";
> >>
> >> cpu at 0 {
> >> device_type = "cpu";
> >> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> >> index fb12d11..051e1ab 100644
> >> --- a/arch/arm/mach-exynos/common.h
> >> +++ b/arch/arm/mach-exynos/common.h
> >> @@ -143,8 +143,6 @@ static inline void exynos_pm_init(void) {}
> >> extern void exynos_cpu_resume(void);
> >> extern void exynos_cpu_resume_ns(void);
> >>
> >> -extern const struct smp_operations exynos_smp_ops;
> >> -
> >> extern void exynos_cpu_power_down(int cpu);
> >> extern void exynos_cpu_power_up(int cpu);
> >> extern int exynos_cpu_power_state(int cpu);
> >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> >> index fa08ef9..f0a766e 100644
> >> --- a/arch/arm/mach-exynos/exynos.c
> >> +++ b/arch/arm/mach-exynos/exynos.c
> >> @@ -211,7 +211,6 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
> >> /* Maintainer: Kukjin Kim <kgene.kim@samsung.com> */
> >> .l2c_aux_val = 0x3c400001,
> >> .l2c_aux_mask = 0xc20fffff,
> >> - .smp = smp_ops(exynos_smp_ops),
> >> .map_io = exynos_init_io,
> >> .init_early = exynos_firmware_init,
> >> .init_irq = exynos_init_irq,
> >> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> >> index 94405c7..43eec10 100644
> >> --- a/arch/arm/mach-exynos/platsmp.c
> >> +++ b/arch/arm/mach-exynos/platsmp.c
> >> @@ -474,3 +474,5 @@ const struct smp_operations exynos_smp_ops __initconst = {
> >> .cpu_die = exynos_cpu_die,
> >> #endif
> >> };
> >> +
> >> +CPU_METHOD_OF_DECLARE(exynos_smp, "samsung,exynos-smp", &exynos_smp_ops);
> >
> > Three issues:
> > 1. This has to be documented. Checkpatch did not complain about it?
>
> No it didn't.
>
> > 2. I think this breaks the ABI with existing DTBs because now the
> > enable-method of cpus becomes mandatory. But the
> > Documentation/devicetree/bindings/arm/cpus.txt is saying that:
> > "On ARM 32-bit systems this property is optional and can be one of"
> >
>
> I am not very sure that this is an ABI break, as other platforms (e.g
> hisilicon,hip01-smp) also adopted this as some later stage and they also
> removed smp hook support from their machine files when they adopted to
> this enable-method in DTS files.
So they broke the ABI. :)
>
> If we want to keep older DTBs keep working with new Kernel image, then I
> need to drop patch from mach-exynos and keep smp_ops hook in machine
> descriptor as it is to keep supporting older DTBs. I can see some
> platforms have adopted this way as well.
Please, go ahead with this solution. The bindings documentation clearly
states that this is an optional field so changing it to "required" is an
ABI break.
>
> Surely I will add new bindings details in
> Documentation/devicetree/bindings/arm/cpus.txt file. I am not sure why
> checkpatch did not complain about this?
>
> > 3. Please split DTS changes to separate patches. This is, by the way,
> > connected with #2 above: if there was no ABI break, then the DTS
> > could go to separate branch easily.
> >
>
> Since I am not sure if this will considered as ABI break or not, I just
> looked how this was handled in other platforms, I can see some platforms
> have clubbed DTS change along with mach files, and some have done in
> separate patch as well. So I will look for suggestion from you for this
> how we can go for exynos platform?
Please split it and make safe for legacy DTBs without enable-method
property.
Best regards,
Krzysztof
prev parent reply other threads:[~2016-12-09 14:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-08 3:02 [RESEND-PATCH] ARM: EXYNOS: remove smp hook from machine descriptor Pankaj Dubey
2016-12-08 17:33 ` Krzysztof Kozlowski
2016-12-09 9:12 ` pankaj.dubey
2016-12-09 14:24 ` Krzysztof Kozlowski [this message]
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=20161209142433.GA10080@kozik-lap \
--to=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).