[U-Boot] [RFC PATCH 1/2] dm: Add support for scsi/sata based devices
Michal Simek
michal.simek at xilinx.com
Mon Sep 26 13:06:25 CEST 2016
On 24.9.2016 19:26, Simon Glass wrote:
> Hi Michal,
>
> On 8 September 2016 at 07:57, 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>
>> ---
>>
>> 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 <sjg at 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
More information about the U-Boot
mailing list