| Message ID | 1464169758-26975-2-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 --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");
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