[U-Boot] [PATCH 0/7] *** SUBJECT HERE ***
From: Ken Ma make@marvell.com
*** BLURB HERE *** 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
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
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), };
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
Hi Simon
Please see my inline reply, thanks a lot!
-----Original Message----- From: sjg@google.com [mailto:sjg@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; #endifdiff --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
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@google.com [mailto:sjg@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; #endifdiff --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
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) = {
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
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), };
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
Hi Simon
Please see my inline reply, thanks a lot!
Yours, Ken
-----Original Message----- From: sjg@google.com [mailto:sjg@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(structscsi_platdata), };
1.9.1
Regards, Simon
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@google.com [mailto:sjg@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(structscsi_platdata), };
1.9.1
Regards, Simon
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@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
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
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, +}; +
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
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
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
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@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@e0000 { + compatible = "marvell,armada-3700-ahci"; + reg = <0xe0000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + }; };
gic: interrupt-controller@1d00000 {
Hi Ken,
On 23.03.2017 10:29, make@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@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@e0000 {compatible = "marvell,armada-3700-ahci";reg = <0xe0000 0x2000>;interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;status = "disabled";}; }; gic: interrupt-controller@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
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@denx.de] Sent: 2017年3月23日 22:06 To: Ken Ma; 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@marvell.commailto:make@marvell.com wrote:
From: Ken Ma <make@marvell.commailto: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.commailto:make@marvell.com>
Cc: Simon Glass <sjg@chromium.orgmailto:sjg@chromium.org>
Cc: Stefan Roese <sr@denx.demailto:sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.commailto:michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
Tested-by: iSoC Platform CI <ykjenk@marvell.commailto:ykjenk@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.commailto:kostap@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.commailto: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@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@e0000 {
compatible = "marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>;
interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
};
};
gic: interrupt-controller@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
+ Hua, Wilson
From: Ken Ma Sent: 2017年3月24日 11:04 To: 'Stefan Roese'; u-boot@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@denx.de] Sent: 2017年3月23日 22:06 To: Ken Ma; u-boot@lists.denx.demailto: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@marvell.commailto:make@marvell.com wrote:
From: Ken Ma <make@marvell.commailto: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.commailto:make@marvell.com>
Cc: Simon Glass <sjg@chromium.orgmailto:sjg@chromium.org>
Cc: Stefan Roese <sr@denx.demailto:sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.commailto:michal.simek@xilinx.com>
Reviewed-on: http://vgitil04.il.marvell.com:8080/35303
Tested-by: iSoC Platform CI <ykjenk@marvell.commailto:ykjenk@marvell.com>
Reviewed-by: Kostya Porotchkin <kostap@marvell.commailto:kostap@marvell.com>
Reviewed-by: Omri Itach <omrii@marvell.commailto: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@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@e0000 {
compatible = "marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>;
interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
};
};
gic: interrupt-controller@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
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@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@e0000 {
compatible = "marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>;
interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
};};gic: interrupt-controller@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 BUS1HDD2 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
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
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@denx.de] Sent: 2017年3月24日 21:24 To: Ken Ma; u-boot@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
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-managementhttps://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 adapterhttps://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 adapterhttps://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@e0000 {
compatible = "marvell,mvebu-scsi";
reg = <0xe0000 0x2000>;
sata: sata@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@denx.de] Sent: 2017年3月24日 21:22 To: Ken Ma; u-boot@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@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@e0000 {
compatible = "marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>;
interrupts = <GIC_SPI 27
- IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
};
};
gic: interrupt-controller@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
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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@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
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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@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
Hi Stefan, Hi Simon
Please see my inline reply, thanks!
Yours, Ken
-----Original Message----- From: Stefan Roese [mailto:sr@denx.de] Sent: 2017年4月3日 14:14 To: Simon Glass; Ken Ma Cc: u-boot@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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@e0000 { compatible ="marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>; interrupts = <GIC_SPI 27IRQ_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
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@denx.de] Sent: 2017年4月3日 14:14 To: Simon Glass; Ken Ma Cc: u-boot@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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@e0000 { compatible ="marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>; interrupts = <GIC_SPI 27IRQ_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
Hi Stefan
Please see my inline reply, thanks!
Yours, Ken
-----Original Message----- From: Stefan Roese [mailto:sr@denx.de] Sent: 2017年4月5日 21:46 To: Ken Ma; Simon Glass Cc: u-boot@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@denx.de] Sent: 2017年4月3日 14:14 To: Simon Glass; Ken Ma Cc: u-boot@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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@e0000 { compatible ="marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>; interrupts = <GIC_SPI 27IRQ_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
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@denx.de] Sent: 2017年4月5日 21:46 To: Ken Ma; Simon Glass Cc: u-boot@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@denx.de] Sent: 2017年4月3日 14:14 To: Simon Glass; Ken Ma Cc: u-boot@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:
- 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.
- 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@e0000 {
compatible = "marvell,mvebu-scsi";reg = <0xe0000 0x2000>;
sata: sata@e0000 { compatible ="marvell,armada-3700-ahci";
reg = <0xe0000 0x2000>; interrupts = <GIC_SPI 27IRQ_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 freameworkUpper 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
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.
- Move base, max_lun and max_id such scsi generic data from platdata to uclass plat data;
- 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.
- 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@lists.denx.de https://lists.denx.de/listinfo/u-boot
Regards, Simon
participants (4)
-
Ken Ma -
make＠marvell.com
-
Simon Glass -
Stefan Roese