From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ken Ma <make@marvell.com>
Date: Fri, 24 Mar 2017 03:03:59 +0000
Subject: [U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
In-Reply-To: <e1d191dc-ba56-86dd-830a-8c41a70899cc@denx.de>
References: <1490261347-11896-1-git-send-email-make@marvell.com>
 <1490261347-11896-8-git-send-email-make@marvell.com>
 <e1d191dc-ba56-86dd-830a-8c41a70899cc@denx.de>
Message-ID: <4c60c8125e6e45d1a0136ce3d5fd0c55@SC-EXCH04.marvell.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

Hi Stefan



Thanks a lot for your kind advice and help!

Please see my reply inline.



Yours,

Ken



-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de]
Sent: 2017年3月23日 22:06
To: Ken Ma; u-boot at lists.denx.de
Cc: Simon Glass; Michal Simek
Subject: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node



External Email



----------------------------------------------------------------------

Hi Ken,



On 23.03.2017 10:29, make at marvell.com<mailto:make@marvell.com> wrote:

> From: Ken Ma <make at marvell.com<mailto:make@marvell.com>>

>

> - Add scsi node which acts as a bus for scsi devices, armada3700 has

>   only 1 scsi interface, so max-id is 1, and the logic unit number is

>   also 1 for armada3700;

> - Since a3700's scsi is sas(serial attached scsi) which is compatible

>   for sata and sata hard disk is a sas device, so move sata node to be

>   under scsi node.

>

> Signed-off-by: Ken Ma <make at marvell.com<mailto:make@marvell.com>>

> Cc: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>

> Cc: Stefan Roese <sr at denx.de<mailto:sr@denx.de>>

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

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

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

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

> Reviewed-by: Omri Itach <omrii at marvell.com<mailto:omrii@marvell.com>>

> ---

>  arch/arm/dts/armada-3720-db.dts |  4 ++++

>  arch/arm/dts/armada-37xx.dtsi   | 16 ++++++++++++----

>  2 files changed, 16 insertions(+), 4 deletions(-)

>

> diff --git a/arch/arm/dts/armada-3720-db.dts

> b/arch/arm/dts/armada-3720-db.dts index 85761af..9fc60f6 100644

> --- a/arch/arm/dts/armada-3720-db.dts

> +++ b/arch/arm/dts/armada-3720-db.dts

> @@ -89,6 +89,10 @@

>     status = "okay";

>  };

>

> +&scsi {

> +   status = "okay";

> +};

> +

>  /* CON3 */

>  &sata {

>     status = "okay";

> diff --git a/arch/arm/dts/armada-37xx.dtsi

> b/arch/arm/dts/armada-37xx.dtsi index 062f2a6..de5d3a1 100644

> --- a/arch/arm/dts/armada-37xx.dtsi

> +++ b/arch/arm/dts/armada-37xx.dtsi

> @@ -149,11 +149,19 @@

>                       status = "disabled";

>                 };

>

> -               sata: sata at e0000 {

> -                     compatible = "marvell,armada-3700-ahci";

> -                     reg = <0xe0000 0x2000>;

> -                     interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;

> +               scsi: scsi {

> +                     compatible = "marvell,mvebu-scsi";

> +                     #address-cells = <1>;

> +                     #size-cells = <1>;

> +                     max-id = <1>;

> +                     max-lun = <1>;

>                       status = "disabled";

> +                     sata: sata at e0000 {

> +                           compatible = "marvell,armada-3700-ahci";

> +                           reg = <0xe0000 0x2000>;

> +                           interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;

> +                           status = "disabled";

> +                     };

>                 };

>

>                 gic: interrupt-controller at 1d00000 {

>



I see that you introduce a "scsi" DT node and move the SATA controller one "level up". I'm not sure if such a change is acceptable as we try to re-use the DT from Linux. Or thinking more about this, I'm pretty sure that such a change is not acceptable in general.



Can't you use the existing DT layout and use the "marvell,armada-3700-ahci" (and other perhaps?) compatible property instead for driver probing? Not sure how to handle the "max-id" and "max-lun" properties though. We definitely can't just add some ad-hoc properties here in U-Boot which have no chance for Linux upstream acceptance.



[Ken] Because scsi is a bus, for example, if there are 2 scsi buses, each bus has some scsi device controllers connected as below.



Scsi ID 0         Scsi ID 1         Scsi ID 2         Scsi ID 3



HDD0              HDD1               tape0              cd-rom0

||                ||                ||                ||

===============================================================

                            SCSI BUS1



HDD2              HDD3               tape1              cd-rom2

||                ||                ||                ||

===============================================================

                            SCSI BUS2





Then in my opinion, since now scsi has its own class id and its compatible string, then the scsi device controllers dts node should be above the scsi node.

If we keep existing DT layout and keep "marvell,armada-3700-ahci"’s uclass id as UCLASS_AHCI(there are no scsi nodes but only ahci nodes), then scsi_scan() can not find a3700’s sata at all since there are no UCLASS_SCSI devices;



If we keep existing DT layout and set scsi devices’ uclass id to be UCLASS_SCSI, how can we know that hdd0 and hdd1 are in scsi bus1 but hdd2 and hdd3 are in scsi bus2?  For each scsi bus, its max id should be 4; but now how to set each scsi device’ scsi id?



So I think we should move scsi devices “level up” on the scsio bus.



Thanks,

Stefan