Instead of open coding mask and shift operations and to increase
readability use FIELD_PREP() to encode the FIFO Status register.
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/spi/spi-fsl-lpspi.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index e75b9acaa9e5..139d6573a013 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -78,6 +78,8 @@
#define CFGR1_PCSPOL_MASK GENMASK(11, 8)
#define CFGR1_NOSTALL BIT(3)
#define CFGR1_HOST BIT(0)
+#define FCR_RXWATER GENMASK(18, 16)
+#define FCR_TXWATER GENMASK(2, 0)
#define FSR_TXCOUNT (0xFF)
#define RSR_RXEMPTY BIT(1)
#define TCR_CPOL BIT(31)
@@ -326,17 +328,18 @@ static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi,
static void fsl_lpspi_set_watermark(struct fsl_lpspi_data *fsl_lpspi)
{
+ u8 watermark = fsl_lpspi->watermark >> 1;
u32 temp;
if (!fsl_lpspi->usedma)
- temp = fsl_lpspi->watermark >> 1 |
- (fsl_lpspi->watermark >> 1) << 16;
+ temp = FIELD_PREP(FCR_TXWATER, watermark) |
+ FIELD_PREP(FCR_RXWATER, watermark);
else
- temp = fsl_lpspi->watermark >> 1;
+ temp = FIELD_PREP(FCR_TXWATER, watermark);
writel(temp, fsl_lpspi->base + IMX7ULP_FCR);
- dev_dbg(fsl_lpspi->dev, "FCR=0x%x\n", temp);
+ dev_dbg(fsl_lpspi->dev, "FCR=0x%08x\n", temp);
}
static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
--
2.51.0On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote: > +#define FCR_RXWATER GENMASK(18, 16) > +#define FCR_TXWATER GENMASK(2, 0) The source of the watermark values appears to be temp = readl(fsl_lpspi->base + IMX7ULP_PARAM); fsl_lpspi->txfifosize = 1 << (temp & 0x0f); fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f); which suggests 4 bits of watermark not 3? Or at least that something isn't joined up somewhere.
On 16.03.2026 14:39:03, Mark Brown wrote: > On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote: > > > +#define FCR_RXWATER GENMASK(18, 16) > > +#define FCR_TXWATER GENMASK(2, 0) That's the width from the i.MX93 datasheet. > The source of the watermark values appears to be > > temp = readl(fsl_lpspi->base + IMX7ULP_PARAM); > fsl_lpspi->txfifosize = 1 << (temp & 0x0f); > fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f); > > which suggests 4 bits of watermark not 3? Or at least that something > isn't joined up somewhere. The param register's default value on the i.MX93 is: | LPSPI1–LPSPI3: 0002_0303h | LPSPI4: 0003_0303h | LPSPI5–LPSPI8: 0002_0303h This means a RX/TX-FIFO size of 1 << 3 == 8. In the FCR register the RX/TX watermark are 3 bits. So this is consistent. On the imx7ulp and s32g2 the param gives 1 << 2 = 4 and the RX/TX watermark are just 2 bits wide. We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide, the rest of the register is reserved. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote: > On 16.03.2026 14:39:03, Mark Brown wrote: > > On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote: > > The source of the watermark values appears to be > > temp = readl(fsl_lpspi->base + IMX7ULP_PARAM); > > fsl_lpspi->txfifosize = 1 << (temp & 0x0f); > > fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f); > > which suggests 4 bits of watermark not 3? Or at least that something > > isn't joined up somewhere. > The param register's default value on the i.MX93 is: > | LPSPI1–LPSPI3: 0002_0303h > | LPSPI4: 0003_0303h > | LPSPI5–LPSPI8: 0002_0303h > This means a RX/TX-FIFO size of 1 << 3 == 8. Right, so the parsing code is using the wrong mask to extract the width here but with actual values it's fine. > We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide, > the rest of the register is reserved. It seems safer to restrict the mask used to extract the FIFO sizes, that way we don't trample over any bits that get defined in future and people comparing with the datasheet aren't confused.
On 16.03.2026 17:37:45, Mark Brown wrote: > On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote: > > On 16.03.2026 14:39:03, Mark Brown wrote: > > > On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote: > > > > The source of the watermark values appears to be > > > > temp = readl(fsl_lpspi->base + IMX7ULP_PARAM); > > > fsl_lpspi->txfifosize = 1 << (temp & 0x0f); > > > fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f); > > > > which suggests 4 bits of watermark not 3? Or at least that something > > > isn't joined up somewhere. > > > The param register's default value on the i.MX93 is: > > > | LPSPI1–LPSPI3: 0002_0303h > > | LPSPI4: 0003_0303h > > | LPSPI5–LPSPI8: 0002_0303h > > > This means a RX/TX-FIFO size of 1 << 3 == 8. > > Right, so the parsing code is using the wrong mask to extract the width > here but with actual values it's fine. No, the parsing code is correct, the width of the fields are 8 bits (datasheet of all supported SoCs), but the values are "2" (i.MX7ulp, S32G2) resp. "3" (i.MX93). > > We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide, > > the rest of the register is reserved. > > It seems safer to restrict the mask used to extract the FIFO sizes, that > way we don't trample over any bits that get defined in future and people > comparing with the datasheet aren't confused. The width of the watermark fields are "2" (i.MX7ulp, S32G2) resp. "3" (i.MX93). In fsl_lpspi_setup_transfer() the driver restricts the watermark to: | fsl_lpspi->watermark = min(fsl_lpspi->txfifosize, t->len); and writes | fsl_lpspi->watermark >> 1 into the watermark fields. So using the 3 bit mask in FIELD_PREP should not be a problem, even on the IP cores with just 2 bit wide mask. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Mar 16, 2026 at 06:59:12PM +0100, Marc Kleine-Budde wrote: > On 16.03.2026 17:37:45, Mark Brown wrote: > > On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote: > > > The param register's default value on the i.MX93 is: > > > | LPSPI1–LPSPI3: 0002_0303h > > > | LPSPI4: 0003_0303h > > > | LPSPI5–LPSPI8: 0002_0303h > > > This means a RX/TX-FIFO size of 1 << 3 == 8. > > Right, so the parsing code is using the wrong mask to extract the width > > here but with actual values it's fine. > No, the parsing code is correct, the width of the fields are 8 bits > (datasheet of all supported SoCs), but the values are "2" (i.MX7ulp, > S32G2) resp. "3" (i.MX93). Oh, dear, so that's potentially going to go badly if there's a SoC with a bigger FIFO that didn't also increase the watermark field width. We should at least warn about that, and ideally do something sensible. > into the watermark fields. So using the 3 bit mask in FIELD_PREP should > not be a problem, even on the IP cores with just 2 bit wide mask. Yeah, it's not broken right now - just landmines for potential future SoCs to trip over.
On 16.03.2026 18:16:01, Mark Brown wrote: > On Mon, Mar 16, 2026 at 06:59:12PM +0100, Marc Kleine-Budde wrote: > > On 16.03.2026 17:37:45, Mark Brown wrote: > > > On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote: > > > > > The param register's default value on the i.MX93 is: > > > > > | LPSPI1–LPSPI3: 0002_0303h > > > > | LPSPI4: 0003_0303h > > > > | LPSPI5–LPSPI8: 0002_0303h > > > > > This means a RX/TX-FIFO size of 1 << 3 == 8. > > > > Right, so the parsing code is using the wrong mask to extract the width > > > here but with actual values it's fine. > > > No, the parsing code is correct, the width of the fields are 8 bits > > (datasheet of all supported SoCs), but the values are "2" (i.MX7ulp, > > S32G2) resp. "3" (i.MX93). > > Oh, dear, so that's potentially going to go badly if there's a SoC with > a bigger FIFO that didn't also increase the watermark field width. We > should at least warn about that, and ideally do something sensible. Good idea, I'll add a warning if the FIFO is bigger than the watermark field. To handle this situation I see 2 options: - warning + limit the FIFO to the usable size due to the currently hard-coded watermark width - this will result in reduced throughput - increase the watermark mask to 8 bits - which is basically how the unpatched driver works. As outlined in my previous mail, the other code in the driver takes care that the watermark stays within the limits. > > into the watermark fields. So using the 3 bit mask in FIELD_PREP should > > not be a problem, even on the IP cores with just 2 bit wide mask. > > Yeah, it's not broken right now - just landmines for potential future > SoCs to trip over. ACK regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Mon, Mar 16, 2026 at 07:31:04PM +0100, Marc Kleine-Budde wrote: > On 16.03.2026 18:16:01, Mark Brown wrote: > > Oh, dear, so that's potentially going to go badly if there's a SoC with > > a bigger FIFO that didn't also increase the watermark field width. We > > should at least warn about that, and ideally do something sensible. > Good idea, I'll add a warning if the FIFO is bigger than the watermark > field. > To handle this situation I see 2 options: > - warning + limit the FIFO to the usable size due to the currently > hard-coded watermark width - this will result in reduced throughput > - increase the watermark mask to 8 bits - which is basically how the > unpatched driver works. > As outlined in my previous mail, the other code in the driver takes > care that the watermark stays within the limits. I guess it depends how optimistic we are about hardware engineers keeping the FIFO and watermark depths in sync. The first option is more defensive, but like you say it'll hit performance on future devices that do expand the FIFO. I guess I have a slight preference for that?
On 16.03.2026 18:49:15, Mark Brown wrote: > > > Oh, dear, so that's potentially going to go badly if there's a SoC with > > > a bigger FIFO that didn't also increase the watermark field width. We > > > should at least warn about that, and ideally do something sensible. > > > Good idea, I'll add a warning if the FIFO is bigger than the watermark > > field. > > > To handle this situation I see 2 options: > > - warning + limit the FIFO to the usable size due to the currently > > hard-coded watermark width - this will result in reduced throughput > > - increase the watermark mask to 8 bits - which is basically how the > > unpatched driver works. > > As outlined in my previous mail, the other code in the driver takes > > care that the watermark stays within the limits. > > I guess it depends how optimistic we are about hardware engineers > keeping the FIFO and watermark depths in sync. Right, we're talking about FSL/NXP hardware :D > The first option is more > defensive, but like you say it'll hit performance on future devices that > do expand the FIFO. I guess I have a slight preference for that? Yes - I'll increase the watermark width to 8 bits and add a comment next to the GENMASK. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 17.03.2026 10:44:51, Marc Kleine-Budde wrote: > On 16.03.2026 18:49:15, Mark Brown wrote: > > > > Oh, dear, so that's potentially going to go badly if there's a SoC with > > > > a bigger FIFO that didn't also increase the watermark field width. We > > > > should at least warn about that, and ideally do something sensible. > > > > > Good idea, I'll add a warning if the FIFO is bigger than the watermark > > > field. > > > > > To handle this situation I see 2 options: > > > - warning + limit the FIFO to the usable size due to the currently > > > hard-coded watermark width - this will result in reduced throughput > > > - increase the watermark mask to 8 bits - which is basically how the > > > unpatched driver works. > > > As outlined in my previous mail, the other code in the driver takes > > > care that the watermark stays within the limits. > > > > I guess it depends how optimistic we are about hardware engineers > > keeping the FIFO and watermark depths in sync. > > Right, we're talking about FSL/NXP hardware :D > > > The first option is more > > defensive, but like you say it'll hit performance on future devices that > > do expand the FIFO. I guess I have a slight preference for that? > > Yes - I'll increase the watermark width to 8 bits and add a comment next > to the GENMASK. FTR: I decided to use the watermark width of the i.MX93 as an upper limit for the FIFO order and print an info message. See https://lore.kernel.org/all/20260319-spi-fsl-lpspi-cleanups-v2-3-02b56c5d44a8@pengutronix.de regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
© 2016 - 2026 Red Hat, Inc.