[U-Boot] [RFC PATCH 1/2] dm: Add support for scsi/sata based devices
Simon Glass
sjg at chromium.org
Tue Sep 27 02:35:35 CEST 2016
Hi Michal,
On 26 September 2016 at 05:06, Michal Simek <michal.simek at 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 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.
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 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.
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
More information about the U-Boot
mailing list