[PATCH v4 0/4] ARM: Broadcom BCM4760 support

[PATCH v4 0/4] ARM: Broadcom BCM4760 support

[Domenico Andreoli]

Hi,

  here is the summary of the changes I made since v3:

* added low-level debug PL01X parameters
* added clocks to the VICs DT stanzas
* added bcm4760_readl/writel helpers to the system driver code and
  removed (my) previous (naive) helpers
* inverted the order of clockevents_config_and_register() and setup_irq()
  so to simplify the irq handler
* drop the clock frequency option from the DT (left-over form v2)
* moved the watchdog based implementation to drivers/watchdog

This patchset is not autonomous for the build of mach-bcm if ARCH_BCM
is not enabled, a detail that should get a fix from Christian soon.

In the meanwhile I post for kind review.

Best regards,
Domenico

[PATCH v4 1/4] ARM: bcm4760: Add platform infrastructure

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-bcm476x-add-infrastructure.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130914/1a50166b/attachment.ksh>

[PATCH v4 2/4] ARM: bcm4760: Add system timer

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-bcm476x-add-system-timer.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130914/6c8986c1/attachment.ksh>

[PATCH v4 3/4] ARM: bcm4760: Add ripple counter

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-bcm476x-add-ripple-counter.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130914/5daf9224/attachment.ksh>

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Domenico Andreoli]

An embedded and charset-unspecified text was scrubbed...
Name: arm-bcm476x-add-restart-hook.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130914/93178550/attachment.ksh>

[PATCH v4 1/4] ARM: bcm4760: Add platform infrastructure

[Arnd Bergmann]

On Saturday 14 September 2013, Domenico Andreoli wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
> 
> Platform infrastructure for the Broadcom BCM4760 based ARM11 SoCs.
> 
> v4:
> * added low-level debug PL01X parameters
> * added clocks to the VICs DT stanzas
> 
> v3:
> * dropped the idea of unconditionally build mach-bcm
> 
> v2:
> * clocks are now configured via DT
> * uart DT nodes have been renamed and hooked to the proper clock nodes
> * dropped unneeded config options
> * dropped misused initialization of system_rev
> * dropped unneeded early io mapping 
> * build rule of mach-bcm is moved to separated patch
> 
> v1:
> * initial release
> 
> Cc: devicetree at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
> ---

Acked-by: Arnd Bergmann <arnd@arndb.de>

Note that normally we put the patch revision history below the '---' line, so it 
doesn't get included in the git history.

> +
> +static void __init bcm4760_init(void)
> +{
> +	of_clk_init(NULL);
> +	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +}

With the series that Sebastian Hesselbarth prepared to automatically do
of_clk_init(NULL), this entire function should go away as well, but I think
the cleanest way is to handle that in the arm-soc tree during the merge
of your patches with Sebastian's patches.

	Arnd

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Arnd Bergmann]

On Saturday 14 September 2013, Domenico Andreoli wrote:
> +
> +static int __init bcm4760_wdt_init(void)
> +{
> +	struct device_node *node;
> +
> +	node = of_find_matching_node(NULL, bcm4760_pm_wdt_match);
> +	if (!node) {
> +		pr_info("No bcm4760 watchdog node\n");
> +		return -1;
> +	}
> +
> +	wdt_regs = of_iomap(node, 0);
> +	of_node_put(node);

Since this is now in the drivers directory and initialized at regular
module_init level, I'd ask you to register it as a proper platform_driver
and move the initialization into the probe() callback. You also need to
put Wim as the watchdog subsystem maintainer on Cc (I did in this reply)
and ask him to merge the driver.

I assume that Wim will ask you to add a watchdog_register_device() call
and a set of watchdog_ops to make this a functional driver. From the
platform perspective, I don't care, but it is certainly a logical step.

	Arnd

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Domenico Andreoli]

On Sun, Sep 15, 2013 at 08:09:26PM +0200, Arnd Bergmann wrote:
> On Saturday 14 September 2013, Domenico Andreoli wrote:
> > +
> > +static int __init bcm4760_wdt_init(void)
> > +{
> > +	struct device_node *node;
> > +
> > +	node = of_find_matching_node(NULL, bcm4760_pm_wdt_match);
> > +	if (!node) {
> > +		pr_info("No bcm4760 watchdog node\n");
> > +		return -1;
> > +	}
> > +
> > +	wdt_regs = of_iomap(node, 0);
> > +	of_node_put(node);
> 
> Since this is now in the drivers directory and initialized at regular
> module_init level, I'd ask you to register it as a proper platform_driver
> and move the initialization into the probe() callback. You also need to
> put Wim as the watchdog subsystem maintainer on Cc (I did in this reply)
> and ask him to merge the driver.
> 
> I assume that Wim will ask you to add a watchdog_register_device() call
> and a set of watchdog_ops to make this a functional driver. From the
> platform perspective, I don't care, but it is certainly a logical step.

issue here is that there is already a proper watchdog driver, the sp805.

so I guess now the task shifts to adding restart hook support to it, right?

Kind regards,
Domenico

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Arnd Bergmann]

On Sunday 15 September 2013, Domenico Andreoli wrote:
> issue here is that there is already a proper watchdog driver, the sp805.
> 
> so I guess now the task shifts to adding restart hook support to it, right?

Yes, correct.

There is an interesting question however: we have to deal with the same driver
being used in some machines that need to use it as the only way to reset the
system, as well as the case where you actually want to use some other method.

An easy way to handle this would be a boolean device tree property that
tells the driver whether or not to register, but we might want to
come up with a more sophisticated way to have multiple reset handlers
registered an prioritized so we try the "best" one first. Maybe someone
else has an opinion on this. If not, just do the property.

	Arnd

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Domenico Andreoli]

On Sun, Sep 15, 2013 at 10:10:36PM +0200, Arnd Bergmann wrote:
> On Sunday 15 September 2013, Domenico Andreoli wrote:
> > issue here is that there is already a proper watchdog driver, the sp805.
> > 
> > so I guess now the task shifts to adding restart hook support to it, right?
> 
> Yes, correct.
> 
> There is an interesting question however: we have to deal with the same driver
> being used in some machines that need to use it as the only way to reset the
> system, as well as the case where you actually want to use some other method.

in a certain sense, there is space for a generic watchdog based restart
hook mechanism but the current watchdog ops do not provide the necessary
guarantees in atomic context.

> An easy way to handle this would be a boolean device tree property that
> tells the driver whether or not to register, but we might want to
> come up with a more sophisticated way to have multiple reset handlers
> registered an prioritized so we try the "best" one first. Maybe someone
> else has an opinion on this. If not, just do the property.

the simplest solution I see is adding a DT option to the sp805 driver so
that it registers the restart hook when asked to do so.

need to investigate whether the sp805 DT support is provided by some generic
AMBA mechanism or is completely missing.

> 
> 	Arnd

thanks,
Domenico

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Arnd Bergmann]

On Monday 16 September 2013, Domenico Andreoli wrote:
> On Sun, Sep 15, 2013 at 10:10:36PM +0200, Arnd Bergmann wrote:
> > On Sunday 15 September 2013, Domenico Andreoli wrote:
> > > issue here is that there is already a proper watchdog driver, the sp805.
> > > 
> > > so I guess now the task shifts to adding restart hook support to it, right?
> > 
> > Yes, correct.
> > 
> > There is an interesting question however: we have to deal with the same driver
> > being used in some machines that need to use it as the only way to reset the
> > system, as well as the case where you actually want to use some other method.
> 
> in a certain sense, there is space for a generic watchdog based restart
> hook mechanism but the current watchdog ops do not provide the necessary
> guarantees in atomic context.

It sounds like a good idea. I see another problem there, which is that registering
a restart hook is architecture specific at the moment, so it would also require
generalizing that, or alternatively keeping watchdog based restart specific to
ARM and any architecture that adds support in a similar way.

> > An easy way to handle this would be a boolean device tree property that
> > tells the driver whether or not to register, but we might want to
> > come up with a more sophisticated way to have multiple reset handlers
> > registered an prioritized so we try the "best" one first. Maybe someone
> > else has an opinion on this. If not, just do the property.
> 
> the simplest solution I see is adding a DT option to the sp805 driver so
> that it registers the restart hook when asked to do so.
> 
> need to investigate whether the sp805 DT support is provided by some generic
> AMBA mechanism or is completely missing.

I think it should just work if you put the right properties into DT: it only
uses one memory resource (from "reg" property) and one clk (from "clocks"
property), and those are automatically there for AMBA devices. If the hardware
does not fill the correct primecell ID, you may have to add a
"arm,primecell-periphid=<0x00141805>" property.

	Arnd

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Arnd Bergmann]

On Monday 16 September 2013 14:00:01 Wim Van Sebroeck wrote:
> 
> I remember that the Cobalt Devices have no ability to reboot themselves.
> That's why the driver uses the reboot_notifier for rebooting the device.
> See drivers/watchdog/alim7101_wdt.c .

Hmm, a notifier seems the wrong approach here, because we really want
to handle all reboot_notifiers before actually rebooting, and we'd enter
the wdt driver at a random point in the notifier chain. It probably works
by chance if the watchdog reboots a second after its notifier is called,
and all other calls are done as well, but it doesn't seem like a clean
solution.

> I'll have a look to have an extra ops that adds this functionality in the framework.

Ok, cool. Thanks!

	Arnd

[PATCH v4 4/4] ARM: bcm4760: Add restart hook

[Domenico Andreoli]

Hi Wim,

On Mon, Sep 16, 2013 at 10:01:08PM +0200, Arnd Bergmann wrote:
> On Monday 16 September 2013 14:00:01 Wim Van Sebroeck wrote:
> > 
> > I remember that the Cobalt Devices have no ability to reboot themselves.
> > That's why the driver uses the reboot_notifier for rebooting the device.
> > See drivers/watchdog/alim7101_wdt.c .
> 
> Hmm, a notifier seems the wrong approach here, because we really want
> to handle all reboot_notifiers before actually rebooting, and we'd enter
> the wdt driver at a random point in the notifier chain. It probably works
> by chance if the watchdog reboots a second after its notifier is called,
> and all other calls are done as well, but it doesn't seem like a clean
> solution.
> 
> > I'll have a look to have an extra ops that adds this functionality in the framework.

I'd like to do something here.

I'm thinking at a simple "atomic_reset()" wdt op which purpose it to use
the specific watchdog to trigger a HW reset. As the name implies, the call
needs to be callable in atomic context. Any other constrains I'm missing?

At this point a DT attribute in the wdt stanzas could be used to register
the reboot hook at runtime.

thanks,
Domenico