On 24.9.2016 19:26, Simon Glass wrote: > Hi Michal, > > On 8 September 2016 at 07:57, Michal Simek <michal.si...@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.si...@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. > 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. > >> +#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. > >> #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 <s...@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. > >> - >> +#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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot