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

Sun Apr 9 19:28:45 UTC 2017

Hi Ken,

On 5 April 2017 at 19:32, Ken Ma <make at marvell.com> wrote:
> Hi Stefan
>
> Please see my inline reply, thanks!
>
> Yours,
> Ken
>
> -----Original Message-----
> From: Stefan Roese [mailto:sr at denx.de]
> Sent: 2017年4月5日 21:46
> To: Ken Ma; Simon Glass
> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing; Wilson Ding
> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>
> Hi Ken,
>
> On 05.04.2017 11:29, Ken Ma wrote:
>> Hi Stefan, Hi Simon
>>
>> Please see my inline reply, thanks!
>>
>> Yours,
>> Ken
>>
>> -----Original Message-----
>> From: Stefan Roese [mailto:sr at denx.de]
>> Sent: 2017年4月3日 14:14
>> To: Simon Glass; Ken Ma
>> Cc: u-boot at lists.denx.de; Michal Simek; Kostya Porotchkin; Hua Jing;
>> Wilson Ding
>> Subject: Re: [EXT] Re: [PATCH 7/7] scsi: dts: a3700: add scsi node
>>
>> Hi Simon, Hi Ken,
>>
>> On 01.04.2017 06:22, Simon Glass wrote:
>>> On 27 March 2017 at 02:28, Ken Ma <make at marvell.com> wrote:
>>>> 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 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.
>>>>
>>>> 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 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 at e0000 {
>>>>
>>>>                         compatible = "marvell,mvebu-scsi";
>>>>
>>>> reg = <0xe0000 0x2000>;
>>>>
>>>>                         sata: sata at e0000 {
>>>>
>>>>                               compatible =
>>>> "marvell,armada-3700-ahci";
>>>>
>>>>                               reg = <0xe0000 0x2000>;
>>>>
>>>>                               interrupts = <GIC_SPI 27
>>>> IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>>                         };
>>>
>>> Does this match the Linux DT?
>>
>> No, and this is my main concern. This patch series introduces a new
>> "scsi" DT node and moves the controller(s) one level up into this
>> newly created DT node (Ken please correct me if I'm wrong here).
>> We simply can't make such changes here in U-Boot without this being
>> first discussed and decided on the Linux DT mailing list with the DT
>> maintainers.
>>
>> Ken, how is this problem solved / handled in Linux without this DT
>> changes up until now?
>>
>> [Ken] Actually I do not find any SCSI nodes/compatible strings In
>> Linux; if aligned to linux, then we should have no scsi nodes at all
>> in u-boot.
>
> Thats exactly what I meant. The DT represents the hardware and only controllers / devices etc are listed here. As such, only the SATA, AHCI, IDE (etc.) controllers are listed behind their internal SoC / CPU busses in the DT. Unfortunately we can't come up with this new SCSI DT node for U-Boot and move all the controllers into this node, as we need to try to stay in sync with the Linux DT, which is the reference.
>
> [Ken] If aligned to linux, we should not add the class id of "UCLASS_SCSI" but add a middle scsi layer. We can register to scsi in scsi device driver(like sata.c) after adding some middle scsi layer, but we should not use UCLASS_SCSI  to replace UCLASS_AHCI directly.
>
> Now in u-boot scsi_scan() uses UCLASS_SCSI objects as scsi bus actually, if there are 2 different scsi devices in one same scsi bus and we classify the 2 different scsi devices into UCLASS_SCSI, then scsi_scan() will regard as there are 2 scsi buses but not one. We should not classify scsi devices into UCLASS_SCSI.
>
> In my understanding, since in u-boot UCLASS_SCSI is for SCSI host controller(bus function), we should have such a device bus node.
>
>                                                                 Linux SCSI freamework
> Upper level:            Disk driver(sd)         tape driver(st)         CDROM driver(sr)        Generic driver(sg)
> Middle level:                                   common service layer
> Lower level:            Fibre channel driver    SAS driver                      iSCSI driver    ...     Bridge driver
>
>> I am not familiar with Linux SCSI implementation, it seems that SCSI
>> bus is implemented as a middle layer in Linux since it has no SCSI fdt
>> nodes.
>
> Frankly, I've not looked at SCSI in Linux for quite some time.
> So I can't really tell you how this is handled there. But I'm pretty sure that no SCSI DT nodes / properties are involved here.

There is something funny with your email Ken. Normally you should
quote the original email and add your reply in unquoted lines. Somehow
your reply seems to be quoted to, so we cannot see who is replying to
who.

As you say, SCSI is for the controller and its bus. It would be the
parent device.

The bus has multiple devices e.g. AHCI. So I agree that we should not
combine AHCI and SCSI.

However, I hope that we don't need a separate uclass for the middle
layer. Hopefully it can just be some helper functions.

Overall there is quite a bit of work to do on SCSI for DM still. For
one, we need a sandbox driver and a test, similar to what was done for
USB.

Regards,
Simon