linux-kernel - Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFp=KHYogJE9WkJUYKphJhsrMfLjxxvNKmiAB+35bER4FQ@mail.gmail.com>
Date:   Tue, 29 Nov 2016 12:11:39 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Ziji Hu <huziji@...vell.com>
Cc:     Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Jimmy Xu <zmxu@...vell.com>,
        Jisheng Zhang <jszhang@...vell.com>,
        Nadav Haklai <nadavh@...vell.com>, Ryan Gao <ygao@...vell.com>,
        Doug Jones <dougj@...vell.com>, Victor Gu <xigu@...vell.com>,
        "Wei(SOCP) Liu" <liuw@...vell.com>,
        Wilson Ding <dingwei@...vell.com>,
        Romain Perier <romain.perier@...e-electrons.com>,
        Yehuda Yitschak <yehuday@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Hanna Hawa <hannah@...vell.com>,
        Kostya Porotchkin <kostap@...vell.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell
 Xenon SDHC

[...]

>>>>
>>>
>>>    Sorry that I didn't make myself clear.
>>>
>>>    Our host PHY delay line consists of hundreds of sampling points.
>>>    Each sampling point represents a different phase shift.
>>>
>>>    In lower speed mode, our host driver will scan the delay line.
>>>    It will select and test multiple sampling points, other than testing
>>>    only single sampling point.
>>>
>>>    If a sampling point fails to transfer cmd/data, our host driver will
>>>    move to test next sampling point, until we find out a group of successful
>>>    sampling points which can transfer cmd/data. At last we will select
>>>    a perfect one from them.
>>
>> Ahh, I see. Unfortunate, this is going to be very hard to implement properly.
>>
>> The main problem is that the host driver has *no* knowledge about the
>> internal state of the card, as that is the responsibility of the mmc
>> core to keep track of.
>>
>> If the host driver would send a command during every update of the
>> "ios" setting, from ->set_ios(), for sure it would lead to commands
>> being sent that are "forbidden" in the current internal state of the
>> card.
>> This would lead to that the card initialization sequence fails,
>> because the card may move to an unknown internal state and the mmc
>> core would have no knowledge about what happened.
>>
>
>    Yes. In theory, host layer should not initiate a command by itself.
>
>    We assume that bus is idle and card is stable in Tran state, when core layer
>    asks host to switch "ios".

Understand, but this is a wrong assumption. The card may very well in
another state than Tran state.

>    Besides, we only select the commands which is valid in the whole procedure,
>    such as CMD8 for eMMC.
>    Those test commands are actually like read operations to card registers.
>    The card will return to Tran state even if transfer fails. It is also easy
>    for host to recover.

For example, I would recommend you to investigate in detail the
sequence for when a CMD6 command is sent to the card.
The host must *not* start sending commands from ->set_ios() during a
CMD6 sequence. For example a CMD8 is not allowed.

Moreover, due to this, I wonder if it is even possible to get this HW
to work properly.

>
>> Hmm..
>>
>> Can you specify, *exactly*, under which "ios updates" you need to
>> verify updated PHY setting changes by sending a cmd/data? Also, please
>> specify if it's enough to only test the CMD line or also DATA lines.
>>
>
>    When one of the three parameters in below changes, our host driver needs
>    to adjust PHY in lower speed mode.
>    1. Speed Mode (timing): like legacy mode --> HS DDR
>    2. Bus Clock: like 400KHz --> 50MHz
>    3. Bus Width: like 1-bit --> 4-bit/8-bit
>
>    For eMMC, we use CMD8 to test sampling point.
>    For SD, we use CMD13.
>    For SDIO, currently CMD52 is used to read a register from CCCR.
>    Those commands in above are all valid during the whole procedure to switch
>    to high speed mode from legacy mode.
>
>    It is the best case if the test command can transfer both on CMD and DAT lines.
>    CMD8 for eMMC can test both CMD line and DAT lines. CMD13 and CMD52 only test
>    CMD line. We might use ACMD51 for SD and CMD53 for SDIO later thus DAT lines
>    are also under test.

Thanks for sharing these details!

So, if possible, I would recommend you to discuss these issues with
some of the HW designers. Perhaps you can figure out an alternative
method of confirming/testing PHY setting changes? Sending commands to
the card just doesn't work well for all cases.

Kind regards
Uffe