diff mbox series

[2/2] spi: tegra210-quad: add acpi support

Message ID 1637834152-32093-2-git-send-email-kyarlagadda@nvidia.com
State New
Headers show
Series [1/2] spi: tegra210-quad: use devm call for cdata memory | expand

Commit Message

Krishna Yarlagadda Nov. 25, 2021, 9:55 a.m. UTC
Support ACPI device enumeration for Tegra QUAD SPI driver.
Also pick device features from device tree.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 74 ++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 23 deletions(-)

Comments

Mark Brown Nov. 25, 2021, 12:59 p.m. UTC | #1
On Thu, Nov 25, 2021 at 03:25:52PM +0530, Krishna Yarlagadda wrote:

> +#ifdef CONFIG_ACPI
> +	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
> +					      "_RST", NULL, NULL)))
> +		dev_err(tqspi->dev, "failed to reset device\n");
> +#endif

What happens when this runs on a DT system?
Krishna Yarlagadda Nov. 29, 2021, 9:09 a.m. UTC | #2
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 25 November 2021 18:29
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: Laxman Dewangan <ldewangan@nvidia.com>; Thierry Reding
> <thierry.reding@gmail.com>; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; linux-tegra@vger.kernel.org; linux-
> spi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] spi: tegra210-quad: add acpi support
> 
> On Thu, Nov 25, 2021 at 03:25:52PM +0530, Krishna Yarlagadda wrote:
> 
> > +#ifdef CONFIG_ACPI
> > +	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
> > +					      "_RST", NULL, NULL)))
> > +		dev_err(tqspi->dev, "failed to reset device\n"); #endif
> 
> What happens when this runs on a DT system?
For a DT system reset handle would be present and this code will not run
-KY
Mark Brown Nov. 29, 2021, 12:27 p.m. UTC | #3
On Mon, Nov 29, 2021 at 09:09:30AM +0000, Krishna Yarlagadda wrote:

> > > +#ifdef CONFIG_ACPI
> > > +	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
> > > +					      "_RST", NULL, NULL)))
> > > +		dev_err(tqspi->dev, "failed to reset device\n"); #endif

> > What happens when this runs on a DT system?

> For a DT system reset handle would be present and this code will not run

This code is really unclearly structured, the early return doesn't
match the normal kernel style and the ifdefs aren't helping with
clarity.  Please restructure it so it's clear that *both* cases have
checks for the correct firmware type being present.

That said frankly I'd expect this handling of ACPI reset to be pushed
into the reset code, it's obviously not good to be open coding this in
drivers when this looks like it's completely generic to any ACPI object
so shouldn't be being open coded in individual driers especially with
the ifdefery.  Shouldn't the reset API be able to figure out that an
object with _RST has a reset control and provide access to it through
the reset API?
Krishna Yarlagadda Nov. 30, 2021, 1:50 a.m. UTC | #4
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 29 November 2021 17:57
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>; Philipp Zabel
> <p.zabel@pengutronix.de>
> Cc: Laxman Dewangan <ldewangan@nvidia.com>; Thierry Reding
> <thierry.reding@gmail.com>; Jonathan Hunter <jonathanh@nvidia.com>;
> Sowjanya Komatineni <skomatineni@nvidia.com>; linux-
> tegra@vger.kernel.org; linux-spi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] spi: tegra210-quad: add acpi support
> 
> On Mon, Nov 29, 2021 at 09:09:30AM +0000, Krishna Yarlagadda wrote:
> 
> > > > +#ifdef CONFIG_ACPI
> > > > +	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
> > > > +					      "_RST", NULL, NULL)))
> > > > +		dev_err(tqspi->dev, "failed to reset device\n"); #endif
> 
> > > What happens when this runs on a DT system?
> 
> > For a DT system reset handle would be present and this code will not
> > run
> 
> This code is really unclearly structured, the early return doesn't match the
> normal kernel style and the ifdefs aren't helping with clarity.  Please
> restructure it so it's clear that *both* cases have checks for the correct
> firmware type being present.
I will move each reset method into their own function for readability.
> 
> That said frankly I'd expect this handling of ACPI reset to be pushed into the
> reset code, it's obviously not good to be open coding this in drivers when this
> looks like it's completely generic to any ACPI object so shouldn't be being
> open coded in individual driers especially with the ifdefery.  Shouldn't the
> reset API be able to figure out that an object with _RST has a reset control
> and provide access to it through the reset API?
Common reset apis are not handling _RST. Each driver is implementing
_RST method in ACPI and calling from drivers.
Mark Brown Nov. 30, 2021, 1:05 p.m. UTC | #5
On Tue, Nov 30, 2021 at 01:50:07AM +0000, Krishna Yarlagadda wrote:

> > That said frankly I'd expect this handling of ACPI reset to be pushed into the
> > reset code, it's obviously not good to be open coding this in drivers when this
> > looks like it's completely generic to any ACPI object so shouldn't be being
> > open coded in individual driers especially with the ifdefery.  Shouldn't the
> > reset API be able to figure out that an object with _RST has a reset control
> > and provide access to it through the reset API?

> Common reset apis are not handling _RST. Each driver is implementing
> _RST method in ACPI and calling from drivers.

I can see that.  What I'm saying is that this seems bad and we should
instead be implementing this in common code.
Dmitry Osipenko Nov. 30, 2021, 2:14 p.m. UTC | #6
30.11.2021 16:05, Mark Brown пишет:
> On Tue, Nov 30, 2021 at 01:50:07AM +0000, Krishna Yarlagadda wrote:
> 
>>> That said frankly I'd expect this handling of ACPI reset to be pushed into the
>>> reset code, it's obviously not good to be open coding this in drivers when this
>>> looks like it's completely generic to any ACPI object so shouldn't be being
>>> open coded in individual driers especially with the ifdefery.  Shouldn't the
>>> reset API be able to figure out that an object with _RST has a reset control
>>> and provide access to it through the reset API?
> 
>> Common reset apis are not handling _RST. Each driver is implementing
>> _RST method in ACPI and calling from drivers.

I see only two or three other drivers in kernel which do that.

> I can see that.  What I'm saying is that this seems bad and we should
> instead be implementing this in common code.
> 

The ifdefery, that this patch has, shouldn't be needed. We have a
similar ACPI patch for Tegra I2C [1] and it doesn't have that.

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/1637859224-5179-1-git-send-email-akhilrajeev@nvidia.com/

I assume this patch could be reworked similarly to the I2C patch.

Agree that should be better to have a common reset driver for ACPI
instead of polluting each driver with a boilerplate code.
Mark Brown Nov. 30, 2021, 4:13 p.m. UTC | #7
On Tue, Nov 30, 2021 at 05:14:07PM +0300, Dmitry Osipenko wrote:

> The ifdefery, that this patch has, shouldn't be needed. We have a
> similar ACPI patch for Tegra I2C [1] and it doesn't have that.

> [1]
> https://patchwork.ozlabs.org/project/linux-tegra/patch/1637859224-5179-1-git-send-email-akhilrajeev@nvidia.com/

> I assume this patch could be reworked similarly to the I2C patch.

Yes, that looks much cleaner.

> Agree that should be better to have a common reset driver for ACPI
> instead of polluting each driver with a boilerplate code.

Right, I think that'd be even better.  It's probably best to split
adding reset support to the driver out from adding the ACPI ID - they
shouldn't really have been merged in the first place TBH - and then try
this approach first.
diff mbox series

Patch

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index ce1bdb4..20c1fa6 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -21,6 +21,8 @@ 
 #include <linux/of_device.h>
 #include <linux/reset.h>
 #include <linux/spi/spi.h>
+#include <linux/acpi.h>
+#include <linux/property.h>
 
 #define QSPI_COMMAND1				0x000
 #define QSPI_BIT_LENGTH(x)			(((x) & 0x1f) << 0)
@@ -767,7 +769,7 @@  static u32 tegra_qspi_setup_transfer_one(struct spi_device *spi, struct spi_tran
 	u32 tx_tap = 0, rx_tap = 0;
 	int req_mode;
 
-	if (speed != tqspi->cur_speed) {
+	if (!has_acpi_companion(tqspi->dev) && speed != tqspi->cur_speed) {
 		clk_set_rate(tqspi->clk, speed);
 		tqspi->cur_speed = speed;
 	}
@@ -875,16 +877,15 @@  static int tegra_qspi_start_transfer_one(struct spi_device *spi,
 static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
 {
 	struct tegra_qspi_client_data *cdata;
-	struct device_node *slave_np = spi->dev.of_node;
 
 	cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
 	if (!cdata)
 		return NULL;
 
-	of_property_read_u32(slave_np, "nvidia,tx-clk-tap-delay",
-			     &cdata->tx_clk_tap_delay);
-	of_property_read_u32(slave_np, "nvidia,rx-clk-tap-delay",
-			     &cdata->rx_clk_tap_delay);
+	device_property_read_u32(&spi->dev, "nvidia,tx-clk-tap-delay",
+				 &cdata->tx_clk_tap_delay);
+	device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
+				 &cdata->rx_clk_tap_delay);
 	return cdata;
 }
 
@@ -943,14 +944,27 @@  static void tegra_qspi_dump_regs(struct tegra_qspi *tqspi)
 		tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS));
 }
 
+static void tegra_qspi_reset(struct tegra_qspi *tqspi)
+{
+	if (tqspi->rst) {
+		reset_control_assert(tqspi->rst);
+		udelay(2);
+		reset_control_deassert(tqspi->rst);
+		return;
+	}
+#ifdef CONFIG_ACPI
+	if (ACPI_FAILURE(acpi_evaluate_object(ACPI_HANDLE(tqspi->dev),
+					      "_RST", NULL, NULL)))
+		dev_err(tqspi->dev, "failed to reset device\n");
+#endif
+}
+
 static void tegra_qspi_handle_error(struct tegra_qspi *tqspi)
 {
 	dev_err(tqspi->dev, "error in transfer, fifo status 0x%08x\n", tqspi->status_reg);
 	tegra_qspi_dump_regs(tqspi);
 	tegra_qspi_flush_fifos(tqspi, true);
-	reset_control_assert(tqspi->rst);
-	udelay(2);
-	reset_control_deassert(tqspi->rst);
+	tegra_qspi_reset(tqspi);
 }
 
 static void tegra_qspi_transfer_end(struct spi_device *spi)
@@ -1202,6 +1216,14 @@  static const struct of_device_id tegra_qspi_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tegra_qspi_of_match);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id tegra_qspi_acpi_match[] = {
+	{ .id = "NVDA14E2", },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, tegra_qspi_acpi_match);
+#endif
+
 static int tegra_qspi_probe(struct platform_device *pdev)
 {
 	struct spi_master	*master;
@@ -1242,18 +1264,20 @@  static int tegra_qspi_probe(struct platform_device *pdev)
 	qspi_irq = platform_get_irq(pdev, 0);
 	tqspi->irq = qspi_irq;
 
-	tqspi->clk = devm_clk_get(&pdev->dev, "qspi");
-	if (IS_ERR(tqspi->clk)) {
-		ret = PTR_ERR(tqspi->clk);
-		dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
-		return ret;
-	}
+	if (!has_acpi_companion(tqspi->dev)) {
+		tqspi->clk = devm_clk_get(&pdev->dev, "qspi");
+		if (IS_ERR(tqspi->clk)) {
+			ret = PTR_ERR(tqspi->clk);
+			dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
+			return ret;
+		}
 
-	tqspi->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
-	if (IS_ERR(tqspi->rst)) {
-		ret = PTR_ERR(tqspi->rst);
-		dev_err(&pdev->dev, "failed to get reset control: %d\n", ret);
-		return ret;
+		tqspi->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+		if (IS_ERR(tqspi->rst)) {
+			dev_err(&pdev->dev, "failed to get reset control: %d\n", ret);
+			ret = PTR_ERR(tqspi->rst);
+			return ret;
+		}
 	}
 
 	tqspi->max_buf_size = QSPI_FIFO_DEPTH << 2;
@@ -1277,9 +1301,7 @@  static int tegra_qspi_probe(struct platform_device *pdev)
 		goto exit_pm_disable;
 	}
 
-	reset_control_assert(tqspi->rst);
-	udelay(2);
-	reset_control_deassert(tqspi->rst);
+	tegra_qspi_reset(tqspi);
 
 	tqspi->def_command1_reg = QSPI_M_S | QSPI_CS_SW_HW |  QSPI_CS_SW_VAL;
 	tegra_qspi_writel(tqspi, tqspi->def_command1_reg, QSPI_COMMAND1);
@@ -1353,6 +1375,10 @@  static int __maybe_unused tegra_qspi_resume(struct device *dev)
 	return spi_master_resume(master);
 }
 
+#ifdef CONFIG_ACPI
+static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev) { return 0; }
+static int __maybe_unused tegra_qspi_runtime_resume(struct device *dev) { return 0; }
+#else
 static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
@@ -1378,6 +1404,7 @@  static int __maybe_unused tegra_qspi_runtime_resume(struct device *dev)
 
 	return ret;
 }
+#endif
 
 static const struct dev_pm_ops tegra_qspi_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_qspi_runtime_suspend, tegra_qspi_runtime_resume, NULL)
@@ -1389,6 +1416,7 @@  static struct platform_driver tegra_qspi_driver = {
 		.name		= "tegra-qspi",
 		.pm		= &tegra_qspi_pm_ops,
 		.of_match_table	= tegra_qspi_of_match,
+		.acpi_match_table = ACPI_PTR(tegra_qspi_acpi_match),
 	},
 	.probe =	tegra_qspi_probe,
 	.remove =	tegra_qspi_remove,