Hi Simon,
On 27.9.2016 02:35, Simon Glass wrote:
> 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.
will look at it.
>
>>
>>
>>> 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.
Some new functions need to be created to extract code and don't copy it
everywhere.
>
>>
>>>
>>>> +#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.
Not in this patch but in that functions it is there.
{
#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;
uintptr_t buf_addr;
unsigned short smallblks = 0;
ccb *pccb = (ccb *)&tempccb;
device &= 0xff;
/* Setup device */
pccb->target = scsi_dev_desc[device].target;
pccb->lun = scsi_dev_desc[device].lun;
buf_addr = (unsigned long)buffer;
start = blknr;
>>
>>>
>>>> #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?
Maybe will see when I have a time to look at this.
>>>
>>>> -
>>>> +#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
Thanks,
Michal
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
