[U-Boot] [PATCH v5 080/101] x86: Add a generic Intel pinctrl driver

[Simon Glass]

Hi Bin,

On Fri, 29 Nov 2019 at 00:17, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Nov 25, 2019 at 12:12 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Recent Intel SoCs share a pinctrl mechanism with many common elements. Add
> > an implementation of this core functionality, allowing SoC-specific
> > drivers to avoid adding common code.
> >
> > As well as a pinctrl driver this provides a GPIO driver based on the same
> > code.
> >
> > Once other SoCs use this driver we may consider moving more properties to
> > the device tree (e.g. the community info and pad definitions).
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v5:
> > - Add function to obtain ACPI gpio number
> >
> > Changes in v4:
> > - Add a binding file
> > - Split out GPIO code from the pinctrl driver
> > - Switch over to use pinctrl for pad init/config
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  arch/x86/include/asm/intel_pinctrl.h          | 306 +++++++++
> >  arch/x86/include/asm/intel_pinctrl_defs.h     | 386 +++++++++++
> >  .../pinctrl/intel,apl-pinctrl.txt             |  39 ++
> >  drivers/pinctrl/Kconfig                       |   9 +
> >  drivers/pinctrl/Makefile                      |   1 +
> >  drivers/pinctrl/intel/Kconfig                 |   7 +
> >  drivers/pinctrl/intel/Makefile                |   5 +
> >  drivers/pinctrl/intel/pinctrl.c               | 635 ++++++++++++++++++
> >  8 files changed, 1388 insertions(+)
> >  create mode 100644 arch/x86/include/asm/intel_pinctrl.h
> >  create mode 100644 arch/x86/include/asm/intel_pinctrl_defs.h
> >  create mode 100644 doc/device-tree-bindings/pinctrl/intel,apl-pinctrl.txt
> >  create mode 100644 drivers/pinctrl/intel/Kconfig
> >  create mode 100644 drivers/pinctrl/intel/Makefile
> >  create mode 100644 drivers/pinctrl/intel/pinctrl.c
> >
> > diff --git a/arch/x86/include/asm/intel_pinctrl.h b/arch/x86/include/asm/intel_pinctrl.h
> > new file mode 100644
> > index 0000000000..72fd9246cb
> > --- /dev/null
> > +++ b/arch/x86/include/asm/intel_pinctrl.h
> > @@ -0,0 +1,306 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2017 Intel Corporation.
> > + * Copyright 2019 Google LLC
> > + *
> > + * Modified from coreboot gpio.h
> > + */
> > +
> > +#ifndef __ASM_INTEL_PINCTRL_H
> > +#define __ASM_INTEL_PINCTRL_H
>
> Is this ApolloLake specific pinctrl, or Intel common?

It's actually common to recent intel SoCs.
[..]

> > +/**
> > + * intel_pinctrl_ofdata_to_platdata() - Handle common platdata setup
> > + *
> > + * @dev: Pinctrl device
> > + * @comm: Pad community for this device
> > + * @num_cfgs: Number of configuration words for each pad
> > + * @return 0 if OK, -EDOM if @comm is NULL, other -ve value on other error
> > + */
> > +int intel_pinctrl_ofdata_to_platdata(struct udevice *dev,
> > +                                    const struct pad_community *comm,
> > +                                    int num_cfgs);
> > +
> > +/**
> > + * pinctrl_route_gpe() - set GPIO groups for the general-purpose-event blocks
> > + *
> > + * The values from PMC register GPE_CFG are passed which is then mapped to
> > + * proper groups for MISCCFG. This basically sets the MISCCFG register bits:
> > + *  dw0 = gpe0_route[11:8]. This is ACPI GPE0b.
> > + *  dw1 = gpe0_route[15:12]. This is ACPI GPE0c.
> > + *  dw2 = gpe0_route[19:16]. This is ACPI GPE0d.
>
> no GPE0a?

I guess not, but I'm no expert here.

[..]

> > +#if IS_ENABLED(INTEL_PINCTRL_DUAL_ROUTE_SUPPORT)
>
> Where is this Kconfig option defined?

It is in patch 86 as I moved things around when converting to pinctrl.
Will move it here.

>
> > +#define PAD_IRQ_CFG_DUAL_ROUTE(route1, route2, trig, inv)  \
> > +                               (PAD_CFG0_ROUTE_##route1 | \
> > +                               PAD_CFG0_ROUTE_##route2 | \
> > +                               PAD_CFG0_TRIG_##trig | \
> > +                               PAD_CFG0_RX_POL_##inv)
> > +#endif /* CONFIG_INTEL_PINCTRL_DUAL_ROUTE_SUPPORT */
> > +
> > +#define _PAD_CFG_STRUCT(__pad, __config0, __config1)   \
> > +               __pad(__config0) (__config1)
> > +
> > +#if GPIO_NUM_PAD_CFG_REGS > 2
>
> Where is this macro defined?

Actually it isn't in this series. I should remove this code.

>
> > +#define _PAD_CFG_STRUCT_3(__pad, __config0, __config1, __config2)      \
> > +       {                                       \
> > +               .pad = __pad,                   \
> > +               .pad_config[0] = __config0,     \
> > +               .pad_config[1] = __config1,     \
> > +               .pad_config[2] = __config2,     \
> > +       }
> > +#else
> > +#define _PAD_CFG_STRUCT_3(__pad, __config0, __config1, __config2)      \
> > +       _PAD_CFG_STRUCT(__pad, __config0, __config1)
> > +#endif
> > +
> > +/* Native function configuration */
> > +#define PAD_CFG_NF(pad, pull, rst, func) \
> > +       _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) | \
> > +               PAD_IOSSTATE(TX_LAST_RXE))
> > +
> > +#if IS_ENABLED(CONFIG_INTEL_GPIO_PADCFG_PADTOL)
>
> Where is this Kconfig option defined?

Patch 86 again; will move.
[..]

> > diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig
> > new file mode 100644
> > index 0000000000..727b743431
> > --- /dev/null
> > +++ b/drivers/pinctrl/intel/Kconfig
> > @@ -0,0 +1,7 @@
> > +#
> > +# Intel PINCTRL drivers
> > +#
> > +
> > +if PINCTRL_INTEL
> > +
>
> Empty?

Yes, for now. But the APL pinctrl patch adds stuff here.

[..]

> > +const struct pinctrl_ops intel_pinctrl_ops = {
>
> empty?

Yes, nothing is supported here so far, but it is not allowed to have
no operations struct with DM.

I'll hold off on the next version until you get through the last 20 patches.

Regards,
SImon