Hi Stefan
Thanks a lot for your kind reply. But I still do not think it's very good to change sata's uclass id from "UCLASS_AHCI" to "UCLASS_SCSI". If we do such change, UCLASS_AHCI is lost since from the sata.c codes, it does the AHCI initialization work but not SCSI initialization work. If u-boot supports ISIS scanner which supports SCSI, I think its uclass id should be like UCLASS_ISIS but not UCLASS_SCSI. And if we set sata's uclass id as "UCLASS_SCSI", it should provide basic SCSI function, then why can’t we connect a parallel SCSI device like SCSI scanner or cd-rom to the SATA interface? From https://en.wikipedia.org/wiki/Serial_ATA#SATA_and_SCSI In general, SATA devices link compatibly to SAS enclosures and adapters, whereas SCSI devices cannot be directly connected to a SATA bus Actually Marvell’s sata controller is SAS(Serial Attached SCSI system), it integrates SCSI and SATA(AHCI); SAS provides a SCSI bus which works only in SAS range(for example, 2 sata ports in SAS), so actually the SCSI bus controller is not "virtual" controllers but has the same device base register as SATA. From https://en.wikipedia.org/wiki/Serial_Attached_SCSI A typical Serial Attached SCSI system consists of the following basic components: 1. An initiator: a device that originates device-service and task-management<https://en.wikipedia.org/wiki/Task_Management_Function> requests for processing by a target device and receives responses for the same requests from other target devices. Initiators may be provided as an on-board component on the motherboard (as is the case with many server-oriented motherboards) or as an add-on host bus adapter<https://en.wikipedia.org/wiki/Host_bus_adapter>. 2. A target: … So in my opinion, there are two ways to implement SAS as below A. If our codes provide SAS controller as an on-board component – then a uclass id of UCLASS_SAS should be defined and then in scsi_scan() of scsi_scan.c, both devices of UCLASS_SCSI and UCLASS_SAS should be scanned. In such implementation, UCLASS_SCSI is for parallel SCSI while UCLASS_SAS is for serial attached SCSI; B. SAS works as an add-on host bus adapter<https://en.wikipedia.org/wiki/Host_bus_adapter> as above said, SAS’s SCSI controller and AHCI controller are both itself as below - SCSI controller is not a virtual device, it exists and only works in SAS internal range(since there is no UCLASS_SAS, I take this way); Although the SAS’s SCSI controller does not need to any special hardware configuration; but actually I think there is something to do, we should bind special scsi_exec() to SCSI devices in SCSI driver or SAS driver (For different SCSI controls, SAS must have different implementation of scsi_exec() comparing to SCSI scanner, or other SCSI devices) By the way, I think we should move the work of creating block devices to scsi-uclass.c scsi: scsi@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:s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
