[U-Boot] [PATCH] misc: Add simple driver for some Nuvoton NCT6102D devices

[Stefan Roese <sr@denx.de>]

Hi Bin,

On 13.07.2016 11:45, Bin Meng wrote:
> On Wed, Jul 13, 2016 at 2:03 PM, Stefan Roese <sr@denx.de> wrote:
>> This simple driver provides some functions to control some of the
>> integrated devices. The watchdog is enabled per default. This driver
>> adds a function to disable the watchdog. Also the internal legacy
>> UART (io address 0x3f8/0x2f8) is enabled per default.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>   drivers/misc/Kconfig            |  8 ++++++
>>   drivers/misc/Makefile           |  1 +
>>   drivers/misc/nuvoton_nct6102d.c | 57 +++++++++++++++++++++++++++++++++++++++++
>>   include/nuvoton_nct6102d.h      | 29 +++++++++++++++++++++
>>   4 files changed, 95 insertions(+)
>>   create mode 100644 drivers/misc/nuvoton_nct6102d.c
>>   create mode 100644 include/nuvoton_nct6102d.h
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 2373037..5d212a3 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -90,6 +90,14 @@ config MXC_OCOTP
>>            Programmable memory pages that are stored on the some
>>            Freescale i.MX processors.
>>
>> +config NUVOTON_NCT6102D
>> +       bool "Enable Nuvoton NCT6102D Super I/O driver"
>> +       help
>> +         If you say Y here, you will get support for the Nuvoton
>> +         NCT6102D Super I/O driver. This can be used to enable or
>> +         disable the legacy UART, the watchdog or other devices
>> +         in the Nuvoton Super IO chips on X86 platforms.
>> +
>>   config PWRSEQ
>>          bool "Enable power-sequencing drivers"
>>          depends on DM
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 066639b..1b0c7b6 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_I2C_EEPROM) += i2c_eeprom.o
>>   obj-$(CONFIG_FSL_MC9SDZ60) += mc9sdz60.o
>>   obj-$(CONFIG_MXC_OCOTP) += mxc_ocotp.o
>>   obj-$(CONFIG_MXS_OCOTP) += mxs_ocotp.o
>> +obj-$(CONFIG_NUVOTON_NCT6102D) += nuvoton_nct6102d.o
>>   obj-$(CONFIG_NS87308) += ns87308.o
>>   obj-$(CONFIG_PDSP188x) += pdsp188x.o
>>   obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o
>> diff --git a/drivers/misc/nuvoton_nct6102d.c b/drivers/misc/nuvoton_nct6102d.c
>> new file mode 100644
>> index 0000000..38355db
>> --- /dev/null
>> +++ b/drivers/misc/nuvoton_nct6102d.c
>> @@ -0,0 +1,57 @@
>> +/*
>> + * Copyright (C) 2016 Stefan Roese <sr@denx.de>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <nuvoton_nct6102d.h>
>> +#include <asm/io.h>
>> +#include <asm/pnp_def.h>
>> +
>> +static void superio_outb(int reg, int val)
>> +{
>> +       outb(reg, WDT_EFER);
>> +       outb(val, WDT_EFDR);
>> +}
>> +
>> +static inline int superio_inb(int reg)
>> +{
>> +       outb(reg, WDT_EFER);
>> +       return inb(WDT_EFDR);
>> +}
>> +
>> +static int superio_enter(void)
>> +{
>> +       outb(0x87, WDT_EFER); /* Enter extended function mode */
>
> Can we use a macro for 0x87 if possible?

Sure. Will change in v2.

>> +       outb(0x87, WDT_EFER); /* Again according to manual */
>> +
>> +       return 0;
>> +}
>> +
>> +static void superio_select(int ld)
>> +{
>> +       superio_outb(0x07, ld);
>
> Can we use a macro for 0x07 if possible?

Okay.

>> +}
>> +
>> +static void superio_exit(void)
>> +{
>> +       outb(0xAA, WDT_EFER); /* Leave extended function mode */
>
> Can we use a macro for 0xaa (lower case) if possible?

Okay.

>> +}
>> +
>> +/*
>> + * The Nuvoton NCT6102D starts per default after reset with both,
>> + * the internal watchdog and the internal legacy UART enabled. This
>> + * code provides functions to disable the watchdog and the UART
>> + * if this is needed on platforms.
>> + */
>> +int nct6102d_wdt_disable(void)
>> +{
>> +       superio_enter();
>> +       /* Select logical device for WDT */
>> +       superio_select(NCT6102D_LD_WDT);
>> +       superio_outb(NCT6102D_WDT_TIMEOUT, 0x00);
>> +       superio_exit();
>> +
>> +       return 0;
>> +}
>> diff --git a/include/nuvoton_nct6102d.h b/include/nuvoton_nct6102d.h
>> new file mode 100644
>> index 0000000..297cef4
>> --- /dev/null
>> +++ b/include/nuvoton_nct6102d.h
>> @@ -0,0 +1,29 @@
>> +/*
>> + * Copyright (C) 2016 Stefan Roese <sr@denx.de>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef _NUVOTON_NCT6102D_H_
>> +#define _NUVOTON_NCT6102D_H_
>> +
>> +/* I/O address of Nuvoton Super IO chip */
>> +#define NCT6102D_IO_PORT       0x4e
>
> Is this I/O address configurable at several predefined address? eg:
> can be 0x4e, or 0x6e depending on some I/O strap pins. If it is
> configurable, maybe we need add a parameter for the APIs that this
> driver provides for the caller to pass the I/O address.

Other similar Super IO chips may have different IO addresses, yes.
Not sure if this Nuvoton can be configured to a different address.
We could add some sort of autodetection, similar to the one
implemented in the Linux watchdog driver I've cloned most of this
"driver" from (drivers/watchdog/w83627hf_wdt.c). I would vote to
postpone such a change to a later date when this multiple device
support is needed.

>> +
>> +/* Extended Function Enable Registers */
>> +#define WDT_EFER (NCT6102D_IO_PORT + 0)
>
> WDT_ prefix looks odd to me? Should it be NCT_?

Makes sense, yes.

>> +/* Extended Function Index Register (same as EFER) */
>> +#define WDT_EFIR (NCT6102D_IO_PORT + 0)
>> +/* Extended Function Data Register */
>> +#define WDT_EFDR (WDT_EFIR + 1)
>> +
>> +/* Logical device number */
>> +#define NCT6102D_LD_UARTA      0x02
>
> Is this needed?

No. Its a remnant from my UART tests. I could remove it in v2 but it
might be helpful if somebody want to work on this driver for UART
support.

>> +#define NCT6102D_LD_WDT                0x08
>> +
>> +#define NCT6102D_UARTA_ENABLE  0x30
>
> Is this needed?

Again, I can remove it in v2 if thats preferred.

Thanks for the review.

Thanks,
Stefan