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

From: Emil Renner Berthing
Date: Tue Nov 13 2018 - 14:48:57 EST


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