On Wednesday 29 April 2015 01:49:53 fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> ---
This needs a changelog. Please explain what this is good for here.
> +/*
> + * Architected system timer support.
> + */
> +static __always_inline
Just drop the __always_inline here, unless it's broken to do so (in which case
you should add a comment). The compiler will inline static functions by itself,
and you can use 'inline' to annotate the fact that you expect the function
to be inlined.
> +void sbsa_gwdt_reg_write(enum sbsa_gwdt_frame frame, unsigned int reg, u32 val,
> + struct watchdog_device *wdd)
> +{
> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
> + struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
> +
> + switch (frame) {
> + case SBSA_GWDT_RF:
> + writel_relaxed(val, device_data->refresh_base + reg);
> + break;
> + case SBSA_GWDT_CF:
> + writel_relaxed(val, device_data->control_base + reg);
> + break;
> + default:
> + pr_err("sbsa_gwdt: reg write fail(frame not available) \n");
> + }
> +}
[coding style]
How about having two separate functions to do this, it seems that would
simplify both the wrapper and its callers.
> +
> +static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> + void __user *argp = (void __user *)arg;
> + int ret = -ENOIOCTLCMD;
> + unsigned int __user *p = argp;
> + unsigned int new_value;
> + int __user *preaction = argp;
> + int new_preaction;
> + /* FIXME: what else do we need here?? */
Answer: as little as possible.
> + switch (cmd) {
> + case WDIOC_SETPRETIMEOUT:
> + if (get_user(new_value, p))
> + return -EFAULT;
> + ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
> + break;
> + case WDIOC_GETPRETIMEOUT:
> + ret = put_user(gwdt->pretimeout, (unsigned int __user *)arg);
> + break;
> + case SBSA_GWDIOC_SETPREACTION:
> + if (get_user(new_preaction, preaction))
> + return -EFAULT;
> + ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
> + break;
> + case SBSA_GWDIOC_GETPREACTION:
> + ret = put_user(gwdt->preaction, (int __user *)arg);
> + break;
> + }
> +
> + return ret;
> +}
Don't introduce driver specific commands here. Instead, make a separate patch
to add new generic callbacks so the user API is shared with all drivers.
It would be helpful to add support for the WDIOC_SETPRETIMEOUT
and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> +{
> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> + struct watchdog_device *wdd = &gwdt->wdd;
> + u32 status;
> + unsigned int left;
> +
> + pr_debug("SBSA Watchdog: in %s\n", __func__);
> +
> + status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
> +
> + if (status & SBSA_GWDT_WCS_WS0) {
> + if (printk_ratelimit()) {
> + pr_crit("SBSA Watchdog WS0 is triggered\n");
> + left = sbsa_gwdt_get_timeleft(wdd);
> + pr_crit("please try to feed the dog in %ds!\n", left);
> + }
> + /* FIXME:anything we can do here?
> + * send_signal(sig, info, p, group); ??
> + */
> + if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
> + sbsa_gwdt_keepalive(wdd);
> + pr_warn("kernel keep system running by feeding dog!\n");
What is the point of this? Is there any realistic use case for setting
SBSA_GWDT_PREACT_FEEDDOG?
> + } else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
> + panic("SBSA Watchdog pre-timeout");
> + }
Same question here: what is this good for?
> +
> +static int sbsa_gwdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> +
> + struct sbsa_gwdt *gwdt;
> + struct sbsa_gwdt_device_data *device_data;
> +
> + struct watchdog_device *wdd;
> +
> + struct resource *res;
> + void *rf_base = NULL, *cf_base = NULL;
Don't initialize local variables to NULL unless you have to use that
value later.
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + SBSA_GWDT_RF_RES_NAME);
> + rf_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(rf_base))
> + return PTR_ERR(rf_base);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + SBSA_GWDT_CF_RES_NAME);
> + cf_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(rf_base))
> + return PTR_ERR(rf_base);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> + SBSA_GWDT_IRQ_RES_NAME);
Please open-code the resource names here, the macros make it harder
to read what you do here.
Use platform_get_irq() here to allow deferred probing of the irqchip.
> +static const struct of_device_id sbsa_gwdt_of_match[] = {
> + { .compatible = "arm,sbsa_gwdt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory
properties and resources.
> +#ifdef CONFIG_ACPI
> +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
> +{
> + int trigger, polarity;
The ACPI parts of this patch should probably be in a separate file, and
submitted as a separate patch.
This all looks like code that should be handled by the ACPI core, and
> +static int __always_inline acpi_gtdt_set_resource_irq_flags(u32 timer_flags)
> +{
> + int flags;
> +
> + switch (timer_flags &
> + (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY)) {
> + case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
> + flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;
> + pr_info("sbsa_gwdt: device irq flags is low edge");
> + break;
> + case ACPI_GTDT_INTERRUPT_MODE:
> + flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;
> + pr_info("sbsa_gwdt: device irq flags is high edge");
> + break;
> + case ACPI_GTDT_INTERRUPT_POLARITY:
> + flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;
> + pr_info("sbsa_gwdt: device irq flags is low level");
> + break;
> + default:
> + flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;
> + pr_info("sbsa_gwdt: device irq flags is high level");
> + }
> + pr_info("sbsa_gwdt: device irq flags:%d\n", flags);
> + return flags;
> +}
> +
not be driver specific.
> +
> +/* Initialize the SBSA Generic Watchdog presented in GTDT */
> +static int __init sbsa_gwdt_init(void)
> +{
> + if (acpi_disabled)
> + return 0;
> +
> + return acpi_table_parse(ACPI_SIG_GTDT, sbsa_gwdt_acpi_init);
> +}
> +
> +arch_initcall(sbsa_gwdt_init);
> +#endif
You can't have arch_initcall() and module_platform_driver() in the same
file, it prevents you from making the driver a loadable module.
> diff --git a/include/linux/sbsa_gwdt.h b/include/linux/sbsa_gwdt.h
> new file mode 100644
> index 0000000..58425ef
> --- /dev/null
> +++ b/include/linux/sbsa_gwdt.h
> +/* refresh frame */
> +#define SBSA_GWDT_WRR (0x000)
> +
> +/* control frame */
> +#define SBSA_GWDT_WCS (0x000)
> +#define SBSA_GWDT_WOR (0x008)
> +#define SBSA_GWDT_WCV_LO (0x010)
> +#define SBSA_GWDT_WCV_HI (0x014)
> +
> +/* refresh/control frame */
> +#define SBSA_GWDT_W_IIDR (0xfcc)
> +#define SBSA_GWDT_IDR (0xfd0)
> +
> +/* Watchdog Control and Status Register */
> +#define SBSA_GWDT_WCS_EN (1 << 0)
> +#define SBSA_GWDT_WCS_WS0 (1 << 1)
> +#define SBSA_GWDT_WCS_WS1 (1 << 2)
> +
Put all the register definitions into the driver itself. Only put things
in a global header that need to be shared between multiple drivers. In case
of a watchdog driver, that should be nothing.
> +#define SBSA_GWDT_WS1_KERNEL_KEEPER IS_BUILTIN(CONFIG_SBSA_GWDT_WS1_KERNEL_KEEPER)
> +
> +#define SBSA_GWD_IOCTL_BASE ('S')
> +#define SBSA_GWDIOC_GETPREACTION _IOR(SBSA_GWD_IOCTL_BASE, 0, int)
> +#define SBSA_GWDIOC_SETPREACTION _IOWR(SBSA_GWD_IOCTL_BASE, 1, int)
ioctl definitions have to be in include/uapi/linux/ so they are visible to user
space programs. Just put them into include/uapi/linux/watchdog.h and remove the
SBSA_ prefix in the command and data structure names to make it easier to reuse
these for other drivers.
Arnd