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

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@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@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@huau-gabriel.fr In-Reply-To: CAPnjgZ2wsg+0d-WXgbwN7Gvm_9TRX9y+Exo6e4a0xW6FAx6_AA@mail.gmail.com References: 1424037328-31636-1-git-send-email-contact@huau-gabriel.fr CAPnjgZ2wsg+0d-WXgbwN7Gvm_9TRX9y+Exo6e4a0xW6FAx6_AA@mail.gmail.com Patch-Date: Wed Feb 25 17:12:56 +0100 2015