Hi Michal, On 26 September 2016 at 05:06, Michal Simek <michal.si...@xilinx.com> wrote: > 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.
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 <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. 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot