[U-Boot] [PATCH 07/25] x86: Add a simple superio driver for SMSC LPC47M

[Bin Meng]

Hi Simon,

On Fri, Dec 5, 2014 at 6:37 AM, Simon Glass <sjg at chromium.org> wrote:
> On 4 December 2014 at 08:01, Bin Meng <bmeng.cn at gmail.com> wrote:
>> On most x86 boards, the legacy serial ports (io address 0x3f8/0x2f8)
>> are provided by a superio chip connected to the LPC bus. We must
>> program the superio chip so that serial ports are available for us.
>>
>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>
> Acked-by: Simon Glass <sjg at chromium.org>
>
> But please see bits below.
>
>> ---
>>  arch/x86/include/asm/pnp_def.h | 83 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/misc/Makefile          |  1 +
>>  drivers/misc/smsc_lpc47m.c     | 31 ++++++++++++++++
>>  include/smsc_lpc47m.h          | 19 ++++++++++
>>  4 files changed, 134 insertions(+)
>>  create mode 100644 arch/x86/include/asm/pnp_def.h
>>  create mode 100644 drivers/misc/smsc_lpc47m.c
>>  create mode 100644 include/smsc_lpc47m.h
>>
>> diff --git a/arch/x86/include/asm/pnp_def.h b/arch/x86/include/asm/pnp_def.h
>> new file mode 100644
>> index 0000000..a16e993
>> --- /dev/null
>> +++ b/arch/x86/include/asm/pnp_def.h
>> @@ -0,0 +1,83 @@
>> +/*
>> + * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
>> + *
>> + * Adapted from coreboot src/include/device/pnp_def.h
>> + * and arch/x86/include/arch/io.h
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef _ASM_PNP_DEF_H_
>> +#define _ASM_PNP_DEF_H_
>> +
>> +#include <asm/io.h>
>> +
>> +#define PNP_IDX_EN   0x30
>> +#define PNP_IDX_IO0  0x60
>> +#define PNP_IDX_IO1  0x62
>> +#define PNP_IDX_IO2  0x64
>> +#define PNP_IDX_IO3  0x66
>> +#define PNP_IDX_IRQ0 0x70
>> +#define PNP_IDX_IRQ1 0x72
>> +#define PNP_IDX_DRQ0 0x74
>> +#define PNP_IDX_DRQ1 0x75
>> +#define PNP_IDX_MSC0 0xf0
>> +#define PNP_IDX_MSC1 0xf1
>> +
>> +/* Generic functions for pnp devices */
>> +
>> +#define PNP_DEV(PORT, FUNC) (((PORT) << 8) | (FUNC))
>> +
>> +static inline void pnp_write_config(uint32_t dev, uint8_t reg, uint8_t value)
>
> What is dev? Some sort of integer? How about a comment on PNP_DEV or here.

Sure. Will add a comment to explain dev.

>> +{
>> +       unsigned port = dev >> 8;
>
> Blank line between decls and code

OK.

>> +       outb(reg, port);
>> +       outb(value, port + 1);
>> +}
>> +
>> +static inline uint8_t pnp_read_config(uint32_t dev, uint8_t reg)
>> +{
>> +       unsigned port = dev >> 8;
>
> and here, etc.

OK.

>> +       outb(reg, port);
>> +       return inb(port + 1);
>> +}
>> +
>> +static inline void pnp_set_logical_device(uint32_t dev)
>> +{
>> +       unsigned device = dev & 0xff;
>> +       pnp_write_config(dev, 0x07, device);
>> +}
>> +
>> +static inline void pnp_set_enable(uint32_t dev, int enable)
>> +{
>> +       pnp_write_config(dev, 0x30, enable ? 1 : 0);
>> +}
>> +
>> +static inline int pnp_read_enable(uint32_t dev)
>> +{
>> +       return !!pnp_read_config(dev, 0x30);
>> +}
>> +
>> +static inline void pnp_set_iobase(uint32_t dev, unsigned index, unsigned iobase)
>> +{
>> +       pnp_write_config(dev, index + 0, (iobase >> 8) & 0xff);
>> +       pnp_write_config(dev, index + 1, iobase & 0xff);
>> +}
>> +
>> +static inline uint16_t pnp_read_iobase(uint32_t dev, unsigned index)
>> +{
>> +       return ((uint16_t)(pnp_read_config(dev, index)) << 8) |
>> +               pnp_read_config(dev, index + 1);
>> +}
>> +
>> +static inline void pnp_set_irq(uint32_t dev, unsigned index, unsigned irq)
>> +{
>> +       pnp_write_config(dev, index, irq);
>> +}
>> +
>> +static inline void pnp_set_drq(uint32_t dev, unsigned index, unsigned drq)
>> +{
>> +       pnp_write_config(dev, index, drq & 0xff);
>> +}
>> +
>> +#endif /* _ASM_PNP_DEF_H_ */
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 2f2e48f..eb57497 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_MXC_OCOTP) += mxc_ocotp.o
>>  obj-$(CONFIG_MXS_OCOTP) += mxs_ocotp.o
>>  obj-$(CONFIG_NS87308) += ns87308.o
>>  obj-$(CONFIG_PDSP188x) += pdsp188x.o
>> +obj-$(CONFIG_SMSC_LPC47M) += smsc_lpc47m.o
>>  obj-$(CONFIG_STATUS_LED) += status_led.o
>>  obj-$(CONFIG_TWL4030_LED) += twl4030_led.o
>>  obj-$(CONFIG_FSL_IFC) += fsl_ifc.o
>> diff --git a/drivers/misc/smsc_lpc47m.c b/drivers/misc/smsc_lpc47m.c
>> new file mode 100644
>> index 0000000..08c627d
>> --- /dev/null
>> +++ b/drivers/misc/smsc_lpc47m.c
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/io.h>
>> +#include <asm/pnp_def.h>
>> +
>> +static void pnp_enter_conf_state(u16 dev)
>> +{
>> +       u16 port = dev >> 8;
>
> here too...

OK

>> +       outb(0x55, port);
>> +}
>> +
>> +static void pnp_exit_conf_state(u16 dev)
>> +{
>> +       u16 port = dev >> 8;
>> +       outb(0xaa, port);
>> +}
>> +
>> +void lpc47m_enable_serial(u16 dev, u16 iobase)
>> +{
>> +       pnp_enter_conf_state(dev);
>> +       pnp_set_logical_device(dev);
>> +       pnp_set_enable(dev, 0);
>> +       pnp_set_iobase(dev, PNP_IDX_IO0, iobase);
>> +       pnp_set_enable(dev, 1);
>> +       pnp_exit_conf_state(dev);
>> +}
>> diff --git a/include/smsc_lpc47m.h b/include/smsc_lpc47m.h
>> new file mode 100644
>> index 0000000..4b11adc
>> --- /dev/null
>> +++ b/include/smsc_lpc47m.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef _SMSC_LPC47M_H_
>> +#define _SMSC_LPC47M_H_
>> +
>> +/**
>> + * Configure the base I/O port of the specified serial device and enable the
>> + * serial device.
>> + *
>> + * @param dev High 8 bits = Super I/O port, low 8 bits = logical device number.
>> + * @param iobase Processor I/O port address to assign to this serial device.
>
> @dev: High 8 bits...
>
> I know we use @param dev in various places, but should use the docbook
> format now.

Will change in v2.

Regards,
Bin