ARM: at91: smc update

PATCH New Jean-Christophe PLAGNIOL-VILLARD 4 participants 16 messages Dec 8, 2011 – Dec 9, 2011

Subsystems (4)

Arm (32-bit) Arm sub-architectures libata Arm/Microchip (AT91) SoC support

lore

Share & tools

lore

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-08 15:03 UTC #0 lore raw

Hi,

this patch series introduce new accessor to manipulate the smc and be allow to abstract it of the soc rm9200 vs sam9

The following changes since commit 5c4ee7a68bc518ec79ac04f7c3259899ecff61db:

Merge branch 'next/cleanup' into for-next (2011-12-01 12:36:29 +0000)

are available in the git repository at:

git://github.com/at91linux/linux-at91.git ..BRANCH.NOT.VERIFIED..

Jean-Christophe PLAGNIOL-VILLARD (3):
ARM: at91: add accessor to manage smc
pata/at91: use new introduce smc accessor
ide/at91: use new introduce smc accessor

 arch/arm/mach-at91/include/mach/at91sam9_smc.h |   29 ++++++++
 arch/arm/mach-at91/sam9_smc.c                  |   93 ++++++++++++++++++++++--
 arch/arm/mach-at91/sam9_smc.h                  |   23 ------
 drivers/ata/pata_at91.c                        |   43 ++++++------
 drivers/ide/at91_ide.c                         |   65 ++++++++++-------
 5 files changed, 173 insertions(+), 80 deletions(-)

Best Regards,
J.

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-08 15:23 UTC #1 lore raw

PATCH 1/3 ARM: at91: add accessor to manage smc

this will allow to configure the smc independtly of the register configuration as example on rm9200 different from sam9

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc Nicolas Ferre <[email protected]>

---
 arch/arm/mach-at91/include/mach/at91sam9_smc.h |   29 ++++++++
 arch/arm/mach-at91/sam9_smc.c                  |   93 ++++++++++++++++++++++--
 arch/arm/mach-at91/sam9_smc.h                  |   23 ------
 3 files changed, 115 insertions(+), 30 deletions(-)

arch/arm/mach-at91/include/mach/at91sam9_smc.h +29 -0

index eb18a70..b5eff0e 100644
@@ -18,6 +18,35 @@
 
 #include <mach/cpu.h>
 
+#ifndef __ASSEMBLY__
+struct sam9_smc_config {
+	/* Setup register */
+	u8 ncs_read_setup;
+	u8 nrd_setup;
+	u8 ncs_write_setup;
+	u8 nwe_setup;
+
+	/* Pulse register */
+	u8 ncs_read_pulse;
+	u8 nrd_pulse;
+	u8 ncs_write_pulse;
+	u8 nwe_pulse;
+
+	/* Cycle register */
+	u16 read_cycle;
+	u16 write_cycle;
+
+	/* Mode register */
+	u32 mode;
+	u8 tdf_cycles:4;
+};
+
+extern int sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read_mode(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_write_mode(int id, int cs, struct sam9_smc_config* config);
+#endif
+
 #define AT91_SMC_SETUP		0x00				/* Setup Register for CS n */
 #define		AT91_SMC_NWESETUP	(0x3f << 0)			/* NWE Setup Length */
 #define			AT91_SMC_NWESETUP_(x)	((x) << 0)

arch/arm/mach-at91/sam9_smc.c +86 -7

119 lines not on this page · raw message

arch/arm/mach-at91/sam9_smc.h +0 ~~-23~~

index 039c5ce..3e52dcd4 100644
@@ -8,27 +8,4 @@
  * published by the Free Software Foundation.
  */
 
-struct sam9_smc_config {
-	/* Setup register */
-	u8 ncs_read_setup;
-	u8 nrd_setup;
-	u8 ncs_write_setup;
-	u8 nwe_setup;
-
-	/* Pulse register */
-	u8 ncs_read_pulse;
-	u8 nrd_pulse;
-	u8 ncs_write_pulse;
-	u8 nwe_pulse;
-
-	/* Cycle register */
-	u16 read_cycle;
-	u16 write_cycle;
-
-	/* Mode register */
-	u32 mode;
-	u8 tdf_cycles:4;
-};
-
-extern void __init sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
 extern void __init at91sam9_ioremap_smc(int id, u32 addr);

Signature · 1 line

1.7.7

Ryan Mallon

2011-12-08 22:20 UTC #2 lore raw

PATCH 1/3 ARM: at91: add accessor to manage smc

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to configure the smc independtly of the register configuration as example on rm9200 different from sam9
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>

Hi Jean,

Couple of comments below.

~Ryan

Quoted 60 lines

---
 arch/arm/mach-at91/include/mach/at91sam9_smc.h |   29 ++++++++
 arch/arm/mach-at91/sam9_smc.c                  |   93 ++++++++++++++++++++++--
 arch/arm/mach-at91/sam9_smc.h                  |   23 ------
 3 files changed, 115 insertions(+), 30 deletions(-)

arch/arm/mach-at91/include/mach/at91sam9_smc.h +29 -0

index eb18a70..b5eff0e 100644
@@ -18,6 +18,35 @@
 
 #include <mach/cpu.h>
 
+#ifndef __ASSEMBLY__
+struct sam9_smc_config {
+	/* Setup register */
+	u8 ncs_read_setup;
+	u8 nrd_setup;
+	u8 ncs_write_setup;
+	u8 nwe_setup;
+
+	/* Pulse register */
+	u8 ncs_read_pulse;
+	u8 nrd_pulse;
+	u8 ncs_write_pulse;
+	u8 nwe_pulse;
+
+	/* Cycle register */
+	u16 read_cycle;
+	u16 write_cycle;
+
+	/* Mode register */
+	u32 mode;
+	u8 tdf_cycles:4;
+};
+
+extern int sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read_mode(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_write_mode(int id, int cs, struct sam9_smc_config* config);
+#endif
+
 #define AT91_SMC_SETUP		0x00				/* Setup Register for CS n */
 #define		AT91_SMC_NWESETUP	(0x3f << 0)			/* NWE Setup Length */
 #define			AT91_SMC_NWESETUP_(x)	((x) << 0)

arch/arm/mach-at91/sam9_smc.c +1 -0

index 8294783..e5936dc 100644
@@ -2,6 +2,7 @@
  * linux/arch/arm/mach-at91/sam9_smc.c
  *
  * Copyright (C) 2008 Andrew Victor
+ * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -17,13 +18,58 @@

arch/arm/mach-at91/sam9_smc.c line 26

Quoted 10 lines

arch/arm/mach-at91/sam9_smc.c +2 -2

 #include "sam9_smc.h"
 
-
 #define AT91_SMC_CS(id, n)	(smc_base_addr[id] + ((n) * 0x10))
 
 static void __iomem *smc_base_addr[2];
 
-static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_config* config)
+static void __init_refok sam9_smc_cs_write_mode(void __iomem *base,
+					struct sam9_smc_config* config)

include/linux/kernel.h says that __init_refok means that this function can reference __init code, without being __init itself and without generating a modpost warning. It also says that such cases should be documented as to why the reference is ok. Why is __init_refok being used here and on the other fucntions in this file?

Quoted 31 lines

arch/arm/mach-at91/sam9_smc.c +31 -0

+{
+	__raw_writel(config->mode
+		   | AT91_SMC_TDF_(config->tdf_cycles),
+		   base + AT91_SMC_MODE);
+}
+
+int __init_refok sam9_smc_write_mode(int id, int cs,
+					struct sam9_smc_config* config)
+{
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	sam9_smc_cs_write_mode(AT91_SMC_CS(id, cs), config);
+
+	return 0;
+}
+
+int __init_refok sam9_smc_configure(int id, int cs,
+					struct sam9_smc_config* config)
+{
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	sam9_smc_cs_configure(AT91_SMC_CS(id, cs), config);
+
+	return 0;
+}
+
+static void __init_refok sam9_smc_cs_read_mode(void __iomem *base,
+					struct sam9_smc_config* config)
+{

arch/arm/mach-at91/sam9_smc.c line 67

Quoted 10 lines

arch/arm/mach-at91/sam9_smc.c +9 -0

+	u32 val = __raw_readl(base + AT91_SMC_MODE);
+
+	config->mode = (val & ~AT91_SMC_NWECYCLE);
+	config->tdf_cycles = (val & AT91_SMC_NWECYCLE) >> 16 ;
+}
+
+int __init_refok sam9_smc_read_mode(int id, int cs,
+					struct sam9_smc_config* config)
 {
+	void __iomen *base;

Typo: __iomem.

Quoted 15 lines

arch/arm/mach-at91/sam9_smc.c +5 -3

+
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	base = AT91_SMC_CS(id, cs);
 
 	/* Setup register */
 	__raw_writel(AT91_SMC_NWESETUP_(config->nwe_setup)

@@ -45,14 +91,47 @@ static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_con
 		   base + AT91_SMC_CYCLE);
 
 	/* Mode register */
-	__raw_writel(config->mode
-		   | AT91_SMC_TDF_(config->tdf_cycles),
-		   base + AT91_SMC_MODE);

arch/arm/mach-at91/sam9_smc.c static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_con · line 101

Quoted 10 lines

arch/arm/mach-at91/sam9_smc.c +5 -2

+	sam9_smc_cs_write_mode(base, config);
+
+	return 0;
 }
 
-void __init sam9_smc_configure(int id, int cs, struct sam9_smc_config* config)
+int __init_refok sam9_smc_read(int id, int cs, struct sam9_smc_config* config)
 {
-	sam9_smc_cs_configure(AT91_SMC_CS(id, cs), config);
+	void __iomen *base;

Typo: __iomem. Has this been compile tested?

Quoted 68 lines

arch/arm/mach-at91/sam9_smc.c +33 -0

+	u32 val;
+
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	base = AT91_SMC_CS(id, cs);
+
+	/* Setup register */
+	val = __raw_readl(base + AT91_SMC_SETUP);
+
+	config->nwe_setup = val & AT91_SMC_NWESETUP;
+	config->ncs_write_setup = (val & AT91_SMC_NCS_WRSETUP) >> 8;
+	config->nrd_setup = (val & AT91_SMC_NRDSETUP) >> 16;
+	config->ncs_read_setup = (val & AT91_SMC_NCS_RDSETUP) >> 24;
+
+	/* Pulse register */
+	val = __raw_readl(base + AT91_SMC_PULSE);
+
+	config->nwe_setup = val & AT91_SMC_NWEPULSE;
+	config->ncs_write_pulse = (val & AT91_SMC_NCS_WRPULSE) >> 8;
+	config->nrd_pulse = (val & AT91_SMC_NRDPULSE) >> 16;
+	config->ncs_read_pulse = (val & AT91_SMC_NCS_RDPULSE) >> 24;
+
+	/* Cycle register */
+	val = __raw_readl(base + AT91_SMC_CYCLE);
+
+	config->write_cycle = val & AT91_SMC_NWECYCLE;
+	config->read_cycle = (val & AT91_SMC_NRDCYCLE) >> 16;
+
+	/* Mode register */
+	sam9_smc_cs_read_mode(base, config);
+
+	return 0;
 }
 
 void __init at91sam9_ioremap_smc(int id, u32 addr)

arch/arm/mach-at91/sam9_smc.h +0 ~~-23~~

index 039c5ce..3e52dcd4 100644
@@ -8,27 +8,4 @@
  * published by the Free Software Foundation.
  */
 
-struct sam9_smc_config {
-	/* Setup register */
-	u8 ncs_read_setup;
-	u8 nrd_setup;
-	u8 ncs_write_setup;
-	u8 nwe_setup;
-
-	/* Pulse register */
-	u8 ncs_read_pulse;
-	u8 nrd_pulse;
-	u8 ncs_write_pulse;
-	u8 nwe_pulse;
-
-	/* Cycle register */
-	u16 read_cycle;
-	u16 write_cycle;
-
-	/* Mode register */
-	u32 mode;
-	u8 tdf_cycles:4;
-};
-
-extern void __init sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
 extern void __init at91sam9_ioremap_smc(int id, u32 addr);

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-09 06:27 UTC #3 lore raw

PATCH 1/3 ARM: at91: add accessor to manage smc

Quoted 82 lines · Ryan Mallon

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to configure the smc independtly of the register configuration as example on rm9200 different from sam9
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>

Hi Jean,

Couple of comments below.

~Ryan

Quoted 67 lines

---
 arch/arm/mach-at91/include/mach/at91sam9_smc.h |   29 ++++++++
 arch/arm/mach-at91/sam9_smc.c                  |   93 ++++++++++++++++++++++--
 arch/arm/mach-at91/sam9_smc.h                  |   23 ------
 3 files changed, 115 insertions(+), 30 deletions(-)

arch/arm/mach-at91/include/mach/at91sam9_smc.h +29 -0

index eb18a70..b5eff0e 100644
@@ -18,6 +18,35 @@
 
 #include <mach/cpu.h>
 
+#ifndef __ASSEMBLY__
+struct sam9_smc_config {
+	/* Setup register */
+	u8 ncs_read_setup;
+	u8 nrd_setup;
+	u8 ncs_write_setup;
+	u8 nwe_setup;
+
+	/* Pulse register */
+	u8 ncs_read_pulse;
+	u8 nrd_pulse;
+	u8 ncs_write_pulse;
+	u8 nwe_pulse;
+
+	/* Cycle register */
+	u16 read_cycle;
+	u16 write_cycle;
+
+	/* Mode register */
+	u32 mode;
+	u8 tdf_cycles:4;
+};
+
+extern int sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read_mode(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_write_mode(int id, int cs, struct sam9_smc_config* config);
+#endif
+
 #define AT91_SMC_SETUP		0x00				/* Setup Register for CS n */
 #define		AT91_SMC_NWESETUP	(0x3f << 0)			/* NWE Setup Length */
 #define			AT91_SMC_NWESETUP_(x)	((x) << 0)

arch/arm/mach-at91/sam9_smc.c +1 -1

index 8294783..e5936dc 100644
@@ -2,6 +2,7 @@
  * linux/arch/arm/mach-at91/sam9_smc.c
  *
  * Copyright (C) 2008 Andrew Victor
+ * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -17,13 +18,58 @@
 
 #include "sam9_smc.h"
 
-
 #define AT91_SMC_CS(id, n)	(smc_base_addr[id] + ((n) * 0x10))
 
 static void __iomem *smc_base_addr[2];

Quoted 10 lines

arch/arm/mach-at91/sam9_smc.c line 26
Quoted 3 lines
arch/arm/mach-at91/sam9_smc.c +2 -1
-static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_config* config)
+static void __init_refok sam9_smc_cs_write_mode(void __iomem *base,
+					struct sam9_smc_config* config)
include/linux/kernel.h says that __init_refok means that this function can reference __init code, without being __init itself and without generating a modpost warning. It also says that such cases should be documented as to why the reference is ok. Why is __init_refok being used here and on the other fucntions in this file?

because you can you this function before and after init and this need to not be free by the kernel after boot

Quoted 66 lines

Quoted 32 lines

arch/arm/mach-at91/sam9_smc.c +31 -0

+{
+	__raw_writel(config->mode
+		   | AT91_SMC_TDF_(config->tdf_cycles),
+		   base + AT91_SMC_MODE);
+}
+
+int __init_refok sam9_smc_write_mode(int id, int cs,
+					struct sam9_smc_config* config)
+{
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	sam9_smc_cs_write_mode(AT91_SMC_CS(id, cs), config);
+
+	return 0;
+}
+
+int __init_refok sam9_smc_configure(int id, int cs,
+					struct sam9_smc_config* config)
+{
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	sam9_smc_cs_configure(AT91_SMC_CS(id, cs), config);
+
+	return 0;
+}
+
+static void __init_refok sam9_smc_cs_read_mode(void __iomem *base,
+					struct sam9_smc_config* config)
+{

arch/arm/mach-at91/sam9_smc.c line 67

Quoted 10 lines

arch/arm/mach-at91/sam9_smc.c +9 -0

+	u32 val = __raw_readl(base + AT91_SMC_MODE);
+
+	config->mode = (val & ~AT91_SMC_NWECYCLE);
+	config->tdf_cycles = (val & AT91_SMC_NWECYCLE) >> 16 ;
+}
+
+int __init_refok sam9_smc_read_mode(int id, int cs,
+					struct sam9_smc_config* config)
 {
+	void __iomen *base;

Typo: __iomem.

Quoted 20 lines

arch/arm/mach-at91/sam9_smc.c +8 -3

+
+	if (!smc_base_addr[id])
+		return -EINVAL;
+
+	base = AT91_SMC_CS(id, cs);
 
 	/* Setup register */
 	__raw_writel(AT91_SMC_NWESETUP_(config->nwe_setup)

@@ -45,14 +91,47 @@ static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_con
 		   base + AT91_SMC_CYCLE);
 
 	/* Mode register */
-	__raw_writel(config->mode
-		   | AT91_SMC_TDF_(config->tdf_cycles),
-		   base + AT91_SMC_MODE);
+	sam9_smc_cs_write_mode(base, config);
+
+	return 0;
 }

Quoted 8 lines

arch/arm/mach-at91/sam9_smc.c static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_con · line 101
Quoted 5 lines
arch/arm/mach-at91/sam9_smc.c +2 -2
-void __init sam9_smc_configure(int id, int cs, struct sam9_smc_config* config)
+int __init_refok sam9_smc_read(int id, int cs, struct sam9_smc_config* config)
 {
-	sam9_smc_cs_configure(AT91_SMC_CS(id, cs), config);
+	void __iomen *base;
Typo: __iomem. Has this been compile tested?

yes and tested but rebased on for-next where the typo could have beed introduced

Best Regards,
J.

Ryan Mallon

2011-12-09 07:06 UTC #4 lore raw

PATCH 1/3 ARM: at91: add accessor to manage smc

Quoted 91 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 81 lines · Ryan Mallon

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to configure the smc independtly of the register configuration as example on rm9200 different from sam9
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>

Hi Jean,

Couple of comments below.

~Ryan

Quoted 67 lines

---
 arch/arm/mach-at91/include/mach/at91sam9_smc.h |   29 ++++++++
 arch/arm/mach-at91/sam9_smc.c                  |   93 ++++++++++++++++++++++--
 arch/arm/mach-at91/sam9_smc.h                  |   23 ------
 3 files changed, 115 insertions(+), 30 deletions(-)

arch/arm/mach-at91/include/mach/at91sam9_smc.h +29 -0

index eb18a70..b5eff0e 100644
@@ -18,6 +18,35 @@
 
 #include <mach/cpu.h>
 
+#ifndef __ASSEMBLY__
+struct sam9_smc_config {
+	/* Setup register */
+	u8 ncs_read_setup;
+	u8 nrd_setup;
+	u8 ncs_write_setup;
+	u8 nwe_setup;
+
+	/* Pulse register */
+	u8 ncs_read_pulse;
+	u8 nrd_pulse;
+	u8 ncs_write_pulse;
+	u8 nwe_pulse;
+
+	/* Cycle register */
+	u16 read_cycle;
+	u16 write_cycle;
+
+	/* Mode register */
+	u32 mode;
+	u8 tdf_cycles:4;
+};
+
+extern int sam9_smc_configure(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_read_mode(int id, int cs, struct sam9_smc_config* config);
+extern int sam9_smc_write_mode(int id, int cs, struct sam9_smc_config* config);
+#endif
+
 #define AT91_SMC_SETUP		0x00				/* Setup Register for CS n */
 #define		AT91_SMC_NWESETUP	(0x3f << 0)			/* NWE Setup Length */
 #define			AT91_SMC_NWESETUP_(x)	((x) << 0)

arch/arm/mach-at91/sam9_smc.c +1 -1

index 8294783..e5936dc 100644
@@ -2,6 +2,7 @@
  * linux/arch/arm/mach-at91/sam9_smc.c
  *
  * Copyright (C) 2008 Andrew Victor
+ * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -17,13 +18,58 @@
 
 #include "sam9_smc.h"
 
-
 #define AT91_SMC_CS(id, n)	(smc_base_addr[id] + ((n) * 0x10))
 
 static void __iomem *smc_base_addr[2];

Quoted 9 lines

arch/arm/mach-at91/sam9_smc.c line 26
Quoted 3 lines
arch/arm/mach-at91/sam9_smc.c +2 -1
-static void __init sam9_smc_cs_configure(void __iomem *base, struct sam9_smc_config* config)
+static void __init_refok sam9_smc_cs_write_mode(void __iomem *base,
+					struct sam9_smc_config* config)
include/linux/kernel.h says that __init_refok means that this function can reference __init code, without being __init itself and without generating a modpost warning. It also says that such cases should be documented as to why the reference is ok. Why is __init_refok being used here and on the other fucntions in this file?

Quoted 2 lines

because you can you this function before and after init and this need to not be free by the kernel after boot

Lots of functions are used before and after init without needing special markers. __init_refok means that this function references __init code even though the function itself is not __init (which would usually generate a modpost warning). This function doesn't reference any __init code, so the __init_refok should not be needed?

~Ryan

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-08 15:23 UTC #5 lore raw

PATCH 2/3 pata/at91: use new introduce smc accessor

In reply to Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc linux-ide at vger.kernel.org

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ata/pata_at91.c |   43 +++++++++++++++++++++----------------------
 1 files changed, 21 insertions(+), 22 deletions(-)

drivers/ata/pata_at91.c +21 ~~-22~~

index 5249e6d..c8d1154 100644
@@ -207,11 +207,11 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 {
 	int ret = 0;
 	int use_iordy;
+	struct sam9_smc_config smc;
 	unsigned int t6z;         /* data tristate time in ns */
 	unsigned int cycle;       /* SMC Cycle width in MCK ticks */
 	unsigned int setup;       /* SMC Setup width in MCK ticks */
 	unsigned int pulse;       /* CFIOR and CFIOW pulse width in MCK ticks */
-	unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */
 	unsigned int cs_pulse;    /* CS4 or CS5 pulse width in MCK ticks*/
 	unsigned int tdf_cycles;  /* SMC TDF MCK ticks */
 	unsigned long mck_hz;     /* MCK frequency in Hz */

@@ -244,26 +244,25 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 	}
 
 	dev_dbg(dev, "Use IORDY=%u, TDF Cycles=%u\n", use_iordy, tdf_cycles);
-	info->mode |= AT91_SMC_TDF_(tdf_cycles);
 
 	/* write SMC Setup Register */
-	at91_sys_write(AT91_SMC_SETUP(info->cs),
-			AT91_SMC_NWESETUP_(setup) |
-			AT91_SMC_NRDSETUP_(setup) |
-			AT91_SMC_NCS_WRSETUP_(cs_setup) |
-			AT91_SMC_NCS_RDSETUP_(cs_setup));
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;
 	/* write SMC Pulse Register */
-	at91_sys_write(AT91_SMC_PULSE(info->cs),
-			AT91_SMC_NWEPULSE_(pulse) |
-			AT91_SMC_NRDPULSE_(pulse) |
-			AT91_SMC_NCS_WRPULSE_(cs_pulse) |
-			AT91_SMC_NCS_RDPULSE_(cs_pulse));
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse =cs_pulse;
+	smc.ncs_write_pulse = smc.ncs_read_pulse;
 	/* write SMC Cycle Register */
-	at91_sys_write(AT91_SMC_CYCLE(info->cs),
-			AT91_SMC_NWECYCLE_(cycle) |
-			AT91_SMC_NRDCYCLE_(cycle));
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;
 	/* write SMC Mode Register*/
-	at91_sys_write(AT91_SMC_MODE(info->cs), info->mode);
+	smc.tdf_cycles = tdf_cycles;
+	smc.mode = info->mode;
+
+	sam9_smc_configure(0, info->cs, &smc);
 }
 
 static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)

@@ -288,20 +287,20 @@ static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev,
 	struct at91_ide_info *info = dev->link->ap->host->private_data;
 	unsigned int consumed;
 	unsigned long flags;
-	unsigned int mode;
+	struct sam9_smc_config smc;
 
 	local_irq_save(flags);
-	mode = at91_sys_read(AT91_SMC_MODE(info->cs));
+	sam9_smc_read_mode(0, info->cs, &smc);
 
 	/* set 16bit mode before writing data */
-	at91_sys_write(AT91_SMC_MODE(info->cs),
-			(mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16);
+	smc.mode = (smc.mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, info->cs, &smc);
 
 	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
 
 	/* restore 8bit mode after data is written */
-	at91_sys_write(AT91_SMC_MODE(info->cs),
-			(mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8);
+	smc.mode = (smc.mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8;
+	sam9_smc_write_mode(0, info->cs, &smc);
 
 	local_irq_restore(flags);
 	return consumed;

Signature · 1 line

1.7.7

Ryan Mallon

2011-12-08 22:34 UTC #6 lore raw

PATCH 2/3 pata/at91: use new introduce smc accessor

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc linux-ide at vger.kernel.org

Some comments below,

~Ryan

Quoted 42 lines

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ata/pata_at91.c |   43 +++++++++++++++++++++----------------------
 1 files changed, 21 insertions(+), 22 deletions(-)

drivers/ata/pata_at91.c +4 -7

index 5249e6d..c8d1154 100644
@@ -207,11 +207,11 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 {
 	int ret = 0;
 	int use_iordy;
+	struct sam9_smc_config smc;
 	unsigned int t6z;         /* data tristate time in ns */
 	unsigned int cycle;       /* SMC Cycle width in MCK ticks */
 	unsigned int setup;       /* SMC Setup width in MCK ticks */
 	unsigned int pulse;       /* CFIOR and CFIOW pulse width in MCK ticks */
-	unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */
 	unsigned int cs_pulse;    /* CS4 or CS5 pulse width in MCK ticks*/
 	unsigned int tdf_cycles;  /* SMC TDF MCK ticks */
 	unsigned long mck_hz;     /* MCK frequency in Hz */

@@ -244,26 +244,25 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 	}
 
 	dev_dbg(dev, "Use IORDY=%u, TDF Cycles=%u\n", use_iordy, tdf_cycles);
-	info->mode |= AT91_SMC_TDF_(tdf_cycles);
 
 	/* write SMC Setup Register */
-	at91_sys_write(AT91_SMC_SETUP(info->cs),
-			AT91_SMC_NWESETUP_(setup) |
-			AT91_SMC_NRDSETUP_(setup) |
-			AT91_SMC_NCS_WRSETUP_(cs_setup) |
-			AT91_SMC_NCS_RDSETUP_(cs_setup));
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 256

Quoted 10 lines

drivers/ata/pata_at91.c +4 -5

+	smc.ncs_write_setup = smc.ncs_read_setup;
 	/* write SMC Pulse Register */
-	at91_sys_write(AT91_SMC_PULSE(info->cs),
-			AT91_SMC_NWEPULSE_(pulse) |
-			AT91_SMC_NRDPULSE_(pulse) |
-			AT91_SMC_NCS_WRPULSE_(cs_pulse) |
-			AT91_SMC_NCS_RDPULSE_(cs_pulse));
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse =cs_pulse;

Nitpick: Whitespace around the = operator.

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 262

Quoted 10 lines

drivers/ata/pata_at91.c +4 -4

+	smc.ncs_write_pulse = smc.ncs_read_pulse;
 	/* write SMC Cycle Register */
-	at91_sys_write(AT91_SMC_CYCLE(info->cs),
-			AT91_SMC_NWECYCLE_(cycle) |
-			AT91_SMC_NRDCYCLE_(cycle));
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;
 	/* write SMC Mode Register*/
-	at91_sys_write(AT91_SMC_MODE(info->cs), info->mode);
+	smc.tdf_cycles = tdf_cycles;

The "write SMC Mode Register" comment should be removed.

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 265

Quoted 3 lines

drivers/ata/pata_at91.c +3 -0

+	smc.mode = info->mode;
+
+	sam9_smc_configure(0, info->cs, &smc);

This new function returns an int, should we be checking the return value here?

Quoted 9 lines

drivers/ata/pata_at91.c +1 -1

 }
 
 static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)

@@ -288,20 +287,20 @@ static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev,
 	struct at91_ide_info *info = dev->link->ap->host->private_data;
 	unsigned int consumed;
 	unsigned long flags;
-	unsigned int mode;
+	struct sam9_smc_config smc;

drivers/ata/pata_at91.c static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev, · line 297

Quoted 10 lines

drivers/ata/pata_at91.c +3 -3

 
 	local_irq_save(flags);
-	mode = at91_sys_read(AT91_SMC_MODE(info->cs));
+	sam9_smc_read_mode(0, info->cs, &smc);
 
 	/* set 16bit mode before writing data */
-	at91_sys_write(AT91_SMC_MODE(info->cs),
-			(mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16);
+	smc.mode = (smc.mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, info->cs, &smc);

Do sam9_smc_read/write_mode really need to pass the whole smc structure? The only fields used are mode and tdf_cycles. It might be clearer to pass those directly.

Also the original code here doesn't write tdf_cycles as part of the mode. Perhaps it would be better to have sam9_smc_write_mode to be:

int sam9_smc_write_mode(int id, int cs, unsigned mode);

and in set_smc_timing above explicitly or in the tdf_cycles bits?

Quoted 11 lines

drivers/ata/pata_at91.c +2 -2

 	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
 
 	/* restore 8bit mode after data is written */
-	at91_sys_write(AT91_SMC_MODE(info->cs),
-			(mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8);
+	smc.mode = (smc.mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8;
+	sam9_smc_write_mode(0, info->cs, &smc);
 
 	local_irq_restore(flags);
 	return consumed;

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-09 06:24 UTC #7 lore raw

PATCH 2/3 pata/at91: use new introduce smc accessor

Quoted 116 lines · Ryan Mallon

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc linux-ide at vger.kernel.org

Some comments below,

~Ryan

Quoted 42 lines

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ata/pata_at91.c |   43 +++++++++++++++++++++----------------------
 1 files changed, 21 insertions(+), 22 deletions(-)

drivers/ata/pata_at91.c +4 -7

index 5249e6d..c8d1154 100644
@@ -207,11 +207,11 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 {
 	int ret = 0;
 	int use_iordy;
+	struct sam9_smc_config smc;
 	unsigned int t6z;         /* data tristate time in ns */
 	unsigned int cycle;       /* SMC Cycle width in MCK ticks */
 	unsigned int setup;       /* SMC Setup width in MCK ticks */
 	unsigned int pulse;       /* CFIOR and CFIOW pulse width in MCK ticks */
-	unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */
 	unsigned int cs_pulse;    /* CS4 or CS5 pulse width in MCK ticks*/
 	unsigned int tdf_cycles;  /* SMC TDF MCK ticks */
 	unsigned long mck_hz;     /* MCK frequency in Hz */

@@ -244,26 +244,25 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 	}
 
 	dev_dbg(dev, "Use IORDY=%u, TDF Cycles=%u\n", use_iordy, tdf_cycles);
-	info->mode |= AT91_SMC_TDF_(tdf_cycles);
 
 	/* write SMC Setup Register */
-	at91_sys_write(AT91_SMC_SETUP(info->cs),
-			AT91_SMC_NWESETUP_(setup) |
-			AT91_SMC_NRDSETUP_(setup) |
-			AT91_SMC_NCS_WRSETUP_(cs_setup) |
-			AT91_SMC_NCS_RDSETUP_(cs_setup));
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 256

Quoted 10 lines

drivers/ata/pata_at91.c +4 -5

+	smc.ncs_write_setup = smc.ncs_read_setup;
 	/* write SMC Pulse Register */
-	at91_sys_write(AT91_SMC_PULSE(info->cs),
-			AT91_SMC_NWEPULSE_(pulse) |
-			AT91_SMC_NRDPULSE_(pulse) |
-			AT91_SMC_NCS_WRPULSE_(cs_pulse) |
-			AT91_SMC_NCS_RDPULSE_(cs_pulse));
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse =cs_pulse;

Nitpick: Whitespace around the = operator.

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 262

Quoted 10 lines

drivers/ata/pata_at91.c +4 -4

+	smc.ncs_write_pulse = smc.ncs_read_pulse;
 	/* write SMC Cycle Register */
-	at91_sys_write(AT91_SMC_CYCLE(info->cs),
-			AT91_SMC_NWECYCLE_(cycle) |
-			AT91_SMC_NRDCYCLE_(cycle));
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;
 	/* write SMC Mode Register*/
-	at91_sys_write(AT91_SMC_MODE(info->cs), info->mode);
+	smc.tdf_cycles = tdf_cycles;

The "write SMC Mode Register" comment should be removed.

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 265

Quoted 3 lines

drivers/ata/pata_at91.c +3 -0

+	smc.mode = info->mode;
+
+	sam9_smc_configure(0, info->cs, &smc);

This new function returns an int, should we be checking the return value here?

Quoted 9 lines

drivers/ata/pata_at91.c +1 -1

 }
 
 static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)

@@ -288,20 +287,20 @@ static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev,
 	struct at91_ide_info *info = dev->link->ap->host->private_data;
 	unsigned int consumed;
 	unsigned long flags;
-	unsigned int mode;
+	struct sam9_smc_config smc;

drivers/ata/pata_at91.c static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev, · line 297

Quoted 10 lines

drivers/ata/pata_at91.c +3 -3

 
 	local_irq_save(flags);
-	mode = at91_sys_read(AT91_SMC_MODE(info->cs));
+	sam9_smc_read_mode(0, info->cs, &smc);
 
 	/* set 16bit mode before writing data */
-	at91_sys_write(AT91_SMC_MODE(info->cs),
-			(mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16);
+	smc.mode = (smc.mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, info->cs, &smc);

Do sam9_smc_read/write_mode really need to pass the whole smc structure? The only fields used are mode and tdf_cycles. It might be clearer to pass those directly.

Quoted 4 lines

Also the original code here doesn't write tdf_cycles as part of the mode. Perhaps it would be better to have sam9_smc_write_mode to be:
int sam9_smc_write_mode(int id, int cs, unsigned mode);

no as you will force the write to read to content before updating it which I do not want the the need to you update the mode

Quoted 2 lines

and in set_smc_timing above explicitly or in the tdf_cycles bits?

the mode and tdf_cycles are closely related so I choose to manipulate them together

and as it's supposed to be register independant you always pass the struct

and the implemetation manage the write

Best Regards,
J.

Sergei Shtylyov

2011-12-09 10:59 UTC #8 lore raw

PATCH 2/3 pata/at91: use new introduce smc accessor

In reply to Jean-Christophe PLAGNIOL-VILLARD

Hello.

On 08-12-2011 19:23, Jean-Christophe PLAGNIOL-VILLARD wrote:

Your subject doesn't parse. Maybe you meant "use newly introduced smc
accessor"?

Quoted 38 lines

this will allow to use the pata_at91 on a single zImage

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD<[email protected]>

Cc Nicolas Ferre<[email protected]>

Cc linux-ide at vger.kernel.org

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

  drivers/ata/pata_at91.c |   43 +++++++++++++++++++++----------------------
  1 files changed, 21 insertions(+), 22 deletions(-)

drivers/ata/pata_at91.c +1 -2

index 5249e6d..c8d1154 100644
@@ -207,11 +207,11 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
  {
  	int ret = 0;
  	int use_iordy;
+	struct sam9_smc_config smc;
  	unsigned int t6z;         /* data tristate time in ns */
  	unsigned int cycle;       /* SMC Cycle width in MCK ticks */
  	unsigned int setup;       /* SMC Setup width in MCK ticks */
  	unsigned int pulse;       /* CFIOR and CFIOW pulse width in MCK ticks */
-	unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */
  	unsigned int cs_pulse;    /* CS4 or CS5 pulse width in MCK ticks*/
  	unsigned int tdf_cycles;  /* SMC TDF MCK ticks */
  	unsigned long mck_hz;     /* MCK frequency in Hz */

@@ -244,26 +244,25 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
  	}
 
  	dev_dbg(dev, "Use IORDY=%u, TDF Cycles=%u\n", use_iordy, tdf_cycles);
-	info->mode |= AT91_SMC_TDF_(tdf_cycles);

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 252

Quoted 10 lines

drivers/ata/pata_at91.c +4 -5

  	/* write SMC Setup Register */
-	at91_sys_write(AT91_SMC_SETUP(info->cs),
-			AT91_SMC_NWESETUP_(setup) |
-			AT91_SMC_NRDSETUP_(setup) |
-			AT91_SMC_NCS_WRSETUP_(cs_setup) |
-			AT91_SMC_NCS_RDSETUP_(cs_setup));
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;

    Why not:

	smc.nrd_setup = smc.nwe_setup = setup;
	smc.ncs_read_setup = smc.ncs_write_setup = 0;

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 256

Quoted 9 lines

drivers/ata/pata_at91.c +3 -5

  	/* write SMC Pulse Register */
-	at91_sys_write(AT91_SMC_PULSE(info->cs),
-			AT91_SMC_NWEPULSE_(pulse) |
-			AT91_SMC_NRDPULSE_(pulse) |
-			AT91_SMC_NCS_WRPULSE_(cs_pulse) |
-			AT91_SMC_NCS_RDPULSE_(cs_pulse));
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse =cs_pulse;

Add space after =, please.

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 257

Quoted 1 line

drivers/ata/pata_at91.c +1 -0
+	smc.ncs_write_pulse = smc.ncs_read_pulse;

    Why not:

	smc.nrd_pulse = smc.nwe_pulse = pulse;
	smc.ncs_read_pulse = smc.ncs_write_pulse = cs_pulse;

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 260

Quoted 6 lines

drivers/ata/pata_at91.c +2 -3

  	/* write SMC Cycle Register */
-	at91_sys_write(AT91_SMC_CYCLE(info->cs),
-			AT91_SMC_NWECYCLE_(cycle) |
-			AT91_SMC_NRDCYCLE_(cycle));
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;

    Why not:

	smc.read_cycle = smc.write_cycle = cycle;

WBR, Sergei

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-09 18:42 UTC #9 lore raw

PATCH 2/3 pata/at91: use new introduce smc accessor

Quoted 6 lines · Sergei Shtylyov

Hello.
On 08-12-2011 19:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
Your subject doesn't parse. Maybe you meant "use newly introduced
smc accessor"?

yeap

Quoted 39 lines

this will allow to use the pata_at91 on a single zImage

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD<[email protected]>

Cc Nicolas Ferre<[email protected]>

Cc linux-ide at vger.kernel.org

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ata/pata_at91.c |   43 +++++++++++++++++++++----------------------
 1 files changed, 21 insertions(+), 22 deletions(-)

drivers/ata/pata_at91.c +1 -2

index 5249e6d..c8d1154 100644
@@ -207,11 +207,11 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 {
 	int ret = 0;
 	int use_iordy;
+	struct sam9_smc_config smc;
 	unsigned int t6z;         /* data tristate time in ns */
 	unsigned int cycle;       /* SMC Cycle width in MCK ticks */
 	unsigned int setup;       /* SMC Setup width in MCK ticks */
 	unsigned int pulse;       /* CFIOR and CFIOW pulse width in MCK ticks */
-	unsigned int cs_setup = 0;/* CS4 or CS5 setup width in MCK ticks */
 	unsigned int cs_pulse;    /* CS4 or CS5 pulse width in MCK ticks*/
 	unsigned int tdf_cycles;  /* SMC TDF MCK ticks */
 	unsigned long mck_hz;     /* MCK frequency in Hz */

@@ -244,26 +244,25 @@ static void set_smc_timing(struct device *dev, struct ata_device *adev,
 	}
 
 	dev_dbg(dev, "Use IORDY=%u, TDF Cycles=%u\n", use_iordy, tdf_cycles);
-	info->mode |= AT91_SMC_TDF_(tdf_cycles);

Quoted 15 lines

drivers/ata/pata_at91.c static void set_smc_timing(struct device *dev, struct ata_device *adev, · line 252

Quoted 10 lines

drivers/ata/pata_at91.c +4 -5

 	/* write SMC Setup Register */
-	at91_sys_write(AT91_SMC_SETUP(info->cs),
-			AT91_SMC_NWESETUP_(setup) |
-			AT91_SMC_NRDSETUP_(setup) |
-			AT91_SMC_NCS_WRSETUP_(cs_setup) |
-			AT91_SMC_NCS_RDSETUP_(cs_setup));
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;

   Why not:

	smc.nrd_setup = smc.nwe_setup = setup;
	smc.ncs_read_setup = smc.ncs_write_setup = 0;

why not

Best Regards,
J.

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-08 15:23 UTC #10 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

In reply to Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc linux-ide at vger.kernel.org

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ide/at91_ide.c |   65 +++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 28 deletions(-)

drivers/ide/at91_ide.c +37 ~~-28~~

index 41d4155..407595b 100644
@@ -59,41 +59,50 @@
 #define ALT_MODE	0x00e00000
 #define REGS_SIZE	8
 
-#define enter_16bit(cs, mode) do {					\
-	mode = at91_sys_read(AT91_SMC_MODE(cs));			\
-	at91_sys_write(AT91_SMC_MODE(cs), mode | AT91_SMC_DBW_16);	\
-} while (0)
+static inline void enter_16bit(int cs, struct sam9_smc_config *smc)
+{
+	sam9_smc_read_mode(0, cs, smc);
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
-#define leave_16bit(cs, mode) at91_sys_write(AT91_SMC_MODE(cs), mode);
+static inline void leave_16bit(int cs, struct sam9_smc_config *smc)
+{
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
 static void set_smc_timings(const u8 chipselect, const u16 cycle,
 			    const u16 setup, const u16 pulse,
 			    const u16 data_float, int use_iordy)
 {
-	unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
+	struct sam9_smc_config smc;
+
+	smc.mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
 			     AT91_SMC_BAT_SELECT;
 
 	/* disable or enable waiting for IORDY signal */
 	if (use_iordy)
-		mode |= AT91_SMC_EXNWMODE_READY;
+		smc.mode |= AT91_SMC_EXNWMODE_READY;
 
-	/* add data float cycles if needed */
-	if (data_float)
-		mode |= AT91_SMC_TDF_(data_float);
+	/* add data ofloat cycles if needed */
+	smc.tdf_cycles = data_float;
 
-	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	/* write SMC Setup Register */
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;
+	/* write SMC Pulse Register */
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse = cycle;
+	smc.ncs_write_pulse = smc.ncs_read_pulse;
+	/* write SMC Cycle Register */
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;
 
-	/* setup timings in SMC */
-	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
-						   AT91_SMC_NCS_WRSETUP_(0) |
-						   AT91_SMC_NRDSETUP_(setup) |
-						   AT91_SMC_NCS_RDSETUP_(0));
-	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
-						   AT91_SMC_NCS_WRPULSE_(cycle) |
-						   AT91_SMC_NRDPULSE_(pulse) |
-						   AT91_SMC_NCS_RDPULSE_(cycle));
-	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
-						   AT91_SMC_NRDCYCLE_(cycle));
+	sam9_smc_configure(0, chipselect, &smc);
 }
 
 static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)

@@ -146,15 +155,15 @@ static void at91_ide_input_data(ide_drive_t *drive, struct ide_cmd *cmd,
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
 	u8 chipselect = hwif->select_data;
-	unsigned long mode;
+	struct sam9_smc_config smc;
 
 	pdbg("cs %u buf %p len %d\n", chipselect, buf, len);
 
 	len++;
 
-	enter_16bit(chipselect, mode);
+	enter_16bit(chipselect, &smc);
 	readsw((void __iomem *)io_ports->data_addr, buf, len / 2);
-	leave_16bit(chipselect, mode);
+	leave_16bit(chipselect, &smc);
 }
 
 static void at91_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,

@@ -163,13 +172,13 @@ static void at91_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
 	u8 chipselect = hwif->select_data;
-	unsigned long mode;
+	struct sam9_smc_config smc;
 
 	pdbg("cs %u buf %p len %d\n", chipselect,  buf, len);
 
-	enter_16bit(chipselect, mode);
+	enter_16bit(chipselect, &smc);
 	writesw((void __iomem *)io_ports->data_addr, buf, len / 2);
-	leave_16bit(chipselect, mode);
+	leave_16bit(chipselect, &smc);
 }
 
 static void at91_ide_set_pio_mode(ide_hwif_t *hwif, ide_drive_t *drive)

Signature · 1 line

1.7.7

Ryan Mallon

2011-12-08 22:38 UTC #11 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc linux-ide at vger.kernel.org

Couple of minor comments below,

~Ryan

Quoted 45 lines

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ide/at91_ide.c |   65 +++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 28 deletions(-)

drivers/ide/at91_ide.c +14 -6

index 41d4155..407595b 100644
@@ -59,41 +59,50 @@
 #define ALT_MODE	0x00e00000
 #define REGS_SIZE	8
 
-#define enter_16bit(cs, mode) do {					\
-	mode = at91_sys_read(AT91_SMC_MODE(cs));			\
-	at91_sys_write(AT91_SMC_MODE(cs), mode | AT91_SMC_DBW_16);	\
-} while (0)
+static inline void enter_16bit(int cs, struct sam9_smc_config *smc)
+{
+	sam9_smc_read_mode(0, cs, smc);
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
-#define leave_16bit(cs, mode) at91_sys_write(AT91_SMC_MODE(cs), mode);
+static inline void leave_16bit(int cs, struct sam9_smc_config *smc)
+{
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
 static void set_smc_timings(const u8 chipselect, const u16 cycle,
 			    const u16 setup, const u16 pulse,
 			    const u16 data_float, int use_iordy)
 {
-	unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
+	struct sam9_smc_config smc;
+
+	smc.mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
 			     AT91_SMC_BAT_SELECT;

drivers/ide/at91_ide.c line 88

Quoted 10 lines

drivers/ide/at91_ide.c +2 -4

 
 	/* disable or enable waiting for IORDY signal */
 	if (use_iordy)
-		mode |= AT91_SMC_EXNWMODE_READY;
+		smc.mode |= AT91_SMC_EXNWMODE_READY;
 
-	/* add data float cycles if needed */
-	if (data_float)
-		mode |= AT91_SMC_TDF_(data_float);
+	/* add data ofloat cycles if needed */

Typo: ofloat -> float?

drivers/ide/at91_ide.c line 91

Quoted 4 lines

drivers/ide/at91_ide.c +2 -1

+	smc.tdf_cycles = data_float;
 
-	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	/* write SMC Setup Register */

This comment is no longer correct.

drivers/ide/at91_ide.c line 96

Quoted 5 lines

drivers/ide/at91_ide.c +5 -0

+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;
+	/* write SMC Pulse Register */

Nor is this one.

drivers/ide/at91_ide.c line 101

Quoted 5 lines

drivers/ide/at91_ide.c +5 -0

+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse = cycle;
+	smc.ncs_write_pulse = smc.ncs_read_pulse;
+	/* write SMC Cycle Register */

or this one.

Quoted 5 lines

drivers/ide/at91_ide.c +2 -2

+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;
 
-	/* setup timings in SMC */
-	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |

drivers/ide/at91_ide.c line 105

Quoted 10 lines

drivers/ide/at91_ide.c +1 -9

-						   AT91_SMC_NCS_WRSETUP_(0) |
-						   AT91_SMC_NRDSETUP_(setup) |
-						   AT91_SMC_NCS_RDSETUP_(0));
-	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
-						   AT91_SMC_NCS_WRPULSE_(cycle) |
-						   AT91_SMC_NRDPULSE_(pulse) |
-						   AT91_SMC_NCS_RDPULSE_(cycle));
-	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
-						   AT91_SMC_NRDCYCLE_(cycle));
+	sam9_smc_configure(0, chipselect, &smc);

Check return value?

Quoted 39 lines

drivers/ide/at91_ide.c +6 -6

 }
 
 static unsigned int calc_mck_cycles(unsigned int ns, unsigned int mck_hz)

@@ -146,15 +155,15 @@ static void at91_ide_input_data(ide_drive_t *drive, struct ide_cmd *cmd,
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
 	u8 chipselect = hwif->select_data;
-	unsigned long mode;
+	struct sam9_smc_config smc;
 
 	pdbg("cs %u buf %p len %d\n", chipselect, buf, len);
 
 	len++;
 
-	enter_16bit(chipselect, mode);
+	enter_16bit(chipselect, &smc);
 	readsw((void __iomem *)io_ports->data_addr, buf, len / 2);
-	leave_16bit(chipselect, mode);
+	leave_16bit(chipselect, &smc);
 }
 
 static void at91_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,

@@ -163,13 +172,13 @@ static void at91_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
 	u8 chipselect = hwif->select_data;
-	unsigned long mode;
+	struct sam9_smc_config smc;
 
 	pdbg("cs %u buf %p len %d\n", chipselect,  buf, len);
 
-	enter_16bit(chipselect, mode);
+	enter_16bit(chipselect, &smc);
 	writesw((void __iomem *)io_ports->data_addr, buf, len / 2);
-	leave_16bit(chipselect, mode);
+	leave_16bit(chipselect, &smc);
 }
 
 static void at91_ide_set_pio_mode(ide_hwif_t *hwif, ide_drive_t *drive)

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-09 06:19 UTC #12 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

Quoted 101 lines · Ryan Mallon

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc linux-ide at vger.kernel.org

Couple of minor comments below,

~Ryan

Quoted 45 lines

---
Hi,

it's depends on other patch for AT91 can we apply via at91

Best Regards,
J.

 drivers/ide/at91_ide.c |   65 +++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 28 deletions(-)

drivers/ide/at91_ide.c +14 -6

index 41d4155..407595b 100644
@@ -59,41 +59,50 @@
 #define ALT_MODE	0x00e00000
 #define REGS_SIZE	8
 
-#define enter_16bit(cs, mode) do {					\
-	mode = at91_sys_read(AT91_SMC_MODE(cs));			\
-	at91_sys_write(AT91_SMC_MODE(cs), mode | AT91_SMC_DBW_16);	\
-} while (0)
+static inline void enter_16bit(int cs, struct sam9_smc_config *smc)
+{
+	sam9_smc_read_mode(0, cs, smc);
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_16;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
-#define leave_16bit(cs, mode) at91_sys_write(AT91_SMC_MODE(cs), mode);
+static inline void leave_16bit(int cs, struct sam9_smc_config *smc)
+{
+	smc->mode = (smc->mode & ~AT91_SMC_DBW) | AT91_SMC_DBW_8;
+	sam9_smc_write_mode(0, cs, smc);
+}
 
 static void set_smc_timings(const u8 chipselect, const u16 cycle,
 			    const u16 setup, const u16 pulse,
 			    const u16 data_float, int use_iordy)
 {
-	unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
+	struct sam9_smc_config smc;
+
+	smc.mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
 			     AT91_SMC_BAT_SELECT;

drivers/ide/at91_ide.c line 88

Quoted 10 lines

drivers/ide/at91_ide.c +2 -4

 
 	/* disable or enable waiting for IORDY signal */
 	if (use_iordy)
-		mode |= AT91_SMC_EXNWMODE_READY;
+		smc.mode |= AT91_SMC_EXNWMODE_READY;
 
-	/* add data float cycles if needed */
-	if (data_float)
-		mode |= AT91_SMC_TDF_(data_float);
+	/* add data ofloat cycles if needed */

Typo: ofloat -> float?

drivers/ide/at91_ide.c line 91

Quoted 4 lines

drivers/ide/at91_ide.c +2 -1

+	smc.tdf_cycles = data_float;
 
-	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	/* write SMC Setup Register */

This comment is no longer correct.

drivers/ide/at91_ide.c line 96

Quoted 5 lines

drivers/ide/at91_ide.c +5 -0

+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;
+	/* write SMC Pulse Register */

Nor is this one.

drivers/ide/at91_ide.c line 101

Quoted 5 lines

drivers/ide/at91_ide.c +5 -0

+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse = cycle;
+	smc.ncs_write_pulse = smc.ncs_read_pulse;
+	/* write SMC Cycle Register */

or this one.

Quoted 3 lines

drivers/ide/at91_ide.c +2 -0

+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;

Quoted 15 lines

drivers/ide/at91_ide.c line 105

Quoted 12 lines

drivers/ide/at91_ide.c +1 ~~-11~~

-	/* setup timings in SMC */
-	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(setup) |
-						   AT91_SMC_NCS_WRSETUP_(0) |
-						   AT91_SMC_NRDSETUP_(setup) |
-						   AT91_SMC_NCS_RDSETUP_(0));
-	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(pulse) |
-						   AT91_SMC_NCS_WRPULSE_(cycle) |
-						   AT91_SMC_NRDPULSE_(pulse) |
-						   AT91_SMC_NCS_RDPULSE_(cycle));
-	at91_sys_write(AT91_SMC_CYCLE(chipselect), AT91_SMC_NWECYCLE_(cycle) |
-						   AT91_SMC_NRDCYCLE_(cycle));
+	sam9_smc_configure(0, chipselect, &smc);

Check return value?

set_timings don't care about it

so no need

Best Regards,
J.

Sergei Shtylyov

2011-12-09 11:06 UTC #13 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

In reply to Jean-Christophe PLAGNIOL-VILLARD

Hello.

On 08-12-2011 19:23, Jean-Christophe PLAGNIOL-VILLARD wrote:

Same comment about the subject.

Quoted 1 line

this will allow to use the pata_at91 on a single zImage

    s/pata_at91/at91_ide/

Quoted 7 lines

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD<[email protected]>
Cc Nicolas Ferre<[email protected]>
Cc linux-ide at vger.kernel.org
---
Hi,
it's depends on other patch for AT91 can we apply via at91

I'm afraid David won't accept this, as the IDE tree is frozen for anything
but the fixes.

Quoted 5 lines

drivers/ide/at91_ide.c +0 -0
index 41d4155..407595b 100644
@@ -59,41 +59,50 @@

[...]

Quoted 17 lines

static void set_smc_timings(const u8 chipselect, const u16 cycle,
const u16 setup, const u16 pulse, const u16 data_float, int use_iordy)
{

drivers/ide/at91_ide.c +4 -4

-	unsigned long mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
+	struct sam9_smc_config smc;
+
+	smc.mode = AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
  			     AT91_SMC_BAT_SELECT;
 
  	/* disable or enable waiting for IORDY signal */
  	if (use_iordy)
-		mode |= AT91_SMC_EXNWMODE_READY;
+		smc.mode |= AT91_SMC_EXNWMODE_READY;
 
-	/* add data float cycles if needed */
-	if (data_float)

drivers/ide/at91_ide.c line 95

Quoted 10 lines

drivers/ide/at91_ide.c +7 -2

-		mode |= AT91_SMC_TDF_(data_float);
+	/* add data ofloat cycles if needed */
+	smc.tdf_cycles = data_float;
 
-	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
+	/* write SMC Setup Register */
+	smc.nrd_setup = setup;
+	smc.nwe_setup = smc.nrd_setup;
+	smc.ncs_read_setup = 0;
+	smc.ncs_write_setup = smc.ncs_read_setup;

	smc.nrd_setup = smc.nwe_setup = setup;
	smc.ncs_read_setup = smc.ncs_write_setup = 0;

drivers/ide/at91_ide.c line 100

Quoted 5 lines

drivers/ide/at91_ide.c +5 -0

+	/* write SMC Pulse Register */
+	smc.nrd_pulse = pulse;
+	smc.nwe_pulse = smc.nrd_pulse;
+	smc.ncs_read_pulse = cycle;
+	smc.ncs_write_pulse = smc.ncs_read_pulse;

	smc.nrd_pulse = smc.nwe_pulse = pulse;

drivers/ide/at91_ide.c line 103

Quoted 5 lines

drivers/ide/at91_ide.c +4 -0

+	smc.ncs_read_pulse = smc.ncs_write_pulse = cycle;
 
+	/* write SMC Cycle Register */
+	smc.read_cycle = cycle;
+	smc.write_cycle = smc.read_cycle;

	smc.read_cycle = smc.write_cycle = cycle;

WBR, Sergei

Jean-Christophe PLAGNIOL-VILLARD

author

2011-12-09 18:40 UTC #14 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

Quoted 16 lines · Sergei Shtylyov

Hello.
On 08-12-2011 19:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
Same comment about the subject.
Quoted 1 line
this will allow to use the pata_at91 on a single zImage
   s/pata_at91/at91_ide/
Quoted 5 lines
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD<[email protected]>
Cc Nicolas Ferre<[email protected]>
Cc linux-ide at vger.kernel.org
---
Hi,

Quoted 4 lines

Quoted 1 line
it's depends on other patch for AT91 can we apply via at91
I'm afraid David won't accept this, as the IDE tree is frozen for
anything but the fixes.

basicaly it's a fix on at91 not ide. we can not compile all at91 together because of the at91_sys_read/write so I need to fix it on at91 this one of the place I need to touch.

I'll not touch it otherwise

Best Regards,
J.

Arnd Bergmann

2011-12-09 13:59 UTC #15 lore raw

PATCH 3/3 ide/at91: use new introduce smc accessor

In reply to Jean-Christophe PLAGNIOL-VILLARD

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

this will allow to use the pata_at91 on a single zImage

 drivers/ide/at91_ide.c |   65 +++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 28 deletions(-)

The description doesn't match the file you are changing. Is there still a strong reason to keep using the at91_ide driver instead of migrating everybody to pata_at91 now?

Arnd