| Message ID | 1401983326-19205-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Antoine, thank you for the patch. I have a few comments below. Am Donnerstag, den 05.06.2014, 17:48 +0200 schrieb Antoine Ténart: > Add a reset controller for Marvell Berlin SoCs which is used by the > USB PHYs drivers (for now). > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/reset/Makefile | 1 + > drivers/reset/reset-berlin.c | 113 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 drivers/reset/reset-berlin.c > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 4f60caf750ce..fffe2a3dd255 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_RESET_CONTROLLER) += core.o > +obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o > obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o > obj-$(CONFIG_ARCH_STI) += sti/ > diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c > new file mode 100644 > index 000000000000..78b42c882cb2 > --- /dev/null > +++ b/drivers/reset/reset-berlin.c > @@ -0,0 +1,113 @@ > +/* > + * Copyright (C) 2014 Marvell Technology Group Ltd. > + * > + * Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> Is there a reason this is not actually implemented as platform device? > +#include <linux/reset-controller.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +#define BERLIN_RESET_REGISTER 0x104 How many reset registers are there? (See below). > +#define to_berlin_reset_priv(p) \ > + container_of((p), struct berlin_reset_priv, rcdev) > + > +struct berlin_reset_priv { > + spinlock_t lock; > + void __iomem *base; > + struct reset_controller_dev rcdev; > +}; > + > +static int berlin_reset_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev); > + unsigned long flags; > + int bank = id / BITS_PER_LONG; > + int offset = id % BITS_PER_LONG; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + writel(BIT(offset), priv->base + bank * 4); > + > + spin_unlock_irqrestore(&priv->lock, flags); Since this is a single write into an apparently self-clearing register, I see no need for the spinlock here. > + /* let the reset be effective */ > + udelay(10); > + > + return 0; > +} > + > +static struct reset_control_ops berlin_reset_ops = { > + .reset = berlin_reset_reset, > +}; > + > +static int __berlin_reset_init(struct device_node *np) > +{ > + struct berlin_reset_priv *priv; > + struct resource res; > + resource_size_t size; > + int ret; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + ret = of_address_to_resource(np, 0, &res); > + if (ret) > + goto err; > + > + size = resource_size(&res); > + > + priv->base = ioremap(res.start, size); > + if (!priv->base) { > + ret = -ENOMEM; > + goto err; > + } A platform driver could use devm_kzalloc, platform_get_resource, and devm_ioremap_resource here. > + priv->base += BERLIN_RESET_REGISTER; > + > + priv->rcdev.owner = THIS_MODULE; > + priv->rcdev.nr_resets = size * 32; This doesn't seem right. The device tree patch shows that size = 0x400. > + priv->rcdev.ops = &berlin_reset_ops; > + priv->rcdev.of_node = np; > + > + reset_controller_register(&priv->rcdev); > + > + return 0; > + > +err: > + kfree(priv); > + return ret; > +} > + > +static const struct of_device_id berlin_reset_of_match[] __initdata = { > + { .compatible = "marvell,berlin2q-chip-ctrl" }, > + { }, > +}; > + > +static int __init berlin_reset_init(void) > +{ > + struct device_node *np; > + int ret; > + > + for_each_matching_node(np, berlin_reset_of_match) { > + ret = __berlin_reset_init(np); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +arch_initcall(berlin_reset_init); regards Philipp
Hi Philipp, On Thu, Jun 05, 2014 at 06:36:45PM +0200, Philipp Zabel wrote: > > +#include <linux/platform_device.h> > > Is there a reason this is not actually implemented as platform device? The node describing this driver is shared with a pin controller and a clock driver. The pin controller is implemented as a platform device. I don't think we can have two platform device drivers using the device tree for the same node. Or I'm maybe missing something. > > > +#include <linux/reset-controller.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > +#include <linux/types.h> > > + > > +#define BERLIN_RESET_REGISTER 0x104 > > How many reset registers are there? (See below). I don't have lots of information about this. For now only the one used to reset the USB PHY but others may come later. > > + > > +static int berlin_reset_reset(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev); > > + unsigned long flags; > > + int bank = id / BITS_PER_LONG; > > + int offset = id % BITS_PER_LONG; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > + writel(BIT(offset), priv->base + bank * 4); > > + > > + spin_unlock_irqrestore(&priv->lock, flags); > > Since this is a single write into an apparently self-clearing > register, I see no need for the spinlock here. Sure. > > > + /* let the reset be effective */ > > + udelay(10); > > + > > + return 0; > > +} > > + > > +static struct reset_control_ops berlin_reset_ops = { > > + .reset = berlin_reset_reset, > > +}; > > + > > +static int __berlin_reset_init(struct device_node *np) > > +{ > > + struct berlin_reset_priv *priv; > > + struct resource res; > > + resource_size_t size; > > + int ret; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + ret = of_address_to_resource(np, 0, &res); > > + if (ret) > > + goto err; > > + > > + size = resource_size(&res); > > + > > + priv->base = ioremap(res.start, size); > > + if (!priv->base) { > > + ret = -ENOMEM; > > + goto err; > > + } > > A platform driver could use devm_kzalloc, platform_get_resource, > and devm_ioremap_resource here. > > > + priv->base += BERLIN_RESET_REGISTER; > > + > > + priv->rcdev.owner = THIS_MODULE; > > + priv->rcdev.nr_resets = size * 32; > > This doesn't seem right. The device tree patch shows that > size = 0x400. The reg property is shared between drivers using the common chip controller node. I do not know how many registers are actually used to reset. We then could hardcode the size, with the registers actually used here? Thanks for the review! Antoine
On 06/05/2014 06:36 PM, Philipp Zabel wrote: > Am Donnerstag, den 05.06.2014, 17:48 +0200 schrieb Antoine Ténart: >> Add a reset controller for Marvell Berlin SoCs which is used by the >> USB PHYs drivers (for now). >> >> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> >> --- >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-berlin.c | 113 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 114 insertions(+) >> create mode 100644 drivers/reset/reset-berlin.c >> [...] >> obj-$(CONFIG_ARCH_STI) += sti/ >> diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c >> new file mode 100644 >> index 000000000000..78b42c882cb2 >> --- /dev/null >> +++ b/drivers/reset/reset-berlin.c >> @@ -0,0 +1,113 @@ >> +/* >> + * Copyright (C) 2014 Marvell Technology Group Ltd. >> + * >> + * Antoine Ténart <antoine.tenart@free-electrons.com> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/platform_device.h> > > Is there a reason this is not actually implemented as platform device? > [...] >> +static int __berlin_reset_init(struct device_node *np) >> +{ >> + struct berlin_reset_priv *priv; >> + struct resource res; >> + resource_size_t size; >> + int ret; >> + >> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + ret = of_address_to_resource(np, 0, &res); >> + if (ret) >> + goto err; >> + >> + size = resource_size(&res); >> + >> + priv->base = ioremap(res.start, size); >> + if (!priv->base) { >> + ret = -ENOMEM; >> + goto err; >> + } > > A platform driver could use devm_kzalloc, platform_get_resource, > and devm_ioremap_resource here. > >> + priv->base += BERLIN_RESET_REGISTER; >> + >> + priv->rcdev.owner = THIS_MODULE; >> + priv->rcdev.nr_resets = size * 32; > > This doesn't seem right. The device tree patch shows that > size = 0x400. Actually, not using a platform_device now is the outcome of some late DT node rework we had for the clock driver. The reason we only have one node for the whole register set providing pinctrl, reset, clock, ... is that it would require tiny separate register ranges spread over the whole register set. Instead, the idea is to have a single DT node, register a driver providing a mmio regmap, and registering individual platform_devices for the non-early drivers using the regmap. We also evaluated syscon, but it will require dummy nodes for each proper platform_device and that is something we really want to avoid here. Of course, writing that driver is delayed on my side because of other non-Linux stuff that has to be taken care of first. As I cannot tell how much time I can spend on it now, I prefer to take this as is and provide update patches as soon as I have worked out the regset driver. Sebastian
On 06/05/2014 05:48 PM, Antoine Ténart wrote: > Add a reset controller for Marvell Berlin SoCs which is used by the > USB PHYs drivers (for now). > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/reset/Makefile | 1 + > drivers/reset/reset-berlin.c | 113 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 114 insertions(+) > create mode 100644 drivers/reset/reset-berlin.c > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 4f60caf750ce..fffe2a3dd255 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_RESET_CONTROLLER) += core.o > +obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o > obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o > obj-$(CONFIG_ARCH_STI) += sti/ > diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c > new file mode 100644 > index 000000000000..78b42c882cb2 > --- /dev/null > +++ b/drivers/reset/reset-berlin.c > @@ -0,0 +1,113 @@ > +/* > + * Copyright (C) 2014 Marvell Technology Group Ltd. > + * > + * Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/reset-controller.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +#define BERLIN_RESET_REGISTER 0x104 > + > +#define to_berlin_reset_priv(p) \ > + container_of((p), struct berlin_reset_priv, rcdev) > + > +struct berlin_reset_priv { > + spinlock_t lock; > + void __iomem *base; > + struct reset_controller_dev rcdev; > +}; > + > +static int berlin_reset_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev); > + unsigned long flags; > + int bank = id / BITS_PER_LONG; > + int offset = id % BITS_PER_LONG; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + writel(BIT(offset), priv->base + bank * 4); > + > + spin_unlock_irqrestore(&priv->lock, flags); > + > + /* let the reset be effective */ > + udelay(10); > + > + return 0; > +} > + > +static struct reset_control_ops berlin_reset_ops = { > + .reset = berlin_reset_reset, > +}; > + > +static int __berlin_reset_init(struct device_node *np) > +{ > + struct berlin_reset_priv *priv; > + struct resource res; > + resource_size_t size; > + int ret; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + ret = of_address_to_resource(np, 0, &res); > + if (ret) > + goto err; > + > + size = resource_size(&res); > + > + priv->base = ioremap(res.start, size); > + if (!priv->base) { > + ret = -ENOMEM; > + goto err; > + } > + priv->base += BERLIN_RESET_REGISTER; I currently have no way to look it up myself, but is reset API providing a way to deal with phandle+specifier with more than one parameter? Chip-ctrl has a bunch of other reset bits spread over the regset, having the offset encoded in the specifier would save us some SoC specific boiler plate, i.e. reset = <&chip 0x104 14>; Sebastian > + priv->rcdev.owner = THIS_MODULE; > + priv->rcdev.nr_resets = size * 32; > + priv->rcdev.ops = &berlin_reset_ops; > + priv->rcdev.of_node = np; > + > + reset_controller_register(&priv->rcdev); > + > + return 0; > + > +err: > + kfree(priv); > + return ret; > +} > + > +static const struct of_device_id berlin_reset_of_match[] __initdata = { > + { .compatible = "marvell,berlin2q-chip-ctrl" }, > + { }, > +}; > + > +static int __init berlin_reset_init(void) > +{ > + struct device_node *np; > + int ret; > + > + for_each_matching_node(np, berlin_reset_of_match) { > + ret = __berlin_reset_init(np); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +arch_initcall(berlin_reset_init); >
Hi Sebastian, On Mon, Jun 09, 2014 at 12:32:50PM +0200, Sebastian Hesselbarth wrote: > I currently have no way to look it up myself, but is reset API providing > a way to deal with phandle+specifier with more than one parameter? You could provide a custom rcdev->of_xlate callback and encode multiple phandle args into the reset line "index". static int of_reset_xlate(struct reset_controller_dev *rcdev, const struct of_phandle_args *reset_spec) { return reset_spec->args[0] * 32 + reset_spec->args[1]; } > Chip-ctrl has a bunch of other reset bits spread over the regset, having > the offset encoded in the specifier would save us some SoC specific > boiler plate, i.e. > > reset = <&chip 0x104 14>; That should be possible. regards Philipp
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 4f60caf750ce..fffe2a3dd255 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_RESET_CONTROLLER) += core.o +obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o obj-$(CONFIG_ARCH_STI) += sti/ diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c new file mode 100644 index 000000000000..78b42c882cb2 --- /dev/null +++ b/drivers/reset/reset-berlin.c @@ -0,0 +1,113 @@ +/* + * Copyright (C) 2014 Marvell Technology Group Ltd. + * + * Antoine Ténart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h> + +#define BERLIN_RESET_REGISTER 0x104 + +#define to_berlin_reset_priv(p) \ + container_of((p), struct berlin_reset_priv, rcdev) + +struct berlin_reset_priv { + spinlock_t lock; + void __iomem *base; + struct reset_controller_dev rcdev; +}; + +static int berlin_reset_reset(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev); + unsigned long flags; + int bank = id / BITS_PER_LONG; + int offset = id % BITS_PER_LONG; + + spin_lock_irqsave(&priv->lock, flags); + + writel(BIT(offset), priv->base + bank * 4); + + spin_unlock_irqrestore(&priv->lock, flags); + + /* let the reset be effective */ + udelay(10); + + return 0; +} + +static struct reset_control_ops berlin_reset_ops = { + .reset = berlin_reset_reset, +}; + +static int __berlin_reset_init(struct device_node *np) +{ + struct berlin_reset_priv *priv; + struct resource res; + resource_size_t size; + int ret; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + ret = of_address_to_resource(np, 0, &res); + if (ret) + goto err; + + size = resource_size(&res); + + priv->base = ioremap(res.start, size); + if (!priv->base) { + ret = -ENOMEM; + goto err; + } + priv->base += BERLIN_RESET_REGISTER; + + priv->rcdev.owner = THIS_MODULE; + priv->rcdev.nr_resets = size * 32; + priv->rcdev.ops = &berlin_reset_ops; + priv->rcdev.of_node = np; + + reset_controller_register(&priv->rcdev); + + return 0; + +err: + kfree(priv); + return ret; +} + +static const struct of_device_id berlin_reset_of_match[] __initdata = { + { .compatible = "marvell,berlin2q-chip-ctrl" }, + { }, +}; + +static int __init berlin_reset_init(void) +{ + struct device_node *np; + int ret; + + for_each_matching_node(np, berlin_reset_of_match) { + ret = __berlin_reset_init(np); + if (ret) + return ret; + } + + return 0; +} +arch_initcall(berlin_reset_init);
Add a reset controller for Marvell Berlin SoCs which is used by the USB PHYs drivers (for now). Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/reset/Makefile | 1 + drivers/reset/reset-berlin.c | 113 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 drivers/reset/reset-berlin.c