[U-Boot] [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node

[Stefan Roese]

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 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
>

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ahci-Add-DM-based-support-for-the-Marvell-MVEBU-SATA.patch
Type: text/x-diff
Size: 3502 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170324/866bd363/attachment.patch>