On Wednesday 25 March 2009, Kazuaki Ichinohe wrote:
Hi Stefan,
Thank you for the reply. This is the patch that corrects the point.
Signed-off-by: Kazuaki Ichinohe <kazuichi at fsi.co.jp>
OK, now you added your Signed-off-by line and the patch itself doesn't seem to be line wrapped. But the commit text above is not really describing the patch. Your first version was better.
Please find some more comments below.
[patch] diff -purN u-boot-2009.03/common/cmd_scsi.c u-boot-2009.03-sata/common/cmd_scsi.c --- u-boot-2009.03/common/cmd_scsi.c 2009-03-22 06:04:41.000000000 +0900 +++ u-boot-2009.03-sata/common/cmd_scsi.c 2009-03-25 18:53:29.000000000 +0900 @@ -47,8 +47,10 @@ #define SCSI_DEV_ID 0x5288
#else +#ifndef CONFIG_SATA_DWC #error no scsi device defined #endif +#endif
Why do you add this 460EX SATA support to cmd_scsi.c? Wouldn't cmd_sata.c be a better place? Or did I miss something here?
<snip>
+++ u-boot-2009.03-sata/drivers/block/sata_dwc.c 2009-03-25 18:02:07.000000000 +0900 @@ -0,0 +1,2175 @@ +/*
- sata_dwc.c
- Synopsys DesignWare Cores (DWC) SATA host driver
- Author: Mark Miesfeld mmiesfeld@amcc.com
- Ported from 2.6.19.2 to 2.6.25/26 by Stefan Roese sr@denx.de
- Copyright 2008 DENX Software Engineering
- Based on versions provided by AMCC and Synopsys which are:
Copyright 2006 Applied Micro Circuits Corporation
COPYRIGHT (C) 2005 SYNOPSYS, INC. ALL RIGHTS RESERVED
- This program is free software; you can redistribute
- it and/or modify it under the terms of the GNU
- General Public License as published by the
- Free Software Foundation; either version 2 of the License,
- or (at your option) any later version.
- */
As it seems you added the Linux driver version with some changes so that it fits into U-Boot. You should document this in the commit text of this patch.
+#include <common.h> +#include <command.h> +#include <pci.h> +#include <asm/processor.h> +#include <asm/errno.h> +#include <asm/io.h> +#include <malloc.h> +#include <scsi.h> +#include <ata.h> +#include <linux/ctype.h>
+/* sata_dwc.h */ +#include "sata_dwc.h"
+/* Base Address */ +#define SATA_BASE_ADDR 0xe20d1000 +#define SATA_DMA_REG_ADDR 0xe20d0800
These defines should be moved to another (Canyonlands specific) header. It could be that at some time another platform/board will be added using the same DesignWare SATA core but with different addresses.
<snip>
+u8 ata_check_altstatus(struct ata_port *ap) +{
- u8 val = 0;
- val = readb(ap->ioaddr.altstatus_addr);
- return val;
+}
+int check_sata_dev_state(void) +{
- static ccb tempccb; /* temporary scsi command buffer */
- ccb *pccb = (ccb *)&tempccb;
- int ret = 0;
- int i = 0;
- while(1){
Space after "while" please. Please check this file again. Here are multiple places where you don't have a space after the "while" or "if" statement.
udelay (10000); /* 10 ms */pccb->cmd[0] = SCSI_READ10;pccb->cmd[1] = 0;pccb->cmd[2] = 0;pccb->cmd[3] = 0;pccb->cmd[4] = 0;pccb->cmd[5] = 0;pccb->cmd[6] = 0;pccb->cmd[7] = 0;pccb->cmd[8] = 1;pccb->cmdlen = 10;pccb->pdata = &temp_data_buf[0]; /* dummy */pccb->datalen = 512;/* Send Read Command */ret = ata_dev_read_sectors(pccb);/* result TRUE => break */if(ret == 0){
Here again.
break;}
And these curly braces are not needed (because of a single line statement). Please remove them,
i++;if (i > (ATA_RESET_TIME * 100)){printf("** TimeOUT **\n");dev_state = SATA_NODEVICE; /* set device status flag */return FALSE;}if ((i >= 100) && ((i%100)==0)) {printf(".");}
Again, single line statement.
- }
- /* Set device status flag */
- dev_state = SATA_READY;
- return TRUE;
+}
+void ata_id_string(const u16 *id, unsigned char *s,
unsigned int ofs, unsigned int len)+{
- unsigned int c;
- while (len > 0) {
c = id[ofs] >> 8;*s = c;s++;c = id[ofs] & 0xff;*s = c;s++;ofs++;len -= 2;- }
+}
+static int waiting_for_reg_state(volatile u8 *offset,
int timeout_msec,u32 sign)+{
- int i;
- u32 status;
- for (i = 0; i < timeout_msec; i++){
status = readl(offset);if ( ( status & sign ) != 0 ){
This should be:
if ((status & sign) != 0) {
break;}
Remove curly braces.
msleep(1);- }
- return (i < timeout_msec) ? 0 : -1;
+}
+static inline u32 qcmd_tag_to_mask(u8 tag) +{
- return (0x00000001 << (tag & 0x1f));
+}
+static u8 ata_check_status(struct ata_port *ap) +{
- u8 val = 0;
- val = readb(ap->ioaddr.status_addr);
- return val;
+}
+static u8 ata_busy_wait(struct ata_port *ap,
unsigned int bits,unsigned int max)+{
- u8 status;
- do {
udelay(10);status = ata_check_status(ap);max--;- } while (status != 0xff && (status & bits) && (max > 0));
- return status;
+}
+static u8 ata_wait_idle(struct ata_port *ap) +{
- u8 status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
- return status;
+}
+static int sata_dwc_scsiop_inq(ccb *pccb) +{
- struct ata_device *ata_dev = &ata_device;
- u16 *id;
- /* Set IDENTIFY Data */
- id = ata_dev->id;
- /* make INQUIRY header */
- u8 hdr[] = {
0,0,0x5, /* claim SPC-3 version compatibility */2,95 - 4- };
- /* set scsi removeable (RMB) bit per ata bit */
- if (ata_id_removeable(id)){
hdr[1] |= (1 << 7);- }
Single line statement. Please remove the curly braces. There are most likely further places with this issue. I won't comment on them again. Please check the complete file for it.
- memcpy(pccb->pdata, hdr, sizeof(hdr));
- if (pccb->datalen > 35) {
memcpy(&pccb->pdata[8], "ATA ", 8);ata_id_string(id, &pccb->pdata[16], ATA_ID_PROD, 16);ata_id_string(id, &pccb->pdata[32], ATA_ID_FW_REV, 4);if (pccb->pdata[32] == 0 || pccb->pdata[32] == ' '){memcpy(&pccb->pdata[32], "n/a ", 4);}- }
- if (pccb->datalen > 63) {
const u8 versions[] = {0x60, /* SAM-3 (no version claimed) */0x03,0x20, /* SBC-2 (no version claimed) */0x02,0x60 /* SPC-3 (no version claimed) */};memcpy(pccb->pdata + 59, versions, sizeof(versions));- }
- return 0;
+}
+#define ATA_SCSI_PDATA_SET(idx, val) do { \
- if ((idx) < pccb->datalen) pccb->pdata[(idx)] = (u8)(val); \
- } while (0)
+static int sata_dwc_scsiop_read_cap(ccb *pccb) +{
- struct ata_device *ata_dev = &ata_device;
- u32 last_lba;
- last_lba = ata_dev->n_sectors; /* LBA of the last block */
- if (pccb->cmd[0] == SCSI_RD_CAPAC) {
if (last_lba >= 0xffffffffULL){last_lba = 0xffffffff;}/* sector count, 32-bit */ATA_SCSI_PDATA_SET(0, last_lba >> (8 * 3));ATA_SCSI_PDATA_SET(1, last_lba >> (8 * 2));ATA_SCSI_PDATA_SET(2, last_lba >> (8 * 1));ATA_SCSI_PDATA_SET(3, last_lba);/* buffer clear */ATA_SCSI_PDATA_SET(4, 0);ATA_SCSI_PDATA_SET(5, 0);/* sector size */ATA_SCSI_PDATA_SET(6, ATA_SECT_SIZE >> 8);ATA_SCSI_PDATA_SET(7, ATA_SECT_SIZE & 0xff);- } else {
/* sector count, 64-bit */ATA_SCSI_PDATA_SET(0, last_lba >> (8 * 7));ATA_SCSI_PDATA_SET(1, last_lba >> (8 * 6));ATA_SCSI_PDATA_SET(2, last_lba >> (8 * 5));ATA_SCSI_PDATA_SET(3, last_lba >> (8 * 4));ATA_SCSI_PDATA_SET(4, last_lba >> (8 * 3));ATA_SCSI_PDATA_SET(5, last_lba >> (8 * 2));ATA_SCSI_PDATA_SET(6, last_lba >> (8 * 1));ATA_SCSI_PDATA_SET(7, last_lba);/* sector size */ATA_SCSI_PDATA_SET(10, ATA_SECT_SIZE >> 8);ATA_SCSI_PDATA_SET(11, ATA_SECT_SIZE & 0xff);- }
- return 0;
+}
+/*
- SCSI TEST UNIT READY command operation.
No operation. Simply returns success to caller, to indicate
that the caller should successfully complete this SCSI command.- */
+static int sata_dwc_scsiop_test_unit_ready(ccb *pccb) +{
- /* No operation */
- return 0;
+}
+int scsi_exec(ccb *pccb) +{
- int ret;
- /* check device status */
- if(dev_state != SATA_READY){
return FALSE;- }
- switch (pccb->cmd[0]) {
- case SCSI_READ10:
ret = ata_dev_read_sectors(pccb);break;- case SCSI_RD_CAPAC:
ret = sata_dwc_scsiop_read_cap(pccb);break;- case SCSI_TST_U_RDY:
ret = sata_dwc_scsiop_test_unit_ready(pccb);break;- case SCSI_INQUIRY:
ret = sata_dwc_scsiop_inq(pccb);break;- default:
printf("Unsupport SCSI command 0x%02x\n", pccb->cmd[0]);return FALSE;- }
- if (ret) {
debug("SCSI command 0x%02x ret errno %d\n", pccb->cmd[0], ret);return FALSE;- }
- return TRUE;
+}
+static const struct ata_port_info sata_dwc_port_info[] = {
- {
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |ATA_FLAG_MMIO | ATA_FLAG_PIO_POLLING |ATA_FLAG_SRST | ATA_FLAG_NCQ,.pio_mask = 0x1f, /* pio 0-4 */.mwdma_mask = 0x07,.udma_mask = 0x7f,- },
+};
+int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
unsigned int flags, u16 *id)+{
- struct ata_port *ap = pap;
- unsigned int class = *p_class;
- struct ata_taskfile tf;
- unsigned int err_mask = 0;
- const char *reason;
- int may_fallback = 1, tried_spinup = 0;
- u8 status;
- int rc;
- /* cheack BSY = 0 */
- status = ata_busy_wait(ap, ATA_BUSY, 30000);
- if (status & ATA_BUSY) {
printf("BSY = 0 check. timeout.\n");rc = FALSE;return rc;- }
ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */+retry:
- memset(&tf, 0, sizeof(tf));
- ap->print_id = 1;
- ap->flags &= ~ATA_FLAG_DISABLED;
- tf.ctl = ap->ctl; /* 0x08 */
- tf.device = ATA_DEVICE_OBS;
- tf.command = ATA_CMD_ID_ATA;
- tf.protocol = ATA_PROT_PIO;
- /* Some devices choke if TF registers contain garbage. Make
* sure those are properly initialized.*/- tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
- /* Device presence detection is unreliable on some
* controllers. Always poll IDENTIFY if available.*/- tf.flags |= ATA_TFLAG_POLLING;
- temp_n_block = 1;
- err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
sizeof(id[0]) * ATA_ID_WORDS, 0);- if (err_mask) {
if (err_mask & AC_ERR_NODEV_HINT) {printf("NODEV after polling detection\n");return -ENOENT;}if ((err_mask == AC_ERR_DEV) && (tf.feature & ATA_ABORTED)) {/* Device or controller might have reported* the wrong device class. Give a shot at the* other IDENTIFY if the current one is* aborted by the device.*/if (may_fallback) {may_fallback = 0;if (class == ATA_DEV_ATA) {class = ATA_DEV_ATAPI;} else {class = ATA_DEV_ATA;}goto retry;}/* Control reaches here iff the device aborted* both flavors of IDENTIFYs which happens* sometimes with phantom devices.*/printf("both IDENTIFYs aborted, assuming NODEV\n");return -ENOENT;}rc = -EIO;reason = "I/O error";goto err_out;- }
- /* Falling back doesn't make sense if ID data was read
* successfully at least once.*/- may_fallback = 0;
+#ifdef __BIG_ENDIAN
- unsigned int id_cnt;
- for (id_cnt = 0; id_cnt < ATA_ID_WORDS; id_cnt++)
id[id_cnt] = le16_to_cpu(id[id_cnt]);+#endif /* __BIG_ENDIAN */
And what happens if the platform is little-endian? Doesn't the le16_to_cpu() function handle the conversion? I think you can remove the #ifdef and use this code unconditionally.
<big snip>
u-boot-2009.03/drivers/block/sata_dwc.h 1970-01-01 09:00:00.000000000 +0900 +++ u-boot-2009.03-sata/drivers/block/sata_dwc.h 2009-03-25 17:27:01.000000000 +0900 @@ -0,0 +1,496 @@ +/*
- sata_dwc.h
- Synopsys DesignWare Cores (DWC) SATA host driver
- Author: Mark Miesfeld mmiesfeld@amcc.com
- Ported from 2.6.19.2 to 2.6.25/26 by Stefan Roese sr@denx.de
- Copyright 2008 DENX Software Engineering
- Based on versions provided by AMCC and Synopsys which are:
Copyright 2006 Applied Micro Circuits Corporation
COPYRIGHT (C) 2005 SYNOPSYS, INC. ALL RIGHTS RESERVED
- This program is free software; you can redistribute
- it and/or modify it under the terms of the GNU
- General Public License as published by the
- Free Software Foundation; either version 2 of the License,
- or (at your option) any later version.
- */
+#ifndef _SATA_DWC_H_ +#define _SATA_DWC_H_
+#define HZ 100
+#define READ 0 +#define WRITE 1
+static int ata_id_has_hipm(const u16 *id) +{
- u16 val = id[76];
- if (val == 0 || val == 0xffff)
return -1;- return val & (1 << 9);
+}
Don't add code to header. Only inline functions should be added here. So either add "inline" or move it to a ".c" file.
+static int ata_id_has_dipm(const u16 *id) +{
- u16 val = id[78];
- if (val == 0 || val == 0xffff)
return -1;- return val & (1 << 3);
+}
Same here.
<snip>
u-boot-2009.03/include/configs/canyonlands.h 2009-03-22 06:04:41.000000000 +0900 +++ u-boot-2009.03-sata/include/configs/canyonlands.h 2009-03-25 17:31:40.000000000 +0900 @@ -34,6 +34,7 @@ #ifdef CONFIG_CANYONLANDS #define CONFIG_460EX 1 /* Specific PPC460EX */ #define CONFIG_HOSTNAME canyonlands +#define CONFIG_SATA_DWC /* PPC460EX SATA support */
Not sure if we really should enable this per default on the Canyonlands platform. What's the size impact of this SATA support?
#else #define CONFIG_460GT 1 /* Specific PPC460GT */ #ifdef CONFIG_GLACIER @@ -454,6 +455,9 @@ #define CONFIG_CMD_SDRAM #define CONFIG_CMD_SNTP #define CONFIG_CMD_USB +#if defined(CONFIG_SATA_DWC) +#define CONFIG_CMD_SCSI +#endif
Again, I think this should go into CMD_SATA instead of SCSI.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================