diff mbox

[v2,6/7] ata: Add support for the Tegra124 SATA controller

Message ID 1403101406-15439-7-git-send-email-mperttunen@nvidia.com
State Superseded, archived
Headers show

Commit Message

Mikko Perttunen June 18, 2014, 2:23 p.m. UTC
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v2:
- removed redundant of_match_device
- fixed fuse offsets
- added newlines to dev_err calls
- get 5v/12v supplies
- mask calibration fuse value
- removed sata io helpers
- changed defines to use BIT macro
- removed use of ahci_platform resource management
- changed powergate code to use sequence function

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

Comments

Stephen Warren June 24, 2014, 7:35 p.m. UTC | #1
On 06/18/2014 08:23 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.

At a quick glance, this looks fine to me now. I'll wait for an ack to
take this patch through the Tegra tree, for the reasons I mentioned in
response to v1.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen June 26, 2014, 11:18 a.m. UTC | #2
FWIW, from IRC, the series

 > Tested-by: Steev Klimaszewski <steev@gentoo.org>

On 24/06/14 22:35, Stephen Warren wrote:
> On 06/18/2014 08:23 AM, Mikko Perttunen wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.
>
> At a quick glance, this looks fine to me now. I'll wait for an ack to
> take this patch through the Tegra tree, for the reasons I mentioned in
> response to v1.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 8, 2014, 8:20 a.m. UTC | #3
Hey Tejun,

the prerequisites for this series have now been acked and it would be 
nice to have it for 3.17. Could you take a look at it?

Thanks,
Mikko

On 26/06/14 14:18, Mikko Perttunen wrote:
> FWIW, from IRC, the series
>
>   > Tested-by: Steev Klimaszewski <steev@gentoo.org>
>
> On 24/06/14 22:35, Stephen Warren wrote:
>> On 06/18/2014 08:23 AM, Mikko Perttunen wrote:
>>> This adds support for the integrated AHCI-compliant Serial ATA
>>> controller present on the NVIDIA Tegra124 system-on-chip.
>>
>> At a quick glance, this looks fine to me now. I'll wait for an ack to
>> take this patch through the Tegra tree, for the reasons I mentioned in
>> response to v1.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo July 8, 2014, 1:22 p.m. UTC | #4
(cc'ing Hans)

Hans, can you please review this patch?

On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
> +#define SATA_CONFIGURATION_0				0x180
> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)

Let's just indent uniformly.  The new line should give enough visual
hint on grouping.

> +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 clk *sata_clk;
> +	struct clk *sata_oob_clk;
> +	struct clk *cml1_clk;
> +	struct clk *plle_clk;
> +	struct regulator_bulk_data supplies[5];
> +	struct phy *padctl_phy;
> +};

And please indent the declared fields uniformly too.

Except for the above nitpicks, generally looks good to me but let's
wait for Hans' review.

Thanks.
Mikko Perttunen July 8, 2014, 1:38 p.m. UTC | #5
Thanks, I'll fix these.

On 08/07/14 16:22, Tejun Heo wrote:
> (cc'ing Hans)
>
> Hans, can you please review this patch?
>
> On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
>> +#define SATA_CONFIGURATION_0				0x180
>> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)
>
> Let's just indent uniformly.  The new line should give enough visual
> hint on grouping.
>
>> +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 clk *sata_clk;
>> +	struct clk *sata_oob_clk;
>> +	struct clk *cml1_clk;
>> +	struct clk *plle_clk;
>> +	struct regulator_bulk_data supplies[5];
>> +	struct phy *padctl_phy;
>> +};
>
> And please indent the declared fields uniformly too.
>
> Except for the above nitpicks, generally looks good to me but let's
> wait for Hans' review.
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding July 9, 2014, 6:49 a.m. UTC | #6
On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
[...]
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
[...]
> +#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 <linux/tegra-soc.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0				0x180
> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)

This, and the register/field definitions below look weirdly indented,
but I guess that's mostly a matter of taste.

> +#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)

Perhaps single spaces around "<<"? The same for other occurrences below.

> +struct sata_pad_calibration {
> +	u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, gen2_tx_peak;
> +};

I think a more idiomatic way would be to put the fields in a structure
declaration on separate lines.

> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> +	{0x18, 0x04, 0x18, 0x0a},

Maybe spaces after '{' and before '}'?

> +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);

These could be aligned with TEGRA_POWERGATE_SATA.

> +	ret = clk_prepare_enable(tegra->cml1_clk);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}
> +
> +	ret = clk_prepare_enable(tegra->plle_clk);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->cml1_clk);
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +
> +	ret = phy_power_on(tegra->padctl_phy);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->plle_clk);
> +		clk_disable_unprepare(tegra->cml1_clk);
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}

These error paths might be more readable if you did unwind them similar
to how you did with sata_clk below.

> +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);

s/ahci/AHCI/

> +		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 */
> +
> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to read sata calibration fuse: %d\n", ret);

s/sata/SATA/

> +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,
> +};

I prefer these to not be arbitrarily indented, but Tejun seemed to
indicate he did, so it's ultimately his call.

> +static const struct of_device_id tegra_ahci_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-ahci" },
> +	{},

Technically there's no need for the comma at the end here. Usually you'd
put one here so that if somebody ever were to add a new entry after this
the diff wouldn't need to include the line that you only add a comma to.
But in this particular case the last entry is used as a sentinel, so
leaving away the comma is actually useful as a hint that entries should
be added in front rather than at the end.

> +	hpriv->mmio = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 0));

Perhaps a temporary variable for the result of platform_get_resource()
would help make this look less cluttered.

> +	if (IS_ERR(hpriv->mmio)) {
> +		dev_err(&pdev->dev, "Failed to get AHCI IO memory\n");

This isn't necessary. devm_ioremap_resource() will already output an
error message.

> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +	if (IS_ERR(tegra->sata_regs)) {
> +		dev_err(&pdev->dev, "Failed to get SATA IO memory\n");

Same here.

> +	tegra->padctl_phy = devm_phy_get(&pdev->dev, "sata-phy");
> +	if (IS_ERR(tegra->padctl_phy)) {
> +		dev_err(&pdev->dev, "Failed to get phy\n");

s/phy/PHY/

> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +					tegra->supplies);

The trailing argument here isn't aligned as in other places.

> +static struct platform_driver tegra_ahci_driver = {
> +	.probe = tegra_ahci_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = "tegra-ahci",
> +		.owner = THIS_MODULE,

This isn't really needed anymore, module_platform_driver will end up
setting this for you anyway.

Thierry
Mikko Perttunen July 9, 2014, 1:45 p.m. UTC | #7
Thanks, will fix these. Looks like I will have to change the fuse code 
to the old style as well.

On 09/07/14 09:49, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
> [...]
>> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> [...]
>> +#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 <linux/tegra-soc.h>
>> +#include "ahci.h"
>> +
>> +#define SATA_CONFIGURATION_0				0x180
>> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)
>
> This, and the register/field definitions below look weirdly indented,
> but I guess that's mostly a matter of taste.
>
>> +#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)
>
> Perhaps single spaces around "<<"? The same for other occurrences below.
>
>> +struct sata_pad_calibration {
>> +	u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, gen2_tx_peak;
>> +};
>
> I think a more idiomatic way would be to put the fields in a structure
> declaration on separate lines.
>
>> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
>> +	{0x18, 0x04, 0x18, 0x0a},
>
> Maybe spaces after '{' and before '}'?
>
>> +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);
>
> These could be aligned with TEGRA_POWERGATE_SATA.
>
>> +	ret = clk_prepare_enable(tegra->cml1_clk);
>> +	if (ret) {
>> +		clk_disable_unprepare(tegra->sata_oob_clk);
>> +		goto disable_power;
>> +	}
>> +
>> +	ret = clk_prepare_enable(tegra->plle_clk);
>> +	if (ret) {
>> +		clk_disable_unprepare(tegra->cml1_clk);
>> +		clk_disable_unprepare(tegra->sata_oob_clk);
>> +		goto disable_power;
>> +	}
>> +
>> +	reset_control_deassert(tegra->sata_cold_rst);
>> +	reset_control_deassert(tegra->sata_oob_rst);
>> +
>> +	ret = phy_power_on(tegra->padctl_phy);
>> +	if (ret) {
>> +		clk_disable_unprepare(tegra->plle_clk);
>> +		clk_disable_unprepare(tegra->cml1_clk);
>> +		clk_disable_unprepare(tegra->sata_oob_clk);
>> +		goto disable_power;
>> +	}
>
> These error paths might be more readable if you did unwind them similar
> to how you did with sata_clk below.
>
>> +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);
>
> s/ahci/AHCI/
>
>> +		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 */
>> +
>> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
>> +	if (ret) {
>> +		dev_err(&tegra->pdev->dev,
>> +			"failed to read sata calibration fuse: %d\n", ret);
>
> s/sata/SATA/
>
>> +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,
>> +};
>
> I prefer these to not be arbitrarily indented, but Tejun seemed to
> indicate he did, so it's ultimately his call.
>
>> +static const struct of_device_id tegra_ahci_of_match[] = {
>> +	{ .compatible = "nvidia,tegra124-ahci" },
>> +	{},
>
> Technically there's no need for the comma at the end here. Usually you'd
> put one here so that if somebody ever were to add a new entry after this
> the diff wouldn't need to include the line that you only add a comma to.
> But in this particular case the last entry is used as a sentinel, so
> leaving away the comma is actually useful as a hint that entries should
> be added in front rather than at the end.
>
>> +	hpriv->mmio = devm_ioremap_resource(&pdev->dev,
>> +		platform_get_resource(pdev, IORESOURCE_MEM, 0));
>
> Perhaps a temporary variable for the result of platform_get_resource()
> would help make this look less cluttered.
>
>> +	if (IS_ERR(hpriv->mmio)) {
>> +		dev_err(&pdev->dev, "Failed to get AHCI IO memory\n");
>
> This isn't necessary. devm_ioremap_resource() will already output an
> error message.
>
>> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
>> +		platform_get_resource(pdev, IORESOURCE_MEM, 1));
>> +	if (IS_ERR(tegra->sata_regs)) {
>> +		dev_err(&pdev->dev, "Failed to get SATA IO memory\n");
>
> Same here.
>
>> +	tegra->padctl_phy = devm_phy_get(&pdev->dev, "sata-phy");
>> +	if (IS_ERR(tegra->padctl_phy)) {
>> +		dev_err(&pdev->dev, "Failed to get phy\n");
>
> s/phy/PHY/
>
>> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
>> +					tegra->supplies);
>
> The trailing argument here isn't aligned as in other places.
>
>> +static struct platform_driver tegra_ahci_driver = {
>> +	.probe = tegra_ahci_probe,
>> +	.remove = ata_platform_remove_one,
>> +	.driver = {
>> +		.name = "tegra-ahci",
>> +		.owner = THIS_MODULE,
>
> This isn't really needed anymore, module_platform_driver will end up
> setting this for you anyway.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 14, 2014, 6:21 a.m. UTC | #8
Hi Hans, have you been able to take a look at this?

Thanks,
Mikko

On 08/07/14 16:22, Tejun Heo wrote:
> (cc'ing Hans)
>
> Hans, can you please review this patch?
>
> On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
>> +#define SATA_CONFIGURATION_0				0x180
>> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)
>
> Let's just indent uniformly.  The new line should give enough visual
> hint on grouping.
>
>> +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 clk *sata_clk;
>> +	struct clk *sata_oob_clk;
>> +	struct clk *cml1_clk;
>> +	struct clk *plle_clk;
>> +	struct regulator_bulk_data supplies[5];
>> +	struct phy *padctl_phy;
>> +};
>
> And please indent the declared fields uniformly too.
>
> Except for the above nitpicks, generally looks good to me but let's
> wait for Hans' review.
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede July 14, 2014, 6:25 a.m. UTC | #9
Hi Miko,

On 07/14/2014 08:21 AM, Mikko Perttunen wrote:
> Hi Hans, have you been able to take a look at this?

Not yet, it is on my todo list I hope to get around to
it today or tomorrow.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 14, 2014, 6:28 a.m. UTC | #10
Ok, that's good, thanks.

Mikko

On 14/07/14 09:25, Hans de Goede wrote:
> Hi Miko,
>
> On 07/14/2014 08:21 AM, Mikko Perttunen wrote:
>> Hi Hans, have you been able to take a look at this?
>
> Not yet, it is on my todo list I hope to get around to
> it today or tomorrow.
>
> Regards,
>
> Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede July 14, 2014, 1:36 p.m. UTC | #11
Hi,

On 07/08/2014 03:22 PM, Tejun Heo wrote:
> (cc'ing Hans)
> 
> Hans, can you please review this patch?

Done.

Mikko, it looks like you are doing a lot of stuff
the DIY way. I can see there are good reasons for
that though.

Still it would be nice if you could use a little bit
more of the helper functions provided by libahci_platform.c

Specifically I think it would be better if you used
ahci_platform_get_resources, that would remove a lot of
duplicate code from the new driver.

To be specific I would like to suggest that you
raise AHCI_MAX_CLKS to 4, and then specify an order
in which the clks must be listed in the devicetree
binding. Then you can put that order in an enum
and use hpriv->clks[CLK_FOO] in your driver, where
CLK_FOO comes from the enum.

This way you should be able to use ahci_platform_get_resources
and drop doing the iomap of the base registers, all the
clk_gets and the phy_get yourself.

You could then also use ahci_platform_disable_clks() in
tegra_ahci_power_off

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 15, 2014, 7:12 a.m. UTC | #12
Heh, what you are describing is the v1 of this series :)

However,

17/06/14 Thierry Reding wrote:
 > I would've preferred tegra_powergate_sequence_power_up() to be used
 > consistently in all drivers. I'm still not convinced that using the
 > platform AHCI driver this way is really the best option, since we're
 > bending over backwards to fit into what this driver dictates. There
 > shouldn't be a need for that.

As you probably noticed, the issue is that on Tegra we want to use 
tegra_powergate_sequence_power_up to enable the SATA power rail. That 
function assumes that it gets a disabled clock and returns with the 
clock enabled. If we use ahci_platform's resource management functions,
this is not doable, since it wants to handle clocks by itself.

To be honest, I too would prefer handling the resources in the driver. 
The driver needs other resources than what ahci_platform currently 
manages (at least reset_controls and multiple regulators, later we might 
get more) and managing them in the driver allows the driver to do proper 
error handling and manage all resources consistently and in one place. 
Also, I don't think it is sensible to add support for loading every 
possible kind of DT resource to ahci_platform, since most drivers won't 
need them. Other subsystems I've been in don't have this kind of helper 
library, so diverging here seems weird.

That said, if you feel strongly about this, I can do what you described.

Thanks,
Mikko

On 14/07/14 16:36, Hans de Goede wrote:
> Hi,
>
> On 07/08/2014 03:22 PM, Tejun Heo wrote:
>> (cc'ing Hans)
>>
>> Hans, can you please review this patch?
>
> Done.
>
> Mikko, it looks like you are doing a lot of stuff
> the DIY way. I can see there are good reasons for
> that though.
>
> Still it would be nice if you could use a little bit
> more of the helper functions provided by libahci_platform.c
>
> Specifically I think it would be better if you used
> ahci_platform_get_resources, that would remove a lot of
> duplicate code from the new driver.
>
> To be specific I would like to suggest that you
> raise AHCI_MAX_CLKS to 4, and then specify an order
> in which the clks must be listed in the devicetree
> binding. Then you can put that order in an enum
> and use hpriv->clks[CLK_FOO] in your driver, where
> CLK_FOO comes from the enum.
>
> This way you should be able to use ahci_platform_get_resources
> and drop doing the iomap of the base registers, all the
> clk_gets and the phy_get yourself.
>
> You could then also use ahci_platform_disable_clks() in
> tegra_ahci_power_off
>
> Regards,
>
> Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede July 15, 2014, 8:55 a.m. UTC | #13
Hi,

On 07/15/2014 09:12 AM, Mikko Perttunen wrote:
> Heh, what you are describing is the v1 of this series :)
> 
> However,
> 
> 17/06/14 Thierry Reding wrote:
>> I would've preferred tegra_powergate_sequence_power_up() to be used
>> consistently in all drivers. I'm still not convinced that using the
>> platform AHCI driver this way is really the best option, since we're
>> bending over backwards to fit into what this driver dictates. There
>> shouldn't be a need for that.
> 
> As you probably noticed, the issue is that on Tegra we want to use tegra_powergate_sequence_power_up to enable the SATA power rail. That function assumes that it gets a disabled clock and returns with the clock enabled. If we use ahci_platform's resource management functions,
> this is not doable, since it wants to handle clocks by itself.

Right, note I was not suggesting that you use ahci_platform_enable_resources /
ahci_platform_disable_resources, that clearly won't work for your case.

As I said "I can see there are good reasons for that though."

> To be honest, I too would prefer handling the resources in the driver. The driver needs other resources than what ahci_platform currently manages (at least reset_controls and multiple regulators, later we might get more) and managing them in the driver allows the driver to do proper error handling and manage all resources consistently and in one place. Also, I don't think it is sensible to add support for loading every possible kind of DT resource to ahci_platform, since most drivers won't need them.

Actually I have considered asking you to add support for reset controllers
to libahci_platform too, since I see a pattern where more and more
SOCs are starting to use those. I've refrained from asking that for now,
but once a second ahci implementation needing those pops up we will likely
go that route. So you might as well do it now :)

> Other subsystems I've been in don't have this kind of helper library, so diverging here seems weird.

Well the usb uhci-platform and ehci-platform drivers are doing the same,
what we're trying to do here is avoid needless code duplication and
if possible (for ehci and uhci it currently is) make it so that adding
support for a new soc only requires adding dt stuff + a phy driver,
without touching the ?hci code at all. Again I can see that this
is not possible for the Tegra124.

> That said, if you feel strongly about this, I can do what you described.

I think you can safe a nice bit of code duplication (and if we ever find
we have bugs there bug duplication) by switching to
ahci_platform_get_resources, as described in my original mail.

I can understand if you don't want to use any of the other ahci_platform_*
functions, that makes total sense.

I would not say this is something you must do, but I have a strong
preference for you taking a shot at doing this. So can you please give
this a try, and if it turns out to become ugly, just say so (with
some explanation), and then you can keep things as is.
Does that work for you?

Regards,

Hans






> 
> Thanks,
> Mikko
> 
> On 14/07/14 16:36, Hans de Goede wrote:
>> Hi,
>>
>> On 07/08/2014 03:22 PM, Tejun Heo wrote:
>>> (cc'ing Hans)
>>>
>>> Hans, can you please review this patch?
>>
>> Done.
>>
>> Mikko, it looks like you are doing a lot of stuff
>> the DIY way. I can see there are good reasons for
>> that though.
>>
>> Still it would be nice if you could use a little bit
>> more of the helper functions provided by libahci_platform.c
>>
>> Specifically I think it would be better if you used
>> ahci_platform_get_resources, that would remove a lot of
>> duplicate code from the new driver.
>>
>> To be specific I would like to suggest that you
>> raise AHCI_MAX_CLKS to 4, and then specify an order
>> in which the clks must be listed in the devicetree
>> binding. Then you can put that order in an enum
>> and use hpriv->clks[CLK_FOO] in your driver, where
>> CLK_FOO comes from the enum.
>>
>> This way you should be able to use ahci_platform_get_resources
>> and drop doing the iomap of the base registers, all the
>> clk_gets and the phy_get yourself.
>>
>> You could then also use ahci_platform_disable_clks() in
>> tegra_ahci_power_off
>>
>> Regards,
>>
>> Hans
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikko Perttunen July 15, 2014, 9:03 a.m. UTC | #14
On 15/07/14 11:55, Hans de Goede wrote:
> Hi,
>
> On 07/15/2014 09:12 AM, Mikko Perttunen wrote:
>> Heh, what you are describing is the v1 of this series :)
>>
>> However,
>>
>> 17/06/14 Thierry Reding wrote:
>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>> consistently in all drivers. I'm still not convinced that using the
>>> platform AHCI driver this way is really the best option, since we're
>>> bending over backwards to fit into what this driver dictates. There
>>> shouldn't be a need for that.
>>
>> As you probably noticed, the issue is that on Tegra we want to use tegra_powergate_sequence_power_up to enable the SATA power rail. That function assumes that it gets a disabled clock and returns with the clock enabled. If we use ahci_platform's resource management functions,
>> this is not doable, since it wants to handle clocks by itself.
>
> Right, note I was not suggesting that you use ahci_platform_enable_resources /
> ahci_platform_disable_resources, that clearly won't work for your case.
>
> As I said "I can see there are good reasons for that though."
>
>> To be honest, I too would prefer handling the resources in the driver. The driver needs other resources than what ahci_platform currently manages (at least reset_controls and multiple regulators, later we might get more) and managing them in the driver allows the driver to do proper error handling and manage all resources consistently and in one place. Also, I don't think it is sensible to add support for loading every possible kind of DT resource to ahci_platform, since most drivers won't need them.
>
> Actually I have considered asking you to add support for reset controllers
> to libahci_platform too, since I see a pattern where more and more
> SOCs are starting to use those. I've refrained from asking that for now,
> but once a second ahci implementation needing those pops up we will likely
> go that route. So you might as well do it now :)
>
>> Other subsystems I've been in don't have this kind of helper library, so diverging here seems weird.
>
> Well the usb uhci-platform and ehci-platform drivers are doing the same,
> what we're trying to do here is avoid needless code duplication and
> if possible (for ehci and uhci it currently is) make it so that adding
> support for a new soc only requires adding dt stuff + a phy driver,
> without touching the ?hci code at all. Again I can see that this
> is not possible for the Tegra124.
>
>> That said, if you feel strongly about this, I can do what you described.
>
> I think you can safe a nice bit of code duplication (and if we ever find
> we have bugs there bug duplication) by switching to
> ahci_platform_get_resources, as described in my original mail.
>
> I can understand if you don't want to use any of the other ahci_platform_*
> functions, that makes total sense.
>
> I would not say this is something you must do, but I have a strong
> preference for you taking a shot at doing this. So can you please give
> this a try, and if it turns out to become ugly, just say so (with
> some explanation), and then you can keep things as is.
> Does that work for you?
>
> Regards,
>
> Hans
>

Sure, I'll have a go at it. I already sent one bugfix for 
libahci_platform anyway :)

Mikko
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding July 15, 2014, 9:31 a.m. UTC | #15
On Tue, Jul 15, 2014 at 10:55:29AM +0200, Hans de Goede wrote:
> On 07/15/2014 09:12 AM, Mikko Perttunen wrote:
[...]
> > Other subsystems I've been in don't have this kind of helper
> > library, so diverging here seems weird.
> 
> Well the usb uhci-platform and ehci-platform drivers are doing the same,
> what we're trying to do here is avoid needless code duplication and
> if possible (for ehci and uhci it currently is) make it so that adding
> support for a new soc only requires adding dt stuff + a phy driver,
> without touching the ?hci code at all. Again I can see that this
> is not possible for the Tegra124.

I don't think helper libraries are good at managing resources. They are
very good for extracting common /functionality/ so that it doesn't have
to be duplicated. That's where the commonalities really are. Resource
allocation and setup are usually where the differences are. Trying to
extract that into common helpers often results in clumsy code.

Also, since resources and setup sequences are often very different, the
chances are that adding support for a new SoC will always require
extending the helpers to cope with that new type of resource that the
new SoC now requires, or bumping the maximum number of resources of an
existing type.

Thierry
diff mbox

Patch

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..f4f8f33
--- /dev/null
+++ b/drivers/ata/ahci_tegra.c
@@ -0,0 +1,448 @@ 
+/*
+ * drivers/ata/ahci_tegra.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * 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 <linux/tegra-soc.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
+
+struct sata_pad_calibration {
+	u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, 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 clk *sata_clk;
+	struct clk *sata_oob_clk;
+	struct clk *cml1_clk;
+	struct clk *plle_clk;
+	struct regulator_bulk_data supplies[5];
+	struct phy *padctl_phy;
+};
+
+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_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	ret = clk_prepare_enable(tegra->sata_oob_clk);
+	if (ret)
+		goto disable_power;
+
+	ret = clk_prepare_enable(tegra->cml1_clk);
+	if (ret) {
+		clk_disable_unprepare(tegra->sata_oob_clk);
+		goto disable_power;
+	}
+
+	ret = clk_prepare_enable(tegra->plle_clk);
+	if (ret) {
+		clk_disable_unprepare(tegra->cml1_clk);
+		clk_disable_unprepare(tegra->sata_oob_clk);
+		goto disable_power;
+	}
+
+	reset_control_deassert(tegra->sata_cold_rst);
+	reset_control_deassert(tegra->sata_oob_rst);
+
+	ret = phy_power_on(tegra->padctl_phy);
+	if (ret) {
+		clk_disable_unprepare(tegra->plle_clk);
+		clk_disable_unprepare(tegra->cml1_clk);
+		clk_disable_unprepare(tegra->sata_oob_clk);
+		goto disable_power;
+	}
+
+	return 0;
+
+disable_power:
+	clk_disable_unprepare(tegra->sata_clk);
+
+	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;
+
+	phy_power_off(tegra->padctl_phy);
+
+	reset_control_assert(tegra->sata_rst);
+	reset_control_assert(tegra->sata_oob_rst);
+	reset_control_assert(tegra->sata_cold_rst);
+
+	clk_disable_unprepare(tegra->plle_clk);
+	clk_disable_unprepare(tegra->cml1_clk);
+	clk_disable_unprepare(tegra->sata_oob_clk);
+	clk_disable_unprepare(tegra->sata_clk);
+
+	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 */
+
+	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
+	if (ret) {
+		dev_err(&tegra->pdev->dev,
+			"failed to read sata calibration fuse: %d\n", ret);
+		return ret;
+	}
+
+	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;
+	struct tegra_ahci_priv *tegra = hpriv->plat_data;
+
+	tegra_ahci_controller_deinit(hpriv);
+
+	phy_exit(tegra->padctl_phy);
+}
+
+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;
+	int ret;
+
+	hpriv = devm_kzalloc(&pdev->dev, sizeof(*hpriv), GFP_KERNEL);
+	if (!hpriv)
+		return -ENOMEM;
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	hpriv->plat_data = tegra;
+
+	tegra->pdev = pdev;
+
+	hpriv->mmio = devm_ioremap_resource(&pdev->dev,
+		platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(hpriv->mmio)) {
+		dev_err(&pdev->dev, "Failed to get AHCI IO memory\n");
+		return PTR_ERR(hpriv->mmio);
+	}
+
+	tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
+		platform_get_resource(pdev, IORESOURCE_MEM, 1));
+	if (IS_ERR(tegra->sata_regs)) {
+		dev_err(&pdev->dev, "Failed to get SATA IO memory\n");
+		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);
+	}
+
+	tegra->sata_clk = devm_clk_get(&pdev->dev, "sata");
+	if (IS_ERR(tegra->sata_clk)) {
+		dev_err(&pdev->dev, "Failed to get sata clock\n");
+		return PTR_ERR(tegra->sata_clk);
+	}
+
+	tegra->sata_oob_clk = devm_clk_get(&pdev->dev, "sata-oob");
+	if (IS_ERR(tegra->sata_oob_clk)) {
+		dev_err(&pdev->dev, "Failed to get sata-oob clock\n");
+		return PTR_ERR(tegra->sata_oob_clk);
+	}
+
+	tegra->cml1_clk = devm_clk_get(&pdev->dev, "cml1");
+	if (IS_ERR(tegra->cml1_clk)) {
+		dev_err(&pdev->dev, "Failed to get cml1 clock\n");
+		return PTR_ERR(tegra->cml1_clk);
+	}
+
+	tegra->plle_clk = devm_clk_get(&pdev->dev, "pll_e");
+	if (IS_ERR(tegra->plle_clk)) {
+		dev_err(&pdev->dev, "Failed to get pll_e clock\n");
+		return PTR_ERR(tegra->plle_clk);
+	}
+
+	tegra->padctl_phy = devm_phy_get(&pdev->dev, "sata-phy");
+	if (IS_ERR(tegra->padctl_phy)) {
+		dev_err(&pdev->dev, "Failed to get phy\n");
+		return PTR_ERR(tegra->padctl_phy);
+	}
+
+	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 = phy_init(tegra->padctl_phy);
+	if (ret)
+		return ret;
+
+	ret = tegra_ahci_controller_init(hpriv);
+	if (ret)
+		goto exit_phy;
+
+	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);
+
+exit_phy:
+	phy_exit(tegra->padctl_phy);
+
+	return ret;
+};
+
+static struct platform_driver tegra_ahci_driver = {
+	.probe = tegra_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "tegra-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = tegra_ahci_of_match,
+	},
+	/* LP0 suspend support not implemented */
+};
+module_platform_driver(tegra_ahci_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
+MODULE_LICENSE("GPL v2");