[U-Boot] [PATCH v2 6/7] dm: Add support for scsi/sata based devices
Simon Glass
sjg at chromium.org
Wed Nov 30 01:32:32 CET 2016
Hi Michal,
On 25 November 2016 at 08:00, Michal Simek <michal.simek at 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 at xilinx.com>
> ---
>
> Changes in v2:
> - Use CONFIG_DM_SCSI instead of mix of DM_SCSI and DM_SATA
> Ceva sata has never used sata commands that's why keep it in
> SCSI part only.
> - Separate scsi_scan() for DM_SCSI and do not change cmd/scsi.c
> - Extend platdata
>
> common/board_r.c | 4 +--
> common/scsi.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/block/Kconfig | 7 ++++
> drivers/block/Makefile | 1 +
> drivers/block/ahci.c | 30 ++++++++++++-----
> drivers/block/blk-uclass.c | 2 +-
> drivers/block/scsi-uclass.c | 29 ++++++++++++++++
> include/ahci.h | 2 +-
> include/dm/uclass-id.h | 1 +
> include/sata.h | 3 ++
> include/scsi.h | 19 ++++++++++-
> 11 files changed, 166 insertions(+), 13 deletions(-)
> create mode 100644 drivers/block/scsi-uclass.c
This looks really good. I've made some comments below.
>
> diff --git a/common/board_r.c b/common/board_r.c
> index d959ad3c6f90..108cabbd023b 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_SCSI)
> 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_SCSI)
> INIT_FUNC_WATCHDOG_RESET
> initr_scsi,
> #endif
> diff --git a/common/scsi.c b/common/scsi.c
> index 9e4ef54a9be8..bc55512913da 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -7,10 +7,14 @@
>
> #include <common.h>
> #include <dm.h>
> +#if defined(CONFIG_DM_SCSI)
> +#include <dm/uclass-internal.h>
> +#endif
You can drop that #ifdef - also please put the dm/ include at the end,
after scsi.h.
> #include <inttypes.h>
> #include <pci.h>
> #include <scsi.h>
>
> +#if !defined(CONFIG_DM_SCSI)
> #ifdef CONFIG_SCSI_DEV_LIST
> #define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> #else
> @@ -31,6 +35,7 @@
> #endif
> #define SCSI_DEV_LIST {SCSI_VEND_ID, SCSI_DEV_ID}
> #endif
> +#endif
>
> #if defined(CONFIG_PCI) && !defined(CONFIG_SCSI_AHCI_PLAT)
> const struct pci_device_id scsi_device_list[] = { SCSI_DEV_LIST };
> @@ -41,9 +46,11 @@ static unsigned char tempbuff[512]; /* temporary data buffer */
>
> static int scsi_max_devs; /* number of highest available scsi device */
I think this should be bracketed by !CONFIG_DM_SCSI, also.
>
> +#if !defined(CONFIG_DM_SCSI)
> static int scsi_curr_dev; /* current device */
>
> static struct blk_desc scsi_dev_desc[CONFIG_SYS_SCSI_MAX_DEVICE];
> +#endif
>
> /* almost the maximum amount of the scsi_ext command.. */
> #define SCSI_MAX_READ_BLK 0xFFFF
> @@ -555,6 +562,79 @@ removable:
> * (re)-scan the scsi bus and reports scsi device info
> * to the user if mode = 1
> */
> +#if defined(CONFIG_DM_SCSI)
> +void scsi_scan(int mode)
This function can generate errors - can you update it to return 'int'
so you can return them?
> +{
> + unsigned char i, lun;
> + struct uclass *uc;
> + struct udevice *dev; /* SCSI controller */
> + int ret;
> +
> + if (mode == 1)
> + printf("scanning bus for devices...\n");
> +
> + ret = uclass_get(UCLASS_SCSI, &uc);
> + if (ret)
> + return;
> +
> + uclass_foreach_dev(dev, uc) {
> + struct scsi_platdata *plat; /* scsi controller platdata */
> + struct blk_desc *bdesc = NULL; /* block device description */
I don't think you need the = NULL
> + struct udevice *bdev; /* block device */
> + struct udevice **devp = NULL;
You should be able to drop this
> + int dev_num = 0;
> + int last_dev_num = -1;
> +
> + /* probe SCSI controller driver */
> + ret = uclass_get_device_tail(dev, 0, devp);
ret = device_probe(dev);
> + if (ret)
> + return;
> +
> + /* Get controller platdata */
> + plat = dev_get_platdata(dev);
> +
> + for (i = 0; i < plat->max_id; i++) {
> + for (lun = 0; lun < plat->max_lun; lun++) {
> + /*
Can this whole block go in a separate function?
> + * Create only one block device and do detection
> + * to make sure that there won't be a lot of
> + * block devices created
> + */
> + if (last_dev_num != dev_num) {
> + char str[10];
> + snprintf(str, sizeof(str), "lun%d",
> + lun);
> + ret = blk_create_devicef(dev,
> + "scsi_blk",
> + str,
> + IF_TYPE_SCSI,
> + -1, 512,
> + dev_num,
> + &bdev);
> + if (ret) {
> + printf("Can't create device\n");
debug()
> + return;
> + }
> + last_dev_num = dev_num;
Or perhaps set bdev to NULL when it is 'used up'. Then check for NULL
in the if() above.
> + bdesc = dev_get_uclass_platdata(bdev);
Consider moving this down one line, before the scsi_init_dev_desc()
call, so you know it is valid?
> + }
> +
> + scsi_init_dev_desc(bdesc, dev_num);
Actually this is set up by blk_create_device() so you should not
overwrite it. I would much rather that this function be behind
'#ifndef CONFIG_DM_SCSI'. You can update particular fields if needed?
> + bdesc->lun = lun;
> + ret = scsi_detect_dev(i, bdesc);
> + if (ret)
> + continue;
device_unbind() here (or perhaps in the following block), since you
don't want the device hanging around.
Or perhaps you intend to reuse this device for the next one you try?
Which is probably better, I agree. But in that case if you have one
left over that you don't use, it should be unbound when exiting
scsi_scan.
> +
> + if (mode == 1) {
> + printf(" Device %d: ", 0);
> + dev_print(bdesc);
> + dev_num++;
> + } /* if mode */
> + } /* next LUN */
> + }
> + }
> +}
> +#else
> void scsi_scan(int mode)
> {
> unsigned char i, lun;
> @@ -590,6 +670,7 @@ void scsi_scan(int mode)
> setenv_ulong("scsidevs", scsi_max_devs);
> #endif
> }
> +#endif
>
> #ifdef CONFIG_BLK
> static const struct blk_ops scsi_blk_ops = {
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index fe5aa07f921a..55edae71eec3 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -2,6 +2,7 @@ config BLK
> bool "Support block devices"
> depends on DM
> default y if DM_MMC
> + default y if DM_SCSI
> help
> Enable support for block devices, such as SCSI, MMC and USB
> flash sticks. These provide a block-level interface which permits
> @@ -19,6 +20,12 @@ config AHCI
> operations at present. The block device interface has not been converted
> to driver model.
>
> +config DM_SCSI
> + bool "DM for SCSI"
> + depends on DM
&& BLK ?
Or perhaps 'select BLK' ? We should not allow DM_SCSI without BLK.
> + help
> + This option enables the SCSI uclass.
Can you please add more detail here? Write out SCSI in full also and
mention what SCSI is used for.
> +
> config BLOCK_CACHE
> bool "Use block device cache"
> default n
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index 436b79f98165..a72feecd5456 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -12,6 +12,7 @@ obj-y += blk_legacy.o
> endif
>
> obj-$(CONFIG_AHCI) += ahci-uclass.o
> +obj-$(CONFIG_DM_SCSI) += scsi-uclass.o
> obj-$(CONFIG_SCSI_AHCI) += ahci.o
> obj-$(CONFIG_DWC_AHSATA) += dwc_ahsata.o
> obj-$(CONFIG_FSL_SATA) += fsl_sata.o
> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
> index 5139989d0b5f..0dd4a2ea5814 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_SCSI)
> # 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_SCSI)
> # 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_SCSI)
> #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_SCSI)
> +# if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SCSI)
> 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_SCSI)
> 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_SCSI)
> 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_SCSI)
> #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_SCSI)
> +void scsi_low_level_init(int busdevfunc, struct udevice *dev)
Do we need this function for DM? At some point the first param should
go away, since if we are using PCI (device_is_on_pci_bus()) we can use
dm_pci_get_bdf().
> +#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_SCSI)
> + ahci_init_one(dev);
> # else
> ahci_init_one(busdevfunc);
> # endif
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 2e041c2b3dc7..38cb9388da6f 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -26,7 +26,7 @@ static const char *if_typename_str[IF_TYPE_COUNT] = {
>
> static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = {
> [IF_TYPE_IDE] = UCLASS_INVALID,
> - [IF_TYPE_SCSI] = UCLASS_INVALID,
> + [IF_TYPE_SCSI] = UCLASS_SCSI,
> [IF_TYPE_ATAPI] = UCLASS_INVALID,
> [IF_TYPE_USB] = UCLASS_MASS_STORAGE,
> [IF_TYPE_DOC] = UCLASS_INVALID,
> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
> new file mode 100644
> index 000000000000..123b25c406a4
> --- /dev/null
> +++ b/drivers/block/scsi-uclass.c
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2015 Google, Inc
> + * Written by Simon Glass <sjg at chromium.org>
> + * Copyright (c) 2016 Xilinx, Inc
> + * Written by Michal Simek
> + *
> + * Based on ahci-uclass.c
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <scsi.h>
> +
> +#if !defined(CONFIG_SPL_BUILD)
> +static int scsi_post_probe(struct udevice *dev)
> +{
> + debug("%s: device %p\n", __func__, dev);
> + scsi_low_level_init(0, dev);
> + return 0;
> +}
> +
> +UCLASS_DRIVER(scsi) = {
> + .id = UCLASS_SCSI,
> + .name = "scsi",
> + .post_probe = scsi_post_probe,
Should this be pre_probe(). Even if we need the low-level init, I
suspect it needs to be done before the device is used in its probe()
method.
> +};
> +#endif
> diff --git a/include/ahci.h b/include/ahci.h
> index a956c6ff5df7..4876b41e9010 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_SCSI)
> struct udevice *dev;
> #else
> pci_dev_t dev;
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index eb78c4dac485..8c92d0b03088 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -66,6 +66,7 @@ enum uclass_id {
> UCLASS_REMOTEPROC, /* Remote Processor device */
> UCLASS_RESET, /* Reset controller device */
> UCLASS_RTC, /* Real time clock device */
> + UCLASS_SCSI, /* SCSI device */
> UCLASS_SERIAL, /* Serial UART */
> UCLASS_SPI, /* SPI bus */
> UCLASS_SPMI, /* System Power Management Interface bus */
> diff --git a/include/sata.h b/include/sata.h
> index b35359aa5a19..dbf77f8c7039 100644
> --- a/include/sata.h
> +++ b/include/sata.h
> @@ -2,6 +2,8 @@
> #define __SATA_H__
> #include <part.h>
>
> +#if !defined(CONFIG_DM_SCSI)
> +
> 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..7396994b91fc 100644
> --- a/include/scsi.h
> +++ b/include/scsi.h
> @@ -166,8 +166,11 @@ 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_SCSI)
> void scsi_low_level_init(int busdevfunc);
> -
> +#else
> +void scsi_low_level_init(int busdevfunc, struct udevice *dev);
> +#endif
>
> /***************************************************************************
> * functions residing inside cmd_scsi.c
> @@ -175,6 +178,20 @@ void scsi_low_level_init(int busdevfunc);
> void scsi_init(void);
> void scsi_scan(int mode);
>
> +#if defined(CONFIG_DM_SCSI)
> +/**
> + * struct scsi_platdata - stores information about SCSI controller
> + *
@base
> + * @max_lun: Maximum number of logical units
> + * @max_id: Maximum number of target ids
> + */
> +struct scsi_platdata {
> + unsigned long base;
> + unsigned long max_lun;
> + unsigned long max_id;
> +};
> +#endif
> +
> /** @return the number of scsi disks */
> int scsi_get_disk_count(void);
>
> --
> 1.9.1
>
Also we should have a sandbox SCSI driver for tests.
Regards,
SImon
More information about the U-Boot
mailing list