| Message ID | 1490261347-11896-8-git-send-email-make@marvell.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
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.com<mailto:make@marvell.com> wrote: > From: Ken Ma <make@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@marvell.com<mailto:make@marvell.com>> > Cc: Simon Glass <sjg@chromium.org<mailto:sjg@chromium.org>> > Cc: Stefan Roese <sr@denx.de<mailto:sr@denx.de>> > Cc: Michal Simek <michal.simek@xilinx.com<mailto:michal.simek@xilinx.com>> > Reviewed-on: http://vgitil04.il.marvell.com:8080/35303 > Tested-by: iSoC Platform CI <ykjenk@marvell.com<mailto:ykjenk@marvell.com>> > Reviewed-by: Kostya Porotchkin <kostap@marvell.com<mailto:kostap@marvell.com>> > Reviewed-by: Omri Itach <omrii@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@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.de<mailto:u-boot@lists.denx.de> Cc: Simon Glass; Michal Simek Subject: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node External Email ---------------------------------------------------------------------- Hi Ken, On 23.03.2017 10:29, make@marvell.com<mailto:make@marvell.com> wrote: > From: Ken Ma <make@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@marvell.com<mailto:make@marvell.com>> > Cc: Simon Glass <sjg@chromium.org<mailto:sjg@chromium.org>> > Cc: Stefan Roese <sr@denx.de<mailto:sr@denx.de>> > Cc: Michal Simek <michal.simek@xilinx.com<mailto:michal.simek@xilinx.com>> > Reviewed-on: http://vgitil04.il.marvell.com:8080/35303 > Tested-by: iSoC Platform CI <ykjenk@marvell.com<mailto:ykjenk@marvell.com>> > Reviewed-by: Kostya Porotchkin <kostap@marvell.com<mailto:kostap@marvell.com>> > Reviewed-by: Omri Itach <omrii@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@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
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 {