Message ID | 1309275583-11763-6-git-send-email-robherring2@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Dear Rob Herring, In message <1309275583-11763-6-git-send-email-robherring2@gmail.com> you wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Add support for AHCI controllers that are not PCI based. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: Wolfgang Denk <wd@denx.de> > --- > common/cmd_scsi.c | 6 +++- > drivers/block/ahci.c | 70 +++++++++++++++++++++++++++++++++++++++++++------ > include/ahci.h | 4 +++ > include/scsi.h | 1 + > 4 files changed, 70 insertions(+), 11 deletions(-) > > diff --git a/common/cmd_scsi.c b/common/cmd_scsi.c > index be4fe74..25a8299 100644 > --- a/common/cmd_scsi.c > +++ b/common/cmd_scsi.c > @@ -47,7 +47,8 @@ > #define SCSI_DEV_ID 0x5288 > > #else > -#error no scsi device defined > +#define SCSI_VEND_ID 0 > +#define SCSI_DEV_ID 0 > #endif I'm not sure if this is a good idea. Please explain. Also, checkpatch says: ERROR: trailing whitespace WARNING: please, no spaces at the start of a line #287: FILE: include/ahci.h:193: + $ > +#ifdef CONFIG_PCI > pci_read_config_word(pdev, 0x0a, &cc); > if (cc == 0x0101) > scc_s = "IDE"; > @@ -222,7 +227,9 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) > scc_s = "RAID"; > else > scc_s = "unknown"; > - > +#else > + scc_s = "SATA"; > +#endif Is SATA really the only possible option here? > +int ahci_init(u32 base) > +{ ... > +} Should this always be compiled in? Best regards, Wolfgang Denk
Wolfgang, On 07/04/2011 05:17 AM, Wolfgang Denk wrote: > Dear Rob Herring, > > In message <1309275583-11763-6-git-send-email-robherring2@gmail.com> you wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> Add support for AHCI controllers that are not PCI based. >> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> >> Cc: Wolfgang Denk <wd@denx.de> >> --- >> common/cmd_scsi.c | 6 +++- >> drivers/block/ahci.c | 70 +++++++++++++++++++++++++++++++++++++++++++------ >> include/ahci.h | 4 +++ >> include/scsi.h | 1 + >> 4 files changed, 70 insertions(+), 11 deletions(-) >> >> diff --git a/common/cmd_scsi.c b/common/cmd_scsi.c >> index be4fe74..25a8299 100644 >> --- a/common/cmd_scsi.c >> +++ b/common/cmd_scsi.c >> @@ -47,7 +47,8 @@ >> #define SCSI_DEV_ID 0x5288 >> >> #else >> -#error no scsi device defined >> +#define SCSI_VEND_ID 0 >> +#define SCSI_DEV_ID 0 >> #endif > > I'm not sure if this is a good idea. Please explain. This is the PCI ID and is only used here: common/cmd_scsi.c:183: busdevfunc=pci_find_device(SCSI_VEND_ID,SCSI_DEV_ID,0); /* get PCI Device ID */ For a non-PCI AHCI controller, there is no id. For PCI, I don't think 0 is a valid vendor ID anyway. > > Also, checkpatch says: > > ERROR: trailing whitespace > WARNING: please, no spaces at the start of a line > #287: FILE: include/ahci.h:193: > + $ > >> +#ifdef CONFIG_PCI >> pci_read_config_word(pdev, 0x0a, &cc); >> if (cc == 0x0101) >> scc_s = "IDE"; >> @@ -222,7 +227,9 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) >> scc_s = "RAID"; >> else >> scc_s = "unknown"; >> - >> +#else >> + scc_s = "SATA"; >> +#endif > > Is SATA really the only possible option here? Well I suppose anything is possible, but for embbedded SOCs with AHCI I've seen, they are SATA only. It's purely informational. > >> +int ahci_init(u32 base) >> +{ > ... >> +} > > Should this always be compiled in? I can add a config option CONFIG_SCSI_AHCI_PLAT. Perhaps this would be better than using CONFIG_PCI as I suppose you could have non-PCI AHCI controller on a platform with PCI. Rob
Dear Rob, In message <4E11D372.8090708@calxeda.com> you wrote: > > >> -#error no scsi device defined > >> +#define SCSI_VEND_ID 0 > >> +#define SCSI_DEV_ID 0 > >> #endif > > > > I'm not sure if this is a good idea. Please explain. > > This is the PCI ID and is only used here: > > common/cmd_scsi.c:183: > busdevfunc=pci_find_device(SCSI_VEND_ID,SCSI_DEV_ID,0); /* get PCI > Device ID */ > > For a non-PCI AHCI controller, there is no id. For PCI, I don't think 0 > is a valid vendor ID anyway. I think we should rather skip the respective parts of the code instead of inserting invalid vendor IDs. > > Should this always be compiled in? > > I can add a config option CONFIG_SCSI_AHCI_PLAT. Perhaps this would be > better than using CONFIG_PCI as I suppose you could have non-PCI AHCI > controller on a platform with PCI. Yes, please. Best regards, Wolfgang Denk
diff --git a/common/cmd_scsi.c b/common/cmd_scsi.c index be4fe74..25a8299 100644 --- a/common/cmd_scsi.c +++ b/common/cmd_scsi.c @@ -47,7 +47,8 @@ #define SCSI_DEV_ID 0x5288 #else -#error no scsi device defined +#define SCSI_VEND_ID 0 +#define SCSI_DEV_ID 0 #endif @@ -174,7 +175,7 @@ removable: scsi_curr_dev = -1; } - +#ifdef CONFIG_PCI void scsi_init(void) { int busdevfunc; @@ -192,6 +193,7 @@ void scsi_init(void) scsi_low_level_init(busdevfunc); scsi_scan(1); } +#endif block_dev_desc_t * scsi_get_dev(int dev) { diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c index d431c5a..d12cb71 100644 --- a/drivers/block/ahci.c +++ b/drivers/block/ahci.c @@ -78,13 +78,15 @@ static int waiting_for_cmd_completed(volatile u8 *offset, static int ahci_host_init(struct ahci_probe_ent *probe_ent) { +#ifdef CONFIG_PCI pci_dev_t pdev = probe_ent->dev; + u16 tmp16; + unsigned short vendor; +#endif volatile u8 *mmio = (volatile u8 *)probe_ent->mmio_base; u32 tmp, cap_save; - u16 tmp16; int i, j; volatile u8 *port_mmio; - unsigned short vendor; cap_save = readl(mmio + HOST_CAP); cap_save &= ((1 << 28) | (1 << 17)); @@ -110,6 +112,7 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent) writel(cap_save, mmio + HOST_CAP); writel_with_flush(0xf, mmio + HOST_PORTS_IMPL); +#ifdef CONFIG_PCI pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor); if (vendor == PCI_VENDOR_ID_INTEL) { @@ -118,7 +121,7 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent) tmp16 |= 0xf; pci_write_config_word(pdev, 0x92, tmp16); } - +#endif probe_ent->cap = readl(mmio + HOST_CAP); probe_ent->port_map = readl(mmio + HOST_PORTS_IMPL); probe_ent->n_ports = (probe_ent->cap & 0x1f) + 1; @@ -183,22 +186,24 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent) writel(tmp | HOST_IRQ_EN, mmio + HOST_CTL); tmp = readl(mmio + HOST_CTL); debug("HOST_CTL 0x%x\n", tmp); - +#ifdef CONFIG_PCI pci_read_config_word(pdev, PCI_COMMAND, &tmp16); tmp |= PCI_COMMAND_MASTER; pci_write_config_word(pdev, PCI_COMMAND, tmp16); - +#endif return 0; } static void ahci_print_info(struct ahci_probe_ent *probe_ent) { +#ifdef CONFIG_PCI pci_dev_t pdev = probe_ent->dev; + u16 cc; +#endif volatile u8 *mmio = (volatile u8 *)probe_ent->mmio_base; u32 vers, cap, impl, speed; const char *speed_s; - u16 cc; const char *scc_s; vers = readl(mmio + HOST_VERSION); @@ -212,7 +217,7 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) speed_s = "3"; else speed_s = "?"; - +#ifdef CONFIG_PCI pci_read_config_word(pdev, 0x0a, &cc); if (cc == 0x0101) scc_s = "IDE"; @@ -222,7 +227,9 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) scc_s = "RAID"; else scc_s = "unknown"; - +#else + scc_s = "SATA"; +#endif printf("AHCI %02x%02x.%02x%02x " "%u slots %u ports %s Gbps 0x%x impl %s mode\n", (vers >> 24) & 0xff, @@ -249,6 +256,7 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) cap & (1 << 13) ? "part " : ""); } +#ifdef CONFIG_PCI static int ahci_init_one(pci_dev_t pdev) { u16 vendor; @@ -291,7 +299,7 @@ static int ahci_init_one(pci_dev_t pdev) err_out: return rc; } - +#endif #define MAX_DATA_BYTE_COUNT (4*1024*1024) @@ -667,7 +675,9 @@ void scsi_low_level_init(int busdevfunc) int i; u32 linkmap; +#ifdef CONFIG_PCI ahci_init_one(busdevfunc); +#endif linkmap = probe_ent->link_port_map; @@ -682,6 +692,48 @@ void scsi_low_level_init(int busdevfunc) } } +int ahci_init(u32 base) +{ + int i, rc = 0; + u32 linkmap; + + memset(ataid, 0, sizeof(ataid)); + + probe_ent = malloc(sizeof(struct ahci_probe_ent)); + memset(probe_ent, 0, sizeof(struct ahci_probe_ent)); + + probe_ent->host_flags = ATA_FLAG_SATA + | ATA_FLAG_NO_LEGACY + | ATA_FLAG_MMIO + | ATA_FLAG_PIO_DMA + | ATA_FLAG_NO_ATAPI; + probe_ent->pio_mask = 0x1f; + probe_ent->udma_mask = 0x7f; /*Fixme,assume to support UDMA6 */ + + probe_ent->mmio_base = base; + + /* initialize adapter */ + rc = ahci_host_init(probe_ent); + if (rc) + goto err_out; + + ahci_print_info(probe_ent); + + linkmap = probe_ent->link_port_map; + + for (i = 0; i < CONFIG_SYS_SCSI_MAX_SCSI_ID; i++) { + if (((linkmap >> i) & 0x01)) { + if (ahci_port_start((u8) i)) { + printf("Can not start port %d\n", i); + continue; + } + ahci_set_feature((u8) i); + } + } +err_out: + return rc; +} + void scsi_bus_reset(void) { diff --git a/include/ahci.h b/include/ahci.h index 0c6bbbd..c0a0a47 100644 --- a/include/ahci.h +++ b/include/ahci.h @@ -25,6 +25,8 @@ #ifndef _AHCI_H_ #define _AHCI_H_ +#include <pci.h> + #define AHCI_PCI_BAR 0x24 #define AHCI_MAX_SG 56 /* hardware max is 64K */ #define AHCI_CMD_SLOT_SZ 32 @@ -187,4 +189,6 @@ struct ahci_probe_ent { u32 link_port_map; /*linkup port map*/ }; +int ahci_init(u32 base); + #endif diff --git a/include/scsi.h b/include/scsi.h index aaafc9c..c52759c 100644 --- a/include/scsi.h +++ b/include/scsi.h @@ -185,6 +185,7 @@ void scsi_low_level_init(int busdevfunc); * functions residing inside cmd_scsi.c */ void scsi_init(void); +void scsi_scan(int mode); #define SCSI_IDENTIFY 0xC0 /* not used */