diff mbox series

[4/6] spi: altera: use regmap instead of direct mmio register access

Message ID 1591845911-10197-5-git-send-email-yilun.xu@intel.com (mailing list archive)
State New, archived
Headers show
Series Add more configuration and regmap support for spi-altera | expand

Commit Message

Xu Yilun June 11, 2020, 3:25 a.m. UTC 
This patch adds support for regmap. It allows this driver to
be compatible if low layer register access method is changed
in some cases.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/spi/Kconfig        |   1 +
 drivers/spi/spi-altera.c   | 103 ++++++++++++++++++++++++++++++++++++---------
 include/linux/spi/altera.h |   6 +++
 3 files changed, 90 insertions(+), 20 deletions(-)

Comments

Mark Brown June 11, 2020, 11:02 a.m. UTC | #1 
On Thu, Jun 11, 2020 at 11:25:09AM +0800, Xu Yilun wrote:

> +	if (pdata && pdata->use_parent_regmap) {
> +		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +		if (!hw->regmap) {
> +			dev_err(&pdev->dev, "get regmap failed\n");
> +			goto exit;
> +		}
> +		hw->base = pdata->regoff;

This seems very confused - there's some abstraction problem here.  This
looks like the driver should know about whatever is causing it to use
the parent regmap, it shouldn't just be "use the parent regmap" directly
since that is too much of an implementation detail.  This new feature
also seems like a separate change which should be in a separate patch,
the changelog only talked about converting to use regmap which I'd have
expected to be a straight 1:1 replacement of non-regmap stuff with
regmap stuff.
Xu Yilun June 12, 2020, 4:43 a.m. UTC | #2 
On Thu, Jun 11, 2020 at 12:02:11PM +0100, Mark Brown wrote:
> On Thu, Jun 11, 2020 at 11:25:09AM +0800, Xu Yilun wrote:
> 
> > +	if (pdata && pdata->use_parent_regmap) {
> > +		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +		if (!hw->regmap) {
> > +			dev_err(&pdev->dev, "get regmap failed\n");
> > +			goto exit;
> > +		}
> > +		hw->base = pdata->regoff;
> 
> This seems very confused - there's some abstraction problem here.  This
> looks like the driver should know about whatever is causing it to use
> the parent regmap, it shouldn't just be "use the parent regmap" directly
> since that is too much of an implementation detail.  This new feature

Our usecase is that, we have the PCIE card which integrates this
spi-altera soft IP. So It seems reasonable we reuse this driver. But the
IP registers are not directly accessed by MMIO. It is accessed via an
indirect access interfaces, SW need to operate on the indirect access
interfaces to RW the spi-altera registers. like the following:

+------+   +------------+   +------------+
| PCIE |---| indirect   |---| spi-altera |
+------+   | access     |   +------------+
           | interfaces |
           +------------+
           | SPI params |
           +------------+

So we think of creating regmap to abstract the actually register accessing
detail. The parent device driver creates the regmap of indirect access,
and it creates the spi-altera platform device as child. Spi-altera
driver could just get the regmap from parent, don't have to care about
the indirect access detail.

It seems your concern is how to gracefully let spi-altera driver get the
regmap. or not using it. Since our platform doesn't enable device tree
support, seems the only way to talk to platform device is the
platform_data.

Do you have any suggestion on that?


I think the driver may need to figure out the role of the device in
system, whether it is a subdev of other device (like MFD? Many mfd subdev
driver will get parent regmap by default), or it is an independent mmio
device. But I'm not sure how to do it in right way.

> also seems like a separate change which should be in a separate patch,
> the changelog only talked about converting to use regmap which I'd have
> expected to be a straight 1:1 replacement of non-regmap stuff with
> regmap stuff.

Understood. I should make a patch to do 1:1 replacement of regmap first,
then a second patch to handle the parent regmap.
Mark Brown June 12, 2020, 11:52 a.m. UTC | #3 
On Fri, Jun 12, 2020 at 12:43:46PM +0800, Xu Yilun wrote:

> So we think of creating regmap to abstract the actually register accessing
> detail. The parent device driver creates the regmap of indirect access,
> and it creates the spi-altera platform device as child. Spi-altera
> driver could just get the regmap from parent, don't have to care about
> the indirect access detail.

To be clear there's absolutely no problem with the end result, my
concern is the way that we're getting there.

> It seems your concern is how to gracefully let spi-altera driver get the
> regmap. or not using it. Since our platform doesn't enable device tree
> support, seems the only way to talk to platform device is the
> platform_data.

No, the problem is with how that platform data is structured.  Based on
what you're saying I'd suggest adding another device ID for this - you
can use the id_table field in struct platform_driver to have more than
one ID like you can have more than one ACPI ID or OF compatible.  That
would mirror how this would be handled if things were enumerated through
firmware.

> I think the driver may need to figure out the role of the device in
> system, whether it is a subdev of other device (like MFD? Many mfd subdev
> driver will get parent regmap by default), or it is an independent mmio
> device. But I'm not sure how to do it in right way.

Yes, it sounds like this card is a MFD.
Xu Yilun June 12, 2020, 12:31 p.m. UTC | #4 
On Fri, Jun 12, 2020 at 12:52:02PM +0100, Mark Brown wrote:
> On Fri, Jun 12, 2020 at 12:43:46PM +0800, Xu Yilun wrote:
> 
> > So we think of creating regmap to abstract the actually register accessing
> > detail. The parent device driver creates the regmap of indirect access,
> > and it creates the spi-altera platform device as child. Spi-altera
> > driver could just get the regmap from parent, don't have to care about
> > the indirect access detail.
> 
> To be clear there's absolutely no problem with the end result, my
> concern is the way that we're getting there.
> 
> > It seems your concern is how to gracefully let spi-altera driver get the
> > regmap. or not using it. Since our platform doesn't enable device tree
> > support, seems the only way to talk to platform device is the
> > platform_data.
> 
> No, the problem is with how that platform data is structured.  Based on
> what you're saying I'd suggest adding another device ID for this - you
> can use the id_table field in struct platform_driver to have more than
> one ID like you can have more than one ACPI ID or OF compatible.  That
> would mirror how this would be handled if things were enumerated through
> firmware.

Thanks for your quick response. It's very clear and makes sense. I'll
change it.

> 
> > I think the driver may need to figure out the role of the device in
> > system, whether it is a subdev of other device (like MFD? Many mfd subdev
> > driver will get parent regmap by default), or it is an independent mmio
> > device. But I'm not sure how to do it in right way.
> 
> Yes, it sounds like this card is a MFD.
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 8f1f8fc..6d79fc7 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -59,6 +59,7 @@  comment "SPI Master Controller Drivers"
 
 config SPI_ALTERA
 	tristate "Altera SPI Controller"
+	select REGMAP_MMIO
 	help
 	  This is the driver for the Altera SPI Controller.
 
diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index aa9d1a2..8d47c57 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -44,7 +44,6 @@ 
 #define ALTERA_SPI_MAX_CS		32
 
 struct altera_spi {
-	void __iomem *base;
 	int irq;
 	int len;
 	int count;
@@ -54,8 +53,45 @@  struct altera_spi {
 	/* data buffers */
 	const unsigned char *tx;
 	unsigned char *rx;
+
+	struct regmap *regmap;
+	u32 base;
+
+	struct device *dev;
+};
+
+static const struct regmap_config spi_altera_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
 };
 
+static int altr_spi_writel(struct altera_spi *hw, unsigned int reg,
+			   unsigned int val)
+{
+	int ret;
+
+	ret = regmap_write(hw->regmap, hw->base + reg, val);
+	if (ret)
+		dev_err(hw->dev, "fail to write reg 0x%x val 0x%x: %d\n",
+			reg, val, ret);
+
+	return ret;
+}
+
+static int altr_spi_readl(struct altera_spi *hw, unsigned int reg,
+			  unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(hw->regmap, hw->base + reg, val);
+	if (ret)
+		dev_err(hw->dev, "fail to read reg 0x%x: %d\n", reg, ret);
+
+	return ret;
+}
+
 static inline struct altera_spi *altera_spi_to_hw(struct spi_device *sdev)
 {
 	return spi_master_get_devdata(sdev->master);
@@ -67,12 +103,13 @@  static void altera_spi_set_cs(struct spi_device *spi, bool is_high)
 
 	if (is_high) {
 		hw->imr &= ~ALTERA_SPI_CONTROL_SSO_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
-		writel(0, hw->base + ALTERA_SPI_SLAVE_SEL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
+		altr_spi_writel(hw, ALTERA_SPI_SLAVE_SEL, 0);
 	} else {
-		writel(BIT(spi->chip_select), hw->base + ALTERA_SPI_SLAVE_SEL);
+		altr_spi_writel(hw, ALTERA_SPI_SLAVE_SEL,
+				BIT(spi->chip_select));
 		hw->imr |= ALTERA_SPI_CONTROL_SSO_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 	}
 }
 
@@ -99,14 +136,14 @@  static void altera_spi_tx_word(struct altera_spi *hw)
 		}
 	}
 
-	writel(txd, hw->base + ALTERA_SPI_TXDATA);
+	altr_spi_writel(hw, ALTERA_SPI_TXDATA, txd);
 }
 
 static void altera_spi_rx_word(struct altera_spi *hw)
 {
 	unsigned int rxd;
 
-	rxd = readl(hw->base + ALTERA_SPI_RXDATA);
+	altr_spi_readl(hw, ALTERA_SPI_RXDATA, &rxd);
 	if (hw->rx) {
 		switch (hw->bytes_per_word) {
 		case 1:
@@ -133,6 +170,7 @@  static int altera_spi_txrx(struct spi_master *master,
 	struct spi_device *spi, struct spi_transfer *t)
 {
 	struct altera_spi *hw = spi_master_get_devdata(master);
+	u32 val;
 
 	hw->tx = t->tx_buf;
 	hw->rx = t->rx_buf;
@@ -143,7 +181,7 @@  static int altera_spi_txrx(struct spi_master *master,
 	if (hw->irq >= 0) {
 		/* enable receive interrupt */
 		hw->imr |= ALTERA_SPI_CONTROL_IRRDY_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 
 		/* send the first byte */
 		altera_spi_tx_word(hw);
@@ -151,9 +189,13 @@  static int altera_spi_txrx(struct spi_master *master,
 		while (hw->count < hw->len) {
 			altera_spi_tx_word(hw);
 
-			while (!(readl(hw->base + ALTERA_SPI_STATUS) &
-				 ALTERA_SPI_STATUS_RRDY_MSK))
+			for (;;) {
+				altr_spi_readl(hw, ALTERA_SPI_STATUS, &val);
+				if (val & ALTERA_SPI_STATUS_RRDY_MSK)
+					break;
+
 				cpu_relax();
+			}
 
 			altera_spi_rx_word(hw);
 		}
@@ -175,7 +217,7 @@  static irqreturn_t altera_spi_irq(int irq, void *dev)
 	} else {
 		/* disable receive interrupt */
 		hw->imr &= ~ALTERA_SPI_CONTROL_IRRDY_MSK;
-		writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+		altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
 
 		spi_finalize_current_transfer(master);
 	}
@@ -188,7 +230,9 @@  static int altera_spi_probe(struct platform_device *pdev)
 	struct altera_spi_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct altera_spi *hw;
 	struct spi_master *master;
+	void __iomem *res;
 	int err = -ENODEV;
+	u32 val;
 	u16 i;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
@@ -220,19 +264,38 @@  static int altera_spi_probe(struct platform_device *pdev)
 	master->set_cs = altera_spi_set_cs;
 
 	hw = spi_master_get_devdata(master);
+	hw->dev = &pdev->dev;
 
 	/* find and map our resources */
-	hw->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(hw->base)) {
-		err = PTR_ERR(hw->base);
-		goto exit;
+	if (pdata && pdata->use_parent_regmap) {
+		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+		if (!hw->regmap) {
+			dev_err(&pdev->dev, "get regmap failed\n");
+			goto exit;
+		}
+		hw->base = pdata->regoff;
+	} else {
+		res = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(res)) {
+			err = PTR_ERR(res);
+			goto exit;
+		}
+
+		hw->regmap = devm_regmap_init_mmio(&pdev->dev, res,
+						   &spi_altera_config);
+		if (IS_ERR(hw->regmap)) {
+			dev_err(&pdev->dev, "regmap mmio init failed\n");
+			err = PTR_ERR(hw->regmap);
+			goto exit;
+		}
 	}
 	/* program defaults into the registers */
 	hw->imr = 0;		/* disable spi interrupts */
-	writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
-	writel(0, hw->base + ALTERA_SPI_STATUS);	/* clear status reg */
-	if (readl(hw->base + ALTERA_SPI_STATUS) & ALTERA_SPI_STATUS_RRDY_MSK)
-		readl(hw->base + ALTERA_SPI_RXDATA);	/* flush rxdata */
+	altr_spi_writel(hw, ALTERA_SPI_CONTROL, hw->imr);
+	altr_spi_writel(hw, ALTERA_SPI_STATUS, 0);	/* clear status reg */
+	altr_spi_readl(hw, ALTERA_SPI_STATUS, &val);
+	if (val & ALTERA_SPI_STATUS_RRDY_MSK)
+		altr_spi_readl(hw, ALTERA_SPI_RXDATA, &val); /* flush rxdata */
 	/* irq is optional */
 	hw->irq = platform_get_irq(pdev, 0);
 	if (hw->irq >= 0) {
@@ -255,7 +318,7 @@  static int altera_spi_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_info(&pdev->dev, "base %p, irq %d\n", hw->base, hw->irq);
+	dev_info(&pdev->dev, "base %u, irq %d\n", hw->base, hw->irq);
 
 	return 0;
 exit:
diff --git a/include/linux/spi/altera.h b/include/linux/spi/altera.h
index 2d42641..164ab7b 100644
--- a/include/linux/spi/altera.h
+++ b/include/linux/spi/altera.h
@@ -17,6 +17,10 @@ 
  * @num_devices:	Number of devices that shall be added when the driver
  *			is probed.
  * @devices:		The devices to add.
+ * @use_parent_regmap:	If true, device uses parent regmap to access its
+ *			registers. Otherwise try map platform mmio resources.
+ * @regoff:		Offset of the device register base in parent regmap.
+ *			This field is ignored when use_parent_regmap == false.
  */
 struct altera_spi_platform_data {
 	u16				mode_bits;
@@ -24,6 +28,8 @@  struct altera_spi_platform_data {
 	u32				bits_per_word_mask;
 	u16				num_devices;
 	struct spi_board_info		*devices;
+	bool				use_parent_regmap;
+	u32				regoff;
 };
 
 #endif /* __LINUX_SPI_ALTERA_H */