On Thu, Jul 03, 2014 at 10:39:08PM +0800, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > +++ b/drivers/power/reset/at91-reset.c
> > @@ -0,0 +1,202 @@
> > +/*
> > + * Atmel AT91 SAM9 SoCs reset code
> > + *
> > + * Copyright (C) 2014 Maxime Ripard
> > + *
> > + * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
>
> you can not own the copyright as it’s basically a copy of other
> people code
The previous names are missing, right.
> > + *
> > + * 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/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +
> > +#include <asm/system_misc.h>
> > +
> > +#include <mach/at91sam9_ddrsdr.h>
> > +#include <mach/at91sam9_sdramc.h>
> > +
> > +#define AT91_RSTC_CR 0x00 /* Reset Controller Control Register */
> > +#define AT91_RSTC_PROCRST BIT(0) /* Processor Reset */
> > +#define AT91_RSTC_PERRST BIT(2) /* Peripheral Reset */
> > +#define AT91_RSTC_EXTRST BIT(3) /* External Reset */
> > +#define AT91_RSTC_KEY (0xa5 << 24) /* KEY Password */
> > +
> > +#define AT91_RSTC_SR 0x04 /* Reset Controller Status Register */
> > +#define AT91_RSTC_URSTS BIT(0) /* User Reset Status */
> > +#define AT91_RSTC_RSTTYP GENMASK(10, 8) /* Reset Type */
> > +#define AT91_RSTC_NRSTL BIT(16) /* NRST Pin Level */
> > +#define AT91_RSTC_SRCMP BIT(17) /* Software Reset Command in Progress */
> > +
> > +#define AT91_RSTC_MR 0x08 /* Reset Controller Mode Register */
> > +#define AT91_RSTC_URSTEN BIT(0) /* User Reset Enable */
> > +#define AT91_RSTC_URSTIEN BIT(4) /* User Reset Interrupt Enable */
> > +#define AT91_RSTC_ERSTL GENMASK(11, 8) /* External Reset Length */
> > +
> > +enum reset_type {
> > + RESET_TYPE_GENERAL = 0,
> > + RESET_TYPE_WAKEUP = 1,
> > + RESET_TYPE_WATCHDOG = 2,
> > + RESET_TYPE_SOFTWARE = 3,
> > + RESET_TYPE_USER = 4,
> > +};
> > +
> > +static void __iomem *at91_ramc_base[2], *at91_rstc_base;
> > +
> > +static void at91sam9_restart(enum reboot_mode mode, const char *cmd)
> > +{
> > + asm volatile(
> > + ".balign 32\n\t"
> > +
> > + "str %2, [%0, #" __stringify(AT91_SDRAMC_TR) "]\n\t"
> > + "str %3, [%0, #" __stringify(AT91_SDRAMC_LPR) "]\n\t"
> > + "str %4, [%1, #" __stringify(AT91_RSTC_CR) "]\n\t"
> > +
> > + "b .\n\t"
> > + :
> > + : "r" (at91_ramc_base[0]),
> > + "r" (at91_rstc_base),
> > + "r" (1),
> > + "r" (AT91_SDRAMC_LPCB_POWER_DOWN),
> > + "r" (AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST));
> > +}
> > +
> > +static void at91sam9g45_restart(enum reboot_mode mode, const char *cmd)
> > +{
> > + asm volatile(
> > + "cmp %1, #0\n\t"
> > + "beq 1f\n\t"
> > +
> > + "ldr r0, [%1]\n\t"
> > + "cmp r0, #0\n\t"
> > +
> > + ".balign 32\n\t"
> > +
> > + "1: str %3, [%0, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > + " str %4, [%0, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > + " strne %3, [%1, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > + " strne %4, [%1, #" __stringify(AT91_DDRSDRC_RTR) "]\n\t"
> > + " str %5, [%2, #" __stringify(AT91_RSTC_CR) "]\n\t"
> > +
> > + " b .\n\t"
> > + :
> > + : "r" (at91_ramc_base[0]),
> > + "r" (at91_ramc_base[1]),
> > + "r" (at91_rstc_base),
> > + "r" (1),
> > + "r" (AT91_DDRSDRC_LPCB_POWER_DOWN),
> > + "r" (AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST)
> > + : "r0");
> > +}
> > +
> move this to an assembly file more easy to read than a C code
Nope. It's a pain to pass variable to an external assembly file, and
this makes the use of global variable pretty much mandatory, which is
definitely not great.
>
> > +static void __init at91_reset_status(struct platform_device *pdev)
> > +{
> > + u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
> > + char *reason;
> > +
> > + switch ((reg & AT91_RSTC_RSTTYP) >> 8) {
> > + case RESET_TYPE_GENERAL:
> > + reason = "general reset";
> > + break;
> > + case RESET_TYPE_WAKEUP:
> > + reason = "wakeup";
> > + break;
> > + case RESET_TYPE_WATCHDOG:
> > + reason = "watchdog reset";
> > + break;
> > + case RESET_TYPE_SOFTWARE:
> > + reason = "software reset";
> > + break;
> > + case RESET_TYPE_USER:
> > + reason = "user reset";
> > + break;
> > + default:
> > + reason = "unknown reset";
> > + break;
> > + }
> > +
> > + pr_info("AT91: Starting after %s\n", reason);
> > +}
> > +
> > +static struct of_device_id at91_ramc_of_match[] = {
> > + { .compatible = "atmel,at91sam9260-sdramc", },
> > + { .compatible = "atmel,at91sam9g45-ddramc", },
> > + { /* sentinel */ }
> > +};
> > +
> > +static struct of_device_id at91_reset_of_match[] = {
> > + { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9_restart },
> > + { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> > + { /* sentinel */ }
> > +};
> > +
> > +static int at91_reset_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + at91_rstc_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(at91_rstc_base)) {
> > + dev_err(&pdev->dev, "Could not map reset controller address\n");
> > + return PTR_ERR(at91_rstc_base);
> > + }
> > +
> > + if (pdev->dev.of_node) {
>
> split in 2 function more easy to ready and less indentation
ok.
> > + const struct of_device_id *match;
> > + struct device_node *np;
> > + int idx = 0;
> > +
> > + for_each_matching_node(np, at91_ramc_of_match) {
> > + at91_ramc_base[idx] = of_iomap(np, 0);
> > + if (!at91_ramc_base[idx]) {
> > + dev_err(&pdev->dev, "Could not map ram controller address\n");
> > + return -ENODEV;
> > + }
> > + idx++;
> > + }
>
> and if you can not probe the ram controler it’s a panic not a -ENODEV
>
> as you have an unstable platform
I don't really see why. That the pm code and the reset code won't be
able to work, it's obvious. But making the assumption that the
platforms don't have a RAM properly setup just because it doesn't have
a DT node seems quite weak.
> > +
> > + match = of_match_node(at91_reset_of_match, pdev->dev.of_node);
> > + arm_pm_restart = match->data;
> > + } else {
> > + const struct platform_device_id *match;
> > + int idx = 0;
> > +
> > + for (idx = 0; idx < 2; idx++) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, idx + 1 );
> > + at91_ramc_base[idx] = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(at91_ramc_base[idx])) {
> > + dev_err(&pdev->dev, "Could not map ram controller address\n");
> > + return PTR_ERR(at91_rstc_base);
> > + }
> > + }
> > +
> > + match = platform_get_device_id(pdev);
> > + arm_pm_restart = (void (*)(enum reboot_mode, const char*))
> > + match->driver_data;
> > + }
> > +
> > + at91_reset_status(pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_device_id at91_reset_plat_match[] = {
> > + { "at91-sam9-reset", (unsigned long)at91sam9_restart },
> > + { "at91-g45-reset", (unsigned long)at91sam9g45_restart },
> at91-sam9???
>
> from the beginning of DT we put the first SoC were the
> reset was introduce and why do you change the DT binding?
Except that this is not about DT probing, but the old-style board
files one.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
