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 <cont...@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 <cont...@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.8060...@huau-gabriel.fr> >In-Reply-To: ><capnjgz2wsg+0d-wxgbwn7gvm_9trx9y+exo6e4a0xw6fax6...@mail.gmail.com> >References: <1424037328-31636-1-git-send-email-cont...@huau-gabriel.fr> ><capnjgz2wsg+0d-wxgbwn7gvm_9trx9y+exo6e4a0xw6fax6...@mail.gmail.com> >Patch-Date: Wed Feb 25 17:12:56 +0100 2015 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot