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

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, 25 Jun 2019 at 15:06, Marek Vasut <marex@denx.de> wrote:
>
> On 6/25/19 1:59 AM, Vladimir Oltean wrote:
> > On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@denx.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@denx.de>
> >>> Cc: Andrew Lunn <andrew@lunn.ch>
> >>> Cc: Florian Fainelli <f.fainelli@gmail.com>
> >>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> >>> Cc: Woojung Huh <Woojung.Huh@microchip.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)
* 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!

> But I'm glad to see TJA1105 driver mainline :)
>
> --
> Best regards,
> Marek Vasut

Regards,
-Vladimir