[U-Boot] unassigned-patches/133: Re: [PATCH] x86: minnowmax: add GPIO mapping support

u-boot at bugs.denx.de u-boot at bugs.denx.de
Wed Feb 25 17:15:01 CET 2015


Hi Simon,

With a little bit of delay here are the responses ... :)

On 02/17/2015 08:04 PM, Simon Glass wrote:
> Hi Gabriel,
>
> On 15 February 2015 at 14:55, Gabriel Huau <contact at huau-gabriel.fr> wrote:
>> Configure the pinctrl as it required to make some IO controllers
>> working (USB/UART/I2C/...).
>> The idea would be in the next version to modify the pch GPIO driver and
>> configure these pins through the device tree.
>>
>> These modifications are ported from the coreboot project.
>>
>> Signed-off-by: Gabriel Huau <contact at huau-gabriel.fr>
> Thanks for the patch!
>
> I have mostly nits except for one comment about register access which
> is different in U-Boot...
I read all the comments and I agree on almost all of them but I have
some questions.

>> +
>> +               /* Add correct func to GPIO pad config */
>> +               pad_conf0 = config->pad_conf0;
>> +               if (config->is_gpio) {
>> +                       if (gpio >= bank->gpio_f1_range_start &&
>> +                           gpio <= bank->gpio_f1_range_end)
>> +                               pad_conf0 |= PAD_FUNC1;
>> +                       else
>> +                               pad_conf0 |= PAD_FUNC0;
>> +               }
>> +
>> +               writel(reg + PAD_CONF0_REG, pad_conf0);
>> +               writel(reg + PAD_CONF1_REG, config->pad_conf1);
>> +               writel(reg + PAD_VAL_REG, config->pad_val);
>> +       }
>> +
>> +       if (bank->legacy_base != GP_LEGACY_BASE_NONE)
>> +               for (set = 0; set <= (bank->gpio_count - 1) / 32; ++set) {
>> +                       reg = bank->legacy_base + 0x20 * set;
>> +
>> +                       outl(use_sel[set], reg + LEGACY_USE_SEL_REG);
>> +                       outl(io_sel[set], reg + LEGACY_IO_SEL_REG);
>> +                       outl(gp_lvl[set], reg + LEGACY_GP_LVL_REG);
>> +                       outl(tpe[set], reg + LEGACY_TPE_REG);
>> +                       outl(tne[set], reg + LEGACY_TNE_REG);
>> +
>> +                       /* TS registers are WOC  */
> If you know what this comment means, please spell it out without abbreviations.

Actually, I don't know the meaning of WOC and I couldn't find a
definition in the datasheet.

>
>> +                       outl(0, reg + LEGACY_TS_REG);
>> +
>> +                       if (bank->has_wake_en)
>> +                               outl(wake_en[set], reg + LEGACY_WAKE_EN_REG);
>> +               }
>> +}
>> +
>> +static void setup_gpio_route(const struct byt_gpio_map *sus,
>> +               const struct byt_gpio_map *core)
>> +{
>> +       uint32_t route_reg = 0;
>> +       int i;
>> +
>> +       for (i = 0; i < 8; i++) {
>> +               /* SMI takes precedence and wake_en implies SCI. */
>> +               if (sus[i].smi)
>> +                       route_reg |= ROUTE_SMI << (2 * i);
>> +               else if (sus[i].sci)
>> +                       route_reg |= ROUTE_SCI << (2 * i);
>> +
>> +               if (core[i].smi)
>> +                       route_reg |= ROUTE_SMI << (2 * (i + 8));
>> +               else if (core[i].sci)
>> +                       route_reg |= ROUTE_SCI << (2 * (i + 8));
>> +       }
> What happens to route_reg after this? I don't see it get returned.
>

I will remove the code, actually it was used when the SMI was enabled.

>> +
>> +#define GPIO_LEVEL_LOW         0
>> +#define GPIO_LEVEL_HIGH                1
>> +
>> +#define GPIO_PEDGE_DISABLE     0
>> +#define GPIO_PEDGE_ENABLE      1
>> +
>> +#define GPIO_NEDGE_DISABLE     0
>> +#define GPIO_NEDGE_ENABLE      1
>> +
>> +/* config0[29] - Disable second mask */
>> +#define PAD_MASK2_DISABLE      (1 << 29)
>> +
>> +/* config0[27] - Direct Irq En */
>> +#define PAD_IRQ_EN             (1 << 27)
>> +
>> +/* config0[26] - gd_tne */
>> +#define PAD_TNE_IRQ            (1 << 26)
>> +
>> +/* config0[25] - gd_tpe */
>> +#define PAD_TPE_IRQ            (1 << 25)
>> +
>> +/* config0[24] - Gd Level */
>> +#define PAD_LEVEL_IRQ          (1 << 24)
>> +#define PAD_EDGE_IRQ           (0 << 24)
>> +
>> +/* config0[17] - Slow clkgate / glitch filter */
>> +#define PAD_SLOWGF_ENABLE      (1 << 17)
>> +
>> +/* config0[16] - Fast clkgate / glitch filter */
>> +#define PAD_FASTGF_ENABLE      (1 << 16)
>> +
>> +/* config0[15] - Hysteresis enable (inverted) */
>> +#define PAD_HYST_DISABLE       (1 << 15)
>> +#define PAD_HYST_ENABLE                (0 << 15)
>> +
>> +/* config0[14:13] - Hysteresis control */
>> +#define PAD_HYST_CTRL_DEFAULT  (2 << 13)
>> +
>> +/* config0[11] - Bypass Flop */
>> +#define PAD_FLOP_BYPASS                (1 << 11)
>> +#define PAD_FLOP_ENABLE                (0 << 11)
>> +
>> +/* config0[10:9] - Pull str */
>> +#define PAD_PU_2K              (0 << 9)
>> +#define PAD_PU_10K             (1 << 9)
>> +#define PAD_PU_20K             (2 << 9)
>> +#define PAD_PU_40K             (3 << 9)
>> +
>> +/* config0[8:7] - Pull assign */
>> +#define PAD_PULL_DISABLE       (0 << 7)
>> +#define PAD_PULL_UP            (1 << 7)
>> +#define PAD_PULL_DOWN          (2 << 7)
>> +
>> +/* config0[2:0] - Func. pin mux */
>> +#define PAD_FUNC0              0x0
>> +#define PAD_FUNC1              0x1
>> +#define PAD_FUNC2              0x2
>> +#define PAD_FUNC3              0x3
>> +#define PAD_FUNC4              0x4
>> +#define PAD_FUNC5              0x5
>> +#define PAD_FUNC6              0x6
> These could be an anonymous enum (optional)
For me, only the PAD_FUNCX could be part of an enum.
>> +
>> +/* pad config0 power-on values - We will not often want to change these */
>> +#define PAD_CONFIG0_DEFAULT    (PAD_MASK2_DISABLE     | PAD_SLOWGF_ENABLE | \
>> +                                PAD_FASTGF_ENABLE     | PAD_HYST_DISABLE | \
>> +                                PAD_HYST_CTRL_DEFAULT | PAD_FLOP_BYPASS)
> Then this could be part of the same enum, and you avoid the line continuations.

Actually, I don't really see how the enum will avoid this? Do you have
an example somewhere of what you are thinking about?

>
>> +
>> +/* pad config1 reg power-on values - Shouldn't need to change this */
>> +#define PAD_CONFIG1_DEFAULT    0x8000
>> +
>> +/* pad_val[2] - Iinenb - active low */
>> +#define PAD_VAL_INPUT_DISABLE  (1 << 2)
>> +#define PAD_VAL_INPUT_ENABLE   (0 << 2)
>> +
>> +/* pad_val[1] - Ioutenb - active low */
>> +#define PAD_VAL_OUTPUT_DISABLE (1 << 1)
>> +#define PAD_VAL_OUTPUT_ENABLE  (0 << 1)
>> +
>> +/* Input / Output state should usually be mutually exclusive */
>> +#define PAD_VAL_INPUT          (PAD_VAL_INPUT_ENABLE | PAD_VAL_OUTPUT_DISABLE)
>> +#define PAD_VAL_OUTPUT         (PAD_VAL_OUTPUT_ENABLE | PAD_VAL_INPUT_DISABLE)
>> +
>> +/* pad_val[0] - Value */
>> +#define PAD_VAL_HIGH           (1 << 0)
>> +#define PAD_VAL_LOW            (0 << 0)
>> +
>> +/* pad_val reg power-on default varies by pad, and apparently can cause issues
>> + * if not set correctly, even if the pin isn't configured as GPIO. */
>> +#define PAD_VAL_DEFAULT                PAD_VAL_INPUT
>> +
>> +/* Configure GPIOs as MMIO by default */
>> +#define GPIO_INPUT_PU_10K(_func) \
>> +       { .pad_conf0 = PAD_FUNC##_func | PAD_PU_10K | \
>> +               PAD_PULL_UP | \
>> +               PAD_CONFIG0_DEFAULT, \
>> +         .pad_conf1 = PAD_CONFIG1_DEFAULT, \
>> +         .pad_val   = PAD_VAL_INPUT, \
>> +         .use_sel   = GPIO_USE_MMIO, \
>> +         .is_gpio   = 1 }
> I'm not a big fan of this sort of thing- #defines for structures in
> header files. It feels pretty ugly?
>
> I wonder if there is another way of doing it?
>
I agree, it's ugly, but I don't see any other 'clean' way to that. I
believe with the device tree support we should be able to configure
everything outside of this file.

>> +#define PIRQA                          0
>> +#define PIRQB                          1
>> +#define PIRQC                          2
>> +#define PIRQD                          3
>> +#define PIRQE                          4
>> +#define PIRQF                          5
>> +#define PIRQG                          6
>> +#define PIRQH                          7
>> +
>> +/* These registers live behind the ILB_BASE_ADDRESS */
> What what are they?
It was used for the PCI IRQ routing, but I think I can drop these
modifications, we don't really need this in this patch.
>
>> +#define ACTL                           0x00
>> +# define SCIS_MASK                             0x07
>> +# define SCIS_IRQ9                             0x00
>> +# define SCIS_IRQ10                            0x01
>> +# define SCIS_IRQ11                            0x02
>> +# define SCIS_IRQ20                            0x04
>> +# define SCIS_IRQ21                            0x05
>> +# define SCIS_IRQ22                            0x06
>> +# define SCIS_IRQ23                            0x07
>> +
>> +#endif /* _BAYTRAIL_IRQ_H_ */
>> diff --git a/arch/x86/include/asm/arch-baytrail/irqroute.h b/arch/x86/include/asm/arch-baytrail/irqroute.h
>> new file mode 100644
>> index 0000000..f129880
>> --- /dev/null
>> +++ b/arch/x86/include/asm/arch-baytrail/irqroute.h
>> @@ -0,0 +1,67 @@
>> +/*
>> + * From Coreboot file of same name
>> + *
>> + * Copyright (C) 2014 Google, Inc
>> + * Copyright (C) 2014 Sage Electronic Engineering, LLC.
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0
>> + */
>> +
>> +#ifndef IRQROUTE_H
>> +#define IRQROUTE_H
>> +
>> +#include <asm/arch/irq.h>
>> +#include <asm/arch/pci_devs.h>
>> +
>> +/*
>> + *IR02h GFX      INT(A)                - PIRQ A
>> + *IR10h EMMC    INT(ABCD)      - PIRQ DEFG
>> + *IR11h SDIO     INT(A)                - PIRQ B
>> + *IR12h SD       INT(A)                - PIRQ C
>> + *IR13h SATA     INT(A)                - PIRQ D
>> + *IR14h XHCI     INT(A)                - PIRQ E
>> + *IR15h LP Audio INT(A)                - PIRQ F
>> + *IR17h MMC      INT(A)                - PIRQ F
>> + *IR18h SIO      INT(ABCD)     - PIRQ BADC
>> + *IR1Ah TXE      INT(A)                - PIRQ F
>> + *IR1Bh HD Audio INT(A)                - PIRQ G
>> + *IR1Ch PCIe     INT(ABCD)     - PIRQ EFGH
>> + *IR1Dh EHCI     INT(A)                - PIRQ D
>> + *IR1Eh SIO      INT(ABCD)     - PIRQ BDEF
>> + *IR1Fh LPC      INT(ABCD)     - PIRQ HGBC
>> + */
>> +#define PCI_DEV_PIRQ_ROUTES \
>> +       PCI_DEV_PIRQ_ROUTE(GFX_DEV,    A, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(EMMC_DEV,   D, E, F, G), \
>> +       PCI_DEV_PIRQ_ROUTE(SDIO_DEV,   B, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(SD_DEV,     C, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(SATA_DEV,   D, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(XHCI_DEV,   E, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(LPE_DEV,    F, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(MMC45_DEV,  F, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(SIO1_DEV,   B, A, D, C), \
>> +       PCI_DEV_PIRQ_ROUTE(TXE_DEV,    F, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(HDA_DEV,    G, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(PCIE_DEV,   E, F, G, H), \
>> +       PCI_DEV_PIRQ_ROUTE(EHCI_DEV,   D, A, A, A), \
>> +       PCI_DEV_PIRQ_ROUTE(SIO2_DEV,   B, D, E, F), \
>> +       PCI_DEV_PIRQ_ROUTE(PCU_DEV,    H, G, B, C)
>> +
> Is this actually used? In general I think this sort of monstrosity is
> better off in a C file.

It was when I was doing the IRQ routing, but I dropped the function as
this is not necessary at the moment, I will remove it.

Thanks,
Regards,
Gabriel

---
Added to GNATS database as unassigned-patches/133
>Responsible:    patch-coord
>Message-Id:     <54EDF488.8060102 at huau-gabriel.fr>
>In-Reply-To:    <CAPnjgZ2wsg+0d-WXgbwN7Gvm_9TRX9y+Exo6e4a0xW6FAx6_AA at mail.gmail.com>
>References:     <1424037328-31636-1-git-send-email-contact at huau-gabriel.fr> <CAPnjgZ2wsg+0d-WXgbwN7Gvm_9TRX9y+Exo6e4a0xW6FAx6_AA at mail.gmail.com>
>Patch-Date:     Wed Feb 25 17:12:56 +0100 2015



More information about the U-Boot mailing list