| Message ID | 20111208150348.GD23884@game.jcrosoft.org |
|---|---|
| State | New |
| Headers | show |
On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote: > 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 <plagnioj@jcrosoft.com> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> Hi Jean, Couple of comments below. ~Ryan > --- > 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(-) > > diff --git a/arch/arm/mach-at91/include/mach/at91sam9_smc.h b/arch/arm/mach-at91/include/mach/at91sam9_smc.h > index eb18a70..b5eff0e 100644 > --- a/arch/arm/mach-at91/include/mach/at91sam9_smc.h > +++ b/arch/arm/mach-at91/include/mach/at91sam9_smc.h > @@ -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) > diff --git a/arch/arm/mach-at91/sam9_smc.c b/arch/arm/mach-at91/sam9_smc.c > index 8294783..e5936dc 100644 > --- a/arch/arm/mach-at91/sam9_smc.c > +++ b/arch/arm/mach-at91/sam9_smc.c > @@ -2,6 +2,7 @@ > * linux/arch/arm/mach-at91/sam9_smc.c > * > * Copyright (C) 2008 Andrew Victor > + * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > * > * 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]; > > -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? > +{ > + __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) > +{ > + 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. > + > + 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; > } > > -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? > + 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) > diff --git a/arch/arm/mach-at91/sam9_smc.h b/arch/arm/mach-at91/sam9_smc.h > index 039c5ce..3e52dcd4 100644 > --- a/arch/arm/mach-at91/sam9_smc.h > +++ b/arch/arm/mach-at91/sam9_smc.h > @@ -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);
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;
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)
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.
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.
On 09:20 Fri 09 Dec , Ryan Mallon wrote: > On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > 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 <plagnioj@jcrosoft.com> > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > > Hi Jean, > > Couple of comments below. > > ~Ryan > > > --- > > 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(-) > > > > diff --git a/arch/arm/mach-at91/include/mach/at91sam9_smc.h b/arch/arm/mach-at91/include/mach/at91sam9_smc.h > > index eb18a70..b5eff0e 100644 > > --- a/arch/arm/mach-at91/include/mach/at91sam9_smc.h > > +++ b/arch/arm/mach-at91/include/mach/at91sam9_smc.h > > @@ -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) > > diff --git a/arch/arm/mach-at91/sam9_smc.c b/arch/arm/mach-at91/sam9_smc.c > > index 8294783..e5936dc 100644 > > --- a/arch/arm/mach-at91/sam9_smc.c > > +++ b/arch/arm/mach-at91/sam9_smc.c > > @@ -2,6 +2,7 @@ > > * linux/arch/arm/mach-at91/sam9_smc.c > > * > > * Copyright (C) 2008 Andrew Victor > > + * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > > * > > * 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]; > > > > -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 > > > +{ > > + __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) > > +{ > > + 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. > > > + > > + 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; > > } > > > > -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.
On 09/12/11 17:27, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 09:20 Fri 09 Dec , Ryan Mallon wrote: >> On 09/12/11 02:23, Jean-Christophe PLAGNIOL-VILLARD wrote: >> >>> 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 <plagnioj@jcrosoft.com> >>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com> >> >> Hi Jean, >> >> Couple of comments below. >> >> ~Ryan >> >>> --- >>> 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(-) >>> >>> diff --git a/arch/arm/mach-at91/include/mach/at91sam9_smc.h b/arch/arm/mach-at91/include/mach/at91sam9_smc.h >>> index eb18a70..b5eff0e 100644 >>> --- a/arch/arm/mach-at91/include/mach/at91sam9_smc.h >>> +++ b/arch/arm/mach-at91/include/mach/at91sam9_smc.h >>> @@ -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) >>> diff --git a/arch/arm/mach-at91/sam9_smc.c b/arch/arm/mach-at91/sam9_smc.c >>> index 8294783..e5936dc 100644 >>> --- a/arch/arm/mach-at91/sam9_smc.c >>> +++ b/arch/arm/mach-at91/sam9_smc.c >>> @@ -2,6 +2,7 @@ >>> * linux/arch/arm/mach-at91/sam9_smc.c >>> * >>> * Copyright (C) 2008 Andrew Victor >>> + * Copyright (C) 2011 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> >>> * >>> * 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]; >>> >>> -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 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
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
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
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
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.
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.
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.