Sigma Designs Tango4 port

arch/arm/boot/dts/tango4-common.dtsi line 67

Quoted 7 lines · Marc Gonzalez

arch/arm/boot/dts/tango4-common.dtsi new file +7 -0

+		clkgen: clkgen at 10000 {
+			compatible = "sigma,tango4-clkgen";
+			reg = <0x10000 0x40>;
+			clocks = <&xtal>;
+			clock-output-names = "cpuclk", "sysclk";
+			#clock-cells = <1>;
+		};

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

Quoted 13 lines · M?ns Rullg?rd

arch/arm/boot/dts/tango4-common.dtsi line 67

Quoted 7 lines · Marc Gonzalez

arch/arm/boot/dts/tango4-common.dtsi new file +7 -0

+		clkgen: clkgen at 10000 {
+			compatible = "sigma,tango4-clkgen";
+			reg = <0x10000 0x40>;
+			clocks = <&xtal>;
+			clock-output-names = "cpuclk", "sysclk";
+			#clock-cells = <1>;
+		};

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

I was hoping to take baby steps to work up to a fully-functional port. The first step (in my mind) is this submission: a minimal port which only requires the two "main" clocks.

The next step will add to the minimal port by supporting as many peripherals as possible, as well as their required clocks.

Would you agree to discuss the clkgen driver in a few weeks, when I can concentrate full-time on the subject? Working on other drivers will give me a better understanding of the required clocks.

Quoted 16 lines · Marc Gonzalez

Quoted 13 lines · M?ns Rullg?rd

arch/arm/boot/dts/tango4-common.dtsi line 67

Quoted 7 lines · Marc Gonzalez

arch/arm/boot/dts/tango4-common.dtsi new file +7 -0

+           clkgen: clkgen at 10000 {
+                   compatible = "sigma,tango4-clkgen";
+                   reg = <0x10000 0x40>;
+                   clocks = <&xtal>;
+                   clock-output-names = "cpuclk", "sysclk";
+                   #clock-cells = <1>;
+           };

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

Quoted 6 lines

I was hoping to take baby steps to work up to a fully-functional port. The first step (in my mind) is this submission: a minimal port which only requires the two "main" clocks.

The next step will add to the minimal port by supporting as many peripherals as possible, as well as their required clocks.

But the code already exists. Why start over?

Quoted 22 lines · M?ns Rullg?rd

Quoted 20 lines · Marc Gonzalez

Quoted 10 lines · M?ns Rullg?rd

arch/arm/boot/dts/tango4-common.dtsi line 67

Quoted 7 lines · Marc Gonzalez

arch/arm/boot/dts/tango4-common.dtsi new file +7 -0

+           clkgen: clkgen at 10000 {
+                   compatible = "sigma,tango4-clkgen";
+                   reg = <0x10000 0x40>;
+                   clocks = <&xtal>;
+                   clock-output-names = "cpuclk", "sysclk";
+                   #clock-cells = <1>;
+           };

Quoted 3 lines

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

I was hoping to take baby steps to work up to a fully-functional port. The first step (in my mind) is this submission: a minimal port which only requires the two "main" clocks.

Quoted 4 lines

Quoted 2 lines

The next step will add to the minimal port by supporting as many peripherals as possible, as well as their required clocks.

But the code already exists. Why start over?

M?ns, I don't understand your role in this. Can you clarify?

If you've already done a port, why haven't you contributed it yourself?

Why are you driving Marc's work from the back seat like this instead of submitting your own work?

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

---
 arch/arm/Kconfig              |  2 ++
 arch/arm/Makefile             |  1 +
 arch/arm/mach-tangox/Kconfig  | 12 ++++++++++++
 arch/arm/mach-tangox/Makefile |  2 ++
 arch/arm/mach-tangox/setup.c  | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tangox/smc.S    |  9 +++++++++
 arch/arm/mach-tangox/smc.h    |  5 +++++

Quoted 8 lines · Marc Gonzalez

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

Quoted 8 lines

---
 arch/arm/Kconfig              |  2 ++
 arch/arm/Makefile             |  1 +
 arch/arm/mach-tangox/Kconfig  | 12 ++++++++++++
 arch/arm/mach-tangox/Makefile |  2 ++
 arch/arm/mach-tangox/setup.c  | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tangox/smc.S    |  9 +++++++++
 arch/arm/mach-tangox/smc.h    |  5 +++++

Potential bike-shed fodder, but, a dumb question: is the family name actually "tangox" or is the "x" for the number (tango3, tango4, etc.)

Assuming it's the later based on usage throughout the patch, I think it'd be better to just use "tango" throughout instead of tangox.

Quoted 22 lines · Kevin Hilman

Quoted 8 lines · Marc Gonzalez

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

Quoted 8 lines

---
 arch/arm/Kconfig              |  2 ++
 arch/arm/Makefile             |  1 +
 arch/arm/mach-tangox/Kconfig  | 12 ++++++++++++
 arch/arm/mach-tangox/Makefile |  2 ++
 arch/arm/mach-tangox/setup.c  | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tangox/smc.S    |  9 +++++++++
 arch/arm/mach-tangox/smc.h    |  5 +++++

Potential bike-shed fodder, but, a dumb question: is the family name actually "tangox" or is the "x" for the number (tango3, tango4, etc.)

Quoted 2 lines

Assuming it's the later based on usage throughout the patch, I think it'd be better to just use "tango" throughout instead of tangox.

The x indeed stands for a number. I have no idea what tango1 was or if it ever existed. Tango2 (SMP863x) and tango3 (SMP86[457]x) were MIPS based. Tango4 is ARM based (mostly, the SMP8910 is MIPS) but otherwise very similar to tango3.

Since we don't know what tango5 will look like, mach-tango4 might be more suitable here. If tango5 turns out to be sufficiently similar, there's no harm from adding support for that to the mach-tango4 code (just look at mach-omap2).

Most of the drivers support both tango3 and tango4, but apparently some changes are planned for tango5.

Quoted 20 lines · Olof Johansson

Quoted 18 lines · M?ns Rullg?rd

Quoted 16 lines · Marc Gonzalez

Quoted 13 lines · M?ns Rullg?rd

arch/arm/boot/dts/tango4-common.dtsi line 67

Quoted 7 lines · Marc Gonzalez

arch/arm/boot/dts/tango4-common.dtsi new file +7 -0

+           clkgen: clkgen at 10000 {
+                   compatible = "sigma,tango4-clkgen";
+                   reg = <0x10000 0x40>;
+                   clocks = <&xtal>;
+                   clock-output-names = "cpuclk", "sysclk";
+                   #clock-cells = <1>;
+           };

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

I was hoping to take baby steps to work up to a fully-functional port. The first step (in my mind) is this submission: a minimal port which only requires the two "main" clocks.

The next step will add to the minimal port by supporting as many peripherals as possible, as well as their required clocks.

But the code already exists. Why start over?

Quoted 1 line

M?ns, I don't understand your role in this. Can you clarify?

Oh, I'm just the guy who did all the work and then got screwed over by Sigma.

Quoted 2 lines

If you've already done a port, why haven't you contributed it yourself?

Because it's not yet in a shape to be contributed, just like Marc's isn't.

Quoted 2 lines

Why are you driving Marc's work from the back seat like this instead of submitting your own work?

I have submitted bits and pieces. It's a slow process.

Quoted 29 lines · M?ns Rullg?rd

Quoted 27 lines · Olof Johansson

Quoted 25 lines · M?ns Rullg?rd

Quoted 22 lines · Marc Gonzalez

Quoted 10 lines · M?ns Rullg?rd

Review

Quoted 7 lines · Marc Gonzalez

(quoted diff) +7 -0

+           clkgen: clkgen at 10000 {
+                   compatible = "sigma,tango4-clkgen";
+                   reg = <0x10000 0x40>;
+                   clocks = <&xtal>;
+                   clock-output-names = "cpuclk", "sysclk";
+                   #clock-cells = <1>;
+           };

Quoted 3 lines

Would you please consider using my clock driver that matches the actual hardware, supports all the clock outputs required for USB, SATA, etc, and works on tango3 as well?

I was hoping to take baby steps to work up to a fully-functional port. The first step (in my mind) is this submission: a minimal port which only requires the two "main" clocks.

The next step will add to the minimal port by supporting as many peripherals as possible, as well as their required clocks.

Quoted 1 line

Quoted 1 line

Quoted 1 line

But the code already exists. Why start over?

"La perfection est atteinte, non pas lorsqu'il n'y a plus rien ? ajouter, mais lorsqu'il n'y a plus rien ? retirer."

For example, what is the point of not ignoring sysclk_premux, when the boot loader has always hard-coded "PLL1 drives sys_clk, PLL2 drives cd_clk".

Having one clk driver for tango3, and another for tango4 allows you to submit your own tango3 clk driver, and I can then ignore all the insane tango3 clk legacy, and focus on the tango4 clean-ups. Would that work for you?

(BTW, are you aware that the clk maintainers will NAK your clk driver in its current form, based on the fact that they insist on a single node for the entire clkgen block?)

Quoted 4 lines

Quoted 1 line

M?ns, I don't understand your role in this. Can you clarify?

Oh, I'm just the guy who did all the work and then got screwed over by Sigma.

Here's the sequence of events, to the best of my recollection.

In 2010, you hacked the Popcorn Hour C-200 (Tango3 SoC)
In 2014-11, I mentioned on LAKML that I planned to upstream Sigma's kernel In 2014-12, you pushed your tango3 port to github (3.18 at the time IIRC)
https://github.com/mansr/linux-tangox
In late 2015-02, you blogged about your work

	http://hardwarebug.org/2015/02/26/popcorn-hour-revisited/

I contacted you the next day, and you offered your services. You met management in late March.
Then radio silence for several months.
Sometime in July, I was told the deal had fallen apart :-(

Quoted 5 lines

Quoted 2 lines

If you've already done a port, why haven't you contributed it yourself?

Because it's not yet in a shape to be contributed, just like Marc's isn't.

Are you saying the DT needs to be perfect on the first submission? Has this been true for other mach?

Quoted 4 lines

Quoted 2 lines

Why are you driving Marc's work from the back seat like this instead of submitting your own work?

I have submitted bits and pieces. It's a slow process.

Indeed. Especially when a maintainer NAKs a patch because one used 'unsigned' instead of 'unsigned int'.

Quoted 8 lines · Marc Gonzalez

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

Quoted 8 lines

---
 arch/arm/Kconfig              |  2 ++
 arch/arm/Makefile             |  1 +
 arch/arm/mach-tangox/Kconfig  | 12 ++++++++++++
 arch/arm/mach-tangox/Makefile |  2 ++
 arch/arm/mach-tangox/setup.c  | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tangox/smc.S    |  9 +++++++++
 arch/arm/mach-tangox/smc.h    |  5 +++++

Potential bike-shed fodder, but, a dumb question: is the family name actually "tangox" or is the "x" for the number (tango3, tango4, etc.)

Assuming it's the later based on usage throughout the patch, I think it'd be better to just use "tango" throughout instead of tangox.

Also a MAINTAINERS file entry is appropriate for this new platform support (as scripts/checkpatch.pl should have told you.)

Quoted 19 lines · Kevin Hilman

Quoted 16 lines · Marc Gonzalez

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

---
 arch/arm/Kconfig              |  2 ++
 arch/arm/Makefile             |  1 +
 arch/arm/mach-tangox/Kconfig  | 12 ++++++++++++
 arch/arm/mach-tangox/Makefile |  2 ++
 arch/arm/mach-tangox/setup.c  | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tangox/smc.S    |  9 +++++++++
 arch/arm/mach-tangox/smc.h    |  5 +++++

Quoted 5 lines

Potential bike-shed fodder, but, a dumb question: is the family name actually "tangox" or is the "x" for the number (tango3, tango4, etc.)

Assuming it's the later based on usage throughout the patch, I think it'd be better to just use "tango" throughout instead of tangox.

I should just change tangox to tango everywhere?

This port supports tango4. I will submit a tango5 port in 2016. Does that change anything?

Quoted 2 lines

Also a MAINTAINERS file entry is appropriate for this new platform support (as scripts/checkpatch.pl should have told you.)

Thanks for pointing that out. I'll send a v10.
Are these the only issues in your opinion?

Add support for Sigma Designs ARM-based Tango4 "Secure Media Processor" platforms (i.e. smp8734, smp8756, smp8758, smp8759) built around the Cortex-A9 MPCore r3p0 (all dual-core, except the 8756).

Support for older MIPS-based platforms can be found elsewhere:
https://github.com/mansr/linux-tangox

Signed-off-by Marc Gonzalez <[email protected]>

arch/arm/mach-tangox/setup.c new file +4 -0

+	if (reg == L2X0_CTRL)
+		tango_set_l2_control(val);
+}
+

arch/arm/mach-tangox/setup.c line 21

Quoted 3 lines · Marc Gonzalez

arch/arm/mach-tangox/setup.c new file +3 -0

+static void tango_l2c_write(unsigned long val, unsigned int reg)
+{
+	pr_debug("%s: reg=0x%x val=0x%lx\n", __func__, reg, val);

leftover debugging aid?

arch/arm/mach-tangox/smc.S line 6

Quoted 4 lines

arch/arm/mach-tangox/smc.S new file +4 -0

+ENTRY(tango_smc)
+	push	{lr}
+	mov	ip, r1
+	dsb	/* This barrier is probably unnecessary */

Then remove it?

Quoted 7 lines · M?ns Rullg?rd

Quoted 2 lines · Marc Gonzalez

For example, what is the point of not ignoring sysclk_premux, when the boot loader has always hard-coded "PLL1 drives sys_clk, PLL2 drives cd_clk".

What if the boot loader changes? Since we know the structure of the clock tree, it's safer to check how it is actually configured.

In my opinion, this might be the problem.

You think you grok the clkgen block, based on

1) buggy driver code written by Sigma
2) obsolete and/or incorrect documentation (it mentions tango1 for crying out loud)
3) limited testing on 8642 and 8759

Quoted 7 lines · Kevin Hilman

arch/arm/mach-tangox/setup.c line 21

Quoted 3 lines · Marc Gonzalez

arch/arm/mach-tangox/setup.c new file +3 -0

+static void tango_l2c_write(unsigned long val, unsigned int reg)
+{
+	pr_debug("%s: reg=0x%x val=0x%lx\n", __func__, reg, val);

leftover debugging aid?

I'll remove it.

(For my education, we're not supposed to use any pr_debug calls?)

Quoted 6 lines

arch/arm/mach-tangox/smc.S line 6

Quoted 4 lines

arch/arm/mach-tangox/smc.S new file +4 -0

+ENTRY(tango_smc)
+	push	{lr}
+	mov	ip, r1
+	dsb	/* This barrier is probably unnecessary */

Then remove it?

This was discussed in v8. It's probably cargo cult from OMAP, but the performance hit is negligible, and I don't have time to properly analyze the code path. I just wanted to add the comment in case someone copied my code.