Re: [U-Boot] [PATCH v3 1/2] dm: Add support for scsi/sata based devices
On 1.12.2016 03:20, Simon Glass wrote:
> Hi Michal,
>
> On 30 November 2016 at 13:48, Michal Simek 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
>> ---
>>
>> Changes in v3:
>> - Fix scsi_scan return path
>> - Fix header location uclass-internal.h
>> - Add scsi_max_devs under !DM_SCSI
>> - Add new header device-internal because of device_probe()
>> - Redesign block device creation algorithm
>> - Use device_unbind in error path
>> - Create block device with id and lun numbers (lun was there in v2)
>> - Cleanup dev_num initialization in block device description
>> with fixing parameters in blk_create_devicef
>> - Create new Kconfig menu for SATA/SCSI drivers
>> - Extend description for DM_SCSI
>> - Fix Kconfig dependencies
>> - Fix kernel doc format in scsi_platdata
>> - Fix ahci_init_one - vendor variable
>>
>> 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 | 76
>> +
>> drivers/block/Kconfig | 12 +++
>> 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 | 2 ++
>> include/scsi.h | 20 +++-
>> 11 files changed, 166 insertions(+), 13 deletions(-)
>> create mode 100644 drivers/block/scsi-uclass.c
>
> Reviewed-by: Simon Glass
>
> With a few nits below.
>
> Also, as mentioned we do need a test (in subsequent patches). For an
> example that is fairly close see:
>
> test/dm/mmc.c
> drivers/mmc/sandbox_mmc.c
>
> There is a sandbox_scsi.c but it is empty because there is no
> interface to the SCSI driver. From what I can tell your SCSI uclass
> needs to have an operations struct (perhaps with just an 'exec'
> method). The SCSI uclass can then implement the scsi_exec() call by
> calling this operation. Does that sound right?
>
>>
>> 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 04add624958f..e7efa5ae797c 100644
>> --- a/common/scsi.c
>> +++ b/common/scsi.c
>> @@ -10,7 +10,10 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>>
>> +#if !defined(CONFIG_DM_SCSI)
>> #ifdef CONFIG_SCSI_DEV_LIST
>> #define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
>> #else
>> @@ -31,6 +34,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 };
>> @@ -39,11 +43,13 @@ static ccb tempccb; /* temporary scsi command buffer */
>>
>> static unsigned char tempbuff[512]; /* temporary data buffer */
>>
>> +#if !defined(CONFIG_DM_SCSI)
>> static int scsi_max_devs; /* number of highest available scsi device */
>>
>> 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 0x
>> @@ -444,6 +450,7 @@ static void scsi_init_dev_desc_priv(struct blk_desc
>> *dev_desc)
>> #endif
>> }
>>
>> +
Re: [U-Boot] [PATCH v3 1/2] dm: Add support for scsi/sata based devices
Hi Michal,
On 30 November 2016 at 13:48, Michal Simek 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
> ---
>
> Changes in v3:
> - Fix scsi_scan return path
> - Fix header location uclass-internal.h
> - Add scsi_max_devs under !DM_SCSI
> - Add new header device-internal because of device_probe()
> - Redesign block device creation algorithm
> - Use device_unbind in error path
> - Create block device with id and lun numbers (lun was there in v2)
> - Cleanup dev_num initialization in block device description
> with fixing parameters in blk_create_devicef
> - Create new Kconfig menu for SATA/SCSI drivers
> - Extend description for DM_SCSI
> - Fix Kconfig dependencies
> - Fix kernel doc format in scsi_platdata
> - Fix ahci_init_one - vendor variable
>
> 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 | 76
> +
> drivers/block/Kconfig | 12 +++
> 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 | 2 ++
> include/scsi.h | 20 +++-
> 11 files changed, 166 insertions(+), 13 deletions(-)
> create mode 100644 drivers/block/scsi-uclass.c
Reviewed-by: Simon Glass
With a few nits below.
Also, as mentioned we do need a test (in subsequent patches). For an
example that is fairly close see:
test/dm/mmc.c
drivers/mmc/sandbox_mmc.c
There is a sandbox_scsi.c but it is empty because there is no
interface to the SCSI driver. From what I can tell your SCSI uclass
needs to have an operations struct (perhaps with just an 'exec'
method). The SCSI uclass can then implement the scsi_exec() call by
calling this operation. Does that sound right?
>
> 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 04add624958f..e7efa5ae797c 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -10,7 +10,10 @@
> #include
> #include
> #include
> +#include
> +#include
>
> +#if !defined(CONFIG_DM_SCSI)
> #ifdef CONFIG_SCSI_DEV_LIST
> #define SCSI_DEV_LIST CONFIG_SCSI_DEV_LIST
> #else
> @@ -31,6 +34,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 };
> @@ -39,11 +43,13 @@ static ccb tempccb; /* temporary scsi command buffer */
>
> static unsigned char tempbuff[512]; /* temporary data buffer */
>
> +#if !defined(CONFIG_DM_SCSI)
> static int scsi_max_devs; /* number of highest available scsi device */
>
> 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 0x
> @@ -444,6 +450,7 @@ static void scsi_init_dev_desc_priv(struct blk_desc
> *dev_desc)
> #endif
> }
>
> +#if !defined(CONFIG_DM_SCSI)
> /**
> * scsi_init_dev_desc - initialize all SCSI specific blk_desc properties
> *
> @@ -460,6 +467,7 @@ static void scsi_init_dev_desc(struct blk_desc *dev_desc,
> int
