Re: [RFC PATCH] spi: add driver for the SiFive SPI controller

[Emil Renner Berthing <kernel@xxxxxxxx>]

Hi Mark,
On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:
>
> > I know the discussions about the sifive devicetree compatible
> > strings haven't come to a conclusion, so I'm sending this as
> > an RFC to get some feedback on the rest of the code.
>
> I've not seen any of these discussions or earlier versions of this
> driver so I've no idea what's going on here :(

No, sorry. This has been discussed on linux-riscv for other drivers
like the uart. See my last answer.

> > +Optional properties:
> > +- sifive,fifo-depth          : Depth of hardware queues; defaults to 8
> > +- sifive,max-bits-per-word   : Maximum bits per word; defaults to 8
> > +
>
> If the hardware isn't fixed yet making these enumerable from the
> hardware would be good...

Agreed, but unfortunately this is already in the FU540-C000 chip on
the HiFive Unleashed board sold by SiFive.

> > @@ -0,0 +1,442 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SiFive SPI controller driver (master mode only)
> > + *
>
> Please make the entire comment a C++ one to make this look more
> intentinal.

Will do.

> > +/* for consistency we need this symbol */
> > +#ifdef REG_FMT
> > +#undef REG_FMT
> > +#endif
>
> We do?  For consistency with what?

Below all the register offsets are defined as
REG_<register name>. This is is a pattern I
copied from other drivers, but here we have a
register called "fmt" - hence REG_FMT.
If you have a better pattern that doesn't clash
with REG_FMT please let me know.

> > +static void sifive_spi_init(struct sifive_spi *spi)
> > +{
>
> > +     /* Set CS/SCK Delays and Inactive Time to defaults */
> > +
> > +     /* Exit specialized memory-mapped SPI flash mode */
>
> ...or not?

Right. Will add that or just delete the comment.

> > +     /* Set frame format */
> > +     cr = FMT_LEN(t->bits_per_word);
> > +     switch (mode) {
> > +     case SPI_NBITS_QUAD:
> > +             cr |= FMT_PROTO_QUAD;
> > +             break;
>
> Some namespacing on the driver #defines would be a bit safer against the
> possibility of collision with future changes in headers.

Right.

> > +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> > +{
> > +     if (poll) {
> > +             u32 cr;
> > +             do cr = sifive_spi_read(spi, REG_IP);
> > +             while (!(cr & bit));
>
> Please add some braces, indentation or something to make it more clear
> that the read is part of a do/while loop - right now it's not
> immediately obvious that this is correct.

Good point. Will do.

> > +static int sifive_spi_transfer_one(struct spi_master *master,
> > +             struct spi_device *device, struct spi_transfer *t)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(master);
> > +     int poll = sifive_spi_prep_transfer(spi, device, t);
> > +
> > +     sifive_spi_execute(spi, t, poll);
> > +
>
> Why not just inline the execute function here?  It's the only caller
> AFAICT.

Yeah, it is. Will do.

> > +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> > +{
> > +     struct sifive_spi *spi = spi_master_get_devdata(device->master);
> > +
> > +     /* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> > +     if (device->mode & SPI_CS_HIGH)
> > +             is_high = !is_high;
>
> spi_set_cs() will handle CS_HIGH for you.
>
> > +     master->bits_per_word_mask = SPI_BPW_MASK(8);
>
> I thought the device supported other bits per word values?

It does, but the driver doesn't yet. When bits per word is < 8
we need to shift the bits in each byte to be "left-aligned"
(unless SPI_LSB_FIRST is set).

> > +     /* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> > +      * Probably it should rely on the SPI core auto mapping instead.
> > +      */
> > +     pdev->dev.dma_mask = NULL;
>
> If this is a problem please fix it in the MMC core, don't bodge it like
> this.

Gotcha. Will remove this.

> > +static const struct of_device_id sifive_spi_of_match[] = {
> > +     { .compatible = "sifive,spi0", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);
>
> spi0 is a *weird* compatible name.

Exactly. Hence the discussion about the compatible strings.
Once this discussion comes to a conclusion I'll update this.

Thank you for the review!
/Emil