linux-kernel - Re: [PATCH v7 3/4] mtd: spi-nor: introduce Double Transfer Rate (DTR) SPI protocols

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: <69db61ee-5de3-6518-6d44-fd4988d18bb6@wedev4u.fr>
Date:   Wed, 19 Apr 2017 22:20:48 +0200
From:   Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>
To:     Marek Vasut <marek.vasut@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>,
        linux-mtd@...ts.infradead.org
Cc:     boris.brezillon@...e-electrons.com, computersforpeace@...il.com,
        dwmw2@...radead.org, linux-kernel@...r.kernel.org, richard@....at
Subject: Re: [PATCH v7 3/4] mtd: spi-nor: introduce Double Transfer Rate (DTR)
 SPI protocols

Hi Marek,

Le 19/04/2017 à 01:05, Marek Vasut a écrit :
> On 04/19/2017 12:51 AM, Cyrille Pitchen wrote:
>> This patch introduces support to Double Transfer Rate (DTR) SPI protocols.
>> DTR is used only for Fast Read operations.
>>
>> According to manufacturer datasheets, whatever the number of I/O lines
>> used during instruction (x) and address/mode/dummy (y) clock cycles, DTR
>> is used only during data (z) clock cycles of SPI x-y-z protocols.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@...el.com>
> 
> [...]
> 
>> @@ -282,19 +305,22 @@ struct spi_nor_hwcaps {
>>   * As a matter of performances, it is relevant to use Quad SPI protocols first,
>>   * then Dual SPI protocols before Fast Read and lastly (Slow) Read.
>>   */
>> -#define SNOR_HWCAPS_READ_MASK		GENMASK(7, 0)
>> +#define SNOR_HWCAPS_READ_MASK		GENMASK(10, 0)
>>  #define SNOR_HWCAPS_READ		BIT(0)
>>  #define SNOR_HWCAPS_READ_FAST		BIT(1)
>> -
>> -#define SNOR_HWCAPS_READ_DUAL		GENMASK(4, 2)
>> -#define SNOR_HWCAPS_READ_1_1_2		BIT(2)
>> -#define SNOR_HWCAPS_READ_1_2_2		BIT(3)
>> -#define SNOR_HWCAPS_READ_2_2_2		BIT(4)
>> -
>> -#define SNOR_HWCAPS_READ_QUAD		GENMASK(7, 5)
>> -#define SNOR_HWCAPS_READ_1_1_4		BIT(5)
>> -#define SNOR_HWCAPS_READ_1_4_4		BIT(6)
>> -#define SNOR_HWCAPS_READ_4_4_4		BIT(7)
>> +#define SNOR_HWCAPS_READ_1_1_1_DTR	BIT(2)
>> +
>> +#define SNOR_HWCAPS_READ_DUAL		GENMASK(6, 3)
>> +#define SNOR_HWCAPS_READ_1_1_2		BIT(3)
>> +#define SNOR_HWCAPS_READ_1_2_2		BIT(4)
>> +#define SNOR_HWCAPS_READ_2_2_2		BIT(5)
>> +#define SNOR_HWCAPS_READ_1_2_2_DTR	BIT(6)
>> +
>> +#define SNOR_HWCAPS_READ_QUAD		GENMASK(10, 7)
>> +#define SNOR_HWCAPS_READ_1_1_4		BIT(7)
>> +#define SNOR_HWCAPS_READ_1_4_4		BIT(8)
>> +#define SNOR_HWCAPS_READ_4_4_4		BIT(9)
>> +#define SNOR_HWCAPS_READ_1_4_4_DTR	BIT(10)
> 
> I can't say I'm a big fan on this re-numeration when you add a new
> entry. But unless you have a better idea, we'll have to live with this ...
> 

Well, the other solution would be to reserve unused bit position in
patch 1 but would look odd too, wouldn't it?

As explained in the comments just above those definitions, the order of
the bits *does* matter. So maybe in the future, those bits would have to
be reordered again depending on the new features we would add then.

Thanks for your review!

Best regards,

Cyrille