Re: [PATCH v2] microblaze: Switch to standard restart handler

[Michal Simek <monstr@monstr.eu>]

[-- Attachment #1.1: Type: text/plain, Size: 7882 bytes --]

On 23. 08. 19 11:47, Linus Walleij wrote:
> The microblaze uses the legacy APIs to dig out a GPIO pin
> defined in the root of the device tree to issue a hard
> reset of the platform.
> 
> Asserting a hard reset should be done using the standard
> DT-enabled and fully GPIO descriptor aware driver in
> drivers/power/reset/gpio-restart.c using the bindings
> from Documentation/devicetree/bindings/power/reset/gpio-restart.txt
> 
> To achieve this, first make sure microblaze makes use of
> the standard kernel restart path utilizing do_kernel_restart()
> from <linux/reboot.h>. Put in some grace time and an
> emergency print if the restart does not properly assert.
> 
> As this is basic platform functionality we patch the DTS
> file and defconfig in one go for a lockstep change.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michal Simek <monstr@monstr.eu>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix the reset GPIOs property in the DTS file to be
>   just "gpios" as expected by the driver.
> 
> Hi Michal, would be great if you could test and fix this up
> so I can get rid of one more site where legacy GPIO is used.
> I am unsure of the appropriate polarity and delays, hence the
> comments in the DTS file.
> ---
>  arch/microblaze/boot/dts/system.dts     | 16 +++++-
>  arch/microblaze/configs/mmu_defconfig   |  2 +
>  arch/microblaze/configs/nommu_defconfig |  2 +
>  arch/microblaze/include/asm/setup.h     |  1 -
>  arch/microblaze/kernel/reset.c          | 76 -------------------------
>  arch/microblaze/mm/init.c               | 11 ++++
>  6 files changed, 30 insertions(+), 78 deletions(-)
> 
> diff --git a/arch/microblaze/boot/dts/system.dts b/arch/microblaze/boot/dts/system.dts
> index 5a8a9d090c37..5b236527176e 100644
> --- a/arch/microblaze/boot/dts/system.dts
> +++ b/arch/microblaze/boot/dts/system.dts
> @@ -18,7 +18,6 @@
>  	#address-cells = <1>;
>  	#size-cells = <1>;
>  	compatible = "xlnx,microblaze";
> -	hard-reset-gpios = <&LEDs_8Bit 2 1>;
>  	model = "testing";
>  	DDR2_SDRAM: memory@90000000 {
>  		device_type = "memory";
> @@ -281,6 +280,21 @@
>  				gpios = <&LEDs_8Bit 7 1>;
>  			};
>  		} ;
> +
> +		gpio-restart {
> +			compatible = "gpio-restart";
> +			/*
> +			 * FIXME: is this active low or active high?
> +			 * the current flag (1) indicates active low.
> +			 * delay measures are templates, should be adjusted
> +			 * to datasheet or trial-and-error with real hardware.
> +			 */
> +			gpios = <&LEDs_8Bit 2 1>;
> +			active-delay = <100>;
> +			inactive-delay = <10>;
> +			wait-delay = <100>;
> +		};
> +
>  		RS232_Uart_1: serial@84000000 {
>  			clock-frequency = <125000000>;
>  			compatible = "xlnx,xps-uartlite-1.00.a";
> diff --git a/arch/microblaze/configs/mmu_defconfig b/arch/microblaze/configs/mmu_defconfig
> index 92fd4e95b488..ae8d7d407ff4 100644
> --- a/arch/microblaze/configs/mmu_defconfig
> +++ b/arch/microblaze/configs/mmu_defconfig
> @@ -59,6 +59,8 @@ CONFIG_SPI_XILINX=y
>  CONFIG_GPIOLIB=y
>  CONFIG_GPIO_SYSFS=y
>  CONFIG_GPIO_XILINX=y
> +CONFIG_POWER_RESET=y
> +CONFIG_POWER_RESET_GPIO_RESTART=y
>  # CONFIG_HWMON is not set
>  CONFIG_WATCHDOG=y
>  CONFIG_XILINX_WATCHDOG=y
> diff --git a/arch/microblaze/configs/nommu_defconfig b/arch/microblaze/configs/nommu_defconfig
> index 06d69a6e192d..a2a6be511551 100644
> --- a/arch/microblaze/configs/nommu_defconfig
> +++ b/arch/microblaze/configs/nommu_defconfig
> @@ -62,6 +62,8 @@ CONFIG_SPI_XILINX=y
>  CONFIG_GPIOLIB=y
>  CONFIG_GPIO_SYSFS=y
>  CONFIG_GPIO_XILINX=y
> +CONFIG_POWER_RESET=y
> +CONFIG_POWER_RESET_GPIO_RESTART=y
>  # CONFIG_HWMON is not set
>  CONFIG_WATCHDOG=y
>  CONFIG_XILINX_WATCHDOG=y
> diff --git a/arch/microblaze/include/asm/setup.h b/arch/microblaze/include/asm/setup.h
> index ce9b7b786156..54d634ed98e6 100644
> --- a/arch/microblaze/include/asm/setup.h
> +++ b/arch/microblaze/include/asm/setup.h
> @@ -29,7 +29,6 @@ void machine_early_init(const char *cmdline, unsigned int ram,
>  		unsigned int fdt, unsigned int msr, unsigned int tlb0,
>  		unsigned int tlb1);
>  
> -void machine_restart(char *cmd);
>  void machine_shutdown(void);
>  void machine_halt(void);
>  void machine_power_off(void);
> diff --git a/arch/microblaze/kernel/reset.c b/arch/microblaze/kernel/reset.c
> index fcbe1daf6316..b56af4eb91bf 100644
> --- a/arch/microblaze/kernel/reset.c
> +++ b/arch/microblaze/kernel/reset.c
> @@ -10,82 +10,6 @@
>  #include <linux/init.h>
>  #include <linux/of_platform.h>
>  
> -/* Trigger specific functions */
> -#ifdef CONFIG_GPIOLIB
> -
> -#include <linux/of_gpio.h>
> -
> -static int handle; /* reset pin handle */
> -static unsigned int reset_val;
> -
> -static int of_platform_reset_gpio_probe(void)
> -{
> -	int ret;
> -	handle = of_get_named_gpio(of_find_node_by_path("/"),
> -				   "hard-reset-gpios", 0);
> -
> -	if (!gpio_is_valid(handle)) {
> -		pr_info("Skipping unavailable RESET gpio %d (%s)\n",
> -				handle, "reset");
> -		return -ENODEV;
> -	}
> -
> -	ret = gpio_request(handle, "reset");
> -	if (ret < 0) {
> -		pr_info("GPIO pin is already allocated\n");
> -		return ret;
> -	}
> -
> -	/* get current setup value */
> -	reset_val = gpio_get_value(handle);
> -	/* FIXME maybe worth to perform any action */
> -	pr_debug("Reset: Gpio output state: 0x%x\n", reset_val);
> -
> -	/* Setup GPIO as output */
> -	ret = gpio_direction_output(handle, 0);
> -	if (ret < 0)
> -		goto err;
> -
> -	/* Setup output direction */
> -	gpio_set_value(handle, 0);
> -
> -	pr_info("RESET: Registered gpio device: %d, current val: %d\n",
> -							handle, reset_val);
> -	return 0;
> -err:
> -	gpio_free(handle);
> -	return ret;
> -}
> -device_initcall(of_platform_reset_gpio_probe);
> -
> -
> -static void gpio_system_reset(void)
> -{
> -	if (gpio_is_valid(handle))
> -		gpio_set_value(handle, 1 - reset_val);
> -	else
> -		pr_notice("Reset GPIO unavailable - halting!\n");
> -}
> -#else
> -static void gpio_system_reset(void)
> -{
> -	pr_notice("No reset GPIO present - halting!\n");
> -}
> -
> -void of_platform_reset_gpio_probe(void)
> -{
> -	return;
> -}
> -#endif
> -
> -void machine_restart(char *cmd)
> -{
> -	pr_notice("Machine restart...\n");
> -	gpio_system_reset();
> -	while (1)
> -		;
> -}
> -
>  void machine_shutdown(void)
>  {
>  	pr_notice("Machine shutdown...\n");
> diff --git a/arch/microblaze/mm/init.c b/arch/microblaze/mm/init.c
> index a015a951c8b7..4a45e037107f 100644
> --- a/arch/microblaze/mm/init.c
> +++ b/arch/microblaze/mm/init.c
> @@ -17,6 +17,8 @@
>  #include <linux/slab.h>
>  #include <linux/swap.h>
>  #include <linux/export.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
>  
>  #include <asm/page.h>
>  #include <asm/mmu_context.h>
> @@ -265,6 +267,15 @@ static void __init mmu_init_hw(void)
>  			: : : "r11");
>  }
>  
> +void machine_restart(char *cmd)
> +{
> +	do_kernel_restart(cmd);
> +	/* Give the restart hook 1 s to take us down */
> +	mdelay(1000);
> +	pr_emerg("Reboot failed -- System halted\n");
> +	while (1);
> +}
> +
>  /*
>   * MMU_init sets up the basic memory mappings for the kernel,
>   * including both RAM and possibly some I/O regions,
> 

I will test this for sure. What's the reason to add machine_restart to
mm/ folder? You can simply keep it in a location where it was.

Do you know why of_find_gpio can failed in connection to gpio-xilinx driver?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]