diff mbox

[v2,1/4] reset: Add support for the Amlogic Meson SoC Reset Controller

Message ID 1464169758-26975-2-git-send-email-narmstrong@baylibre.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Neil Armstrong May 25, 2016, 9:49 a.m. UTC 
This patch adds the platform driver for the Amlogic Meson SoC Reset
Controller.

The Meson8b and GXBB SoCs are supported.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/reset/Makefile      |   1 +
 drivers/reset/reset-meson.c | 137 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 drivers/reset/reset-meson.c

Comments

Philipp Zabel May 25, 2016, 10:14 a.m. UTC | #1 
Hi Neil,

looks fine, just two comments below.

Am Mittwoch, den 25.05.2016, 11:49 +0200 schrieb Neil Armstrong:
> This patch adds the platform driver for the Amlogic Meson SoC Reset
> Controller.
> 
> The Meson8b and GXBB SoCs are supported.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/reset/Makefile      |   1 +
>  drivers/reset/reset-meson.c | 137 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 138 insertions(+)
>  create mode 100644 drivers/reset/reset-meson.c
> 
[...]
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> new file mode 100644
> index 0000000..98087ae
> --- /dev/null
> +++ b/drivers/reset/reset-meson.c
[...]
> +static int meson_reset_reset(struct reset_controller_dev *rcdev,
> +			      unsigned long id)
> +{
> +	struct meson_reset *data =
> +		container_of(rcdev, struct meson_reset, rcdev);
> +	unsigned int bank = id / BITS_PER_REG;
> +	unsigned int offset = id % BITS_PER_REG;
> +	void *reg_addr = data->reg_base + (bank << 2);

We lost the __iomem here.

> +
> +	if (bank >= REG_COUNT)
> +		return -EINVAL;
> +
> +	writel(BIT(offset), reg_addr);
> +
> +	return 0;
> +}
> +
> +static const struct reset_control_ops meson_reset_ops = {
> +	.reset		= meson_reset_reset,
> +};
> +
> +static const struct of_device_id meson_reset_dt_ids[] = {
> +	 { .compatible = "amlogic,meson8b-reset", },
> +	 { .compatible = "amlogic,meson-gxbb-reset", },
> +	 { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
[...]
> +MODULE_ALIAS("platform:meson_reset");

And I think you can drop the MODULE_ALIAS since of: modaliases will be
generated from the MODULE_DEVICE_TABLE above. This driver is only ever
going to be probed from device tree, isn't it?

> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
> +MODULE_LICENSE("Dual BSD/GPL");

regards
Philipp
Kevin Hilman May 26, 2016, 2:42 a.m. UTC | #2 
Neil Armstrong <narmstrong@baylibre.com> writes:

> This patch adds the platform driver for the Amlogic Meson SoC Reset
> Controller.
>
> The Meson8b and GXBB SoCs are supported.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Maybe a question for Philipp, but when testog this version with the
stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was
never getting called.

Turns out, the stmmac driver only uses reset_control_assert() and
reset_control_deassert(), both of which return -ENOTSUPP since this
driver doesn't provide ->assert or ->deassert, so the driver's ->reset()
never gets called.

I haven't looked into the reset framework in detail, but if there's only
a ->reset hook, I kind of expected that reset_control_deassert() would
use that instead of returning -ENOTSUPP.

If not, what's the proper way of handling hardware that only supports a
write-only reset pulse?  Should users of reset_control_* be adapted to
check if ->deassert returns -ENOTSUPP and call ->reset?

Kevin
Philipp Zabel May 26, 2016, 11:02 a.m. UTC | #3 
Hi Kevin,

Am Mittwoch, den 25.05.2016, 19:42 -0700 schrieb Kevin Hilman:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
> > This patch adds the platform driver for the Amlogic Meson SoC Reset
> > Controller.
> >
> > The Meson8b and GXBB SoCs are supported.
> >
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Maybe a question for Philipp, but when testog this version with the
> stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was
> never getting called.
> 
> Turns out, the stmmac driver only uses reset_control_assert() and
> reset_control_deassert(), both of which return -ENOTSUPP since this
> driver doesn't provide ->assert or ->deassert, so the driver's ->reset()
> never gets called.
> 
> I haven't looked into the reset framework in detail, but if there's only
> a ->reset hook, I kind of expected that reset_control_deassert() would
> use that instead of returning -ENOTSUPP.
> 
> If not, what's the proper way of handling hardware that only supports a
> write-only reset pulse?  Should users of reset_control_* be adapted to
> check if ->deassert returns -ENOTSUPP and call ->reset?

I initially came up with this for i.MX6, which has a reset controller
that just like the Meson ones doesn't support manual assertion and
deassertion of the reset line.
My assumption then was that reset() is the default for all devices that
just need their internal state to be reset at some point. I didn't
expect the clock-like usage of manual deassert()/assert() for power
management to become so common.
So assert() and deassert() return -ENOTSUPP if the reset controller
doesn't support manually asserting or deasserting the reset line.

Of course you can argue that after a reset() pulse the reset line is
deasserted, and if you instead interpret the API in terms of expected
outcome, that would be similar to the state after call to deassert()
(iff the line was asserted before).
That would blur the line between reset() and deassert() somewhat, and in
my opinion having a call to deassert() first doing the exact opposite
isn't good nomenclature.
Personally, I'd prefer if the driver could switch to only using
	reset_control_reset(rstc);
or, if the reset line needs to stay asserted during powerdown where the
reset controller supports it, but doesn't matter whert not, use:
	ret = reset_control_deassert(rstc);
	if (ret == -ENOTSUPP)
		ret = reset_control_reset(rstc);

We could add a helper reset_control_deassert_or_reset() for that.

regards
Philipp
Kevin Hilman May 26, 2016, 6:42 p.m. UTC | #4 
Philipp Zabel <p.zabel@pengutronix.de> writes:

> Am Mittwoch, den 25.05.2016, 19:42 -0700 schrieb Kevin Hilman:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>> 
>> > This patch adds the platform driver for the Amlogic Meson SoC Reset
>> > Controller.
>> >
>> > The Meson8b and GXBB SoCs are supported.
>> >
>> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> 
>> Maybe a question for Philipp, but when testog this version with the
>> stmmac ethernet driver on the Amlogic boards, I noticed that ->reset was
>> never getting called.
>> 
>> Turns out, the stmmac driver only uses reset_control_assert() and
>> reset_control_deassert(), both of which return -ENOTSUPP since this
>> driver doesn't provide ->assert or ->deassert, so the driver's ->reset()
>> never gets called.
>> 
>> I haven't looked into the reset framework in detail, but if there's only
>> a ->reset hook, I kind of expected that reset_control_deassert() would
>> use that instead of returning -ENOTSUPP.
>> 
>> If not, what's the proper way of handling hardware that only supports a
>> write-only reset pulse?  Should users of reset_control_* be adapted to
>> check if ->deassert returns -ENOTSUPP and call ->reset?
>
> I initially came up with this for i.MX6, which has a reset controller
> that just like the Meson ones doesn't support manual assertion and
> deassertion of the reset line.
> My assumption then was that reset() is the default for all devices that
> just need their internal state to be reset at some point. I didn't
> expect the clock-like usage of manual deassert()/assert() for power
> management to become so common.
> So assert() and deassert() return -ENOTSUPP if the reset controller
> doesn't support manually asserting or deasserting the reset line.
>
> Of course you can argue that after a reset() pulse the reset line is
> deasserted, and if you instead interpret the API in terms of expected
> outcome, that would be similar to the state after call to deassert()
> (iff the line was asserted before).
> That would blur the line between reset() and deassert() somewhat, and in
> my opinion having a call to deassert() first doing the exact opposite
> isn't good nomenclature.
> Personally, I'd prefer if the driver could switch to only using
> 	reset_control_reset(rstc);

Then what would happen if that same driver is used on platforms that
have ->assert and ->deassert but no -reset?

The bind I'm in is that I'm using an exsting network driver
(stmicro/stmmac) which is used on a bunch of platforms and I dont' know
what all those reset controllers are actually doing, so going and
changing the driver seems problematic.

> or, if the reset line needs to stay asserted during powerdown where the
> reset controller supports it, but doesn't matter whert not, use:
> 	ret = reset_control_deassert(rstc);
> 	if (ret == -ENOTSUPP)
> 		ret = reset_control_reset(rstc);
>
> We could add a helper reset_control_deassert_or_reset() for that.

IMO, that would be a useful helper function, but I still think that
should be the default behavior of _deassert, otherwise it will still
require changing all the drivers.

Kevin
Philipp Zabel May 30, 2016, 4:14 p.m. UTC | #5 
Hi Kevin,

Am Donnerstag, den 26.05.2016, 11:42 -0700 schrieb Kevin Hilman:
[...]
> > Personally, I'd prefer if the driver could switch to only using
> > 	reset_control_reset(rstc);
> 
> Then what would happen if that same driver is used on platforms that
> have ->assert and ->deassert but no -reset?

If the implementer of the reset controller driver doesn't have enough
information about all connected devices to implement ->reset, then
_reset() has to return -ENOTSUPP, so this is not an option in the
general case.

> The bind I'm in is that I'm using an exsting network driver
> (stmicro/stmmac) which is used on a bunch of platforms and I dont' know
> what all those reset controllers are actually doing, so going and
> changing the driver seems problematic.

I see. It's all the more important that the API clearly states the
intended effects.

> > or, if the reset line needs to stay asserted during powerdown where the
> > reset controller supports it, but doesn't matter whert not, use:
> > 	ret = reset_control_deassert(rstc);
> > 	if (ret == -ENOTSUPP)
> > 		ret = reset_control_reset(rstc);
> >
> > We could add a helper reset_control_deassert_or_reset() for that.
> 
> IMO, that would be a useful helper function, but I still think that
> should be the default behavior of _deassert, otherwise it will still
> require changing all the drivers.

"All the drivers" makes me reconsider. If all drivers eventually,
reasonably could be changed to use reset_control_deassert_or_reset()
instead of reset_control_deassert(), there would be no reason not to
make _deassert() behave like you expect (well, except for the naming
issue).

One thing that has to be considered is that in the case of synchronous
resets there may be a significant difference in the behaviour of ->reset
and ->deassert: With ->deassert you could prime the reset line before
enabling clocks and letting the reset signal propagate. This is not
possible with ->reset, which could hang or have no effect at all with
the relevant clocks disabled. So it should be clearly documented that
anybody intending to make use of this must call reset_control_assert()
first and back off if that returns -ENOTSUPP.

regards
Philipp
diff mbox

Patch

diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index f173fc3..03dc1bb 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -3,6 +3,7 @@  obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
 obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
+obj-$(CONFIG_ARCH_MESON) += reset-meson.o
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_HISI) += hisilicon/
diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
new file mode 100644
index 0000000..98087ae
--- /dev/null
+++ b/drivers/reset/reset-meson.c
@@ -0,0 +1,137 @@ 
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ *
+ * BSD LICENSE
+ *
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *   * Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *   * Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in
+ *     the documentation and/or other materials provided with the
+ *     distribution.
+ *   * Neither the name of Intel Corporation nor the names of its
+ *     contributors may be used to endorse or promote products derived
+ *     from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define REG_COUNT	8
+#define BITS_PER_REG	32
+
+struct meson_reset {
+	void __iomem *reg_base;
+	struct reset_controller_dev rcdev;
+};
+
+static int meson_reset_reset(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct meson_reset *data =
+		container_of(rcdev, struct meson_reset, rcdev);
+	unsigned int bank = id / BITS_PER_REG;
+	unsigned int offset = id % BITS_PER_REG;
+	void *reg_addr = data->reg_base + (bank << 2);
+
+	if (bank >= REG_COUNT)
+		return -EINVAL;
+
+	writel(BIT(offset), reg_addr);
+
+	return 0;
+}
+
+static const struct reset_control_ops meson_reset_ops = {
+	.reset		= meson_reset_reset,
+};
+
+static const struct of_device_id meson_reset_dt_ids[] = {
+	 { .compatible = "amlogic,meson8b-reset", },
+	 { .compatible = "amlogic,meson-gxbb-reset", },
+	 { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
+
+static int meson_reset_probe(struct platform_device *pdev)
+{
+	struct meson_reset *data;
+	struct resource *res;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->reg_base))
+		return PTR_ERR(data->reg_base);
+
+	platform_set_drvdata(pdev, data);
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = REG_COUNT * BITS_PER_REG;
+	data->rcdev.ops = &meson_reset_ops;
+	data->rcdev.of_node = pdev->dev.of_node;
+
+	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+}
+
+static struct platform_driver meson_reset_driver = {
+	.probe	= meson_reset_probe,
+	.driver = {
+		.name		= "meson_reset",
+		.of_match_table	= meson_reset_dt_ids,
+	},
+};
+
+module_platform_driver(meson_reset_driver);
+
+MODULE_ALIAS("platform:meson_reset");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic Meson Reset Controller driver");
+MODULE_LICENSE("Dual BSD/GPL");