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@xxxxxxxxxxx>
> >> ---
> >>
> >> 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@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@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@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@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@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@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@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@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@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@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@xxxxxxxxxxx> */
> >> .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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
