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

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

[Jean-Christophe PLAGNIOL-VILLARD]

this will allow to use the pata_at91 on a single zImage

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: linux-ide@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(-)

diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
index 5249e6d..c8d1154 100644
--- a/drivers/ata/pata_at91.c
+++ b/drivers/ata/pata_at91.c
@@ -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;
-- 
1.7.7

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

[Jean-Christophe PLAGNIOL-VILLARD]

this will allow to use the pata_at91 on a single zImage

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: linux-ide@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(-)

diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
index 41d4155..407595b 100644
--- a/drivers/ide/at91_ide.c
+++ b/drivers/ide/at91_ide.c
@@ -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)
-- 
1.7.7

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

[Ryan Mallon]

On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote:

> this will allow to use the pata_at91 on a single zImage
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: linux-ide@vger.kernel.org


Some comments below,

~Ryan

> ---
> 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(-)
> 
> diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> index 5249e6d..c8d1154 100644
> --- a/drivers/ata/pata_at91.c
> +++ b/drivers/ata/pata_at91.c
> @@ -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;


Nitpick: Whitespace around the = operator.

> +	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.

> +	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?

>  }
>  
>  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);


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?

>  
>  	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;

Re: [PATCH 3/3] ide/at91: use new introduce smc accessor

[Ryan Mallon]

On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote:

> this will allow to use the pata_at91 on a single zImage
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: linux-ide@vger.kernel.org


Couple of minor comments below,

~Ryan

> ---
> 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(-)
> 
> diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> index 41d4155..407595b 100644
> --- a/drivers/ide/at91_ide.c
> +++ b/drivers/ide/at91_ide.c
> @@ -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 */


Typo: ofloat -> float?

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


This comment is no longer correct.

> +	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.

> +	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.

> +	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);


Check return value?

>  }
>  
>  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)

Re: [PATCH 3/3] ide/at91: use new introduce smc accessor

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:38 Fri 09 Dec     , Ryan Mallon wrote:
> On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > this will allow to use the pata_at91 on a single zImage
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: linux-ide@vger.kernel.org
> 
> 
> Couple of minor comments below,
> 
> ~Ryan
> 
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> > index 41d4155..407595b 100644
> > --- a/drivers/ide/at91_ide.c
> > +++ b/drivers/ide/at91_ide.c
> > @@ -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 */
> 
> 
> Typo: ofloat -> float?
> 
> > +	smc.tdf_cycles = data_float;
> >  
> > -	at91_sys_write(AT91_SMC_MODE(chipselect), mode);
> > +	/* write SMC Setup Register */
> 
> 
> This comment is no longer correct.
> 
> > +	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.
> 
> > +	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.
> 
> > +	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);
> 
> 
> Check return value?
set_timings don't care about it

so no need

Best Regards,
J.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:34 Fri 09 Dec     , Ryan Mallon wrote:
> On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > this will allow to use the pata_at91 on a single zImage
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: linux-ide@vger.kernel.org
> 
> 
> Some comments below,
> 
> ~Ryan
> 
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> > index 5249e6d..c8d1154 100644
> > --- a/drivers/ata/pata_at91.c
> > +++ b/drivers/ata/pata_at91.c
> > @@ -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;
> 
> 
> Nitpick: Whitespace around the = operator.
> 
> > +	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.
> 
> > +	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?
> 
> >  }
> >  
> >  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);
> 
> 
> 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);
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
> 
> 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.

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

[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"?

> this will allow to use the pata_at91 on a single zImage

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> Cc: Nicolas Ferre<nicolas.ferre@atmel.com>
> Cc: linux-ide@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(-)

> diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> index 5249e6d..c8d1154 100644
> --- a/drivers/ata/pata_at91.c
> +++ b/drivers/ata/pata_at91.c
> @@ -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;

    Why not:

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

>   	/* 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.

> +	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;

>   	/* 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

Re: [PATCH 3/3] ide/at91: use new introduce smc accessor

[Sergei Shtylyov]

Hello.

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

    Same comment about the subject.

> this will allow to use the pata_at91 on a single zImage

    s/pata_at91/at91_ide/

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> Cc: Nicolas Ferre<nicolas.ferre@atmel.com>
> Cc: linux-ide@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.

> diff --git a/drivers/ide/at91_ide.c b/drivers/ide/at91_ide.c
> index 41d4155..407595b 100644
> --- a/drivers/ide/at91_ide.c
> +++ b/drivers/ide/at91_ide.c
> @@ -59,41 +59,50 @@
[...]
>   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;

	smc.nrd_setup = smc.nwe_setup = setup;
	smc.ncs_read_setup = smc.ncs_write_setup = 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;
 > +	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

Re: [PATCH 3/3] ide/at91: use new introduce smc accessor

[Arnd Bergmann]

On Thursday 08 December 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:
> this will allow to use the pata_at91 on a single zImage
> 
> J.
>  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

Re: [PATCH 3/3] ide/at91: use new introduce smc accessor

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:06 Fri 09 Dec     , Sergei Shtylyov wrote:
> Hello.
> 
> On 08-12-2011 19:23, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
>    Same comment about the subject.
> 
> >this will allow to use the pata_at91 on a single zImage
> 
>    s/pata_at91/at91_ide/
> 
> >Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> >Cc: Nicolas Ferre<nicolas.ferre@atmel.com>
> >Cc: linux-ide@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.
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.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:59 Fri 09 Dec     , Sergei Shtylyov wrote:
> 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
> 
> >this will allow to use the pata_at91 on a single zImage
> 
> >Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD<plagnioj@jcrosoft.com>
> >Cc: Nicolas Ferre<nicolas.ferre@atmel.com>
> >Cc: linux-ide@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(-)
> 
> >diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> >index 5249e6d..c8d1154 100644
> >--- a/drivers/ata/pata_at91.c
> >+++ b/drivers/ata/pata_at91.c
> >@@ -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;
> 
>    Why not:
> 
> 	smc.nrd_setup = smc.nwe_setup = setup;
> 	smc.ncs_read_setup = smc.ncs_write_setup = 0;
why not

Best Regards,
J.