Hi Stefan


Thanks a lot for your kind advice and help!

Please see my reply inline.



Yours,

Ken



-----Original Message-----
From: Stefan Roese [mailto:s...@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, m...@marvell.com<mailto:m...@marvell.com> wrote:

> From: Ken Ma <m...@marvell.com<mailto:m...@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 <m...@marvell.com<mailto:m...@marvell.com>>

> Cc: Simon Glass <s...@chromium.org<mailto:s...@chromium.org>>

> Cc: Stefan Roese <s...@denx.de<mailto:s...@denx.de>>

> Cc: Michal Simek <michal.si...@xilinx.com<mailto:michal.si...@xilinx.com>>

> Reviewed-on: http://vgitil04.il.marvell.com:8080/35303

> Tested-by: iSoC Platform CI <ykj...@marvell.com<mailto:ykj...@marvell.com>>

> Reviewed-by: Kostya Porotchkin <kos...@marvell.com<mailto:kos...@marvell.com>>

> Reviewed-by: Omri Itach <om...@marvell.com<mailto:om...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to