by Mikko Perttunen

[permalink] [raw]

Subject: [PATCH v4 7/8] ata: Add support for the Tegra124 SATA controller

This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

Signed-off-by: Mikko Perttunen <[email protected]>
---
v4: init phy in power_on

drivers/ata/Kconfig | 9 ++
drivers/ata/Makefile | 1 +
drivers/ata/ahci_tegra.c | 406 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 416 insertions(+)
create mode 100644 drivers/ata/ahci_tegra.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b0d5b5a..e1b9278 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -142,6 +142,15 @@ config AHCI_SUNXI

If unsure, say N.

+config AHCI_TEGRA
+ tristate "NVIDIA Tegra124 AHCI SATA support"
+ depends on ARCH_TEGRA
+ help
+ This option enables support for the NVIDIA Tegra124 SoC's
+ onboard AHCI SATA.
+
+ If unsure, say N.
+
config AHCI_XGENE
tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support"
depends on PHY_XGENE
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 5a02aee..ae41107 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_ST) += ahci_st.o libahci.o libahci_platform.o
+obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o

# SFF w/ custom DMA
diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
new file mode 100644
index 0000000..0f59ae4
--- /dev/null
+++ b/drivers/ata/ahci_tegra.c
@@ -0,0 +1,406 @@
+/*
+ * drivers/ata/ahci_tegra.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
+ *
+ * Author:
+ * Mikko Perttunen <[email protected]>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/reset.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/tegra-powergate.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define SATA_CONFIGURATION_0 0x180
+#define SATA_CONFIGURATION_EN_FPCI BIT(0)
+
+#define SCFG_OFFSET 0x1000
+
+#define T_SATA0_CFG_1 0x04
+#define T_SATA0_CFG_1_IO_SPACE BIT(0)
+#define T_SATA0_CFG_1_MEMORY_SPACE BIT(1)
+#define T_SATA0_CFG_1_BUS_MASTER BIT(2)
+#define T_SATA0_CFG_1_SERR BIT(8)
+
+#define T_SATA0_CFG_9 0x24
+#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT 13
+
+#define SATA_FPCI_BAR5 0x94
+#define SATA_FPCI_BAR5_START_SHIFT 4
+
+#define SATA_INTR_MASK 0x188
+#define SATA_INTR_MASK_IP_INT_MASK BIT(16)
+
+#define T_SATA0_AHCI_HBA_CAP_BKDR 0x300
+
+#define T_SATA0_BKDOOR_CC 0x4a4
+
+#define T_SATA0_CFG_SATA 0x54c
+#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN BIT(12)
+
+#define T_SATA0_CFG_MISC 0x550
+
+#define T_SATA0_INDEX 0x680
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN1 0x690
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK 0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT 0
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK (0xff << 8)
+#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT 8
+
+#define T_SATA0_CHX_PHY_CTRL1_GEN2 0x694
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK 0xff
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT 0
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK (0xff << 12)
+#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT 12
+
+#define T_SATA0_CHX_PHY_CTRL2 0x69c
+#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1 0x23
+
+#define T_SATA0_CHX_PHY_CTRL11 0x6d0
+#define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ (0x2800 << 16)
+
+#define FUSE_SATA_CALIB 0x124
+#define FUSE_SATA_CALIB_MASK 0x3
+
+enum sata_clks {
+ AHCI_CLK_SATA = 0,
+ AHCI_CLK_SATA_OOB = 1,
+ AHCI_CLK_CML1 = 2,
+ AHCI_CLK_PLL_E = 3
+};
+
+struct sata_pad_calibration {
+ u8 gen1_tx_amp;
+ u8 gen1_tx_peak;
+ u8 gen2_tx_amp;
+ u8 gen2_tx_peak;
+};
+
+static const struct sata_pad_calibration tegra124_pad_calibration[] = {
+ {0x18, 0x04, 0x18, 0x0a},
+ {0x0e, 0x04, 0x14, 0x0a},
+ {0x0e, 0x07, 0x1a, 0x0e},
+ {0x14, 0x0e, 0x1a, 0x0e},
+};
+
+struct tegra_ahci_priv {
+ struct platform_device *pdev;
+ void __iomem *sata_regs;
+ struct reset_control *sata_rst;
+ struct reset_control *sata_oob_rst;
+ struct reset_control *sata_cold_rst;
+ struct regulator_bulk_data supplies[5];
+};
+
+static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
+{
+ struct tegra_ahci_priv *tegra = hpriv->plat_data;
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
+ tegra->supplies);
+ if (ret)
+ return ret;
+
+ ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
+ hpriv->clks[AHCI_CLK_SATA],
+ tegra->sata_rst);
+ if (ret)
+ goto disable_regulators;
+
+ reset_control_assert(tegra->sata_oob_rst);
+ reset_control_assert(tegra->sata_cold_rst);
+
+ ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_SATA_OOB]);
+ if (ret)
+ goto disable_power;
+
+ ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_CML1]);
+ if (ret)
+ goto disable_oob;
+
+ ret = clk_prepare_enable(hpriv->clks[AHCI_CLK_PLL_E]);
+ if (ret)
+ goto disable_cml1;
+
+ reset_control_deassert(tegra->sata_cold_rst);
+ reset_control_deassert(tegra->sata_oob_rst);
+
+ ret = phy_init(hpriv->phy);
+ if (ret)
+ goto disable_plle;
+
+ ret = phy_power_on(hpriv->phy);
+ if (ret)
+ goto exit_phy;
+
+ return 0;
+
+exit_phy:
+ phy_exit(hpriv->phy);
+
+disable_plle:
+ clk_disable_unprepare(hpriv->clks[AHCI_CLK_PLL_E]);
+
+disable_cml1:
+ clk_disable_unprepare(hpriv->clks[AHCI_CLK_CML1]);
+
+disable_oob:
+ clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA_OOB]);
+
+disable_power:
+ clk_disable_unprepare(hpriv->clks[AHCI_CLK_SATA]);
+
+ tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+disable_regulators:
+ regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+
+ return ret;
+}
+
+static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
+{
+ struct tegra_ahci_priv *tegra = hpriv->plat_data;
+
+ ahci_platform_disable_resources(hpriv);
+
+ reset_control_assert(tegra->sata_rst);
+ reset_control_assert(tegra->sata_oob_rst);
+ reset_control_assert(tegra->sata_cold_rst);
+
+ tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
+
+ regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
+}
+
+static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
+{
+ struct tegra_ahci_priv *tegra = hpriv->plat_data;
+ int ret;
+ unsigned int val;
+ struct sata_pad_calibration calib;
+
+ ret = tegra_ahci_power_on(hpriv);
+ if (ret) {
+ dev_err(&tegra->pdev->dev,
+ "failed to power on AHCI controller: %d\n", ret);
+ return ret;
+ }
+
+ val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
+ val |= SATA_CONFIGURATION_EN_FPCI;
+ writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
+
+ /* Pad calibration */
+
+ /* FIXME Always use calibration 0. Change this to read the calibration
+ * fuse once the fuse driver has landed. */
+ val = 0;
+
+ calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
+
+ writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+ val = readl(tegra->sata_regs +
+ SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
+ val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
+ val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
+ val |= calib.gen1_tx_amp <<
+ T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+ val |= calib.gen1_tx_peak <<
+ T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+ writel(val, tegra->sata_regs + SCFG_OFFSET +
+ T_SATA0_CHX_PHY_CTRL1_GEN1);
+
+ val = readl(tegra->sata_regs +
+ SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
+ val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
+ val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
+ val |= calib.gen2_tx_amp <<
+ T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
+ val |= calib.gen2_tx_peak <<
+ T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
+ writel(val, tegra->sata_regs + SCFG_OFFSET +
+ T_SATA0_CHX_PHY_CTRL1_GEN2);
+
+ writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
+ tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
+ writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
+ tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
+
+ writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
+
+ /* Program controller device ID */
+
+ val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+ val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+ writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+ writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
+
+ val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+ val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
+ writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
+
+ /* Enable IO & memory access, bus master mode */
+
+ val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+ val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
+ T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
+ writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
+
+ /* Program SATA MMIO */
+
+ writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
+ tegra->sata_regs + SATA_FPCI_BAR5);
+
+ writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
+ tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
+
+ /* Unmask SATA interrupts */
+
+ val = readl(tegra->sata_regs + SATA_INTR_MASK);
+ val |= SATA_INTR_MASK_IP_INT_MASK;
+ writel(val, tegra->sata_regs + SATA_INTR_MASK);
+
+ return 0;
+}
+
+static void tegra_ahci_controller_deinit(struct ahci_host_priv *hpriv)
+{
+ tegra_ahci_power_off(hpriv);
+}
+
+static void tegra_ahci_host_stop(struct ata_host *host)
+{
+ struct ahci_host_priv *hpriv = host->private_data;
+
+ tegra_ahci_controller_deinit(hpriv);
+}
+
+static struct ata_port_operations ahci_tegra_port_ops = {
+ .inherits = &ahci_ops,
+ .host_stop = tegra_ahci_host_stop,
+};
+
+static const struct ata_port_info ahci_tegra_port_info = {
+ .flags = AHCI_FLAG_COMMON,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_tegra_port_ops,
+};
+
+static const struct of_device_id tegra_ahci_of_match[] = {
+ { .compatible = "nvidia,tegra124-ahci" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
+
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+ struct ahci_host_priv *hpriv;
+ struct tegra_ahci_priv *tegra;
+ struct resource *res;
+ int ret;
+
+ hpriv = ahci_platform_get_resources(pdev);
+ if (IS_ERR(hpriv))
+ return PTR_ERR(hpriv);
+
+ tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+ if (!tegra)
+ return -ENOMEM;
+
+ hpriv->plat_data = tegra;
+
+ tegra->pdev = pdev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tegra->sata_regs))
+ return PTR_ERR(tegra->sata_regs);
+
+ tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
+ if (IS_ERR(tegra->sata_rst)) {
+ dev_err(&pdev->dev, "Failed to get sata reset\n");
+ return PTR_ERR(tegra->sata_rst);
+ }
+
+ tegra->sata_oob_rst = devm_reset_control_get(&pdev->dev, "sata-oob");
+ if (IS_ERR(tegra->sata_oob_rst)) {
+ dev_err(&pdev->dev, "Failed to get sata-oob reset\n");
+ return PTR_ERR(tegra->sata_oob_rst);
+ }
+
+ tegra->sata_cold_rst = devm_reset_control_get(&pdev->dev, "sata-cold");
+ if (IS_ERR(tegra->sata_cold_rst)) {
+ dev_err(&pdev->dev, "Failed to get sata-cold reset\n");
+ return PTR_ERR(tegra->sata_cold_rst);
+ }
+
+ if (!hpriv->clks[AHCI_CLK_PLL_E])
+ return -EPROBE_DEFER;
+
+ tegra->supplies[0].supply = "avdd";
+ tegra->supplies[1].supply = "hvdd";
+ tegra->supplies[2].supply = "vddio";
+ tegra->supplies[3].supply = "target-5v";
+ tegra->supplies[4].supply = "target-12v";
+
+ ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
+ tegra->supplies);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to get regulators\n");
+ return ret;
+ }
+
+ ret = tegra_ahci_controller_init(hpriv);
+ if (ret)
+ return ret;
+
+ ret = ahci_platform_init_host(pdev, hpriv, &ahci_tegra_port_info,
+ 0, 0, 0);
+ if (ret)
+ goto deinit_controller;
+
+ return 0;
+
+deinit_controller:
+ tegra_ahci_controller_deinit(hpriv);
+
+ return ret;
+};
+
+static struct platform_driver tegra_ahci_driver = {
+ .probe = tegra_ahci_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = "tegra-ahci",
+ .of_match_table = tegra_ahci_of_match,
+ },
+ /* LP0 suspend support not implemented */
+};
+module_platform_driver(tegra_ahci_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <[email protected]>");
+MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
+MODULE_LICENSE("GPL v2");
--
1.8.1.5

2014-07-16 11:42:29

On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
> Hi,
>
> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> > This patch adds device tree binding documentation for the SATA
> > controller found on NVIDIA Tegra SoCs.
> >
> > Signed-off-by: Mikko Perttunen <[email protected]>
> > ---
> > v4: clarify mandatory clock order
>
> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
> both look good to me. So these 2 + v3 for the rest of the series are:
>
> Acked-by: Hans de Goede <[email protected]>

Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
order is a mistake.

Thierry

Attachments:

(No filename) (651.00 B)
(No filename) (819.00 B)
Download all attachments

2014-07-16 14:48:14

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

Hi,

On 07/16/2014 03:13 PM, Thierry Reding wrote:
> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
>>> This patch adds device tree binding documentation for the SATA
>>> controller found on NVIDIA Tegra SoCs.
>>>
>>> Signed-off-by: Mikko Perttunen <[email protected]>
>>> ---
>>> v4: clarify mandatory clock order
>>
>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
>> both look good to me. So these 2 + v3 for the rest of the series are:
>>
>> Acked-by: Hans de Goede <[email protected]>
>
> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
> order is a mistake.

We've plenty of other dt bindings where things need to be specified in
a certain order, e.g. registers. So I don't really see what the problem
is here.

Regards,

Hans

2014-07-16 19:51:47

On Thu, Jul 17, 2014 at 08:51:15AM +0200, Hans de Goede wrote:
> Hi,
>
> On 07/16/2014 09:51 PM, Thierry Reding wrote:
> > On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 07/16/2014 03:13 PM, Thierry Reding wrote:
> >>> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
> >>>>> This patch adds device tree binding documentation for the SATA
> >>>>> controller found on NVIDIA Tegra SoCs.
> >>>>>
> >>>>> Signed-off-by: Mikko Perttunen <[email protected]>
> >>>>> ---
> >>>>> v4: clarify mandatory clock order
> >>>>
> >>>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
> >>>> both look good to me. So these 2 + v3 for the rest of the series are:
> >>>>
> >>>> Acked-by: Hans de Goede <[email protected]>
> >>>
> >>> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
> >>> order is a mistake.
> >>
> >> We've plenty of other dt bindings where things need to be specified in
> >> a certain order, e.g. registers. So I don't really see what the problem
> >> is here.
> >
> > Like I said, the clock-names exists so that drivers can request a clock
> > by name. Therefore the order in which they are listed doesn't matter.
> > The only thing that matters is that the entries in clocks and
> > clock-names match up.
>
> Ok so I've been think about this, and about the unbalance I've noticed
> between tegra_ahci_power_on which does everything DIY and tegra_ahci_power_off
> which uses ahci_platform_disable_resources() in v3 and later.
>
> Really only the "sata" clock needs special handling, so I think the following
> solution is best:
>
> 1) Drop the clock ordering requirement and the clk enum
>
> 2) Make ahci_tegra.c do a devm_clk_get(dev, "sata"), so that it gets its
> own handle to the sata-clk (store this in tegra_ahci_priv). no need to
> get all the other clks which need no special handling
>
> 3) Start using ahci_platform_enable_resources() in tegra_ahci_power_on,
> so it would look something like this:
>
> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> {
> struct tegra_ahci_priv *tegra = hpriv->plat_data;
> int ret;
>
> ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> tegra->supplies);
> if (ret)
> return ret;
>
> ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> tegra->sata_clk,
> tegra->sata_rst);
> if (ret)
> goto disable_regulators;
>
> reset_control_deassert(tegra->sata_cold_rst);
> reset_control_deassert(tegra->sata_oob_rst);
>
> ret = ahci_platform_enable_resources(hpriv);
> if (ret)
> goto powergate_sequence_power_off;
>
> return 0;
> ...
>
> This will make tegra_ahci_power_on and tegra_ahci_power_off symmetrical
> which is something I always like to see in functions like this.
>
> I realize that this changes the reset-deassert vs clock enabling ordering,
> if this is an issue please add reset support to libahci-platform.c I believe
> that is something which we will need to do soonish anyways (reset controllers
> are popping up everywhere in newer SoCs).

I think we could safely move the reset deassert after the call to
ahci_platform_enable_resources().

> This will nicely reduce the amount of code and also greatly simplify the error
> return path of tegra_ahci_power_on.

Agreed, htat sounds like it could work.

> This means that the sata_clk will get enabled twice, but that is harmless
> as long as we disable it twice too. This means that we need to add an
> extra disable to tegra_ahci_power_off because tegra_powergate_power_off
> seems to not do this (unlike power-on, which is rather unsymmetrical
> it would be nice to fix this).

We've never had a need for it because the exact power down sequence
isn't nearly as important. But I guess we could add a new function
tegra_powergate_sequence_power_down() that takes care of disabling the
clock and asserting the reset.

One other thing that I've been thinking about is whether it would make
sense to make the ahci_platform library use a list of clock names that
it should request. This would better mirror the clock bindings
convention and allow drivers (such as the Tegra one) to take ownership
of clocks that need special handling while at the same time leaving it
to the helpers to do the bulk of the work.

One way I can think of to handle this would be by adding a struct
ahci_platform_resources * parameter to ahci_platform_get_resources(),
sowewhat like this:

struct ahci_platform_resources {
const char *const *clocks;
unsigned int num_clocks;

const char *const *resets;
unsigned int num_resets;
};

struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
const struct ahci_platform_resources *res)
{
...

for (i = 0; i < res->num_clocks; i++) {
clk = clk_get(&pdev->dev, res->clocks[i]);
...
}

...

for (i = 0; i < res->num_resets; i++) {
rst = reset_control_get(&pdev->dev, res->resets[i]);
...
}

...
}

And I guess the same could be done for regulators (and even phys). The
Tegra driver for instance could then do this:

static const char *const tegra_ahci_clocks[] = {
"sata-oob", "cml1", pll_e",
};

static const char *const tegra_ahci_resets[] = {
"sata-oob", "sata-cold",
};

static const struct ahci_platform_resources tegra_ahci_resources = {
.num_clocks = ARRAY_SIZE(tegra_ahci_clocks),
.clocks = tegra_ahci_clocks,
.num_resets = ARRAY_SIZE(tegra_ahci_resets),
.resets = tegra_ahci_resets,
};

...

struct tegra_ahci {
struct ahci_host_priv *host;
struct reset_control *rst;
struct clk *clk;
...
};

...

static int tegra_ahci_probe(struct platform_device *pdev)
{
struct tegra_ahci *ahci;

...

ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
if (!ahci)
return -ENOMEM;

ahci->host = ahci_platform_get_resources(pdev, &tegra_ahci_resources);
if (IS_ERR(ahci->host)) {
...
}

...

platform_set_drvdata(pdev, ahci);
}

Does that sound reasonable?

Thierry

Attachments:

(No filename) (5.95 kB)
(No filename) (819.00 B)
Download all attachments

2014-07-17 07:56:52

by Mikko Perttunen

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

On 17/07/14 10:39, Thierry Reding wrote:
> ...
> One other thing that I've been thinking about is whether it would make
> sense to make the ahci_platform library use a list of clock names that
> it should request. This would better mirror the clock bindings
> convention and allow drivers (such as the Tegra one) to take ownership
> of clocks that need special handling while at the same time leaving it
> to the helpers to do the bulk of the work.
>
> One way I can think of to handle this would be by adding a struct
> ahci_platform_resources * parameter to ahci_platform_get_resources(),
> sowewhat like this:
>
> struct ahci_platform_resources {
> const char *const *clocks;
> unsigned int num_clocks;
>
> const char *const *resets;
> unsigned int num_resets;
> };
>
> struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> const struct ahci_platform_resources *res)
> {
> ...
>
> for (i = 0; i < res->num_clocks; i++) {
> clk = clk_get(&pdev->dev, res->clocks[i]);
> ...
> }
>
> ...
>
> for (i = 0; i < res->num_resets; i++) {
> rst = reset_control_get(&pdev->dev, res->resets[i]);
> ...
> }
>
> ...
> }
>

I think something like this would be required to support reset_controls
anyway, as you can only get reset controls by name. This is what I
alluded to (in the cover letter) when saying that adding reset control
support would require an API break.

Also: is there a reason to not use the devm_* variants? I note that the
helper code has not been able to prevent any of the ahci_platform
drivers from messing up by not calling ahci_platform_put_resources.

- Mikko

2014-07-17 10:24:23

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

Hi,

On 07/17/2014 09:39 AM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 08:51:15AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/16/2014 09:51 PM, Thierry Reding wrote:
>>> On Wed, Jul 16, 2014 at 04:47:38PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 07/16/2014 03:13 PM, Thierry Reding wrote:
>>>>> On Wed, Jul 16, 2014 at 01:49:57PM +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 07/16/2014 01:40 PM, Mikko Perttunen wrote:
>>>>>>> This patch adds device tree binding documentation for the SATA
>>>>>>> controller found on NVIDIA Tegra SoCs.
>>>>>>>
>>>>>>> Signed-off-by: Mikko Perttunen <[email protected]>
>>>>>>> ---
>>>>>>> v4: clarify mandatory clock order
>>>>>>
>>>>>> Thanks this and the new v4 of "ata: Add support for the Tegra124 SATA controller"
>>>>>> both look good to me. So these 2 + v3 for the rest of the series are:
>>>>>>
>>>>>> Acked-by: Hans de Goede <[email protected]>
>>>>>
>>>>> Like I said in my reply to PATCH v3 7/8, I think this mandatory clock
>>>>> order is a mistake.
>>>>
>>>> We've plenty of other dt bindings where things need to be specified in
>>>> a certain order, e.g. registers. So I don't really see what the problem
>>>> is here.
>>>
>>> Like I said, the clock-names exists so that drivers can request a clock
>>> by name. Therefore the order in which they are listed doesn't matter.
>>> The only thing that matters is that the entries in clocks and
>>> clock-names match up.
>>
>> Ok so I've been think about this, and about the unbalance I've noticed
>> between tegra_ahci_power_on which does everything DIY and tegra_ahci_power_off
>> which uses ahci_platform_disable_resources() in v3 and later.
>>
>> Really only the "sata" clock needs special handling, so I think the following
>> solution is best:
>>
>> 1) Drop the clock ordering requirement and the clk enum
>>
>> 2) Make ahci_tegra.c do a devm_clk_get(dev, "sata"), so that it gets its
>> own handle to the sata-clk (store this in tegra_ahci_priv). no need to
>> get all the other clks which need no special handling
>>
>> 3) Start using ahci_platform_enable_resources() in tegra_ahci_power_on,
>> so it would look something like this:
>>
>> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>> {
>> struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> int ret;
>>
>> ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
>> tegra->supplies);
>> if (ret)
>> return ret;
>>
>> ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>> tegra->sata_clk,
>> tegra->sata_rst);
>> if (ret)
>> goto disable_regulators;
>>
>> reset_control_deassert(tegra->sata_cold_rst);
>> reset_control_deassert(tegra->sata_oob_rst);
>>
>> ret = ahci_platform_enable_resources(hpriv);
>> if (ret)
>> goto powergate_sequence_power_off;
>>
>> return 0;
>> ...
>>
>> This will make tegra_ahci_power_on and tegra_ahci_power_off symmetrical
>> which is something I always like to see in functions like this.
>>
>> I realize that this changes the reset-deassert vs clock enabling ordering,
>> if this is an issue please add reset support to libahci-platform.c I believe
>> that is something which we will need to do soonish anyways (reset controllers
>> are popping up everywhere in newer SoCs).
>
> I think we could safely move the reset deassert after the call to
> ahci_platform_enable_resources().

Will the resets not interfere with the phy_init / power_on which is done from
ahci_platform_enable_resources().

>> This will nicely reduce the amount of code and also greatly simplify the error
>> return path of tegra_ahci_power_on.
>
> Agreed, htat sounds like it could work.
>
>> This means that the sata_clk will get enabled twice, but that is harmless
>> as long as we disable it twice too. This means that we need to add an
>> extra disable to tegra_ahci_power_off because tegra_powergate_power_off
>> seems to not do this (unlike power-on, which is rather unsymmetrical
>> it would be nice to fix this).
>
> We've never had a need for it because the exact power down sequence
> isn't nearly as important. But I guess we could add a new function
> tegra_powergate_sequence_power_down() that takes care of disabling the
> clock and asserting the reset.

Ok, no need to add it for my sake, I was just wondering about the non
symmetry of the API.

> One other thing that I've been thinking about is whether it would make
> sense to make the ahci_platform library use a list of clock names that
> it should request. This would better mirror the clock bindings
> convention and allow drivers (such as the Tegra one) to take ownership
> of clocks that need special handling while at the same time leaving it
> to the helpers to do the bulk of the work.
>
> One way I can think of to handle this would be by adding a struct
> ahci_platform_resources * parameter to ahci_platform_get_resources(),
> sowewhat like this:
>
> struct ahci_platform_resources {
> const char *const *clocks;
> unsigned int num_clocks;
>
> const char *const *resets;
> unsigned int num_resets;
> };
>
> struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> const struct ahci_platform_resources *res)
> {
> ...
>
> for (i = 0; i < res->num_clocks; i++) {
> clk = clk_get(&pdev->dev, res->clocks[i]);
> ...
> }
>
> ...
>
> for (i = 0; i < res->num_resets; i++) {
> rst = reset_control_get(&pdev->dev, res->resets[i]);
> ...
> }
>
> ...
> }

Interesting, I think this is a quite good idea. I do see some pitfalls though,
to answers Mikko's question from his followup :

On 07/17/2014 09:56 AM, Mikko Perttunen wrote:
> Also: is there a reason to not use the devm_* variants? I note that the helper code has not been able to prevent any of the ahci_platform drivers from messing up by not calling ahci_platform_put_resources.

The libahci_platform.c code / ahci_platform.c code is also used for
devices going way back who may not yet be using the new clk framework,
so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :

for (i = 0; i < AHCI_MAX_CLKS; i++) {
/*
* For now we must use clk_get(dev, NULL) for the first clock,
* because some platforms (da850, spear13xx) are not yet
* converted to use devicetree for clocks. For new platforms
* this is equivalent to of_clk_get(dev->of_node, 0).
*/
if (i == 0)
clk = clk_get(dev, NULL);
else
clk = of_clk_get(dev->of_node, i);

if (IS_ERR(clk)) {
rc = PTR_ERR(clk);
if (rc == -EPROBE_DEFER)
goto err_out;
break;
}
hpriv->clks[i] = clk;
}

And there is no devm variant of that, nor is there one to get clocks by index.
Note that we also need ahci_platform_put_resources for runtime pm support, so
that one is going to stay around anyways and thus there is not that much value
in fixing this.

So although I like Thierry's idea, if we go this way (which sounds good), we
should add support for taking a NULL ahci_platform_resources argument and in
that case behave as before, esp. because of the platforms needing the old
style clock handling. An advantage of doing this, is that we can simply patch
all existing users to pass NULL.

I guess we could have a default ahci_platform_resources struct which gets
used when the argument is NULL, then we only need to special case the clocks
stuff when the argument is NULL.

> And I guess the same could be done for regulators (and even phys). The
> Tegra driver for instance could then do this:

Ack.

> static const char *const tegra_ahci_clocks[] = {
> "sata-oob", "cml1", pll_e",
> };

Note you could also put the "sata" clock here, since then you will
know its index, and can use hpriv->clks to access it. Not putting
it here has the advantage of not doing the double enable / disable
(and the disadvantage of needing to the clk_get yourself).

> static const char *const tegra_ahci_resets[] = {
> "sata-oob", "sata-cold",
> };
>
> static const struct ahci_platform_resources tegra_ahci_resources = {
> .num_clocks = ARRAY_SIZE(tegra_ahci_clocks),
> .clocks = tegra_ahci_clocks,
> .num_resets = ARRAY_SIZE(tegra_ahci_resets),
> .resets = tegra_ahci_resets,
> };
>
> ...
>
> struct tegra_ahci {
> struct ahci_host_priv *host;
> struct reset_control *rst;
> struct clk *clk;
> ...
> };
>
> ...
>
> static int tegra_ahci_probe(struct platform_device *pdev)
> {
> struct tegra_ahci *ahci;
>
> ...
>
> ahci = devm_kzalloc(&pdev->dev, sizeof(*ahci), GFP_KERNEL);
> if (!ahci)
> return -ENOMEM;
>
> ahci->host = ahci_platform_get_resources(pdev, &tegra_ahci_resources);
> if (IS_ERR(ahci->host)) {
> ...
> }
>
> ...
>
> platform_set_drvdata(pdev, ahci);
> }
>
> Does that sound reasonable?

Yep, sounds good to me (with the caveat of the back-compat stuff).

Regards,

Hans

2014-07-17 11:00:39

On 18/07/14 13:28, Hans de Goede wrote:
> Hi,
>
> On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
>> So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
>>
>> I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?
>
> Yes that works for me v3 of all the patches with no newer version +
> v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
> Tejun can you pick these up for 3.17 please?

Note that the DTC step will fail if Thierry's xusb pinctrl series is not
applied before this one. (The DTS changes depend on a node called
'padctl' and the pinctrl-tegra-xusb header)

>
> And thanks for working on a name-based ahci_platform_get_resources function
> since what we've now in v5 is quite clean already I think it will be good
> to take our time to get this right, so postponing this to 3.18 is fine
> with me.

Yes.

>
> Regards,
>
> Hans
>

Thanks, Mikko

2014-07-18 15:06:13

On 07/18/2014 09:06 AM, Thierry Reding wrote:
> On Fri, Jul 18, 2014 at 12:28:54PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/18/2014 09:16 AM, Mikko Perttunen wrote:
>>> So here's v5: this time, as suggested, I handle the sata clock myself and let ahci_platform handle it too, leading it to be prepared+enabled twice. This works fine, and allows us to remove the DT ordering requirement.
>>>
>>> I also have in the works a patchset that adds the name-based ahci_platform_get_resources function, but that is not quite ready yet, even if it is quite far along. Also, I am going on vacation and returning on 28.7., so if this v5 is acceptable maybe it could be merged for 3.17 and I could work on the new get_resources scheme after I get back from vacation?
>>
>> Yes that works for me v3 of all the patches with no newer version +
>> v5 of patch 1 and 7 are pretty clean and can go into 3.17 from my pov,
>> Tejun can you pick these up for 3.17 please?
>
> Tejun,
>
> I think the following patches can go through your tree without causing
> breakage through unresolved dependencies:
...
> Stephen, the rest of the patches would probably best go through the
> Tegra tree so that we can handle the dependencies there. I've already
> sent out pull requests for 3.17 today, but maybe Arnd and Olof can be
> convinced to take one more.

At this point, I'm just going to wait for 3.18 for anything that didn't
make it into 3.17.

Attachments:

signature.asc (901.00 B)
OpenPGP digital signature