Hi Michal,
On 26 September 2016 at 05:06, Michal Simek michal.simek@xilinx.com wrote:
On 24.9.2016 19:26, Simon Glass wrote:
Hi Michal,
On 8 September 2016 at 07:57, Michal Simek michal.simek@xilinx.com wrote:
All sata based drivers are bind and corresponding block device is created. Based on this find_scsi_device() is able to get back block device based on scsi_curr_dev pointer.
intr_scsi() is commented now but it can be replaced by calling find_scsi_device() and scsi_scan().
scsi_dev_desc[] is commented out but common/scsi.c heavily depends on it. That's why CONFIG_SYS_SCSI_MAX_DEVICE is hardcoded to 1 and symbol is reassigned to a block description allocated by uclass. There is only one block description by device now but it doesn't need to be correct when more devices are present.
scsi_bind() ensures corresponding block device creation. uclass post_probe (scsi_post_probe()) is doing low level init.
SCSI/SATA DM based drivers requires to have 64bit base address as the first entry in platform data structure to setup mmio_base.
Signed-off-by: Michal Simek michal.simek@xilinx.com
cmd/scsi.c | 38 ++++++++++++++++++++++++++++++++++++++ common/board_r.c | 4 ++-- common/scsi.c | 17 ++++++++++++++++- drivers/block/ahci-uclass.c | 38 ++++++++++++++++++++++++++++++++++++++ drivers/block/ahci.c | 30 ++++++++++++++++++++++-------- include/ahci.h | 2 +- include/sata.h | 3 +++ include/scsi.h | 15 ++++++++++++++- 8 files changed, 134 insertions(+), 13 deletions(-)
Thanks for looking at this. I've taken a look and have a few comments.
It's confusing that you are changing both scsi and sata. Do you need to add a DM_SCSI also? As far as I can see, they are separate subsystems.
TBH I am confused with that too. This is ceva sata driver but we use scsi subsystem to work with it. From my look sata is mostly copied from scsi but I don't know history of it. I will look at using just one interface - sata or scsi to see how this will look like.
Here is my understanding. I may have it wrong.
SCSI should be a uclass SATA should be a uclass (called AHCI)
SCSI provide a library of functions which can be called by SCSI or SATA code. This is distinct from the uclass so really should be in some sort of common file.
I think you need a uclass which implements the scsi_scan() function. The existing code could be refactored so that the common parts are called from both scsi.c and your scsi-uclass.c. It should look for devices, and then create a block device for each. Since you don't know how many block devices to create, I don't think you can avoid creating them 'on the fly' in scsi_scan(). For an example, see usb_stor_probe_device().
Will look.
Also we will need a sandbox device at some point so we can run tests.
This can be added later.
Minor point - please put #idef CONFIG_DM_SATA first and the legacy path in the #else cause. Mostly you do this but in a few cases it is not consistent.
ok. Will look at it.
A few more notes below.
diff --git a/cmd/scsi.c b/cmd/scsi.c index 387ca1a262ab..dc1176610672 100644 --- a/cmd/scsi.c +++ b/cmd/scsi.c @@ -10,6 +10,7 @@ */ #include <common.h> #include <command.h> +#include <dm.h> #include <scsi.h>
static int scsi_curr_dev; /* current device */ @@ -25,6 +26,23 @@ int do_scsiboot(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) /*
- scsi command intepreter
*/ +#ifdef CONFIG_DM_SATA +struct udevice *find_scsi_device(int dev_num) +{
struct udevice *bdev;int ret;ret = blk_get_device(IF_TYPE_SCSI, dev_num, &bdev);if (ret) {printf("SCSI Device %d not found\n", dev_num);return NULL;}return bdev;+} +#endif
int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) { switch (argc) { @@ -35,7 +53,18 @@ int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) if (strncmp(argv[1], "res", 3) == 0) { printf("\nReset SCSI\n"); scsi_bus_reset();
+#if defined(CONFIG_DM_SATA)
struct udevice *bdev;bdev = find_scsi_device(scsi_curr_dev);if (!bdev)return CMD_RET_FAILURE;scsi_scan(1, bdev);+#else scsi_scan(1); +#endif return 0; } if (strncmp(argv[1], "inf", 3) == 0) { @@ -51,7 +80,16 @@ int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) return 0; } if (strncmp(argv[1], "scan", 4) == 0) { +#if defined(CONFIG_DM_SATA)
struct udevice *bdev;bdev = find_scsi_device(scsi_curr_dev);if (!bdev)return CMD_RET_FAILURE;scsi_scan(1, bdev);+#else scsi_scan(1); +#endif return 0; } if (strncmp(argv[1], "part", 4) == 0) { diff --git a/common/board_r.c b/common/board_r.c index d959ad3c6f90..f3ea457507de 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -620,7 +620,7 @@ static int initr_ambapp_print(void) } #endif
-#if defined(CONFIG_SCSI) +#if defined(CONFIG_SCSI) && !defined(CONFIG_DM_SATA) static int initr_scsi(void) { puts("SCSI: "); @@ -923,7 +923,7 @@ init_fnc_t init_sequence_r[] = { initr_ambapp_print, #endif #endif -#ifdef CONFIG_SCSI +#if defined(CONFIG_SCSI) && !defined(CONFIG_DM_SATA) INIT_FUNC_WATCHDOG_RESET initr_scsi, #endif diff --git a/common/scsi.c b/common/scsi.c index dbbf4043b22a..1726898b06e2 100644 --- a/common/scsi.c +++ b/common/scsi.c @@ -26,7 +26,7 @@ #define SCSI_VEND_ID 0x10b9 #define SCSI_DEV_ID 0x5288
-#elif !defined(CONFIG_SCSI_AHCI_PLAT) +#elif !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA) #error no scsi device defined #endif #define SCSI_DEV_LIST {SCSI_VEND_ID, SCSI_DEV_ID} @@ -43,7 +43,13 @@ static int scsi_max_devs; /* number of highest available scsi device */
static int scsi_curr_dev; /* current device */
+#if !defined(CONFIG_DM_SATA) static struct blk_desc scsi_dev_desc[CONFIG_SYS_SCSI_MAX_DEVICE]; +#else +#undef CONFIG_SYS_SCSI_MAX_DEVICE +#define CONFIG_SYS_SCSI_MAX_DEVICE 1 +#define CONFIG_SYS_SCSI_MAX_LUN 1
Do we need these with driver model, or can we scan?
It is mostly because I didn't want to create another copy of the same functions. We are allocating all stuff on fly that's why we are working with one device at time.
I'd rather not use these options if they are not useful.
+#endif
/* almost the maximum amount of the scsi_ext command.. */ #define SCSI_MAX_READ_BLK 0xFFFF @@ -160,6 +166,7 @@ static ulong scsi_read(struct blk_desc *block_dev, lbaint_t blknr, { #ifdef CONFIG_BLK struct blk_desc *block_dev = dev_get_uclass_platdata(dev);
struct blk_desc *scsi_dev_desc = block_dev;Is this actually used?
yes a lot. It is pointer to device description. For non DM case it is statically described based on selected number of devices.
But I can't see scsi_dev_desc in your function.
#endif int device = block_dev->devnum; lbaint_t start, blks; @@ -235,6 +242,7 @@ static ulong scsi_write(struct blk_desc *block_dev, lbaint_t blknr, { #ifdef CONFIG_BLK struct blk_desc *block_dev = dev_get_uclass_platdata(dev);
struct blk_desc *scsi_dev_desc = block_dev;#endif int device = block_dev->devnum; lbaint_t start, blks; @@ -462,13 +470,20 @@ void scsi_setup_test_unit_ready(ccb *pccb)
- (re)-scan the scsi bus and reports scsi device info
- to the user if mode = 1
*/ +#ifdef CONFIG_DM_SATA +void scsi_scan(int mode, struct udevice *bdev) +#else void scsi_scan(int mode) +#endif { unsigned char i, perq, modi, lun; lbaint_t capacity; unsigned long blksz; ccb *pccb = (ccb *)&tempccb;
+#if defined(CONFIG_DM_SATA)
struct blk_desc *scsi_dev_desc = dev_get_uclass_platdata(bdev);+#endif if (mode == 1) printf("scanning bus for devices...\n"); for (i = 0; i < CONFIG_SYS_SCSI_MAX_DEVICE; i++) { diff --git a/drivers/block/ahci-uclass.c b/drivers/block/ahci-uclass.c index 7b8c32699f53..f25b1bd336ae 100644 --- a/drivers/block/ahci-uclass.c +++ b/drivers/block/ahci-uclass.c @@ -1,14 +1,52 @@ /*
- Copyright (c) 2015 Google, Inc
- Written by Simon Glass sjg@chromium.org
- Copyright (c) 2016 Xilinx, Inc
*/
- Written by Michal Simek
- SPDX-License-Identifier: GPL-2.0+
#include <common.h> #include <dm.h> +#include <scsi.h>
+#if defined(CONFIG_DM_SATA) +int scsi_bind(struct udevice *dev) +{
struct udevice *bdev;struct blk_desc *bdesc;int ret;/** FIXME - maybe loop over CONFIG_SYS_SCSI_MAX_SCSI_ID* here to support more ports*/ret = blk_create_devicef(dev, "scsi_blk", "blk", IF_TYPE_SCSI, -1, 512,0, &bdev);if (ret) {printf("Cannot create block device\n");return ret;}bdesc = dev_get_uclass_platdata(bdev);debug("device %p, block device %p, block description %p\n",dev, bdev, bdesc);return 0;+}
+static int scsi_post_probe(struct udevice *dev) +{
debug("%s: device %p\n", __func__, dev);scsi_low_level_init(0, dev);return 0;+} +#endif
UCLASS_DRIVER(ahci) = { .id = UCLASS_AHCI, .name = "ahci", +#if defined(CONFIG_DM_SATA)
.post_probe = scsi_post_probe,+#endif }; diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c index e3e783a74cfd..03a95179eaa4 100644 --- a/drivers/block/ahci.c +++ b/drivers/block/ahci.c @@ -168,7 +168,7 @@ int ahci_reset(void __iomem *base)
static int ahci_host_init(struct ahci_probe_ent *probe_ent) { -#ifndef CONFIG_SCSI_AHCI_PLAT +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA) # ifdef CONFIG_DM_PCI struct udevice *dev = probe_ent->dev; struct pci_child_platdata *pplat = dev_get_parent_platdata(dev); @@ -198,7 +198,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);
-#ifndef CONFIG_SCSI_AHCI_PLAT +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA) # ifdef CONFIG_DM_PCI if (pplat->vendor == PCI_VENDOR_ID_INTEL) { u16 tmp16; @@ -327,6 +327,7 @@ 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); +#if !defined(CONFIG_DM_SATA) #ifndef CONFIG_SCSI_AHCI_PLAT # ifdef CONFIG_DM_PCI dm_pci_read_config16(dev, PCI_COMMAND, &tmp16); @@ -338,14 +339,15 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent) pci_write_config_word(pdev, PCI_COMMAND, tmp16); # endif #endif +#endif return 0; }
static void ahci_print_info(struct ahci_probe_ent *probe_ent) { -#ifndef CONFIG_SCSI_AHCI_PLAT -# ifdef CONFIG_DM_PCI +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA) +# if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA) struct udevice *dev = probe_ent->dev; # else pci_dev_t pdev = probe_ent->dev; @@ -372,7 +374,7 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) else speed_s = "?";
-#ifdef CONFIG_SCSI_AHCI_PLAT +#if defined(CONFIG_SCSI_AHCI_PLAT) || defined(CONFIG_DM_SATA) scc_s = "SATA"; #else # ifdef CONFIG_DM_PCI @@ -424,13 +426,15 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent) }
#ifndef CONFIG_SCSI_AHCI_PLAT -# ifdef CONFIG_DM_PCI +# if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA) static int ahci_init_one(struct udevice *dev) # else static int ahci_init_one(pci_dev_t dev) # endif { +#if defined(CONFIG_DM_PCI) u16 vendor; +#endif int rc;
probe_ent = malloc(sizeof(struct ahci_probe_ent));@@ -450,6 +454,7 @@ static int ahci_init_one(pci_dev_t dev) probe_ent->pio_mask = 0x1f; probe_ent->udma_mask = 0x7f; /*Fixme,assume to support UDMA6 */
+#if !defined(CONFIG_DM_SATA) #ifdef CONFIG_DM_PCI probe_ent->mmio_base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_5, PCI_REGION_MEM); @@ -473,6 +478,10 @@ static int ahci_init_one(pci_dev_t dev) if (vendor == 0x197b) pci_write_config_byte(dev, 0x41, 0xa1); #endif +#else
struct scsi_platdata *plat = dev_get_platdata(dev);probe_ent->mmio_base = (void *)plat->base;+#endif
debug("ahci mmio_base=0x%p\n", probe_ent->mmio_base); /* initialize adapter */@@ -954,14 +963,17 @@ int scsi_exec(ccb *pccb)
}
+#if defined(CONFIG_DM_SATA) +void scsi_low_level_init(int busdevfunc, struct udevice *dev) +#else void scsi_low_level_init(int busdevfunc) +#endif { int i; u32 linkmap;
#ifndef CONFIG_SCSI_AHCI_PLAT -# ifdef CONFIG_DM_PCI +# if defined(CONFIG_DM_PCI) struct udevice *dev; int ret;
@@ -969,6 +981,8 @@ void scsi_low_level_init(int busdevfunc) if (ret) return; ahci_init_one(dev); +# elif defined(CONFIG_DM_SATA)
ahci_init_one(dev);# else ahci_init_one(busdevfunc); # endif diff --git a/include/ahci.h b/include/ahci.h index a956c6ff5df7..e740273de804 100644 --- a/include/ahci.h +++ b/include/ahci.h @@ -145,7 +145,7 @@ struct ahci_ioports { };
struct ahci_probe_ent { -#ifdef CONFIG_DM_PCI +#if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA) struct udevice *dev; #else pci_dev_t dev; diff --git a/include/sata.h b/include/sata.h index b35359aa5a19..26c8de730399 100644 --- a/include/sata.h +++ b/include/sata.h @@ -2,6 +2,8 @@ #define __SATA_H__ #include <part.h>
+#if !defined(CONFIG_DM_SATA)
int init_sata(int dev); int reset_sata(int dev); int scan_sata(int dev); @@ -15,5 +17,6 @@ int __sata_stop(void); int sata_port_status(int dev, int port);
extern struct blk_desc sata_dev_desc[]; +#endif
#endif diff --git a/include/scsi.h b/include/scsi.h index 7e3759140b34..22d6bd0d02a7 100644 --- a/include/scsi.h +++ b/include/scsi.h @@ -166,14 +166,27 @@ typedef struct SCSI_cmd_block{ void scsi_print_error(ccb *pccb); int scsi_exec(ccb *pccb); void scsi_bus_reset(void); +#if !defined(CONFIG_DM_SATA) void scsi_low_level_init(int busdevfunc);
What will happen to these functions?
PCI is using dm_pci_bus_find_bdf() to find out udevice. I know it from post_probe function that's why I don't need to look for it. Not sure why PCI is using this function.
I think you might need to do DM_SCSI first to separate the SCSI code from the SCSI helper functions (the ones that deal with SCSI messages). Then DM_SATA after that?
+#else +void scsi_low_level_init(int busdevfunc, struct udevice *dev); +#endif
/***************************************************************************
- functions residing inside cmd_scsi.c
*/ void scsi_init(void); +#if !defined(CONFIG_DM_SATA) void scsi_scan(int mode); +#else
+struct scsi_platdata {
unsigned long base;+};
+void scsi_scan(int mode, struct udevice *bdev); +int scsi_bind(struct udevice *dev); +#endif
/** @return the number of scsi disks */ int scsi_get_disk_count(void); -- 1.9.1
Regards, Simon
Thanks, Michal
Regards, Simon