netdev - Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support

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] [day] [month] [year] [list]

Message-ID: <797506e0-dfa5-af23-2b68-61da1bc2b3e1@denx.de>
Date:   Tue, 25 Jun 2019 17:18:20 +0200
From:   Marek Vasut <marex@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Tristram Ha <Tristram.Ha@...rochip.com>,
        Woojung Huh <Woojung.Huh@...rochip.com>
Subject: Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support

On 6/25/19 2:40 PM, Vladimir Oltean wrote:
> On Tue, 25 Jun 2019 at 15:06, Marek Vasut <marex@...x.de> wrote:
>>
>> On 6/25/19 1:59 AM, Vladimir Oltean wrote:
>>> On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@...x.de> wrote:
>>>>
>>>> On 6/24/19 12:35 AM, Marek Vasut wrote:
>>>>> Add basic SPI regmap support into the driver.
>>>>>
>>>>> Previous patches unconver that ksz_spi_write() is always ever called
>>>>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
>>>>> check and we can also drop the allocation of the txbuf which is part
>>>>> of the driver data and wastes 256 bytes for no reason. Regmap covers
>>>>> the whole thing now.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@...x.de>
>>>>> Cc: Andrew Lunn <andrew@...n.ch>
>>>>> Cc: Florian Fainelli <f.fainelli@...il.com>
>>>>> Cc: Tristram Ha <Tristram.Ha@...rochip.com>
>>>>> Cc: Woojung Huh <Woojung.Huh@...rochip.com>
>>>>
>>>> [...]
>>>>
>>>>> +#define KS_SPIOP_FLAG_MASK(opcode)           \
>>>>> +     cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>>>>
>>>> So the robot is complaining about this. I believe this is correct, as
>>>> the mask should be in native endianness on the register and NOT the
>>>> native endianness of the CPU.
>>>>
>>>> I think a cast would help here, e.g.:
>>>> -       cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>>>> -       (__force unsigned long)cpu_to_be32((opcode) << (SPI_ADDR_SHIFT +
>>>> SPI_TURNAROUND_SHIFT))
>>>>
>>>> Does this make sense ?
>>>>
>>>>> +#define KSZ_REGMAP_COMMON(width)                                     \
>>>>> +     {                                                               \
>>>>> +             .val_bits = (width),                                    \
>>>>> +             .reg_stride = (width) / 8,                              \
>>>>> +             .reg_bits = SPI_ADDR_SHIFT + SPI_ADDR_ALIGN,            \
>>>>> +             .pad_bits = SPI_TURNAROUND_SHIFT,                       \
>>>>> +             .max_register = BIT(SPI_ADDR_SHIFT) - 1,                \
>>>>> +             .cache_type = REGCACHE_NONE,                            \
>>>>> +             .read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD),      \
>>>>> +             .write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR),     \
>>>>
>>>> [...]
>>>>
>>>> --
>>>> Best regards,
>>>> Marek Vasut
>>>
>>> Hi Marek,
>>>
>>> I saw SPI buffers and endianness and got triggered :)
>>> Would it make sense to take a look at CONFIG_PACKING for the KSZ9477 driver?
>>> I don't know how bad the field alignment issue is on that hardware,
>>> but on SJA1105 it was such a disaster that I couldn't have managed it
>>> any other way.
>>
>> How does that help me here ? All I really need is a static constant mask
>> for the register/flags , 32bit for KSZ9xxx and 16bit for KSZ87xx. I
>> don't need any dynamic stuff, luckily.
>>
> 
> Ok. I was thinking *instead of* regmap.
> On the SJA1105 I couldn't use a spi regmap because:
> * the enum regmap_endian wasn't enough to describe the hardware's bit
> layout (it is big endian, but high-order and low-order 32 bit words
> are swapped)

Oooooof. Is the switch development some competition in register layout
braindeath ? What's it gonna be next ... I was gonna suggest something,
but then reconsidered, better not give them any wild ideas :-)

> * it doesn't really expose a well-defined register map, but rather,
> you're supposed to dynamically construct a TLV-type buffer and write
> it starting with a fixed address.
> I just thought you were facing some of these problems as well with
> regmap. If not, that's perfectly fine!

All right, that's not the case here, whew.

-- 
Best regards,
Marek Vasut