From: kernel@esmil.dk (Emil Renner Berthing) To: linux-riscv@lists.infradead.org Subject: [RFC PATCH] spi: add driver for the SiFive SPI controller Date: Tue, 13 Nov 2018 20:48:43 +0100 [thread overview] Message-ID: <CANBLGcxF-WgsBVFHf4jGRR+Ox2vz=ewzhwGP1CZX-R5HpDAVBg@mail.gmail.com> (raw) In-Reply-To: <20181113183527.GG2089@sirena.org.uk> Hi Mark, On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Emil Renner Berthing <kernel@esmil.dk> To: Mark Brown <broonie@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, Palmer Dabbelt <palmer@sifive.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-spi@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, linux-riscv@lists.infradead.org Subject: Re: [RFC PATCH] spi: add driver for the SiFive SPI controller Date: Tue, 13 Nov 2018 20:48:43 +0100 [thread overview] Message-ID: <CANBLGcxF-WgsBVFHf4jGRR+Ox2vz=ewzhwGP1CZX-R5HpDAVBg@mail.gmail.com> (raw) Message-ID: <20181113194843.tTRXAgjeryNJGGjsB3O_LmzBMAfIvnGRkHJ8TWKXj3E@z> (raw) In-Reply-To: <20181113183527.GG2089@sirena.org.uk> Hi Mark, On Tue, 13 Nov 2018 at 19:35, Mark Brown <broonie@kernel.org> 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 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2018-11-13 19:48 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-12 14:27 [RFC PATCH] spi: add driver for the SiFive SPI controller Emil Renner Berthing 2018-11-12 14:27 ` Emil Renner Berthing 2018-11-13 18:35 ` Mark Brown 2018-11-13 18:35 ` Mark Brown 2018-11-13 19:48 ` Emil Renner Berthing [this message] 2018-11-13 19:48 ` Emil Renner Berthing 2018-11-13 22:38 ` Mark Brown 2018-11-13 22:38 ` Mark Brown 2019-02-13 10:03 ` Yash Shah 2019-02-13 13:10 ` Emil Renner Berthing
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CANBLGcxF-WgsBVFHf4jGRR+Ox2vz=ewzhwGP1CZX-R5HpDAVBg@mail.gmail.com' \ --to=kernel@esmil.dk \ --cc=linux-riscv@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).