* [U-Boot] [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 2/7] scsi: add children devices binding make at marvell.com
` (6 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- The members in scsi_platdata(base, max_lun and max_id) are generic,
so now they are taken from fdt by the uclass_platdata instead of
platdata code upon call to post bind callback.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
---
common/scsi.c | 2 +-
drivers/block/ahci.c | 2 +-
drivers/block/scsi-uclass.c | 29 +++++++++++++++++++++++++++++
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/common/scsi.c b/common/scsi.c
index fb5b407..117c682 100644
--- a/common/scsi.c
+++ b/common/scsi.c
@@ -574,7 +574,7 @@ int scsi_scan(int mode)
return ret;
/* Get controller platdata */
- plat = dev_get_platdata(dev);
+ plat = dev_get_uclass_platdata(dev);
for (i = 0; i < plat->max_id; i++) {
for (lun = 0; lun < plat->max_lun; lun++) {
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index 3fa14a7..368816e 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -479,7 +479,7 @@ static int ahci_init_one(pci_dev_t dev)
pci_write_config_byte(dev, 0x41, 0xa1);
#endif
#else
- struct scsi_platdata *plat = dev_get_platdata(dev);
+ struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
probe_ent->mmio_base = (void *)plat->base;
#endif
diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
index 05da6cd..3bf026b 100644
--- a/drivers/block/scsi-uclass.c
+++ b/drivers/block/scsi-uclass.c
@@ -11,8 +11,11 @@
#include <common.h>
#include <dm.h>
+#include <dm/device-internal.h>
#include <scsi.h>
+DECLARE_GLOBAL_DATA_PTR;
+
static int scsi_post_probe(struct udevice *dev)
{
debug("%s: device %p\n", __func__, dev);
@@ -20,8 +23,34 @@ static int scsi_post_probe(struct udevice *dev)
return 0;
}
+static void scsi_ofdata_to_uclass_platdata(struct udevice *dev)
+{
+ struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
+ const void *blob = gd->fdt_blob;
+ int node = dev->of_offset;
+
+ plat->base = (unsigned long)dev_get_addr_ptr(dev);
+ plat->max_id = fdtdec_get_uint(blob,
+ node,
+ "max-id",
+ CONFIG_SYS_SCSI_MAX_SCSI_ID);
+ plat->max_lun = fdtdec_get_uint(blob,
+ node,
+ "max-lun",
+ CONFIG_SYS_SCSI_MAX_LUN);
+ return;
+}
+
+static int scsi_post_bind(struct udevice *dev)
+{
+ /* Get uclass plat data from fdt */
+ scsi_ofdata_to_uclass_platdata(dev);
+}
+
UCLASS_DRIVER(scsi) = {
.id = UCLASS_SCSI,
.name = "scsi",
+ .post_bind = scsi_post_bind,
.post_probe = scsi_post_probe,
+ .per_device_platdata_auto_alloc_size = sizeof(struct scsi_platdata),
};
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
2017-03-23 9:29 ` [U-Boot] [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
2017-04-05 8:38 ` [U-Boot] [EXT] " Ken Ma
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - The members in scsi_platdata(base, max_lun and max_id) are generic,
> so now they are taken from fdt by the uclass_platdata instead of
> platdata code upon call to post bind callback.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> common/scsi.c | 2 +-
> drivers/block/ahci.c | 2 +-
> drivers/block/scsi-uclass.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/common/scsi.c b/common/scsi.c
> index fb5b407..117c682 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -574,7 +574,7 @@ int scsi_scan(int mode)
> return ret;
>
> /* Get controller platdata */
> - plat = dev_get_platdata(dev);
> + plat = dev_get_uclass_platdata(dev);
>
> for (i = 0; i < plat->max_id; i++) {
> for (lun = 0; lun < plat->max_lun; lun++) {
> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
> index 3fa14a7..368816e 100644
> --- a/drivers/block/ahci.c
> +++ b/drivers/block/ahci.c
> @@ -479,7 +479,7 @@ static int ahci_init_one(pci_dev_t dev)
> pci_write_config_byte(dev, 0x41, 0xa1);
> #endif
> #else
> - struct scsi_platdata *plat = dev_get_platdata(dev);
> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
> probe_ent->mmio_base = (void *)plat->base;
> #endif
>
> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
> index 05da6cd..3bf026b 100644
> --- a/drivers/block/scsi-uclass.c
> +++ b/drivers/block/scsi-uclass.c
> @@ -11,8 +11,11 @@
>
> #include <common.h>
> #include <dm.h>
> +#include <dm/device-internal.h>
> #include <scsi.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> static int scsi_post_probe(struct udevice *dev)
> {
> debug("%s: device %p\n", __func__, dev);
> @@ -20,8 +23,34 @@ static int scsi_post_probe(struct udevice *dev)
> return 0;
> }
>
> +static void scsi_ofdata_to_uclass_platdata(struct udevice *dev)
Please make this return an int since functions that decode the DT
should return an error code.
> +{
> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
> + const void *blob = gd->fdt_blob;
> + int node = dev->of_offset;
> +
> + plat->base = (unsigned long)dev_get_addr_ptr(dev);
> + plat->max_id = fdtdec_get_uint(blob,
> + node,
> + "max-id",
> + CONFIG_SYS_SCSI_MAX_SCSI_ID);
> + plat->max_lun = fdtdec_get_uint(blob,
> + node,
> + "max-lun",
> + CONFIG_SYS_SCSI_MAX_LUN);
> + return;
return 0
> +}
> +
> +static int scsi_post_bind(struct udevice *dev)
> +{
> + /* Get uclass plat data from fdt */
> + scsi_ofdata_to_uclass_platdata(dev);
Do we need to have this info in bind(), or could it wait until of_to_platdata()?
Also, return the error code here.
> +}
> +
> UCLASS_DRIVER(scsi) = {
> .id = UCLASS_SCSI,
> .name = "scsi",
> + .post_bind = scsi_post_bind,
> .post_probe = scsi_post_probe,
> + .per_device_platdata_auto_alloc_size = sizeof(struct scsi_platdata),
> };
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
2017-04-01 4:21 ` Simon Glass
@ 2017-04-05 8:38 ` Ken Ma
2017-04-09 19:27 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-04-05 8:38 UTC (permalink / raw)
To: u-boot
Hi Simon
Please see my inline reply, thanks a lot!
-----Original Message-----
From: sjg@google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: 2017年4月1日 12:22
To: Ken Ma
Cc: U-Boot Mailing List; Stefan Roese; Michal Simek
Subject: [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
External Email
----------------------------------------------------------------------
Hi Ken,
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - The members in scsi_platdata(base, max_lun and max_id) are generic,
> so now they are taken from fdt by the uclass_platdata instead of
> platdata code upon call to post bind callback.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> common/scsi.c | 2 +-
> drivers/block/ahci.c | 2 +-
> drivers/block/scsi-uclass.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/common/scsi.c b/common/scsi.c index fb5b407..117c682
> 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -574,7 +574,7 @@ int scsi_scan(int mode)
> return ret;
>
> /* Get controller platdata */
> - plat = dev_get_platdata(dev);
> + plat = dev_get_uclass_platdata(dev);
>
> for (i = 0; i < plat->max_id; i++) {
> for (lun = 0; lun < plat->max_lun; lun++) {
> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c index
> 3fa14a7..368816e 100644
> --- a/drivers/block/ahci.c
> +++ b/drivers/block/ahci.c
> @@ -479,7 +479,7 @@ static int ahci_init_one(pci_dev_t dev)
> pci_write_config_byte(dev, 0x41, 0xa1); #endif #else
> - struct scsi_platdata *plat = dev_get_platdata(dev);
> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
> probe_ent->mmio_base = (void *)plat->base; #endif
>
> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
> index 05da6cd..3bf026b 100644
> --- a/drivers/block/scsi-uclass.c
> +++ b/drivers/block/scsi-uclass.c
> @@ -11,8 +11,11 @@
>
> #include <common.h>
> #include <dm.h>
> +#include <dm/device-internal.h>
> #include <scsi.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> static int scsi_post_probe(struct udevice *dev) {
> debug("%s: device %p\n", __func__, dev); @@ -20,8 +23,34 @@
> static int scsi_post_probe(struct udevice *dev)
> return 0;
> }
>
> +static void scsi_ofdata_to_uclass_platdata(struct udevice *dev)
Please make this return an int since functions that decode the DT should return an error code.
> +{
> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
> + const void *blob = gd->fdt_blob;
> + int node = dev->of_offset;
> +
> + plat->base = (unsigned long)dev_get_addr_ptr(dev);
> + plat->max_id = fdtdec_get_uint(blob,
> + node,
> + "max-id",
> + CONFIG_SYS_SCSI_MAX_SCSI_ID);
> + plat->max_lun = fdtdec_get_uint(blob,
> + node,
> + "max-lun",
> + CONFIG_SYS_SCSI_MAX_LUN);
> + return;
return 0
[Ken] got it.
> +}
> +
> +static int scsi_post_bind(struct udevice *dev) {
> + /* Get uclass plat data from fdt */
> + scsi_ofdata_to_uclass_platdata(dev);
Do we need to have this info in bind(), or could it wait until of_to_platdata()?
[Ken] Stefan shows me a patch https://patchwork.ozlabs.org/patch/743160/ , it fills the two field members(max_lun and max_id) in ahci's scsi_low_level_init()
As below, I think it's OK to get active link port number in ahci_host_init(), and in my opinion, it's better to be a default way to get max_lun and max_id
in ahci driver if the two members are not set in fdt since actually max_lun * max_id = ffs(linkmap) and we can also set max_lun = 2 and max_id = fls(linkmap)/2;
And another question is whther ffs() or fls() for max_id?
> + if (plat) {
> + plat->max_lun = 1;
> + plat->max_id = ffs(linkmap);
> + }
Also, return the error code here.
[Ken] got it.
> +}
> +
> UCLASS_DRIVER(scsi) = {
> .id = UCLASS_SCSI,
> .name = "scsi",
> + .post_bind = scsi_post_bind,
> .post_probe = scsi_post_probe,
> + .per_device_platdata_auto_alloc_size = sizeof(struct
> + scsi_platdata),
> };
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
2017-04-05 8:38 ` [U-Boot] [EXT] " Ken Ma
@ 2017-04-09 19:27 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-09 19:27 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 5 April 2017 at 02:38, Ken Ma <make@marvell.com> wrote:
> Hi Simon
>
> Please see my inline reply, thanks a lot!
>
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2017年4月1日 12:22
> To: Ken Ma
> Cc: U-Boot Mailing List; Stefan Roese; Michal Simek
> Subject: [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Ken,
>
> On 23 March 2017 at 03:29, <make@marvell.com> wrote:
>> From: Ken Ma <make@marvell.com>
>>
>> - The members in scsi_platdata(base, max_lun and max_id) are generic,
>> so now they are taken from fdt by the uclass_platdata instead of
>> platdata code upon call to post bind callback.
>>
>> Signed-off-by: Ken Ma <make@marvell.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
>> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
>> Reviewed-by: Omri Itach <omrii@marvell.com>
>> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
>> ---
>> common/scsi.c | 2 +-
>> drivers/block/ahci.c | 2 +-
>> drivers/block/scsi-uclass.c | 29 +++++++++++++++++++++++++++++
>> 3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/scsi.c b/common/scsi.c index fb5b407..117c682
>> 100644
>> --- a/common/scsi.c
>> +++ b/common/scsi.c
>> @@ -574,7 +574,7 @@ int scsi_scan(int mode)
>> return ret;
>>
>> /* Get controller platdata */
>> - plat = dev_get_platdata(dev);
>> + plat = dev_get_uclass_platdata(dev);
>>
>> for (i = 0; i < plat->max_id; i++) {
>> for (lun = 0; lun < plat->max_lun; lun++) {
>> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c index
>> 3fa14a7..368816e 100644
>> --- a/drivers/block/ahci.c
>> +++ b/drivers/block/ahci.c
>> @@ -479,7 +479,7 @@ static int ahci_init_one(pci_dev_t dev)
>> pci_write_config_byte(dev, 0x41, 0xa1); #endif #else
>> - struct scsi_platdata *plat = dev_get_platdata(dev);
>> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
>> probe_ent->mmio_base = (void *)plat->base; #endif
>>
>> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
>> index 05da6cd..3bf026b 100644
>> --- a/drivers/block/scsi-uclass.c
>> +++ b/drivers/block/scsi-uclass.c
>> @@ -11,8 +11,11 @@
>>
>> #include <common.h>
>> #include <dm.h>
>> +#include <dm/device-internal.h>
>> #include <scsi.h>
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> static int scsi_post_probe(struct udevice *dev) {
>> debug("%s: device %p\n", __func__, dev); @@ -20,8 +23,34 @@
>> static int scsi_post_probe(struct udevice *dev)
>> return 0;
>> }
>>
>> +static void scsi_ofdata_to_uclass_platdata(struct udevice *dev)
>
> Please make this return an int since functions that decode the DT should return an error code.
>
>> +{
>> + struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
>> + const void *blob = gd->fdt_blob;
>> + int node = dev->of_offset;
>> +
>> + plat->base = (unsigned long)dev_get_addr_ptr(dev);
>> + plat->max_id = fdtdec_get_uint(blob,
>> + node,
>> + "max-id",
>> + CONFIG_SYS_SCSI_MAX_SCSI_ID);
>> + plat->max_lun = fdtdec_get_uint(blob,
>> + node,
>> + "max-lun",
>> + CONFIG_SYS_SCSI_MAX_LUN);
>> + return;
>
> return 0
> [Ken] got it.
>
>> +}
>> +
>> +static int scsi_post_bind(struct udevice *dev) {
>> + /* Get uclass plat data from fdt */
>> + scsi_ofdata_to_uclass_platdata(dev);
>
> Do we need to have this info in bind(), or could it wait until of_to_platdata()?
> [Ken] Stefan shows me a patch https://patchwork.ozlabs.org/patch/743160/ , it fills the two field members(max_lun and max_id) in ahci's scsi_low_level_init()
> As below, I think it's OK to get active link port number in ahci_host_init(), and in my opinion, it's better to be a default way to get max_lun and max_id
> in ahci driver if the two members are not set in fdt since actually max_lun * max_id = ffs(linkmap) and we can also set max_lun = 2 and max_id = fls(linkmap)/2;
> And another question is whther ffs() or fls() for max_id?
OK well since it doesn't take any time, I am OK with it. I'm not sure
about your last question.
>
>> + if (plat) {
>> + plat->max_lun = 1;
>> + plat->max_id = ffs(linkmap);
>> + }
>
>
> Also, return the error code here.
> [Ken] got it.
>
>> +}
>> +
>> UCLASS_DRIVER(scsi) = {
>> .id = UCLASS_SCSI,
>> .name = "scsi",
>> + .post_bind = scsi_post_bind,
>> .post_probe = scsi_post_probe,
>> + .per_device_platdata_auto_alloc_size = sizeof(struct
>> + scsi_platdata),
>> };
>> --
>> 1.9.1
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 2/7] scsi: add children devices binding
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
2017-03-23 9:29 ` [U-Boot] [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 3/7] scsi: call children devices' probe functions automatically make at marvell.com
` (5 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- When scsi controller acts as a bus, we need to bind its children
scsi devices(scsi hdd, cd, dvd, scanner) to their drivers as spi
controller binds spi flashes, so scsi-uclass's post bind function
calls dm_scan_fdt_dev() to bind scsi subnode devices;
- When scsi controller is a Serial Attached SCSI, it can also work as
a pure controller as an on-board component on the motherboard, it may
has no subnodes in fdt, then dm_scan_fdt_dev() does nothing and has
no effect.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35425
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
---
drivers/block/scsi-uclass.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
index 3bf026b..86eddfc 100644
--- a/drivers/block/scsi-uclass.c
+++ b/drivers/block/scsi-uclass.c
@@ -45,6 +45,9 @@ static int scsi_post_bind(struct udevice *dev)
{
/* Get uclass plat data from fdt */
scsi_ofdata_to_uclass_platdata(dev);
+
+ /* bind subnode devices */
+ return dm_scan_fdt_dev(dev);
}
UCLASS_DRIVER(scsi) = {
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 2/7] scsi: add children devices binding
2017-03-23 9:29 ` [U-Boot] [PATCH 2/7] scsi: add children devices binding make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - When scsi controller acts as a bus, we need to bind its children
> scsi devices(scsi hdd, cd, dvd, scanner) to their drivers as spi
> controller binds spi flashes, so scsi-uclass's post bind function
> calls dm_scan_fdt_dev() to bind scsi subnode devices;
> - When scsi controller is a Serial Attached SCSI, it can also work as
> a pure controller as an on-board component on the motherboard, it may
> has no subnodes in fdt, then dm_scan_fdt_dev() does nothing and has
> no effect.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35425
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> drivers/block/scsi-uclass.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 3/7] scsi: call children devices' probe functions automatically
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
2017-03-23 9:29 ` [U-Boot] [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data make at marvell.com
2017-03-23 9:29 ` [U-Boot] [PATCH 2/7] scsi: add children devices binding make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 4/7] scsi: dt-bindings: add scsi device tree bindings make at marvell.com
` (4 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- For the purpose of accessing peripheral devices through SCSI, the
peripheral devices need to be probed to finish low level
initialization, for example, ahci controller needs to do the ahci
initialization;
- scsi_low_level_init() calling is removed since the detailed scsi low
level initialization work is up to the peripheral scsi devices, for
example, sata controller may do AHCI initialization while scanner
controller may do ISIS initialization; the work should be done in
children devices probe when scsi controller acts as bus or be done
in the pure SAS controller's probe when SCSI controller is a SAS
and works as an on-board component on the motherboard;
- Since u-boot initialization does not probe devices by default, SCSI
children devices can be probed automatically in SCSI post probe
function when SCSI controller acts as a bus.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35426
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
---
drivers/block/scsi-uclass.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
index 86eddfc..119ba53 100644
--- a/drivers/block/scsi-uclass.c
+++ b/drivers/block/scsi-uclass.c
@@ -18,8 +18,26 @@ DECLARE_GLOBAL_DATA_PTR;
static int scsi_post_probe(struct udevice *dev)
{
+ struct udevice *child_dev;
+ int ret;
+
debug("%s: device %p\n", __func__, dev);
- scsi_low_level_init(0, dev);
+
+ /*
+ * For the purpose of accessing peripheral devices through SCSI, the
+ * peripheral devices need to be probed to finish low level
+ * initialization, for example, ahci controller needs to do the ahci
+ * initialization;
+ * Since u-boot initialization does not probe devices by default, SCSI
+ * children devices can be probed automatically in SCSI post probe
+ * function when SCSI controller acts as a bus.
+ */
+ list_for_each_entry(child_dev, &dev->child_head, sibling_node) {
+ ret = device_probe(child_dev);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
@@ -54,6 +72,6 @@ UCLASS_DRIVER(scsi) = {
.id = UCLASS_SCSI,
.name = "scsi",
.post_bind = scsi_post_bind,
- .post_probe = scsi_post_probe,
+ .post_probe = scsi_post_probe,
.per_device_platdata_auto_alloc_size = sizeof(struct scsi_platdata),
};
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 3/7] scsi: call children devices' probe functions automatically
2017-03-23 9:29 ` [U-Boot] [PATCH 3/7] scsi: call children devices' probe functions automatically make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
2017-04-05 8:47 ` [U-Boot] [EXT] " Ken Ma
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
Hi,
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - For the purpose of accessing peripheral devices through SCSI, the
> peripheral devices need to be probed to finish low level
> initialization, for example, ahci controller needs to do the ahci
> initialization;
> - scsi_low_level_init() calling is removed since the detailed scsi low
> level initialization work is up to the peripheral scsi devices, for
> example, sata controller may do AHCI initialization while scanner
> controller may do ISIS initialization; the work should be done in
> children devices probe when scsi controller acts as bus or be done
> in the pure SAS controller's probe when SCSI controller is a SAS
> and works as an on-board component on the motherboard;
> - Since u-boot initialization does not probe devices by default, SCSI
> children devices can be probed automatically in SCSI post probe
> function when SCSI controller acts as a bus.
Do you have to probe everything? The idea in DM is to probe devices
only when they are first used.
(Also please use 'U-Boot' everywhere consistently)
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35426
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> drivers/block/scsi-uclass.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
> index 86eddfc..119ba53 100644
> --- a/drivers/block/scsi-uclass.c
> +++ b/drivers/block/scsi-uclass.c
> @@ -18,8 +18,26 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static int scsi_post_probe(struct udevice *dev)
> {
> + struct udevice *child_dev;
> + int ret;
> +
> debug("%s: device %p\n", __func__, dev);
> - scsi_low_level_init(0, dev);
> +
> + /*
> + * For the purpose of accessing peripheral devices through SCSI, the
> + * peripheral devices need to be probed to finish low level
> + * initialization, for example, ahci controller needs to do the ahci
> + * initialization;
> + * Since u-boot initialization does not probe devices by default, SCSI
U-Boot
> + * children devices can be probed automatically in SCSI post probe
> + * function when SCSI controller acts as a bus.
> + */
> + list_for_each_entry(child_dev, &dev->child_head, sibling_node) {
> + ret = device_probe(child_dev);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -54,6 +72,6 @@ UCLASS_DRIVER(scsi) = {
> .id = UCLASS_SCSI,
> .name = "scsi",
> .post_bind = scsi_post_bind,
> - .post_probe = scsi_post_probe,
> + .post_probe = scsi_post_probe,
> .per_device_platdata_auto_alloc_size = sizeof(struct scsi_platdata),
> };
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 3/7] scsi: call children devices' probe functions automatically
2017-04-01 4:21 ` Simon Glass
@ 2017-04-05 8:47 ` Ken Ma
2017-04-09 19:27 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-04-05 8:47 UTC (permalink / raw)
To: u-boot
Hi Simon
Please see my inline reply, thanks a lot!
Yours,
Ken
-----Original Message-----
From: sjg@google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: 2017年4月1日 12:22
To: Ken Ma
Cc: U-Boot Mailing List; Stefan Roese; Michal Simek
Subject: [EXT] Re: [PATCH 3/7] scsi: call children devices' probe functions automatically
External Email
----------------------------------------------------------------------
Hi,
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - For the purpose of accessing peripheral devices through SCSI, the
> peripheral devices need to be probed to finish low level
> initialization, for example, ahci controller needs to do the ahci
> initialization;
> - scsi_low_level_init() calling is removed since the detailed scsi low
> level initialization work is up to the peripheral scsi devices, for
> example, sata controller may do AHCI initialization while scanner
> controller may do ISIS initialization; the work should be done in
> children devices probe when scsi controller acts as bus or be done
> in the pure SAS controller's probe when SCSI controller is a SAS
> and works as an on-board component on the motherboard;
> - Since u-boot initialization does not probe devices by default, SCSI
> children devices can be probed automatically in SCSI post probe
> function when SCSI controller acts as a bus.
Do you have to probe everything? The idea in DM is to probe devices only when they are first used.
[Ken] If we do not probe SCSI' children nodes such as sata hdds scsi devices, then "scsi scan" will fail since command "scsi scan" only probes SCSI host controllers but not SCSI devices which are SCSI node's children nodes; otherwise we should add SCSI node's children devices' probe to "scsi scan".
How do you think about it? Thanks!
(Also please use 'U-Boot' everywhere consistently)
[Ken] got it, thanks!
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35426
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> drivers/block/scsi-uclass.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
> index 86eddfc..119ba53 100644
> --- a/drivers/block/scsi-uclass.c
> +++ b/drivers/block/scsi-uclass.c
> @@ -18,8 +18,26 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static int scsi_post_probe(struct udevice *dev) {
> + struct udevice *child_dev;
> + int ret;
> +
> debug("%s: device %p\n", __func__, dev);
> - scsi_low_level_init(0, dev);
> +
> + /*
> + * For the purpose of accessing peripheral devices through SCSI, the
> + * peripheral devices need to be probed to finish low level
> + * initialization, for example, ahci controller needs to do the ahci
> + * initialization;
> + * Since u-boot initialization does not probe devices by
> + default, SCSI
U-Boot
> + * children devices can be probed automatically in SCSI post probe
> + * function when SCSI controller acts as a bus.
> + */
> + list_for_each_entry(child_dev, &dev->child_head, sibling_node) {
> + ret = device_probe(child_dev);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -54,6 +72,6 @@ UCLASS_DRIVER(scsi) = {
> .id = UCLASS_SCSI,
> .name = "scsi",
> .post_bind = scsi_post_bind,
> - .post_probe = scsi_post_probe,
> + .post_probe = scsi_post_probe,
> .per_device_platdata_auto_alloc_size = sizeof(struct
> scsi_platdata), };
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 3/7] scsi: call children devices' probe functions automatically
2017-04-05 8:47 ` [U-Boot] [EXT] " Ken Ma
@ 2017-04-09 19:27 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-09 19:27 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 5 April 2017 at 02:47, Ken Ma <make@marvell.com> wrote:
> Hi Simon
>
> Please see my inline reply, thanks a lot!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2017年4月1日 12:22
> To: Ken Ma
> Cc: U-Boot Mailing List; Stefan Roese; Michal Simek
> Subject: [EXT] Re: [PATCH 3/7] scsi: call children devices' probe functions automatically
>
> External Email
>
> ----------------------------------------------------------------------
> Hi,
>
> On 23 March 2017 at 03:29, <make@marvell.com> wrote:
>> From: Ken Ma <make@marvell.com>
>>
>> - For the purpose of accessing peripheral devices through SCSI, the
>> peripheral devices need to be probed to finish low level
>> initialization, for example, ahci controller needs to do the ahci
>> initialization;
>> - scsi_low_level_init() calling is removed since the detailed scsi low
>> level initialization work is up to the peripheral scsi devices, for
>> example, sata controller may do AHCI initialization while scanner
>> controller may do ISIS initialization; the work should be done in
>> children devices probe when scsi controller acts as bus or be done
>> in the pure SAS controller's probe when SCSI controller is a SAS
>> and works as an on-board component on the motherboard;
>> - Since u-boot initialization does not probe devices by default, SCSI
>> children devices can be probed automatically in SCSI post probe
>> function when SCSI controller acts as a bus.
>
> Do you have to probe everything? The idea in DM is to probe devices only when they are first used.
> [Ken] If we do not probe SCSI' children nodes such as sata hdds scsi devices, then "scsi scan" will fail since command "scsi scan" only probes SCSI host controllers but not SCSI devices which are SCSI node's children nodes; otherwise we should add SCSI node's children devices' probe to "scsi scan".
> How do you think about it? Thanks!
I think that probing the scsi driver should be done at the start, then
scanning should be an operation which can be performed afterwards. I'm
not saying you shouldn't do both, but conceptually they are different
and I think we should keep them separate.
Probing a SCSI controller just means that the hardware is prepared so
we can use it..
>
> (Also please use 'U-Boot' everywhere consistently)
> [Ken] got it, thanks!
>
>>
>> Signed-off-by: Ken Ma <make@marvell.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-on: http://vgitil04.il.marvell.com:8080/35426
>> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
>> Reviewed-by: Omri Itach <omrii@marvell.com>
>> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
>> ---
>> drivers/block/scsi-uclass.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
>> index 86eddfc..119ba53 100644
>> --- a/drivers/block/scsi-uclass.c
>> +++ b/drivers/block/scsi-uclass.c
>> @@ -18,8 +18,26 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>> static int scsi_post_probe(struct udevice *dev) {
>> + struct udevice *child_dev;
>> + int ret;
>> +
>> debug("%s: device %p\n", __func__, dev);
>> - scsi_low_level_init(0, dev);
>> +
>> + /*
>> + * For the purpose of accessing peripheral devices through SCSI, the
>> + * peripheral devices need to be probed to finish low level
>> + * initialization, for example, ahci controller needs to do the ahci
>> + * initialization;
>> + * Since u-boot initialization does not probe devices by
>> + default, SCSI
>
> U-Boot
>
>> + * children devices can be probed automatically in SCSI post probe
>> + * function when SCSI controller acts as a bus.
>> + */
>> + list_for_each_entry(child_dev, &dev->child_head, sibling_node) {
>> + ret = device_probe(child_dev);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -54,6 +72,6 @@ UCLASS_DRIVER(scsi) = {
>> .id = UCLASS_SCSI,
>> .name = "scsi",
>> .post_bind = scsi_post_bind,
>> - .post_probe = scsi_post_probe,
>> + .post_probe = scsi_post_probe,
>> .per_device_platdata_auto_alloc_size = sizeof(struct
>> scsi_platdata), };
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 4/7] scsi: dt-bindings: add scsi device tree bindings
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
` (2 preceding siblings ...)
2017-03-23 9:29 ` [U-Boot] [PATCH 3/7] scsi: call children devices' probe functions automatically make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 5/7] scsi: mvebu: add scsi driver make at marvell.com
` (3 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- Add generic scsi device tree bindings doc, the doc includes:
- Brief introduction for scsi;
- Scsi's properties' introduction;
- Add marvell mvebu scsi binding doc with the example of armada3700
SCSI controller.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35427
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
---
.../scsi/marvell,mvebu-scsi.txt | 29 ++++++++++++++++++++++
doc/device-tree-bindings/scsi/scsi-bus.txt | 22 ++++++++++++++++
2 files changed, 51 insertions(+)
create mode 100644 doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt
create mode 100644 doc/device-tree-bindings/scsi/scsi-bus.txt
diff --git a/doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt b/doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt
new file mode 100644
index 0000000..b3d06af
--- /dev/null
+++ b/doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt
@@ -0,0 +1,29 @@
+Binding for marvell mvebu SCSI controller
+
+Required properties:
+- #address-cells - the number of cells used to represent physical base addresses
+- #size-cells - the number of cells used to represent the size of an address
+- compatible - the name of mvebu SCSI bus controller, supported value "marvell,mvebu-scsi",
+ covers the following Marvell SoC families: armada3700, armada70x0 and armada80x0
+
+Optional property:
+- max-id - maximum number of scsi target ids, the default value is CONFIG_SYS_SCSI_MAX_SCSI_ID
+- max-lun - maximum number of scsi logical units, the default value is CONFIG_SYS_SCSI_MAX_LUN
+
+Example for armada3700 SCSI controller which is SAS and acts as an add-on host bus adapter without the
+base register:
+- Armada3700 has only 1 SATA interface, so the property "max-id" is 1;
+- Armada3700 max logical units number is 1, so the property "max-lun" is 1.
+
+ scsi: scsi {
+ compatible = "marvell,mvebu-scsi";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ max-id = <1>;
+ max-lun = <1>;
+ sata: sata at e0000 {
+ compatible = "marvell,armada-3700-ahci";
+ reg = <0xe0000 0x2000>;
+ interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
diff --git a/doc/device-tree-bindings/scsi/scsi-bus.txt b/doc/device-tree-bindings/scsi/scsi-bus.txt
new file mode 100644
index 0000000..01aee06
--- /dev/null
+++ b/doc/device-tree-bindings/scsi/scsi-bus.txt
@@ -0,0 +1,22 @@
+SCSI (Small Computer System Interface) busses
+
+SCSI busses can be described with a node for the SCSI controller device
+and a set of child nodes for each SCSI devices on the bus. An SCSI controller
+node can also be a Serial Attached SCSI (SAS) controller, which can act as an
+add-on host bus adapter or work as a pure controller as an on-board component
+on the motherboard, to offer compatibility with SATA devices.
+
+The SCSI controller node requires the following properties:
+- #address-cells - the number of cells used to represent physical base addresses
+- #size-cells - the number of cells used to represent the size of an address
+- compatible - the name of SCSI bus controller following generic names recommended practice
+
+No other properties are required in the SCSI bus node. It is assumed
+that a driver for an SCSI bus device will understand that it is an SCSI bus.
+
+Optional property:
+- base - scsi register base address
+- max-id - maximum number of scsi target ids, the default value is CONFIG_SYS_SCSI_MAX_SCSI_ID
+- max-lun - maximum number of scsi logical units, the default value is CONFIG_SYS_SCSI_MAX_LUN
+
+SCSI device nodes must be children of the SCSI controller node.
\ No newline at end of file
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 4/7] scsi: dt-bindings: add scsi device tree bindings
2017-03-23 9:29 ` [U-Boot] [PATCH 4/7] scsi: dt-bindings: add scsi device tree bindings make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - Add generic scsi device tree bindings doc, the doc includes:
> - Brief introduction for scsi;
> - Scsi's properties' introduction;
> - Add marvell mvebu scsi binding doc with the example of armada3700
> SCSI controller.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35427
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> ---
> .../scsi/marvell,mvebu-scsi.txt | 29 ++++++++++++++++++++++
> doc/device-tree-bindings/scsi/scsi-bus.txt | 22 ++++++++++++++++
> 2 files changed, 51 insertions(+)
> create mode 100644 doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt
> create mode 100644 doc/device-tree-bindings/scsi/scsi-bus.txt
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 5/7] scsi: mvebu: add scsi driver
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
` (3 preceding siblings ...)
2017-03-23 9:29 ` [U-Boot] [PATCH 4/7] scsi: dt-bindings: add scsi device tree bindings make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 6/7] scsi: a3700: enable mvebu " make at marvell.com
` (2 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- Add mvebu scsi driver which is based on scsi uclass so that
scsi command can work when driver model is enabled for scsi;
- Mvebu scsi is serial attached scsi and act as an add-on host
bus adapter.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35301
Reviewed-by: Omri Itach <omrii@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
---
drivers/block/Kconfig | 10 ++++++++++
drivers/block/Makefile | 1 +
drivers/block/mvebu_scsi.c | 31 +++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+)
create mode 100644 drivers/block/mvebu_scsi.c
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 88e66e2..bb27a7f 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -28,6 +28,16 @@ config DM_SCSI
(IDs/LUNs) a block device is created with RAW read/write and
filesystem support.
+config MVEBU_SCSI
+ bool "Marvell MVEBU SCSI driver"
+ depends on DM_SCSI
+ default n
+ help
+ Say yes here to support Marvell MVEBU SCSI.
+ Marvell MVEBU SCSI supports serial attached SCSI(SAS),
+ which offers backward compatibility with SATA, versions 2 and later.
+ It allows for SATA drives to be connected to SAS backplanes.
+
config BLOCK_CACHE
bool "Use block device cache"
default n
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index a72feec..88fe17d 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -29,5 +29,6 @@ obj-$(CONFIG_SATA_SIL) += sata_sil.o
obj-$(CONFIG_IDE_SIL680) += sil680.o
obj-$(CONFIG_SANDBOX) += sandbox.o sandbox_scsi.o sata_sandbox.o
obj-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
+obj-$(CONFIG_MVEBU_SCSI) += mvebu_scsi.o
obj-$(CONFIG_SYSTEMACE) += systemace.o
obj-$(CONFIG_BLOCK_CACHE) += blkcache.o
diff --git a/drivers/block/mvebu_scsi.c b/drivers/block/mvebu_scsi.c
new file mode 100644
index 0000000..0151edcb
--- /dev/null
+++ b/drivers/block/mvebu_scsi.c
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2016 Marvell International Ltd.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ * https://spdx.org/licenses
+ */
+
+#include <common.h>
+#include <scsi.h>
+#include <dm.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int mvebu_scsi_probe(struct udevice *bus)
+{
+ /* Do nothing */
+ return 0;
+}
+
+static const struct udevice_id mvebu_scsi_ids[] = {
+ { .compatible = "marvell,mvebu-scsi" },
+ { }
+};
+
+U_BOOT_DRIVER(scsi_mvebu_drv) = {
+ .name = "scsi_mvebu",
+ .id = UCLASS_SCSI,
+ .of_match = mvebu_scsi_ids,
+ .probe = mvebu_scsi_probe,
+};
+
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 5/7] scsi: mvebu: add scsi driver
2017-03-23 9:29 ` [U-Boot] [PATCH 5/7] scsi: mvebu: add scsi driver make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - Add mvebu scsi driver which is based on scsi uclass so that
> scsi command can work when driver model is enabled for scsi;
> - Mvebu scsi is serial attached scsi and act as an add-on host
> bus adapter.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35301
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> ---
> drivers/block/Kconfig | 10 ++++++++++
> drivers/block/Makefile | 1 +
> drivers/block/mvebu_scsi.c | 31 +++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+)
> create mode 100644 drivers/block/mvebu_scsi.c
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 6/7] scsi: a3700: enable mvebu scsi driver
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
` (4 preceding siblings ...)
2017-03-23 9:29 ` [U-Boot] [PATCH 5/7] scsi: mvebu: add scsi driver make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-04-01 4:21 ` Simon Glass
2017-03-23 9:29 ` [U-Boot] [PATCH 7/7] scsi: dts: a3700: add scsi node make at marvell.com
2017-04-01 4:21 ` [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** Simon Glass
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- Enable SCSI support in Armada-3700 DB default configuration.
Reviewed-on: http://vgitil04.il.marvell.com:8080/35302
Reviewed-by: Omri Itach <omrii@marvell.com>
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
---
configs/mvebu_db-88f3720_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/configs/mvebu_db-88f3720_defconfig b/configs/mvebu_db-88f3720_defconfig
index 80f2599..53b3c38 100644
--- a/configs/mvebu_db-88f3720_defconfig
+++ b/configs/mvebu_db-88f3720_defconfig
@@ -33,6 +33,8 @@ CONFIG_CMD_FS_GENERIC=y
CONFIG_MAC_PARTITION=y
CONFIG_ISO_PARTITION=y
CONFIG_EFI_PARTITION=y
+CONFIG_DM_SCSI=y
+CONFIG_MVEBU_SCSI=y
CONFIG_BLOCK_CACHE=y
CONFIG_DM_I2C=y
CONFIG_DM_I2C_COMPAT=y
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 6/7] scsi: a3700: enable mvebu scsi driver
2017-03-23 9:29 ` [U-Boot] [PATCH 6/7] scsi: a3700: enable mvebu " make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> - Enable SCSI support in Armada-3700 DB default configuration.
>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35302
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> ---
> configs/mvebu_db-88f3720_defconfig | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
` (5 preceding siblings ...)
2017-03-23 9:29 ` [U-Boot] [PATCH 6/7] scsi: a3700: enable mvebu " make at marvell.com
@ 2017-03-23 9:29 ` make at marvell.com
2017-03-23 14:06 ` Stefan Roese
2017-04-01 4:21 ` [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** Simon Glass
7 siblings, 1 reply; 32+ messages in thread
From: make at marvell.com @ 2017-03-23 9:29 UTC (permalink / raw)
To: u-boot
From: Ken Ma <make@marvell.com>
- Add scsi node which acts as a bus for scsi devices, armada3700 has
only 1 scsi interface, so max-id is 1, and the logic unit number is
also 1 for armada3700;
- Since a3700's scsi is sas(serial attached scsi) which is compatible
for sata and sata hard disk is a sas device, so move sata node to be
under scsi node.
Signed-off-by: Ken Ma <make@marvell.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
Tested-by: iSoC Platform CI <ykjenk@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.com>
---
arch/arm/dts/armada-3720-db.dts | 4 ++++
arch/arm/dts/armada-37xx.dtsi | 16 ++++++++++++----
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/armada-3720-db.dts b/arch/arm/dts/armada-3720-db.dts
index 85761af..9fc60f6 100644
--- a/arch/arm/dts/armada-3720-db.dts
+++ b/arch/arm/dts/armada-3720-db.dts
@@ -89,6 +89,10 @@
status = "okay";
};
+&scsi {
+ status = "okay";
+};
+
/* CON3 */
&sata {
status = "okay";
diff --git a/arch/arm/dts/armada-37xx.dtsi b/arch/arm/dts/armada-37xx.dtsi
index 062f2a6..de5d3a1 100644
--- a/arch/arm/dts/armada-37xx.dtsi
+++ b/arch/arm/dts/armada-37xx.dtsi
@@ -149,11 +149,19 @@
status = "disabled";
};
- sata: sata at e0000 {
- compatible = "marvell,armada-3700-ahci";
- reg = <0xe0000 0x2000>;
- interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ scsi: scsi {
+ compatible = "marvell,mvebu-scsi";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ max-id = <1>;
+ max-lun = <1>;
status = "disabled";
+ sata: sata at e0000 {
+ compatible = "marvell,armada-3700-ahci";
+ reg = <0xe0000 0x2000>;
+ interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
};
gic: interrupt-controller at 1d00000 {
--
1.9.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [U-Boot] [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-23 9:29 ` [U-Boot] [PATCH 7/7] scsi: dts: a3700: add scsi node make at marvell.com
@ 2017-03-23 14:06 ` Stefan Roese
2017-03-24 3:03 ` [U-Boot] [EXT] " Ken Ma
2017-03-24 4:11 ` Ken Ma
0 siblings, 2 replies; 32+ messages in thread
From: Stefan Roese @ 2017-03-23 14:06 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 23.03.2017 10:29, make at marvell.com wrote:
> From: Ken Ma <make@marvell.com>
>
> - Add scsi node which acts as a bus for scsi devices, armada3700 has
> only 1 scsi interface, so max-id is 1, and the logic unit number is
> also 1 for armada3700;
> - Since a3700's scsi is sas(serial attached scsi) which is compatible
> for sata and sata hard disk is a sas device, so move sata node to be
> under scsi node.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> ---
> arch/arm/dts/armada-3720-db.dts | 4 ++++
> arch/arm/dts/armada-37xx.dtsi | 16 ++++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/armada-3720-db.dts b/arch/arm/dts/armada-3720-db.dts
> index 85761af..9fc60f6 100644
> --- a/arch/arm/dts/armada-3720-db.dts
> +++ b/arch/arm/dts/armada-3720-db.dts
> @@ -89,6 +89,10 @@
> status = "okay";
> };
>
> +&scsi {
> + status = "okay";
> +};
> +
> /* CON3 */
> &sata {
> status = "okay";
> diff --git a/arch/arm/dts/armada-37xx.dtsi b/arch/arm/dts/armada-37xx.dtsi
> index 062f2a6..de5d3a1 100644
> --- a/arch/arm/dts/armada-37xx.dtsi
> +++ b/arch/arm/dts/armada-37xx.dtsi
> @@ -149,11 +149,19 @@
> status = "disabled";
> };
>
> - sata: sata at e0000 {
> - compatible = "marvell,armada-3700-ahci";
> - reg = <0xe0000 0x2000>;
> - interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + scsi: scsi {
> + compatible = "marvell,mvebu-scsi";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + max-id = <1>;
> + max-lun = <1>;
> status = "disabled";
> + sata: sata at e0000 {
> + compatible = "marvell,armada-3700-ahci";
> + reg = <0xe0000 0x2000>;
> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> };
>
> gic: interrupt-controller at 1d00000 {
>
I see that you introduce a "scsi" DT node and move the SATA controller
one "level up". I'm not sure if such a change is acceptable as we try
to re-use the DT from Linux. Or thinking more about this, I'm pretty
sure that such a change is not acceptable in general.
Can't you use the existing DT layout and use the
"marvell,armada-3700-ahci" (and other perhaps?) compatible property
instead for driver probing? Not sure how to handle the "max-id" and
"max-lun" properties though. We definitely can't just add some ad-hoc
properties here in U-Boot which have no chance for Linux upstream
acceptance.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-23 14:06 ` Stefan Roese
@ 2017-03-24 3:03 ` Ken Ma
2017-03-24 4:11 ` Ken Ma
1 sibling, 0 replies; 32+ messages in thread
From: Ken Ma @ 2017-03-24 3:03 UTC (permalink / raw)
To: u-boot
Hi Stefan
Thanks a lot for your kind advice and help!
Please see my reply inline.
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年3月23日 22:06
To: Ken Ma; u-boot at lists.denx.de
Cc: Simon Glass; Michal Simek
Subject: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
External Email
----------------------------------------------------------------------
Hi Ken,
On 23.03.2017 10:29, make at marvell.com<mailto:make@marvell.com> wrote:
> From: Ken Ma <make at marvell.com<mailto:make@marvell.com>>
>
> - Add scsi node which acts as a bus for scsi devices, armada3700 has
> only 1 scsi interface, so max-id is 1, and the logic unit number is
> also 1 for armada3700;
> - Since a3700's scsi is sas(serial attached scsi) which is compatible
> for sata and sata hard disk is a sas device, so move sata node to be
> under scsi node.
>
> Signed-off-by: Ken Ma <make at marvell.com<mailto:make@marvell.com>>
> Cc: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
> Cc: Stefan Roese <sr at denx.de<mailto:sr@denx.de>>
> Cc: Michal Simek <michal.simek at xilinx.com<mailto:michal.simek@xilinx.com>>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
> Tested-by: iSoC Platform CI <ykjenk at marvell.com<mailto:ykjenk@marvell.com>>
> Reviewed-by: Kostya Porotchkin <kostap at marvell.com<mailto:kostap@marvell.com>>
> Reviewed-by: Omri Itach <omrii at marvell.com<mailto:omrii@marvell.com>>
> ---
> arch/arm/dts/armada-3720-db.dts | 4 ++++
> arch/arm/dts/armada-37xx.dtsi | 16 ++++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/armada-3720-db.dts
> b/arch/arm/dts/armada-3720-db.dts index 85761af..9fc60f6 100644
> --- a/arch/arm/dts/armada-3720-db.dts
> +++ b/arch/arm/dts/armada-3720-db.dts
> @@ -89,6 +89,10 @@
> status = "okay";
> };
>
> +&scsi {
> + status = "okay";
> +};
> +
> /* CON3 */
> &sata {
> status = "okay";
> diff --git a/arch/arm/dts/armada-37xx.dtsi
> b/arch/arm/dts/armada-37xx.dtsi index 062f2a6..de5d3a1 100644
> --- a/arch/arm/dts/armada-37xx.dtsi
> +++ b/arch/arm/dts/armada-37xx.dtsi
> @@ -149,11 +149,19 @@
> status = "disabled";
> };
>
> - sata: sata at e0000 {
> - compatible = "marvell,armada-3700-ahci";
> - reg = <0xe0000 0x2000>;
> - interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + scsi: scsi {
> + compatible = "marvell,mvebu-scsi";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + max-id = <1>;
> + max-lun = <1>;
> status = "disabled";
> + sata: sata at e0000 {
> + compatible = "marvell,armada-3700-ahci";
> + reg = <0xe0000 0x2000>;
> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> };
>
> gic: interrupt-controller at 1d00000 {
>
I see that you introduce a "scsi" DT node and move the SATA controller one "level up". I'm not sure if such a change is acceptable as we try to re-use the DT from Linux. Or thinking more about this, I'm pretty sure that such a change is not acceptable in general.
Can't you use the existing DT layout and use the "marvell,armada-3700-ahci" (and other perhaps?) compatible property instead for driver probing? Not sure how to handle the "max-id" and "max-lun" properties though. We definitely can't just add some ad-hoc properties here in U-Boot which have no chance for Linux upstream acceptance.
[Ken] Because scsi is a bus, for example, if there are 2 scsi buses, each bus has some scsi device controllers connected as below.
Scsi ID 0 Scsi ID 1 Scsi ID 2 Scsi ID 3
HDD0 HDD1 tape0 cd-rom0
|| || || ||
===============================================================
SCSI BUS1
HDD2 HDD3 tape1 cd-rom2
|| || || ||
===============================================================
SCSI BUS2
Then in my opinion, since now scsi has its own class id and its compatible string, then the scsi device controllers dts node should be above the scsi node.
If we keep existing DT layout and keep "marvell,armada-3700-ahci"’s uclass id as UCLASS_AHCI(there are no scsi nodes but only ahci nodes), then scsi_scan() can not find a3700’s sata at all since there are no UCLASS_SCSI devices;
If we keep existing DT layout and set scsi devices’ uclass id to be UCLASS_SCSI, how can we know that hdd0 and hdd1 are in scsi bus1 but hdd2 and hdd3 are in scsi bus2? For each scsi bus, its max id should be 4; but now how to set each scsi device’ scsi id?
So I think we should move scsi devices “level up” on the scsio bus.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-23 14:06 ` Stefan Roese
2017-03-24 3:03 ` [U-Boot] [EXT] " Ken Ma
@ 2017-03-24 4:11 ` Ken Ma
2017-03-24 13:21 ` Stefan Roese
1 sibling, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-03-24 4:11 UTC (permalink / raw)
To: u-boot
+ Hua, Wilson
From: Ken Ma
Sent: 2017年3月24日 11:04
To: 'Stefan Roese'; u-boot at lists.denx.de
Cc: Simon Glass; Michal Simek; Kostya Porotchkin
Subject: RE: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
Hi Stefan
Thanks a lot for your kind advice and help!
Please see my reply inline.
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年3月23日 22:06
To: Ken Ma; u-boot at lists.denx.de<mailto:u-boot@lists.denx.de>
Cc: Simon Glass; Michal Simek
Subject: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
External Email
----------------------------------------------------------------------
Hi Ken,
On 23.03.2017 10:29, make at marvell.com<mailto:make@marvell.com> wrote:
> From: Ken Ma <make at marvell.com<mailto:make@marvell.com>>
>
> - Add scsi node which acts as a bus for scsi devices, armada3700 has
> only 1 scsi interface, so max-id is 1, and the logic unit number is
> also 1 for armada3700;
> - Since a3700's scsi is sas(serial attached scsi) which is compatible
> for sata and sata hard disk is a sas device, so move sata node to be
> under scsi node.
>
> Signed-off-by: Ken Ma <make at marvell.com<mailto:make@marvell.com>>
> Cc: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
> Cc: Stefan Roese <sr at denx.de<mailto:sr@denx.de>>
> Cc: Michal Simek <michal.simek at xilinx.com<mailto:michal.simek@xilinx.com>>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
> Tested-by: iSoC Platform CI <ykjenk at marvell.com<mailto:ykjenk@marvell.com>>
> Reviewed-by: Kostya Porotchkin <kostap at marvell.com<mailto:kostap@marvell.com>>
> Reviewed-by: Omri Itach <omrii at marvell.com<mailto:omrii@marvell.com>>
> ---
> arch/arm/dts/armada-3720-db.dts | 4 ++++
> arch/arm/dts/armada-37xx.dtsi | 16 ++++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/armada-3720-db.dts
> b/arch/arm/dts/armada-3720-db.dts index 85761af..9fc60f6 100644
> --- a/arch/arm/dts/armada-3720-db.dts
> +++ b/arch/arm/dts/armada-3720-db.dts
> @@ -89,6 +89,10 @@
> status = "okay";
> };
>
> +&scsi {
> + status = "okay";
> +};
> +
> /* CON3 */
> &sata {
> status = "okay";
> diff --git a/arch/arm/dts/armada-37xx.dtsi
> b/arch/arm/dts/armada-37xx.dtsi index 062f2a6..de5d3a1 100644
> --- a/arch/arm/dts/armada-37xx.dtsi
> +++ b/arch/arm/dts/armada-37xx.dtsi
> @@ -149,11 +149,19 @@
> status = "disabled";
> };
>
> - sata: sata at e0000 {
> - compatible = "marvell,armada-3700-ahci";
> - reg = <0xe0000 0x2000>;
> - interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + scsi: scsi {
> + compatible = "marvell,mvebu-scsi";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + max-id = <1>;
> + max-lun = <1>;
> status = "disabled";
> + sata: sata at e0000 {
> + compatible = "marvell,armada-3700-ahci";
> + reg = <0xe0000 0x2000>;
> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> };
>
> gic: interrupt-controller at 1d00000 {
>
I see that you introduce a "scsi" DT node and move the SATA controller one "level up". I'm not sure if such a change is acceptable as we try to re-use the DT from Linux. Or thinking more about this, I'm pretty sure that such a change is not acceptable in general.
Can't you use the existing DT layout and use the "marvell,armada-3700-ahci" (and other perhaps?) compatible property instead for driver probing? Not sure how to handle the "max-id" and "max-lun" properties though. We definitely can't just add some ad-hoc properties here in U-Boot which have no chance for Linux upstream acceptance.
[Ken] Because scsi is a bus, for example, if there are 2 scsi buses, each bus has some scsi device controllers connected as below.
Scsi ID 0 Scsi ID 1 Scsi ID 2 Scsi ID 3
HDD0 HDD1 tape0 cd-rom0
|| || || ||
===============================================================
SCSI BUS1
HDD2 HDD3 tape1 cd-rom2
|| || || ||
===============================================================
SCSI BUS2
Then in my opinion, since now scsi has its own class id and its compatible string, then the scsi device controllers dts node should be above the scsi node.
If we keep existing DT layout and keep "marvell,armada-3700-ahci"’s uclass id as UCLASS_AHCI(there are no scsi nodes but only ahci nodes), then scsi_scan() can not find a3700’s sata at all since there are no UCLASS_SCSI devices;
If we keep existing DT layout and set scsi devices’ uclass id to be UCLASS_SCSI, how can we know that hdd0 and hdd1 are in scsi bus1 but hdd2 and hdd3 are in scsi bus2? For each scsi bus, its max id should be 4; but now how to set each scsi device’ scsi id?
So I think we should move scsi devices “level up” on the scsio bus.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-24 4:11 ` Ken Ma
@ 2017-03-24 13:21 ` Stefan Roese
2017-03-24 13:24 ` Stefan Roese
2017-03-27 8:28 ` Ken Ma
0 siblings, 2 replies; 32+ messages in thread
From: Stefan Roese @ 2017-03-24 13:21 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 24.03.2017 05:11, Ken Ma wrote:
<snip>
>> b/arch/arm/dts/armada-3720-db.dts index 85761af..9fc60f6 100644
>
>> --- a/arch/arm/dts/armada-3720-db.dts
>
>> +++ b/arch/arm/dts/armada-3720-db.dts
>
>> @@ -89,6 +89,10 @@
>
>> status = "okay";
>
>> };
>
>>
>
>> +&scsi {
>
>> + status = "okay";
>
>> +};
>
>> +
>
>> /* CON3 */
>
>> &sata {
>
>> status = "okay";
>
>> diff --git a/arch/arm/dts/armada-37xx.dtsi
>
>> b/arch/arm/dts/armada-37xx.dtsi index 062f2a6..de5d3a1 100644
>
>> --- a/arch/arm/dts/armada-37xx.dtsi
>
>> +++ b/arch/arm/dts/armada-37xx.dtsi
>
>> @@ -149,11 +149,19 @@
>
>> status = "disabled";
>
>> };
>
>>
>
>> - sata: sata at e0000 {
>
>> - compatible = "marvell,armada-3700-ahci";
>
>> - reg = <0xe0000 0x2000>;
>
>> - interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>
>> + scsi: scsi {
>
>> + compatible = "marvell,mvebu-scsi";
>
>> + #address-cells = <1>;
>
>> + #size-cells = <1>;
>
>> + max-id = <1>;
>
>> + max-lun = <1>;
>
>> status = "disabled";
>
>> + sata: sata at e0000 {
>
>> + compatible = "marvell,armada-3700-ahci";
>
>> + reg = <0xe0000 0x2000>;
>
>> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>
>> + status = "disabled";
>
>> + };
>
>> };
>
>>
>
>> gic: interrupt-controller at 1d00000 {
>
>>
>
>
>
> I see that you introduce a "scsi" DT node and move the SATA controller
> one "level up". I'm not sure if such a change is acceptable as we try to
> re-use the DT from Linux. Or thinking more about this, I'm pretty sure
> that such a change is not acceptable in general.
>
>
>
> Can't you use the existing DT layout and use the
> "marvell,armada-3700-ahci" (and other perhaps?) compatible property
> instead for driver probing? Not sure how to handle the "max-id" and
> "max-lun" properties though. We definitely can't just add some ad-hoc
> properties here in U-Boot which have no chance for Linux upstream
> acceptance.
>
>
>
> [Ken] Because scsi is a bus, for example, if there are 2 scsi buses,
> each bus has some scsi device controllers connected as below.
>
>
> Scsi ID 0 Scsi ID 1 Scsi ID 2 Scsi ID 3
>
>
>
> HDD0 HDD1 tape0 cd-rom0
>
> || || || ||
>
> ===============================================================
>
> SCSI BUS1
>
>
>
> HDD2 HDD3 tape1 cd-rom2
>
> || || || ||
>
> ===============================================================
>
> SCSI BUS2
>
As far as I understand, you are looking at this from the external point
of view (SCSI devices connected to the board). But the DT describes
the hardware / interfaces from the CPU / SoC point of view. The
SCSI bus topology can change, depending on which and how the "user"
connects the multiple SCSI devices to the different controllers.
This is definitely not what we can describe in the DT for the
board. Here only the view of the internal controllers / interfaces
is described (UART controller at 0x..., SPI controller at 0x..,
AHCI controller at 0x..., etc).
> Then in my opinion, since now scsi has its own class id and its
> compatible string, then the scsi device controllers dts node should be
> above the scsi node.
As mentioned before, we are not "free" to insert "virtual" controllers
in the DT. Only real hardware interfaces can be described.
> If we keep existing DT layout and keep "marvell,armada-3700-ahci"’s
> uclass id as UCLASS_AHCI(there are no scsi nodes but only ahci nodes),
> then scsi_scan() can not find a3700’s sata at all since there are no
> UCLASS_SCSI devices;
I've attached the small patch that I've send you a few weeks ago.
Didn't this work at all? IIRC, then the devices connected to the
SATA ports cuold be detected this way.
> If we keep existing DT layout and set scsi devices’ uclass id to be
> UCLASS_SCSI, how can we know that hdd0 and hdd1 are in scsi bus1 but
> hdd2 and hdd3 are in scsi bus2? For each scsi bus, its max id should be
> 4; but now how to set each scsi device’ scsi id?
Not sure if I understand you correctly here. Could you please
describe the problem that you are facing, using an example? That
would be clearer, at least to me.
> So I think we should move scsi devices “level up” on the scsio bus.
Might be that I misunderstand you, but I think you are still
viewing this scenario from the external point of view and not
the internal one (as mentioned before). This is not, what the
DT is supposed to describe though.
Thanks,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ahci-Add-DM-based-support-for-the-Marvell-MVEBU-SATA.patch
Type: text/x-diff
Size: 3502 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170324/866bd363/attachment.patch>
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-24 13:21 ` Stefan Roese
@ 2017-03-24 13:24 ` Stefan Roese
2017-03-27 8:32 ` Ken Ma
2017-03-27 8:28 ` Ken Ma
1 sibling, 1 reply; 32+ messages in thread
From: Stefan Roese @ 2017-03-24 13:24 UTC (permalink / raw)
To: u-boot
Hi Ken,
btw, regarding the max-lun and max-id you could perhaps take
a look at this recent patch, also dealing with these parameters:
https://patchwork.ozlabs.org/patch/743160/
I've not looked too close into this, just wanted to point it
out to you.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-24 13:24 ` Stefan Roese
@ 2017-03-27 8:32 ` Ken Ma
0 siblings, 0 replies; 32+ messages in thread
From: Ken Ma @ 2017-03-27 8:32 UTC (permalink / raw)
To: u-boot
Hi Stefan
I think it's a good way, but I wonder why the codes calls ffs() but not fls()?
If the linkmap is 0xffff, it seems that ffs() returns 1 while fls() returns 16, I think max_id should be 16 then.
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年3月24日 21:24
To: Ken Ma; u-boot at lists.denx.de
Cc: Simon Glass; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
Hi Ken,
btw, regarding the max-lun and max-id you could perhaps take a look at this recent patch, also dealing with these parameters:
https://patchwork.ozlabs.org/patch/743160/
I've not looked too close into this, just wanted to point it out to you.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-24 13:21 ` Stefan Roese
2017-03-24 13:24 ` Stefan Roese
@ 2017-03-27 8:28 ` Ken Ma
2017-04-01 4:22 ` Simon Glass
1 sibling, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-03-27 8:28 UTC (permalink / raw)
To: u-boot
Hi Stefan
Thanks a lot for your kind reply.
But I still do not think it's very good to change sata's uclass id from "UCLASS_AHCI" to "UCLASS_SCSI".
If we do such change, UCLASS_AHCI is lost since from the sata.c codes, it does the AHCI initialization work but not SCSI initialization work.
If u-boot supports ISIS scanner which supports SCSI, I think its uclass id should be like UCLASS_ISIS but not UCLASS_SCSI.
And if we set sata's uclass id as "UCLASS_SCSI", it should provide basic SCSI function, then why can’t we connect a parallel SCSI device like SCSI scanner or cd-rom to the SATA interface?
From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
In general, SATA devices link compatibly to SAS enclosures and adapters, whereas SCSI devices cannot be directly connected to a SATA bus
Actually Marvell’s sata controller is SAS(Serial Attached SCSI system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus which works only in SAS range(for example, 2 sata ports in SAS), so actually the SCSI bus controller is not "virtual" controllers but has the same device base register as SATA.
From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
A typical Serial Attached SCSI system consists of the following basic components:
1. An initiator: a device that originates device-service and task-management<https://en.wikipedia.org/wiki/Task_Management_Function> requests for processing by a target device and receives responses for the same requests from other target devices. Initiators may be provided as an on-board component on the motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter<https://en.wikipedia.org/wiki/Host_bus_adapter>.
2. A target: …
So in my opinion, there are two ways to implement SAS as below
A. If our codes provide SAS controller as an on-board component – then a uclass id of UCLASS_SAS should be defined and then in scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned. In such implementation, UCLASS_SCSI is for parallel SCSI while UCLASS_SAS is for serial attached SCSI;
B. SAS works as an add-on host bus adapter<https://en.wikipedia.org/wiki/Host_bus_adapter> as above said, SAS’s SCSI controller and AHCI controller are both itself as below - SCSI controller is not a virtual device, it exists and only works in SAS internal range(since there is no UCLASS_SAS, I take this way);
Although the SAS’s SCSI controller does not need to any special hardware configuration; but actually I think there is something to do, we should bind special scsi_exec() to SCSI devices in SCSI driver or SAS driver (For different SCSI controls, SAS must have different implementation of scsi_exec() comparing to SCSI scanner, or other SCSI devices)
By the way, I think we should move the work of creating block devices to scsi-uclass.c
scsi: scsi at e0000 {
compatible = "marvell,mvebu-scsi";
reg = <0xe0000 0x2000>;
sata: sata at e0000 {
compatible = "marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>;
interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
};
};
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年3月24日 21:22
To: Ken Ma; u-boot at lists.denx.de
Cc: Simon Glass; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
Hi Ken,
On 24.03.2017 05:11, Ken Ma wrote:
<snip>
>> b/arch/arm/dts/armada-3720-db.dts index 85761af..9fc60f6 100644
>
>> --- a/arch/arm/dts/armada-3720-db.dts
>
>> +++ b/arch/arm/dts/armada-3720-db.dts
>
>> @@ -89,6 +89,10 @@
>
>> status = "okay";
>
>> };
>
>>
>
>> +&scsi {
>
>> + status = "okay";
>
>> +};
>
>> +
>
>> /* CON3 */
>
>> &sata {
>
>> status = "okay";
>
>> diff --git a/arch/arm/dts/armada-37xx.dtsi
>
>> b/arch/arm/dts/armada-37xx.dtsi index 062f2a6..de5d3a1 100644
>
>> --- a/arch/arm/dts/armada-37xx.dtsi
>
>> +++ b/arch/arm/dts/armada-37xx.dtsi
>
>> @@ -149,11 +149,19 @@
>
>> status = "disabled";
>
>> };
>
>>
>
>> - sata: sata at e0000 {
>
>> - compatible = "marvell,armada-3700-ahci";
>
>> - reg = <0xe0000 0x2000>;
>
>> - interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>
>> + scsi: scsi {
>
>> + compatible = "marvell,mvebu-scsi";
>
>> + #address-cells = <1>;
>
>> + #size-cells = <1>;
>
>> + max-id = <1>;
>
>> + max-lun = <1>;
>
>> status = "disabled";
>
>> + sata: sata at e0000 {
>
>> + compatible = "marvell,armada-3700-ahci";
>
>> + reg = <0xe0000 0x2000>;
>
>> + interrupts = <GIC_SPI 27
>> + IRQ_TYPE_LEVEL_HIGH>;
>
>> + status = "disabled";
>
>> + };
>
>> };
>
>>
>
>> gic: interrupt-controller at 1d00000 {
>
>>
>
>
>
> I see that you introduce a "scsi" DT node and move the SATA controller
> one "level up". I'm not sure if such a change is acceptable as we try
> to re-use the DT from Linux. Or thinking more about this, I'm pretty
> sure that such a change is not acceptable in general.
>
>
>
> Can't you use the existing DT layout and use the
> "marvell,armada-3700-ahci" (and other perhaps?) compatible property
> instead for driver probing? Not sure how to handle the "max-id" and
> "max-lun" properties though. We definitely can't just add some ad-hoc
> properties here in U-Boot which have no chance for Linux upstream
> acceptance.
>
>
>
> [Ken] Because scsi is a bus, for example, if there are 2 scsi buses,
> each bus has some scsi device controllers connected as below.
>
>
> Scsi ID 0 Scsi ID 1 Scsi ID 2 Scsi ID 3
>
>
>
> HDD0 HDD1 tape0 cd-rom0
>
> || || || ||
>
> ===============================================================
>
> SCSI BUS1
>
>
>
> HDD2 HDD3 tape1 cd-rom2
>
> || || || ||
>
> ===============================================================
>
> SCSI BUS2
>
As far as I understand, you are looking at this from the external point of view (SCSI devices connected to the board). But the DT describes the hardware / interfaces from the CPU / SoC point of view. The SCSI bus topology can change, depending on which and how the "user"
connects the multiple SCSI devices to the different controllers.
This is definitely not what we can describe in the DT for the board. Here only the view of the internal controllers / interfaces is described (UART controller at 0x..., SPI controller at 0x.., AHCI controller at 0x..., etc).
> Then in my opinion, since now scsi has its own class id and its
> compatible string, then the scsi device controllers dts node should be
> above the scsi node.
As mentioned before, we are not "free" to insert "virtual" controllers in the DT. Only real hardware interfaces can be described.
> If we keep existing DT layout and keep "marvell,armada-3700-ahci"’s
> uclass id as UCLASS_AHCI(there are no scsi nodes but only ahci nodes),
> then scsi_scan() can not find a3700’s sata at all since there are no
> UCLASS_SCSI devices;
I've attached the small patch that I've send you a few weeks ago.
Didn't this work at all? IIRC, then the devices connected to the SATA ports cuold be detected this way.
> If we keep existing DT layout and set scsi devices’ uclass id to be
> UCLASS_SCSI, how can we know that hdd0 and hdd1 are in scsi bus1 but
> hdd2 and hdd3 are in scsi bus2? For each scsi bus, its max id should
> be 4; but now how to set each scsi device’ scsi id?
Not sure if I understand you correctly here. Could you please describe the problem that you are facing, using an example? That would be clearer, at least to me.
> So I think we should move scsi devices “level up” on the scsio bus.
Might be that I misunderstand you, but I think you are still viewing this scenario from the external point of view and not the internal one (as mentioned before). This is not, what the DT is supposed to describe though.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-03-27 8:28 ` Ken Ma
@ 2017-04-01 4:22 ` Simon Glass
2017-04-03 6:13 ` Stefan Roese
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:22 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
> Hi Stefan
>
>
>
> Thanks a lot for your kind reply.
>
>
>
> But I still do not think it's very good to change sata's uclass id from
> "UCLASS_AHCI" to "UCLASS_SCSI".
>
> If we do such change, UCLASS_AHCI is lost since from the sata.c codes, it
> does the AHCI initialization work but not SCSI initialization work.
>
> If u-boot supports ISIS scanner which supports SCSI, I think its uclass id
> should be like UCLASS_ISIS but not UCLASS_SCSI.
>
> And if we set sata's uclass id as "UCLASS_SCSI", it should provide basic
> SCSI function, then why can’t we connect a parallel SCSI device like SCSI
> scanner or cd-rom to the SATA interface?
>
> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>
> In general, SATA devices link compatibly to SAS enclosures and adapters,
> whereas SCSI devices cannot be directly connected to a SATA bus
>
>
>
>
>
> Actually Marvell’s sata controller is SAS(Serial Attached SCSI system), it
> integrates SCSI and SATA(AHCI); SAS provides a SCSI bus which works only in
> SAS range(for example, 2 sata ports in SAS), so actually the SCSI bus
> controller is not "virtual" controllers but has the same device base
> register as SATA.
>
>
>
> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>
> A typical Serial Attached SCSI system consists of the following basic
> components:
>
> 1. An initiator: a device that originates device-service and
> task-management requests for processing by a target device and receives
> responses for the same requests from other target devices. Initiators may be
> provided as an on-board component on the motherboard (as is the case with
> many server-oriented motherboards) or as an add-on host bus adapter.
>
> 2. A target: …
>
> So in my opinion, there are two ways to implement SAS as below
>
> A. If our codes provide SAS controller as an on-board component – then a
> uclass id of UCLASS_SAS should be defined and then in scsi_scan() of
> scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
> In such implementation, UCLASS_SCSI is for parallel SCSI while UCLASS_SAS is
> for serial attached SCSI;
>
> B. SAS works as an add-on host bus adapter as above said, SAS’s SCSI
> controller and AHCI controller are both itself as below - SCSI controller is
> not a virtual device, it exists and only works in SAS internal range(since
> there is no UCLASS_SAS, I take this way);
>
> Although the SAS’s SCSI controller does not need to any special hardware
> configuration; but actually I think there is something to do, we should bind
> special scsi_exec() to SCSI devices in SCSI driver or SAS driver (For
> different SCSI controls, SAS must have different implementation of
> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>
> By the way, I think we should move the work of creating block devices to
> scsi-uclass.c
>
> scsi: scsi at e0000 {
>
> compatible = "marvell,mvebu-scsi";
>
> reg = <0xe0000 0x2000>;
>
> sata: sata at e0000 {
>
> compatible = "marvell,armada-3700-ahci";
>
> reg = <0xe0000 0x2000>;
>
> interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>
> };
Does this match the Linux DT?
>
> };
>
>
>
>
>
> Yours,
>
> Ken
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-04-01 4:22 ` Simon Glass
@ 2017-04-03 6:13 ` Stefan Roese
2017-04-05 9:29 ` Ken Ma
0 siblings, 1 reply; 32+ messages in thread
From: Stefan Roese @ 2017-04-03 6:13 UTC (permalink / raw)
To: u-boot
Hi Simon, Hi Ken,
On 01.04.2017 06:22, Simon Glass wrote:
> On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
>> Hi Stefan
>>
>>
>>
>> Thanks a lot for your kind reply.
>>
>>
>>
>> But I still do not think it's very good to change sata's uclass id from
>> "UCLASS_AHCI" to "UCLASS_SCSI".
>>
>> If we do such change, UCLASS_AHCI is lost since from the sata.c codes, it
>> does the AHCI initialization work but not SCSI initialization work.
>>
>> If u-boot supports ISIS scanner which supports SCSI, I think its uclass id
>> should be like UCLASS_ISIS but not UCLASS_SCSI.
>>
>> And if we set sata's uclass id as "UCLASS_SCSI", it should provide basic
>> SCSI function, then why can’t we connect a parallel SCSI device like SCSI
>> scanner or cd-rom to the SATA interface?
>>
>> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>>
>> In general, SATA devices link compatibly to SAS enclosures and adapters,
>> whereas SCSI devices cannot be directly connected to a SATA bus
>>
>>
>>
>>
>>
>> Actually Marvell’s sata controller is SAS(Serial Attached SCSI system), it
>> integrates SCSI and SATA(AHCI); SAS provides a SCSI bus which works only in
>> SAS range(for example, 2 sata ports in SAS), so actually the SCSI bus
>> controller is not "virtual" controllers but has the same device base
>> register as SATA.
>>
>>
>>
>> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>>
>> A typical Serial Attached SCSI system consists of the following basic
>> components:
>>
>> 1. An initiator: a device that originates device-service and
>> task-management requests for processing by a target device and receives
>> responses for the same requests from other target devices. Initiators may be
>> provided as an on-board component on the motherboard (as is the case with
>> many server-oriented motherboards) or as an add-on host bus adapter.
>>
>> 2. A target: …
>>
>> So in my opinion, there are two ways to implement SAS as below
>>
>> A. If our codes provide SAS controller as an on-board component – then a
>> uclass id of UCLASS_SAS should be defined and then in scsi_scan() of
>> scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
>> In such implementation, UCLASS_SCSI is for parallel SCSI while UCLASS_SAS is
>> for serial attached SCSI;
>>
>> B. SAS works as an add-on host bus adapter as above said, SAS’s SCSI
>> controller and AHCI controller are both itself as below - SCSI controller is
>> not a virtual device, it exists and only works in SAS internal range(since
>> there is no UCLASS_SAS, I take this way);
>>
>> Although the SAS’s SCSI controller does not need to any special hardware
>> configuration; but actually I think there is something to do, we should bind
>> special scsi_exec() to SCSI devices in SCSI driver or SAS driver (For
>> different SCSI controls, SAS must have different implementation of
>> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>>
>> By the way, I think we should move the work of creating block devices to
>> scsi-uclass.c
>>
>> scsi: scsi at e0000 {
>>
>> compatible = "marvell,mvebu-scsi";
>>
>> reg = <0xe0000 0x2000>;
>>
>> sata: sata at e0000 {
>>
>> compatible = "marvell,armada-3700-ahci";
>>
>> reg = <0xe0000 0x2000>;
>>
>> interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>>
>> };
>
> Does this match the Linux DT?
No, and this is my main concern. This patch series introduces a new
"scsi" DT node and moves the controller(s) one level up into this
newly created DT node (Ken please correct me if I'm wrong here).
We simply can't make such changes here in U-Boot without this
being first discussed and decided on the Linux DT mailing list
with the DT maintainers.
Ken, how is this problem solved / handled in Linux without this DT
changes up until now?
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-04-03 6:13 ` Stefan Roese
@ 2017-04-05 9:29 ` Ken Ma
2017-04-05 13:45 ` Stefan Roese
0 siblings, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-04-05 9:29 UTC (permalink / raw)
To: u-boot
Hi Stefan, Hi Simon
Please see my inline reply, thanks!
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年4月3日 14:14
To: Simon Glass; Ken Ma
Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
Hi Simon, Hi Ken,
On 01.04.2017 06:22, Simon Glass wrote:
> On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
>> Hi Stefan
>>
>>
>>
>> Thanks a lot for your kind reply.
>>
>>
>>
>> But I still do not think it's very good to change sata's uclass id
>> from "UCLASS_AHCI" to "UCLASS_SCSI".
>>
>> If we do such change, UCLASS_AHCI is lost since from the sata.c
>> codes, it does the AHCI initialization work but not SCSI initialization work.
>>
>> If u-boot supports ISIS scanner which supports SCSI, I think its
>> uclass id should be like UCLASS_ISIS but not UCLASS_SCSI.
>>
>> And if we set sata's uclass id as "UCLASS_SCSI", it should provide
>> basic SCSI function, then why can’t we connect a parallel SCSI device
>> like SCSI scanner or cd-rom to the SATA interface?
>>
>> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>>
>> In general, SATA devices link compatibly to SAS enclosures and
>> adapters, whereas SCSI devices cannot be directly connected to a SATA
>> bus
>>
>>
>>
>>
>>
>> Actually Marvell’s sata controller is SAS(Serial Attached SCSI
>> system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus
>> which works only in SAS range(for example, 2 sata ports in SAS), so
>> actually the SCSI bus controller is not "virtual" controllers but has
>> the same device base register as SATA.
>>
>>
>>
>> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>>
>> A typical Serial Attached SCSI system consists of the following basic
>> components:
>>
>> 1. An initiator: a device that originates device-service and
>> task-management requests for processing by a target device and
>> receives responses for the same requests from other target devices.
>> Initiators may be provided as an on-board component on the
>> motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter.
>>
>> 2. A target: …
>>
>> So in my opinion, there are two ways to implement SAS as below
>>
>> A. If our codes provide SAS controller as an on-board component –
>> then a uclass id of UCLASS_SAS should be defined and then in
>> scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
>> In such implementation, UCLASS_SCSI is for parallel SCSI while
>> UCLASS_SAS is for serial attached SCSI;
>>
>> B. SAS works as an add-on host bus adapter as above said, SAS’s SCSI
>> controller and AHCI controller are both itself as below - SCSI
>> controller is not a virtual device, it exists and only works in SAS
>> internal range(since there is no UCLASS_SAS, I take this way);
>>
>> Although the SAS’s SCSI controller does not need to any special
>> hardware configuration; but actually I think there is something to
>> do, we should bind special scsi_exec() to SCSI devices in SCSI driver
>> or SAS driver (For different SCSI controls, SAS must have different
>> implementation of
>> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>>
>> By the way, I think we should move the work of creating block devices
>> to scsi-uclass.c
>>
>> scsi: scsi at e0000 {
>>
>> compatible = "marvell,mvebu-scsi";
>>
>> reg = <0xe0000 0x2000>;
>>
>> sata: sata at e0000 {
>>
>> compatible =
>> "marvell,armada-3700-ahci";
>>
>> reg = <0xe0000 0x2000>;
>>
>> interrupts = <GIC_SPI 27
>> IRQ_TYPE_LEVEL_HIGH>;
>>
>> };
>
> Does this match the Linux DT?
No, and this is my main concern. This patch series introduces a new "scsi" DT node and moves the controller(s) one level up into this newly created DT node (Ken please correct me if I'm wrong here).
We simply can't make such changes here in U-Boot without this being first discussed and decided on the Linux DT mailing list with the DT maintainers.
Ken, how is this problem solved / handled in Linux without this DT changes up until now?
[Ken] Actually I do not find any SCSI nodes/compatible strings In Linux; if aligned to linux, then we should have no scsi nodes at all in u-boot.
I am not familiar with Linux SCSI implementation, it seems that SCSI bus is implemented as a middle layer in Linux since it has no SCSI fdt nodes.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-04-05 9:29 ` Ken Ma
@ 2017-04-05 13:45 ` Stefan Roese
2017-04-06 1:32 ` Ken Ma
0 siblings, 1 reply; 32+ messages in thread
From: Stefan Roese @ 2017-04-05 13:45 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 05.04.2017 11:29, Ken Ma wrote:
> Hi Stefan, Hi Simon
>
> Please see my inline reply, thanks!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de]
> Sent: 2017年4月3日 14:14
> To: Simon Glass; Ken Ma
> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>
> Hi Simon, Hi Ken,
>
> On 01.04.2017 06:22, Simon Glass wrote:
>> On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
>>> Hi Stefan
>>>
>>>
>>>
>>> Thanks a lot for your kind reply.
>>>
>>>
>>>
>>> But I still do not think it's very good to change sata's uclass id
>>> from "UCLASS_AHCI" to "UCLASS_SCSI".
>>>
>>> If we do such change, UCLASS_AHCI is lost since from the sata.c
>>> codes, it does the AHCI initialization work but not SCSI initialization work.
>>>
>>> If u-boot supports ISIS scanner which supports SCSI, I think its
>>> uclass id should be like UCLASS_ISIS but not UCLASS_SCSI.
>>>
>>> And if we set sata's uclass id as "UCLASS_SCSI", it should provide
>>> basic SCSI function, then why can’t we connect a parallel SCSI device
>>> like SCSI scanner or cd-rom to the SATA interface?
>>>
>>> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>>>
>>> In general, SATA devices link compatibly to SAS enclosures and
>>> adapters, whereas SCSI devices cannot be directly connected to a SATA
>>> bus
>>>
>>>
>>>
>>>
>>>
>>> Actually Marvell’s sata controller is SAS(Serial Attached SCSI
>>> system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus
>>> which works only in SAS range(for example, 2 sata ports in SAS), so
>>> actually the SCSI bus controller is not "virtual" controllers but has
>>> the same device base register as SATA.
>>>
>>>
>>>
>>> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>>>
>>> A typical Serial Attached SCSI system consists of the following basic
>>> components:
>>>
>>> 1. An initiator: a device that originates device-service and
>>> task-management requests for processing by a target device and
>>> receives responses for the same requests from other target devices.
>>> Initiators may be provided as an on-board component on the
>>> motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter.
>>>
>>> 2. A target: …
>>>
>>> So in my opinion, there are two ways to implement SAS as below
>>>
>>> A. If our codes provide SAS controller as an on-board component –
>>> then a uclass id of UCLASS_SAS should be defined and then in
>>> scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
>>> In such implementation, UCLASS_SCSI is for parallel SCSI while
>>> UCLASS_SAS is for serial attached SCSI;
>>>
>>> B. SAS works as an add-on host bus adapter as above said, SAS’s SCSI
>>> controller and AHCI controller are both itself as below - SCSI
>>> controller is not a virtual device, it exists and only works in SAS
>>> internal range(since there is no UCLASS_SAS, I take this way);
>>>
>>> Although the SAS’s SCSI controller does not need to any special
>>> hardware configuration; but actually I think there is something to
>>> do, we should bind special scsi_exec() to SCSI devices in SCSI driver
>>> or SAS driver (For different SCSI controls, SAS must have different
>>> implementation of
>>> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>>>
>>> By the way, I think we should move the work of creating block devices
>>> to scsi-uclass.c
>>>
>>> scsi: scsi at e0000 {
>>>
>>> compatible = "marvell,mvebu-scsi";
>>>
>>> reg = <0xe0000 0x2000>;
>>>
>>> sata: sata at e0000 {
>>>
>>> compatible =
>>> "marvell,armada-3700-ahci";
>>>
>>> reg = <0xe0000 0x2000>;
>>>
>>> interrupts = <GIC_SPI 27
>>> IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> };
>>
>> Does this match the Linux DT?
>
> No, and this is my main concern. This patch series introduces a
> new "scsi" DT node and moves the controller(s) one level up into
> this newly created DT node (Ken please correct me if I'm wrong
> here).
> We simply can't make such changes here in U-Boot without this
> being first discussed and decided on the Linux DT mailing list
> with the DT maintainers.
>
> Ken, how is this problem solved / handled in Linux without this
> DT changes up until now?
>
> [Ken] Actually I do not find any SCSI nodes/compatible strings
> In Linux; if aligned to linux, then we should have no scsi nodes
> at all in u-boot.
Thats exactly what I meant. The DT represents the hardware and
only controllers / devices etc are listed here. As such, only the
SATA, AHCI, IDE (etc.) controllers are listed behind their internal
SoC / CPU busses in the DT. Unfortunately we can't come up with
this new SCSI DT node for U-Boot and move all the controllers
into this node, as we need to try to stay in sync with the Linux
DT, which is the reference.
> I am not familiar with Linux SCSI implementation, it seems that
> SCSI bus is implemented as a middle layer in Linux since it has
> no SCSI fdt nodes.
Frankly, I've not looked at SCSI in Linux for quite some time.
So I can't really tell you how this is handled there. But I'm
pretty sure that no SCSI DT nodes / properties are involved
here.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-04-05 13:45 ` Stefan Roese
@ 2017-04-06 1:32 ` Ken Ma
2017-04-09 19:28 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Ken Ma @ 2017-04-06 1:32 UTC (permalink / raw)
To: u-boot
Hi Stefan
Please see my inline reply, thanks!
Yours,
Ken
-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年4月5日 21:46
To: Ken Ma; Simon Glass
Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
Hi Ken,
On 05.04.2017 11:29, Ken Ma wrote:
> Hi Stefan, Hi Simon
>
> Please see my inline reply, thanks!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de]
> Sent: 2017年4月3日 14:14
> To: Simon Glass; Ken Ma
> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing;
> Wilson Ding
> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>
> Hi Simon, Hi Ken,
>
> On 01.04.2017 06:22, Simon Glass wrote:
>> On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
>>> Hi Stefan
>>>
>>>
>>>
>>> Thanks a lot for your kind reply.
>>>
>>>
>>>
>>> But I still do not think it's very good to change sata's uclass id
>>> from "UCLASS_AHCI" to "UCLASS_SCSI".
>>>
>>> If we do such change, UCLASS_AHCI is lost since from the sata.c
>>> codes, it does the AHCI initialization work but not SCSI initialization work.
>>>
>>> If u-boot supports ISIS scanner which supports SCSI, I think its
>>> uclass id should be like UCLASS_ISIS but not UCLASS_SCSI.
>>>
>>> And if we set sata's uclass id as "UCLASS_SCSI", it should provide
>>> basic SCSI function, then why can’t we connect a parallel SCSI
>>> device like SCSI scanner or cd-rom to the SATA interface?
>>>
>>> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>>>
>>> In general, SATA devices link compatibly to SAS enclosures and
>>> adapters, whereas SCSI devices cannot be directly connected to a
>>> SATA bus
>>>
>>>
>>>
>>>
>>>
>>> Actually Marvell’s sata controller is SAS(Serial Attached SCSI
>>> system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus
>>> which works only in SAS range(for example, 2 sata ports in SAS), so
>>> actually the SCSI bus controller is not "virtual" controllers but
>>> has the same device base register as SATA.
>>>
>>>
>>>
>>> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>>>
>>> A typical Serial Attached SCSI system consists of the following
>>> basic
>>> components:
>>>
>>> 1. An initiator: a device that originates device-service and
>>> task-management requests for processing by a target device and
>>> receives responses for the same requests from other target devices.
>>> Initiators may be provided as an on-board component on the
>>> motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter.
>>>
>>> 2. A target: …
>>>
>>> So in my opinion, there are two ways to implement SAS as below
>>>
>>> A. If our codes provide SAS controller as an on-board component –
>>> then a uclass id of UCLASS_SAS should be defined and then in
>>> scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
>>> In such implementation, UCLASS_SCSI is for parallel SCSI while
>>> UCLASS_SAS is for serial attached SCSI;
>>>
>>> B. SAS works as an add-on host bus adapter as above said, SAS’s
>>> SCSI controller and AHCI controller are both itself as below - SCSI
>>> controller is not a virtual device, it exists and only works in SAS
>>> internal range(since there is no UCLASS_SAS, I take this way);
>>>
>>> Although the SAS’s SCSI controller does not need to any special
>>> hardware configuration; but actually I think there is something to
>>> do, we should bind special scsi_exec() to SCSI devices in SCSI
>>> driver or SAS driver (For different SCSI controls, SAS must have
>>> different implementation of
>>> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>>>
>>> By the way, I think we should move the work of creating block
>>> devices to scsi-uclass.c
>>>
>>> scsi: scsi at e0000 {
>>>
>>> compatible = "marvell,mvebu-scsi";
>>>
>>> reg = <0xe0000 0x2000>;
>>>
>>> sata: sata at e0000 {
>>>
>>> compatible =
>>> "marvell,armada-3700-ahci";
>>>
>>> reg = <0xe0000 0x2000>;
>>>
>>> interrupts = <GIC_SPI 27
>>> IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> };
>>
>> Does this match the Linux DT?
>
> No, and this is my main concern. This patch series introduces a new
> "scsi" DT node and moves the controller(s) one level up into this
> newly created DT node (Ken please correct me if I'm wrong here).
> We simply can't make such changes here in U-Boot without this being
> first discussed and decided on the Linux DT mailing list with the DT
> maintainers.
>
> Ken, how is this problem solved / handled in Linux without this DT
> changes up until now?
>
> [Ken] Actually I do not find any SCSI nodes/compatible strings In
> Linux; if aligned to linux, then we should have no scsi nodes at all
> in u-boot.
Thats exactly what I meant. The DT represents the hardware and only controllers / devices etc are listed here. As such, only the SATA, AHCI, IDE (etc.) controllers are listed behind their internal SoC / CPU busses in the DT. Unfortunately we can't come up with this new SCSI DT node for U-Boot and move all the controllers into this node, as we need to try to stay in sync with the Linux DT, which is the reference.
[Ken] If aligned to linux, we should not add the class id of "UCLASS_SCSI" but add a middle scsi layer. We can register to scsi in scsi device driver(like sata.c) after adding some middle scsi layer, but we should not use UCLASS_SCSI to replace UCLASS_AHCI directly.
Now in u-boot scsi_scan() uses UCLASS_SCSI objects as scsi bus actually, if there are 2 different scsi devices in one same scsi bus and we classify the 2 different scsi devices into UCLASS_SCSI, then scsi_scan() will regard as there are 2 scsi buses but not one. We should not classify scsi devices into UCLASS_SCSI.
In my understanding, since in u-boot UCLASS_SCSI is for SCSI host controller(bus function), we should have such a device bus node.
Linux SCSI freamework
Upper level: Disk driver(sd) tape driver(st) CDROM driver(sr) Generic driver(sg)
Middle level: common service layer
Lower level: Fibre channel driver SAS driver iSCSI driver ... Bridge driver
> I am not familiar with Linux SCSI implementation, it seems that SCSI
> bus is implemented as a middle layer in Linux since it has no SCSI fdt
> nodes.
Frankly, I've not looked at SCSI in Linux for quite some time.
So I can't really tell you how this is handled there. But I'm pretty sure that no SCSI DT nodes / properties are involved here.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 32+ messages in thread* [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
2017-04-06 1:32 ` Ken Ma
@ 2017-04-09 19:28 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-09 19:28 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 5 April 2017 at 19:32, Ken Ma <make@marvell.com> wrote:
> Hi Stefan
>
> Please see my inline reply, thanks!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de]
> Sent: 2017年4月5日 21:46
> To: Ken Ma; Simon Glass
> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>
> Hi Ken,
>
> On 05.04.2017 11:29, Ken Ma wrote:
>> Hi Stefan, Hi Simon
>>
>> Please see my inline reply, thanks!
>>
>> Yours,
>> Ken
>>
>> -----Original Message-----
>> From: Stefan Roese [mailto:sr at denx.de]
>> Sent: 2017年4月3日 14:14
>> To: Simon Glass; Ken Ma
>> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing;
>> Wilson Ding
>> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>>
>> Hi Simon, Hi Ken,
>>
>> On 01.04.2017 06:22, Simon Glass wrote:
>>> On 27 March 2017 at 02:28, Ken Ma <make@marvell.com> wrote:
>>>> Hi Stefan
>>>>
>>>>
>>>>
>>>> Thanks a lot for your kind reply.
>>>>
>>>>
>>>>
>>>> But I still do not think it's very good to change sata's uclass id
>>>> from "UCLASS_AHCI" to "UCLASS_SCSI".
>>>>
>>>> If we do such change, UCLASS_AHCI is lost since from the sata.c
>>>> codes, it does the AHCI initialization work but not SCSI initialization work.
>>>>
>>>> If u-boot supports ISIS scanner which supports SCSI, I think its
>>>> uclass id should be like UCLASS_ISIS but not UCLASS_SCSI.
>>>>
>>>> And if we set sata's uclass id as "UCLASS_SCSI", it should provide
>>>> basic SCSI function, then why can’t we connect a parallel SCSI
>>>> device like SCSI scanner or cd-rom to the SATA interface?
>>>>
>>>> From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI
>>>>
>>>> In general, SATA devices link compatibly to SAS enclosures and
>>>> adapters, whereas SCSI devices cannot be directly connected to a
>>>> SATA bus
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Actually Marvell’s sata controller is SAS(Serial Attached SCSI
>>>> system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus
>>>> which works only in SAS range(for example, 2 sata ports in SAS), so
>>>> actually the SCSI bus controller is not "virtual" controllers but
>>>> has the same device base register as SATA.
>>>>
>>>>
>>>>
>>>> From https://en.wikipedia.org/wiki/Serial_Attached_SCSI
>>>>
>>>> A typical Serial Attached SCSI system consists of the following
>>>> basic
>>>> components:
>>>>
>>>> 1. An initiator: a device that originates device-service and
>>>> task-management requests for processing by a target device and
>>>> receives responses for the same requests from other target devices.
>>>> Initiators may be provided as an on-board component on the
>>>> motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter.
>>>>
>>>> 2. A target: …
>>>>
>>>> So in my opinion, there are two ways to implement SAS as below
>>>>
>>>> A. If our codes provide SAS controller as an on-board component –
>>>> then a uclass id of UCLASS_SAS should be defined and then in
>>>> scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned.
>>>> In such implementation, UCLASS_SCSI is for parallel SCSI while
>>>> UCLASS_SAS is for serial attached SCSI;
>>>>
>>>> B. SAS works as an add-on host bus adapter as above said, SAS’s
>>>> SCSI controller and AHCI controller are both itself as below - SCSI
>>>> controller is not a virtual device, it exists and only works in SAS
>>>> internal range(since there is no UCLASS_SAS, I take this way);
>>>>
>>>> Although the SAS’s SCSI controller does not need to any special
>>>> hardware configuration; but actually I think there is something to
>>>> do, we should bind special scsi_exec() to SCSI devices in SCSI
>>>> driver or SAS driver (For different SCSI controls, SAS must have
>>>> different implementation of
>>>> scsi_exec() comparing to SCSI scanner, or other SCSI devices)
>>>>
>>>> By the way, I think we should move the work of creating block
>>>> devices to scsi-uclass.c
>>>>
>>>> scsi: scsi at e0000 {
>>>>
>>>> compatible = "marvell,mvebu-scsi";
>>>>
>>>> reg = <0xe0000 0x2000>;
>>>>
>>>> sata: sata at e0000 {
>>>>
>>>> compatible =
>>>> "marvell,armada-3700-ahci";
>>>>
>>>> reg = <0xe0000 0x2000>;
>>>>
>>>> interrupts = <GIC_SPI 27
>>>> IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>> };
>>>
>>> Does this match the Linux DT?
>>
>> No, and this is my main concern. This patch series introduces a new
>> "scsi" DT node and moves the controller(s) one level up into this
>> newly created DT node (Ken please correct me if I'm wrong here).
>> We simply can't make such changes here in U-Boot without this being
>> first discussed and decided on the Linux DT mailing list with the DT
>> maintainers.
>>
>> Ken, how is this problem solved / handled in Linux without this DT
>> changes up until now?
>>
>> [Ken] Actually I do not find any SCSI nodes/compatible strings In
>> Linux; if aligned to linux, then we should have no scsi nodes at all
>> in u-boot.
>
> Thats exactly what I meant. The DT represents the hardware and only controllers / devices etc are listed here. As such, only the SATA, AHCI, IDE (etc.) controllers are listed behind their internal SoC / CPU busses in the DT. Unfortunately we can't come up with this new SCSI DT node for U-Boot and move all the controllers into this node, as we need to try to stay in sync with the Linux DT, which is the reference.
>
> [Ken] If aligned to linux, we should not add the class id of "UCLASS_SCSI" but add a middle scsi layer. We can register to scsi in scsi device driver(like sata.c) after adding some middle scsi layer, but we should not use UCLASS_SCSI to replace UCLASS_AHCI directly.
>
> Now in u-boot scsi_scan() uses UCLASS_SCSI objects as scsi bus actually, if there are 2 different scsi devices in one same scsi bus and we classify the 2 different scsi devices into UCLASS_SCSI, then scsi_scan() will regard as there are 2 scsi buses but not one. We should not classify scsi devices into UCLASS_SCSI.
>
> In my understanding, since in u-boot UCLASS_SCSI is for SCSI host controller(bus function), we should have such a device bus node.
>
> Linux SCSI freamework
> Upper level: Disk driver(sd) tape driver(st) CDROM driver(sr) Generic driver(sg)
> Middle level: common service layer
> Lower level: Fibre channel driver SAS driver iSCSI driver ... Bridge driver
>
>> I am not familiar with Linux SCSI implementation, it seems that SCSI
>> bus is implemented as a middle layer in Linux since it has no SCSI fdt
>> nodes.
>
> Frankly, I've not looked at SCSI in Linux for quite some time.
> So I can't really tell you how this is handled there. But I'm pretty sure that no SCSI DT nodes / properties are involved here.
There is something funny with your email Ken. Normally you should
quote the original email and add your reply in unquoted lines. Somehow
your reply seems to be quoted to, so we cannot see who is replying to
who.
As you say, SCSI is for the controller and its bus. It would be the
parent device.
The bus has multiple devices e.g. AHCI. So I agree that we should not
combine AHCI and SCSI.
However, I hope that we don't need a separate uclass for the middle
layer. Hopefully it can just be some helper functions.
Overall there is quite a bit of work to do on SCSI for DM still. For
one, we need a sandbox driver and a test, similar to what was done for
USB.
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [PATCH 0/7] *** SUBJECT HERE ***
2017-03-23 9:29 [U-Boot] [PATCH 0/7] *** SUBJECT HERE *** make at marvell.com
` (6 preceding siblings ...)
2017-03-23 9:29 ` [U-Boot] [PATCH 7/7] scsi: dts: a3700: add scsi node make at marvell.com
@ 2017-04-01 4:21 ` Simon Glass
7 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2017-04-01 4:21 UTC (permalink / raw)
To: u-boot
Hi Ken,
On 23 March 2017 at 03:29, <make@marvell.com> wrote:
> From: Ken Ma <make@marvell.com>
>
> *** BLURB HERE ***
You might want to try patman which can generate patches, cover letter
and change logs for you.
> 1. Move base, max_lun and max_id such scsi generic data from platdata to uclass plat data;
> 2. Make scsi compatible for legacy SCSI devices and new SAS controller
> - Introduce scsi bus DT node, scsi work as bus and scsi disks, scsi scanner and sata are
> its children scsi device; this is similar to the case that spi bus manages spi flashes;
> In such case, scsi bus probe should probe its children devices automatically;
> - SAS controller can also be a scsi node as current.
> 3. Example with mvebu armada 3700 scsi bus node
This looks good as far as it goes. As a general comment I'd like to
see SCSI operations (as we have e.g. for MMC - struct dm_mmc_ops) so
that we can move it fully to DM.
>
> Ken Ma (7):
> scsi: move base, max_lun and max_id to uclass plat data
> scsi: add children devices binding
> scsi: call children devices' probe functions automatically
> scsi: dt-bindings: add scsi device tree bindings
> scsi: mvebu: add scsi driver
> scsi: a3700: enable mvebu scsi driver
> scsi: dts: a3700: add scsi node
>
> arch/arm/dts/armada-3720-db.dts | 4 ++
> arch/arm/dts/armada-37xx.dtsi | 16 +++++--
> common/scsi.c | 2 +-
> configs/mvebu_db-88f3720_defconfig | 2 +
> .../scsi/marvell,mvebu-scsi.txt | 29 ++++++++++++
> doc/device-tree-bindings/scsi/scsi-bus.txt | 22 +++++++++
> drivers/block/Kconfig | 10 ++++
> drivers/block/Makefile | 1 +
> drivers/block/ahci.c | 2 +-
> drivers/block/mvebu_scsi.c | 31 +++++++++++++
> drivers/block/scsi-uclass.c | 54 +++++++++++++++++++++-
> 11 files changed, 165 insertions(+), 8 deletions(-)
> create mode 100644 doc/device-tree-bindings/scsi/marvell,mvebu-scsi.txt
> create mode 100644 doc/device-tree-bindings/scsi/scsi-bus.txt
> create mode 100644 drivers/block/mvebu_scsi.c
>
> --
> 1.9.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Regards,
Simon
^ permalink raw reply [flat|nested] 32+ messages in thread