| Message ID | 20220222175611.58051-5-kyarlagadda@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tegra QUAD SPI combined sequence mode | expand |
On Tue, Feb 22, 2022 at 11:26:10PM +0530, Krishna Yarlagadda wrote: > Add ACPI ID for Tegra QUAD SPI. Switch to common device property calls. > Skip clock calls that are not updated in ACPI boot. > @@ -1377,6 +1400,8 @@ static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev) > struct spi_master *master = dev_get_drvdata(dev); > struct tegra_qspi *tqspi = spi_master_get_devdata(master); > > + if (has_acpi_companion(tqspi->dev)) > + return 0; > /* flush all write which are in PPSB queue by reading back */ > tegra_qspi_readl(tqspi, QSPI_COMMAND1); As well as clock stuff this is also skipping flushing of pending writes - is that intentional? It's not called out in the changelog and seems like something that could cause issues if someone runs on a system where the firmware does implement runtime suspend.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: 23 February 2022 00:07 > To: Krishna Yarlagadda <kyarlagadda@nvidia.com> > Cc: thierry.reding@gmail.com; Jonathan Hunter > <jonathanh@nvidia.com>; linux-spi@vger.kernel.org; linux- > tegra@vger.kernel.org; Sowjanya Komatineni > <skomatineni@nvidia.com>; Laxman Dewangan > <ldewangan@nvidia.com>; robh+dt@kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > p.zabel@pengutronix.de > Subject: Re: [PATCH v2 4/5] spi: tegra210-quad: add acpi support > > On Tue, Feb 22, 2022 at 11:26:10PM +0530, Krishna Yarlagadda wrote: > > > Add ACPI ID for Tegra QUAD SPI. Switch to common device property > calls. > > Skip clock calls that are not updated in ACPI boot. > > > @@ -1377,6 +1400,8 @@ static int __maybe_unused > tegra_qspi_runtime_suspend(struct device *dev) > > struct spi_master *master = dev_get_drvdata(dev); > > struct tegra_qspi *tqspi = spi_master_get_devdata(master); > > > > + if (has_acpi_companion(tqspi->dev)) > > + return 0; > > /* flush all write which are in PPSB queue by reading back */ > > tegra_qspi_readl(tqspi, QSPI_COMMAND1); > > As well as clock stuff this is also skipping flushing of pending writes > - is that intentional? It's not called out in the changelog and seems like > something that could cause issues if someone runs on a system where > the firmware does implement runtime suspend. Runtime suspend is not enabled with ACPI firmware. Converted compiler flag in v1 to runtime check. We must add more changes like setting DPM flags for runtime pm support with ACPI. Can take this as part of a different series.
On Wed, Feb 23, 2022 at 06:32:56AM +0000, Krishna Yarlagadda wrote: > > > + if (has_acpi_companion(tqspi->dev)) > > > + return 0; > > > /* flush all write which are in PPSB queue by reading back */ > > > tegra_qspi_readl(tqspi, QSPI_COMMAND1); > > As well as clock stuff this is also skipping flushing of pending writes > > - is that intentional? It's not called out in the changelog and seems like > > something that could cause issues if someone runs on a system where > > the firmware does implement runtime suspend. > Runtime suspend is not enabled with ACPI firmware. Converted compiler flag in v1 to runtime check. > We must add more changes like setting DPM flags for runtime pm support with ACPI. > Can take this as part of a different series. It at least needs to be clearer what's going on here, the changelog doesn't match the code and it's not obvious from the code that ACPI won't kick in and power manage the device as things stand.
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: 24 February 2022 23:52 > To: Krishna Yarlagadda <kyarlagadda@nvidia.com> > Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>; linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; > Sowjanya Komatineni <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>; robh+dt@kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; p.zabel@pengutronix.de > Subject: Re: [PATCH v2 4/5] spi: tegra210-quad: add acpi support > > On Wed, Feb 23, 2022 at 06:32:56AM +0000, Krishna Yarlagadda wrote: > > > > > + if (has_acpi_companion(tqspi->dev)) > > > > + return 0; > > > > /* flush all write which are in PPSB queue by reading back */ > > > > tegra_qspi_readl(tqspi, QSPI_COMMAND1); > > > > As well as clock stuff this is also skipping flushing of pending writes > > > - is that intentional? It's not called out in the changelog and seems like > > > something that could cause issues if someone runs on a system where > > > the firmware does implement runtime suspend. > > > Runtime suspend is not enabled with ACPI firmware. Converted compiler flag in v1 to runtime check. > > We must add more changes like setting DPM flags for runtime pm support with ACPI. > > Can take this as part of a different series. > > It at least needs to be clearer what's going on here, the changelog > doesn't match the code and it's not obvious from the code that ACPI > won't kick in and power manage the device as things stand. Understood Mark. I will add comments to make it clear. Also update change log indicating runtime suspend does not work with ACPI.
diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c index 3725ee5331ae..0dbcb5fffc03 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) @@ -771,7 +773,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; } @@ -879,16 +881,16 @@ 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; } @@ -1227,6 +1229,24 @@ 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 = "NVDA1213", + .driver_data = (kernel_ulong_t)&tegra210_qspi_soc_data, + }, { + .id = "NVDA1313", + .driver_data = (kernel_ulong_t)&tegra186_qspi_soc_data, + }, { + .id = "NVDA1413", + .driver_data = (kernel_ulong_t)&tegra234_qspi_soc_data, + }, + {} +}; + +MODULE_DEVICE_TABLE(acpi, tegra_qspi_acpi_match); +#endif + static int tegra_qspi_probe(struct platform_device *pdev) { struct spi_master *master; @@ -1269,11 +1289,14 @@ static int tegra_qspi_probe(struct platform_device *pdev) return qspi_irq; 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->max_buf_size = QSPI_FIFO_DEPTH << 2; @@ -1377,6 +1400,8 @@ static int __maybe_unused tegra_qspi_runtime_suspend(struct device *dev) struct spi_master *master = dev_get_drvdata(dev); struct tegra_qspi *tqspi = spi_master_get_devdata(master); + if (has_acpi_companion(tqspi->dev)) + return 0; /* flush all write which are in PPSB queue by reading back */ tegra_qspi_readl(tqspi, QSPI_COMMAND1); @@ -1391,6 +1416,8 @@ static int __maybe_unused tegra_qspi_runtime_resume(struct device *dev) struct tegra_qspi *tqspi = spi_master_get_devdata(master); int ret; + if (has_acpi_companion(tqspi->dev)) + return 0; ret = clk_prepare_enable(tqspi->clk); if (ret < 0) dev_err(tqspi->dev, "failed to enable clock: %d\n", ret); @@ -1408,6 +1435,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,
Add ACPI ID for Tegra QUAD SPI. Switch to common device property calls. Skip clock calls that are not updated in ACPI boot. Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com> --- drivers/spi/spi-tegra210-quad.c | 50 +++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 11 deletions(-)