[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