[PATCH 2/2] mach-spear: fixed spear1340.c file

This is the fixed file after moving sata support to new file in
spear1340_sata.c

Signed-off-by: Nicholas Krause <redacted>
---
 arch/arm/mach-spear/spear1340.c | 111 ----------------------------------------
 1 file changed, 111 deletions(-)

diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index 7b6bff7..f9d8ef3 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c

@@ -21,117 +21,6 @@
 #include "generic.h"
 #include <mach/spear.h>
 
-/* FIXME: Move SATA PHY code into a standalone driver */
-
-/* Base addresses */
-#define SPEAR1340_SATA_BASE			UL(0xB1000000)
-
-/* Power Management Registers */
-#define SPEAR1340_PCM_CFG			(VA_MISC_BASE + 0x100)
-#define SPEAR1340_PCM_WKUP_CFG			(VA_MISC_BASE + 0x104)
-#define SPEAR1340_SWITCH_CTR			(VA_MISC_BASE + 0x108)
-
-#define SPEAR1340_PERIP1_SW_RST			(VA_MISC_BASE + 0x318)
-#define SPEAR1340_PERIP2_SW_RST			(VA_MISC_BASE + 0x31C)
-#define SPEAR1340_PERIP3_SW_RST			(VA_MISC_BASE + 0x320)
-
-/* PCIE - SATA configuration registers */
-#define SPEAR1340_PCIE_SATA_CFG			(VA_MISC_BASE + 0x424)
-	/* PCIE CFG MASks */
-	#define SPEAR1340_PCIE_CFG_DEVICE_PRESENT	(1 << 11)
-	#define SPEAR1340_PCIE_CFG_POWERUP_RESET	(1 << 10)
-	#define SPEAR1340_PCIE_CFG_CORE_CLK_EN		(1 << 9)
-	#define SPEAR1340_PCIE_CFG_AUX_CLK_EN		(1 << 8)
-	#define SPEAR1340_SATA_CFG_TX_CLK_EN		(1 << 4)
-	#define SPEAR1340_SATA_CFG_RX_CLK_EN		(1 << 3)
-	#define SPEAR1340_SATA_CFG_POWERUP_RESET	(1 << 2)
-	#define SPEAR1340_SATA_CFG_PM_CLK_EN		(1 << 1)
-	#define SPEAR1340_PCIE_SATA_SEL_PCIE		(0)
-	#define SPEAR1340_PCIE_SATA_SEL_SATA		(1)
-	#define SPEAR1340_SATA_PCIE_CFG_MASK		0xF1F
-	#define SPEAR1340_PCIE_CFG_VAL	(SPEAR1340_PCIE_SATA_SEL_PCIE | \
-			SPEAR1340_PCIE_CFG_AUX_CLK_EN | \
-			SPEAR1340_PCIE_CFG_CORE_CLK_EN | \
-			SPEAR1340_PCIE_CFG_POWERUP_RESET | \
-			SPEAR1340_PCIE_CFG_DEVICE_PRESENT)
-	#define SPEAR1340_SATA_CFG_VAL	(SPEAR1340_PCIE_SATA_SEL_SATA | \
-			SPEAR1340_SATA_CFG_PM_CLK_EN | \
-			SPEAR1340_SATA_CFG_POWERUP_RESET | \
-			SPEAR1340_SATA_CFG_RX_CLK_EN | \
-			SPEAR1340_SATA_CFG_TX_CLK_EN)
-
-#define SPEAR1340_PCIE_MIPHY_CFG		(VA_MISC_BASE + 0x428)
-	#define SPEAR1340_MIPHY_OSC_BYPASS_EXT		(1 << 31)
-	#define SPEAR1340_MIPHY_CLK_REF_DIV2		(1 << 27)
-	#define SPEAR1340_MIPHY_CLK_REF_DIV4		(2 << 27)
-	#define SPEAR1340_MIPHY_CLK_REF_DIV8		(3 << 27)
-	#define SPEAR1340_MIPHY_PLL_RATIO_TOP(x)	(x << 0)
-	#define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA \
-			(SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
-			SPEAR1340_MIPHY_CLK_REF_DIV2 | \
-			SPEAR1340_MIPHY_PLL_RATIO_TOP(60))
-	#define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK \
-			(SPEAR1340_MIPHY_PLL_RATIO_TOP(120))
-	#define SPEAR1340_PCIE_SATA_MIPHY_CFG_PCIE \
-			(SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
-			SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
-
-/* SATA device registration */
-static int sata_miphy_init(struct device *dev, void __iomem *addr)
-{
-	writel(SPEAR1340_SATA_CFG_VAL, SPEAR1340_PCIE_SATA_CFG);
-	writel(SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK,
-			SPEAR1340_PCIE_MIPHY_CFG);
-	/* Switch on sata power domain */
-	writel((readl(SPEAR1340_PCM_CFG) | (0x800)), SPEAR1340_PCM_CFG);
-	msleep(20);
-	/* Disable PCIE SATA Controller reset */
-	writel((readl(SPEAR1340_PERIP1_SW_RST) & (~0x1000)),
-			SPEAR1340_PERIP1_SW_RST);
-	msleep(20);
-
-	return 0;
-}
-
-void sata_miphy_exit(struct device *dev)
-{
-	writel(0, SPEAR1340_PCIE_SATA_CFG);
-	writel(0, SPEAR1340_PCIE_MIPHY_CFG);
-
-	/* Enable PCIE SATA Controller reset */
-	writel((readl(SPEAR1340_PERIP1_SW_RST) | (0x1000)),
-			SPEAR1340_PERIP1_SW_RST);
-	msleep(20);
-	/* Switch off sata power domain */
-	writel((readl(SPEAR1340_PCM_CFG) & (~0x800)), SPEAR1340_PCM_CFG);
-	msleep(20);
-}
-
-int sata_suspend(struct device *dev)
-{
-	if (dev->power.power_state.event == PM_EVENT_FREEZE)
-		return 0;
-
-	sata_miphy_exit(dev);
-
-	return 0;
-}
-
-int sata_resume(struct device *dev)
-{
-	if (dev->power.power_state.event == PM_EVENT_THAW)
-		return 0;
-
-	return sata_miphy_init(dev, NULL);
-}
-
-static struct ahci_platform_data sata_pdata = {
-	.init = sata_miphy_init,
-	.exit = sata_miphy_exit,
-	.suspend = sata_suspend,
-	.resume = sata_resume,
-};
-
 /* Add SPEAr1340 auxdata to pass platform data */
 static struct of_dev_auxdata spear1340_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("snps,spear-ahci", SPEAR1340_SATA_BASE, NULL,

-- 
1.9.1

On Wed, 2014-07-02 at 22:36 -0400, Nicholas Krause wrote:

This is the fixed file after moving sata support to new file in
spear1340_sata.c

Signed-off-by: Nicholas Krause <redacted>
---
 arch/arm/mach-spear/spear1340.c | 111 ----------------------------------------
 1 file changed, 111 deletions(-)

This patch, together with patch 1/2, basically moves a chunk of code
into a separate file, didn't it? If so, why did you split that move in
two patches?

And how does all this work without any changes to a Makefile?

(I have no idea what this FIXME is about, but I do wonder whether that
new file by itself is the standalone driver this FIXME is about. The
spear developers will surely know.)


Paul Bolle

Yes it is and I did it in two patches in order to be more readable.
Furthermore I don't known Kconfig well enough to do the Makefile
for the file I created.
Cheers Nick

On Thu, Jul 3, 2014 at 2:04 PM, Paul Bolle [off-list ref] wrote:

On Wed, 2014-07-02 at 22:36 -0400, Nicholas Krause wrote:

quoted

This is the fixed file after moving sata support to new file in
spear1340_sata.c

Signed-off-by: Nicholas Krause <redacted>
---
 arch/arm/mach-spear/spear1340.c | 111 ----------------------------------------
 1 file changed, 111 deletions(-)

This patch, together with patch 1/2, basically moves a chunk of code
into a separate file, didn't it? If so, why did you split that move in
two patches?

And how does all this work without any changes to a Makefile?

quoted

diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index 7b6bff7..f9d8ef3 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c

@@ -21,117 +21,6 @@
 #include "generic.h"
 #include <mach/spear.h>

-/* FIXME: Move SATA PHY code into a standalone driver */

(I have no idea what this FIXME is about, but I do wonder whether that
new file by itself is the standalone driver this FIXME is about. The
spear developers will surely know.)


Paul Bolle

[I fixed the top posting.]

On Thu, 2014-07-03 at 14:08 -0400, Nick Krause wrote:

On Thu, Jul 3, 2014 at 2:04 PM, Paul Bolle [off-list ref] wrote:

quoted

This patch, together with patch 1/2, basically moves a chunk of code
into a separate file, didn't it? If so, why did you split that move in
two patches?

Yes it is and I did it in two patches in order to be more readable.

It makes it harder to understand the change (I had to _guess_ it was a
move). Moreover, depending on the order that these two patches would be
merged, we could end up with a chunk of code being either included twice
or not included at all, in some range of commits. Neither would be good.

quoted

And how does all this work without any changes to a Makefile?

Furthermore I don't known Kconfig well enough to do the Makefile
for the file I created.

Then I think you should, well, study the kernel build system before
submitting a change like this. And you can also ask a question or two to
get things going. (Not sure what the relevant list would be.)


Paul Bolle

Very well then I will read the documentation on Kconfig in order to
understand that and fix up my patches for that.
On the other hand I will send a email before the patches to tell in
what order to apply them.
Cheers Nick

On Thu, Jul 3, 2014 at 2:36 PM, Paul Bolle [off-list ref] wrote:

[I fixed the top posting.]

On Thu, 2014-07-03 at 14:08 -0400, Nick Krause wrote:

quoted

On Thu, Jul 3, 2014 at 2:04 PM, Paul Bolle [off-list ref] wrote:

quoted

This patch, together with patch 1/2, basically moves a chunk of code
into a separate file, didn't it? If so, why did you split that move in
two patches?

Yes it is and I did it in two patches in order to be more readable.

It makes it harder to understand the change (I had to _guess_ it was a
move). Moreover, depending on the order that these two patches would be
merged, we could end up with a chunk of code being either included twice
or not included at all, in some range of commits. Neither would be good.

quoted

quoted

And how does all this work without any changes to a Makefile?

Furthermore I don't known Kconfig well enough to do the Makefile
for the file I created.

Then I think you should, well, study the kernel build system before
submitting a change like this. And you can also ask a question or two to
get things going. (Not sure what the relevant list would be.)


Paul Bolle

Hi Nick,

On Fri, Jul 4, 2014 at 2:07 AM, Nick Krause [off-list ref] wrote:

Very well then I will read the documentation on Kconfig in order to
understand that and fix up my patches for that.
On the other hand I will send a email before the patches to tell in
what order to apply them.

As already asked by Paul, please don't top-post:
http://en.wikipedia.org/wiki/Posting_style#Top-posting

Firstly, in case we are going to get these patches in these have to be
merged together, otherwise inbetween these two we will have duplicate
functions for the same thing..

Kernel should work/build properly between commits, so that git bisect
works..

Now about this:

-/* FIXME: Move SATA PHY code into a standalone driver */

I don't think it was just about creating another file for this. Otherwise
how hard is it for the author then?

It was probably more about updating the interface of the sata driver
to accept stub drivers, so that we don't have to keep stuff here.

So, this patch wouldn't fix the FIXME and so better drop this patchset
completely.

Thanks for trying though.

---
Viresh