diff mbox

[3/3] ahci: st: Add support for ST's SATA IP

Message ID 1392641818-23419-3-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones Feb. 17, 2014, 12:56 p.m. UTC
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-ide@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/ata/Kconfig   |   9 ++
 drivers/ata/Makefile  |   1 +
 drivers/ata/ahci_st.c | 275 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/ata/ahci_st.c

Comments

Lee Jones Feb. 19, 2014, 8:30 a.m. UTC | #1
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: linux-ide@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Please consider basing it on top of Hans' refactored ahci_platform.
> 
>  http://www.spinics.net/lists/linux-ide/msg47628.html

Okay, looks reasonable. I'm happy to do the extra work.

Can I do this as a subsequent patch though? The current patch fits
neatly onto our internal stable kernel (currently v3.10). The added
work will require back-porting Hans' patch-set, which has not yet
reached Mainline.
Lee Jones Feb. 19, 2014, 12:14 p.m. UTC | #2
> >> Cc: Tejun Heo <tj@kernel.org>
> >> Cc: linux-ide@vger.kernel.org
> >> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Please consider basing it on top of Hans' refactored ahci_platform.
> > 
> >  http://www.spinics.net/lists/linux-ide/msg47628.html
> 
> Which reminds I must test the version I've rebased on top of 3.14-rc1
> and then send a v6 so that this can get merged.
> 
> Lee, I'll add you to the CC of the v6 I'm about to send.

Thanks Hans.

Once Tejun has accepted this driver I will go ahead and rebase on top
of your work and convert to the new API.
Lee Jones Feb. 19, 2014, 3:01 p.m. UTC | #3
> > Can I do this as a subsequent patch though? The current patch fits
> > neatly onto our internal stable kernel (currently v3.10). The added
> > work will require back-porting Hans' patch-set, which has not yet
> > reached Mainline.
> 
> Hmm.... I'll apply Hans' patchset pretty soon and we still have some
> time in this devel cycle so I'd much prefer applying the final patch.

I think that's a pretty unfair ask. I could understand if the
patch(es) didn't apply, or were hindered by the new API change, but I
do not believe that is the case?

FWIW, I have tried to apply the patches Hans send me, but they don't
seem to apply cleanly.

Base is v3.14-rc1:

Applying: ahci-platform: Add support for devices with more then 1 clock
fatal: sha1 information is lacking or useless (drivers/ata/ahci_platform.c).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 ahci-platform: Add support for devices with more then 1 clock
The copy of the patch that failed is found in:
   .git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Any ideas?
Lee Jones Feb. 19, 2014, 3:23 p.m. UTC | #4
> > I think that's a pretty unfair ask. I could understand if the
> > patch(es) didn't apply, or were hindered by the new API change, but I
> > do not believe that is the case?
> 
> Let me make it very clear to you.  If you want your code in, you help
> maintaining the larger code base.  I'm already extremely annoyed about
> the general behavior of embedded developers where the general attitude
> seems to be doing whatever to get *my* driver in the tree.  So, fuck
> you a little bit and if you want to make it happen faster, help Hans'
> patchset mature.  Unfair my ass.

Since when has maintaining core code been the responsibility of the
leaf driver developers? If you're aware that the core code is
sub-standard then it's you who should be fixing it.

I think it's great that forward thinking developers like Hans take on
challenges to improve subsystems which are lacking in one way or
another, but holding back other development while this process is
ongoing is fundamentally wrong. Especially in this case where you're
still actively reviewing/NACKing the core changes.

Changes to APIs should either support backward-capability or change all
effected drivers. I haven't been able to apply the patches yet, so I
can't tell which one of these holds true, but I believe it's the
former. In which case any new driver using the _current_ (not old)
API should fold neatly in. I've even offered to convert to the new API
once it's Mainlined. How can I say fairer than that?

Finally, do try and stay at least a little bit professional on the
MLs. That sort of disrespectful, rude behaviour may be how you guys do
it at Redhat, but most will think it's nothing more than childish.
Lee Jones Feb. 19, 2014, 4:39 p.m. UTC | #5
> > Since when has maintaining core code been the responsibility of the
> > leaf driver developers? If you're aware that the core code is
> > sub-standard then it's you who should be fixing it.
> 
> No, there isn't this clear divide between core and the leaf
> developers.  People working on the "core" don't just grow off the
> ground.  You can't just go around piling mess around claiming that
> it's not your responsbility and then expect someone else to clean up
> after you.  How can that be *possibly* sustainable?

That's not what I said was it? You're misinterpreting me.

I'm more than happy to work on core code and have done in the past.
However, if every subsystem maintainer I submit patches to forced me
to undertake some API clean-ups/re-write or convert to another
future/development API prior to acceptance I wouldn't be able to get
any of my own work done.

> > I think it's great that forward thinking developers like Hans take on
> > challenges to improve subsystems which are lacking in one way or
> 
> Yeah, which happened only because I pushed back and Hans isn't even
> paid to do it.  Doesn't that say something?

It tells me that Hans has more spare time than I do.

This work would even be something I'd be interested in helping out
with - even in my own time, but the way you speak to people doesn't
exactly inspire them to go out of my way to work with you does it?

> Nobody is actually helping Hans' work.  Not at all, nada, zilch.
> Just complain when directed towards it.

Again, that's not what I said. It's great that your subsystem is being
improved, but insisting that anyone who submits new code to rebase
on top of some development patches which only exist in mail form, and
refusing to take patches until they do so doesn't seem right to me.

> > another, but holding back other development while this process is
> > ongoing is fundamentally wrong. Especially in this case where you're
> > still actively reviewing/NACKing the core changes.
> > 
> > Changes to APIs should either support backward-capability or change all
> > effected drivers. I haven't been able to apply the patches yet, so I
> > can't tell which one of these holds true, but I believe it's the
> > former. In which case any new driver using the _current_ (not old)
> > API should fold neatly in. I've even offered to convert to the new API
> > once it's Mainlined. How can I say fairer than that?
> > 
> > Finally, do try and stay at least a little bit professional on the
> > MLs. That sort of disrespectful, rude behaviour may be how you guys do
> > it at Redhat, but most will think it's nothing more than childish.
> 
> If you want me to be respectful towards you, don't be a crybaby
> screaming "it's not fair".  It apparently isn't clear to you that you
> have to chip in for the whole thing to be maintainable in the long
> term.  Unfair... lol, that was a good one, really.

It appears you've just regressed into childhood again. Let's continue
this when you are able to have an adult conversation which other
grown-ups.
Lee Jones Feb. 19, 2014, 5:24 p.m. UTC | #6
> > Again, that's not what I said. It's great that your subsystem is being
> > improved, but insisting that anyone who submits new code to rebase
> > on top of some development patches which only exist in mail form, and
> > refusing to take patches until they do so doesn't seem right to me.
> 
> No policy is perfect and nothing can be decided solely on single
> policy.  There of course are trade-offs to make depending on the
> specific circumstances.  The problem, here, is that what has been
> going on is skewed towards one extreme and has potential to develop
> into a fairly large mess if left uncorrected.
> 
> The message I've been sending out has been pretty clear.  There are
> multiple people duplicating about the same thing in their drivers.
> Fortunately, Hans' refactoring is pretty close to completion and
> should help simplifying most of them.  I'm not even asking you to do
> the bulk of work.  Just take a look at it and help / push if you can.
> It may be unfortunate that the circumstances haven't been completely
> aligned for your convenience but that's what needs to be done to keep
> things sustainable.

I understand this. Thanks for taking the time to explain properly.

FWIW, I have now managed to rebase the driver on top of Hans' work and
I am now in the process of converting it to the new way of working.

> This is a collaborative work and what I asked you isn't some
> insurmountable amount of extra work.  It's just beyond me that your
> response is "it's not fair".  No wonder the whole thing has been
> drifting towards mess.  That's not how this works.  Judging from your
> linaro address, I assume you have been involved with some upstream
> work, how can this possibly be your response?  Such attitude is
> actively harmful and has no place in upstream development.
>
> Again, of course, there can be trade-offs.  We sometimes do need to
> take termporal hits in maintainability for faster hardware enablement
> or whatnot; however, we can't do that without trust that the people
> dumping stuff which needs later cleanups would actually help.
> Unfortnately, I have close to zero trust given the recent developments
> and your "it's not fair, that's not my responsibility" attitude
> clearly confirms the conclusion.
> 
> So, please take long look at how you perceive upstream development.
> It's a collaborative process.  Other people don't owe you by default.

Please refrain from adding quotation marks around things I didn't
actually say. I didn't say that this whole process was unfair. I was
pertaining to the fact that requesting that a driver is converted to a
non-existing API was wrong. As it currently stands the driver uses the
correct one. I also said that I'd happily convert it over when the
clean-ups are actually applied.
Lee Jones Feb. 19, 2014, 5:40 p.m. UTC | #7
> A few more things just in case.
> 
> On Wed, Feb 19, 2014 at 04:39:37PM +0000, Lee Jones wrote:
> > It tells me that Hans has more spare time than I do.
> 
> This is the crux of the problem, isn't it?  The party who is creating
> load should also partake in and invest resource into making the
> infrastructure for it.  What I can't understand is how one can claim
> "unfairness" at having to contribute to such effort when that is
> clearly the party which is the primary beneficiary of the added load.
> If you have *any* mature sense of fairness, not this childish "it's
> not going my way", the irony should be clear to you.
> 
> > This work would even be something I'd be interested in helping out
> > with - even in my own time, but the way you speak to people doesn't
> > exactly inspire them to go out of my way to work with you does it?
> 
> Given the circumstances, I don't think depending on good wills of the
> involved parties is a viable strategy and wanted to make it clear that
> the responsibility of chipping in for long term maintainability is on
> everyone who wants to make use of the code base.  This is beyond good
> will.  It's the fundamental sharing of responsibility for
> sustainability.  I'd love to have good will but I can't build that on
> top of a notion as rotten as "it's not fair, it's not my
> responsibility".
> 
> > Again, that's not what I said. It's great that your subsystem is being
> > improved, but insisting that anyone who submits new code to rebase
> > on top of some development patches which only exist in mail form, and
> > refusing to take patches until they do so doesn't seem right to me.
> 
> If I apply your patch now, Hans has one more driver to worry about in
> doing the work that he himself isn't directly benefiting from but
> everybody needs.  In what world is that fair?
> 
> So, sorry about going f bomb on you, but you shouldn't be thinking
> what you're thinking.  There's some serious misguidance going on
> there.

Have you listened to a word I've said? Or have you managed to get your
self all tangled up in what you _assumed_ was meant by the 3 words I
mentioned about fairness at the very start of this conversation? Me
thinks the latter might be true.

Just because I don't contribute to your subsystem, it doesn't mean I
don't contribute elsewhere.

Here... let me help you down from that big horse! ;)
diff mbox

Patch

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..a0798f3 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -97,6 +97,15 @@  config SATA_AHCI_PLATFORM
 
 	  If unsure, say N.
 
+config SATA_AHCI_ST
+	tristate "ST SATA support"
+	depends on SATA_AHCI_PLATFORM
+	select GENERIC_PHY
+	help
+	  This option enables support for ST SATA controller.
+
+	  If unsure, say N.
+
 config AHCI_IMX
 	tristate "Freescale i.MX AHCI SATA support"
 	depends on SATA_AHCI_PLATFORM && MFD_SYSCON
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 46518c6..3379a7d 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -5,6 +5,7 @@  obj-$(CONFIG_ATA)		+= libata.o
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
 obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o
+obj-$(CONFIG_SATA_AHCI_ST)      += ahci_st.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
new file mode 100644
index 0000000..23342bf
--- /dev/null
+++ b/drivers/ata/ahci_st.c
@@ -0,0 +1,275 @@ 
+/*
+ * Copyright (C) 2012 STMicroelectronics Limited
+ *
+ * Authors: Francesco Virlinzi <francesco.virlinzi@st.com>
+ *	    Alexandre Torgue <alexandre.torgue@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/export.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/ahci_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/reset.h>
+#include <linux/io.h>
+#include <linux/dma-mapping.h>
+
+#define AHCI_OOBR		0xbc
+#define AHCI_OOBR_WE		BIT(31)
+#define AHCI_OOBR_CWMIN_SHIFT	24
+#define AHCI_OOBR_CWMAX_SHIFT	16
+#define AHCI_OOBR_CIMIN_SHIFT	8
+#define AHCI_OOBR_CIMAX_SHIFT	0
+
+struct ahci_st_drv_data {
+	struct platform_device *ahci;
+	struct phy *phy;
+	struct reset_control *pwr;
+	struct reset_control *sw_rst;
+	struct reset_control *pwr_rst;
+	void __iomem *mmio;
+	struct clk *clk;
+};
+
+static void ahci_st_configure_oob(void __iomem *mmio)
+{
+	unsigned long old_val, new_val;
+
+	new_val = (0x02 << AHCI_OOBR_CWMIN_SHIFT) |
+		  (0x04 << AHCI_OOBR_CWMAX_SHIFT) |
+		  (0x08 << AHCI_OOBR_CIMIN_SHIFT) |
+		  (0x0C << AHCI_OOBR_CIMAX_SHIFT);
+
+	old_val = readl(mmio + AHCI_OOBR);
+	writel(old_val | AHCI_OOBR_WE, mmio + AHCI_OOBR);
+	writel(new_val | AHCI_OOBR_WE, mmio + AHCI_OOBR);
+	writel(new_val, mmio + AHCI_OOBR);
+}
+
+static int ahci_st_probe_resets(struct platform_device *pdev,
+				struct ahci_st_drv_data *ahci_dev)
+{
+	int err;
+
+	ahci_dev->pwr = devm_reset_control_get(&pdev->dev, "pwr-dwn");
+	if (IS_ERR(ahci_dev->pwr)) {
+		dev_info(&pdev->dev, "power reset control not defined\n");
+	} else {
+		err = reset_control_deassert(ahci_dev->pwr);
+		if (err) {
+			dev_err(&pdev->dev, "unable to bring out of pwrdwn\n");
+			return err;
+		}
+	}
+
+	ahci_dev->sw_rst = devm_reset_control_get(&pdev->dev, "sw-rst");
+	if (IS_ERR(ahci_dev->sw_rst)) {
+		dev_info(&pdev->dev, "soft reset control not defined\n");
+	} else {
+		err = reset_control_deassert(ahci_dev->sw_rst);
+		if (err) {
+			dev_err(&pdev->dev, "unable to bring out of reset\n");
+			return err;
+		}
+	}
+
+	ahci_dev->pwr_rst = devm_reset_control_get(&pdev->dev, "pwr-rst");
+	if (IS_ERR(ahci_dev->pwr_rst))
+		dev_dbg(&pdev->dev, "power soft reset control not defined\n");
+	else {
+		err = reset_control_deassert(ahci_dev->pwr_rst);
+		if (err) {
+			dev_err(&pdev->dev, "unable to bring out of reset\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int ahci_st_init(struct device *ahci_dev, void __iomem *mmio)
+{
+	struct platform_device *pdev = to_platform_device(ahci_dev->parent);
+	struct ahci_st_drv_data *drv_data = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = clk_prepare_enable(drv_data->clk);
+	if (ret)
+		return ret;
+
+	ret = phy_init(drv_data->phy);
+	if (ret) {
+		clk_disable_unprepare(drv_data->clk);
+		return ret;
+	}
+
+	drv_data->mmio = mmio;
+	ahci_st_configure_oob(mmio);
+
+	return 0;
+}
+
+static void ahci_st_exit(struct device *ahci_dev)
+{
+	struct platform_device *pdev = to_platform_device(ahci_dev->parent);
+	struct ahci_st_drv_data *drv_data = platform_get_drvdata(pdev);
+	int ret;
+
+	if (drv_data->pwr) {
+		ret = reset_control_assert(drv_data->pwr);
+		if (ret)
+			dev_err(&pdev->dev, "unable to bring out of pwrdwn\n");
+	}
+
+	clk_disable_unprepare(drv_data->clk);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ahci_st_suspend(struct device *ahci_dev)
+{
+	struct platform_device *pdev = to_platform_device(ahci_dev->parent);
+	struct ahci_st_drv_data *drv_data = platform_get_drvdata(pdev);
+	int ret;
+
+	if (drv_data->pwr) {
+		ret = reset_control_assert(drv_data->pwr);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to pwrdwn");
+			return ret;
+		}
+	}
+
+	clk_disable_unprepare(drv_data->clk);
+
+	return 0;
+}
+
+static int ahci_st_resume(struct device *ahci_dev)
+{
+	struct platform_device *pdev = to_platform_device(ahci_dev->parent);
+	struct ahci_st_drv_data *drv_data = platform_get_drvdata(pdev);
+	int ret;
+
+	clk_prepare_enable(drv_data->clk);
+
+	if (drv_data->pwr) {
+		ret = reset_control_deassert(drv_data->pwr);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to bring out of pwrdwn\n");
+			return ret;
+		}
+	}
+
+	ahci_st_configure_oob(drv_data->mmio);
+
+	ret = phy_init(drv_data->phy);
+	if (ret)
+		return ret;
+
+	if (drv_data->sw_rst) {
+		ret = reset_control_deassert(drv_data->sw_rst);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to bring out of sw-rst\n");
+			return ret;
+		}
+	}
+
+	if (drv_data->pwr_rst) {
+		ret = reset_control_deassert(drv_data->pwr_rst);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to bring out of pwr-rst\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+#endif
+
+static struct ahci_platform_data ahci_st_platform_data = {
+	.init = ahci_st_init,
+	.exit = ahci_st_exit,
+#ifdef CONFIG_PM_SLEEP
+	.suspend = ahci_st_suspend,
+	.resume = ahci_st_resume,
+#endif
+};
+
+static int ahci_st_driver_probe(struct platform_device *pdev)
+{
+	int err, id;
+	struct ahci_st_drv_data *drv_data;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct platform_device_info ahci_info = {
+		.parent    = dev,
+		.name      = "ahci",
+		.id        = pdev->id,
+		.res       = pdev->resource,
+		.num_res   = pdev->num_resources,
+		.data      = &ahci_st_platform_data,
+		.size_data = sizeof(ahci_st_platform_data),
+	};
+
+	if (np) {
+		id = of_alias_get_id(dev->of_node, "sata");
+		if (id > 0)
+			ahci_info.id = id;
+
+		ahci_info.dma_mask = DMA_BIT_MASK(32);
+	}
+
+	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	err = ahci_st_probe_resets(pdev, drv_data);
+	if (err)
+		return err;
+
+	drv_data->clk = devm_clk_get(&pdev->dev, "ahci_clk");
+	if (IS_ERR(drv_data->clk)) {
+		dev_err(&pdev->dev, "ahci_clk clk not found\n");
+		return PTR_ERR(drv_data->clk);
+	}
+
+	drv_data->phy = devm_phy_get(&pdev->dev, "ahci_phy");
+	if (IS_ERR(drv_data->phy)) {
+		dev_err(&pdev->dev, "no PHY configured\n");
+		return PTR_ERR(drv_data->phy);
+	}
+
+	platform_set_drvdata(pdev, drv_data);
+
+	drv_data->ahci = platform_device_register_full(&ahci_info);
+
+	return PTR_ERR_OR_ZERO(drv_data->ahci);
+}
+
+static struct of_device_id st_ahci_match[] = {
+	{ .compatible = "st,ahci", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_ahci_match);
+
+static struct platform_driver ahci_st_driver = {
+	.driver = {
+		.name = "ahci_st",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(st_ahci_match),
+	},
+	.probe = ahci_st_driver_probe,
+};
+module_platform_driver(ahci_st_driver);
+
+MODULE_AUTHOR("Alexandre Torgue <alexandre.torgue@st.com>");
+MODULE_AUTHOR("Francesco Virlinzi <francesco.virlinzi@st.com>");
+MODULE_DESCRIPTION("STMicroelectronics Sata Ahci driver");
+MODULE_LICENSE("GPL v2");