Hi Simon,
On 04/05/2015 11:31 AM, Simon Glass wrote:
Hi Gabriel,
On 1 April 2015 at 05:20, Gabriel Huau contact@huau-gabriel.fr wrote:
Hi Simon,
On 03/31/2015 07:32 PM, Simon Glass wrote:
Hi Gabriel,
On 27 February 2015 at 01:52, Bin Meng bmeng.cn@gmail.com wrote:
Hi Gabriel,
On Fri, Feb 27, 2015 at 3:54 PM, gabriel huau contact@huau-gabriel.fr wrote:
Hi Bin,
On 02/26/2015 07:30 PM, Bin Meng wrote:
Hi Gabriel,
On Thu, Feb 26, 2015 at 12:27 AM, Gabriel Huau contact@huau-gabriel.fr wrote: > Hi Bin, > > > On 02/24/2015 11:52 PM, Bin Meng wrote: >> Hi Gabriel, >> >> On Mon, Feb 16, 2015 at 5:55 AM, 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 >>> --- >>> arch/x86/cpu/baytrail/Makefile | 1 + >>> arch/x86/cpu/baytrail/gpio.c | 206 >>> +++++++++++++++ >>> arch/x86/include/asm/arch-baytrail/gpio.h | 364 >>> ++++++++++++++++++++++++++ >>> arch/x86/include/asm/arch-baytrail/iomap.h | 73 ++++++ >>> arch/x86/include/asm/arch-baytrail/irq.h | 119 +++++++++ >>> arch/x86/include/asm/arch-baytrail/irqroute.h | 67 +++++ >>> arch/x86/include/asm/arch-baytrail/pci_devs.h | 144 ++++++++++ >>> arch/x86/include/asm/arch-baytrail/pmc.h | 253 >>> ++++++++++++++++++ >>> board/intel/minnowmax/minnowmax.c | 212 >>> +++++++++++++++ >>> include/configs/minnowmax.h | 11 + >>> 10 files changed, 1450 insertions(+) >>> create mode 100644 arch/x86/cpu/baytrail/gpio.c >>> create mode 100644 arch/x86/include/asm/arch-baytrail/iomap.h >>> create mode 100644 arch/x86/include/asm/arch-baytrail/irq.h >>> create mode 100644 arch/x86/include/asm/arch-baytrail/irqroute.h >>> create mode 100644 arch/x86/include/asm/arch-baytrail/pci_devs.h >>> create mode 100644 arch/x86/include/asm/arch-baytrail/pmc.h >>> >> [snip] >> >>> diff --git a/include/configs/minnowmax.h >>> b/include/configs/minnowmax.h >>> index 823e051..738c6fa 100644 >>> --- a/include/configs/minnowmax.h >>> +++ b/include/configs/minnowmax.h >>> @@ -69,4 +69,15 @@ >>> /* Avoid a warning in the Realtek Ethernet driver */ >>> #define CONFIG_SYS_CACHELINE_SIZE 16 >>> >>> +/* >>> + * Baytrail has 3 GPIOs bank over PCI, there is no >>> + * driver at the moment so let's disable the command >>> + * and the default x86 driver to avoid any collision >>> + * with the GPIO mapping code. >>> + * @TODO: adding a baytrail-gpio driver and configure >>> + * the muxing through the device tree >>> + */ >>> +#undef CONFIG_INTEL_ICH6_GPIO >>> +#undef CONFIG_CMD_GPIO >>> + >> Why undef these two? The BayTrail SoC does support GPIO banks in the >> legacy bridge. > I might misunderstood the GPIO subsystem but I thought there was 2 > banks > available through the PCU iLB GPIO controller which contains the SCORE > and > SSUS (102 / 44 pins). > The intel_ich6_gpio has a limitation of 32 GPIOs per bank and I > thought > it > was just a different controller from the Baytrail, but if I can use it > to > control all the GPIOs + doing the IO mapping, I'll be glad to do it! I checked the BayTrail datasheet. Its GPIO is in the iLB (legacy bridge), which is the same as other IA chipset (Ivybridge, TunnelCreek, Quark). It has 4 banks in core domain and 2 banks in sus domain. So 6 banks in total. You need define 6 gpio nodes in the minnowmax board dts file. You should be able to use the existing gpio driver to configure.
Thanks for the clarification! Actually, I saw it today when I was doing some tests and I configured the 6 banks in the devices tree. I also fixed the GPIO base address to 0x48 but I got some issues like the fact I'm reading only 0 from all the registers.
Yep, the offset should be 0x48 for BayTrail.
The registers are configured to be in the IO Space (0x500), I checked the PCI configuration space to make sure that everything is enabled correctly, but I'm still missing something.
I checked the gpio driver codes, and it currently has:
/* * Okay, I guess we're looking at the right device. The actual * GPIO registers are in the PCI device's I/O space, starting * at the offset that we just read. Bit 0 indicates that it's * an I/O address, not a memory address, so mask that off. */ gpiobase = tmplong & 0xfffe;This should be changed to
gpiobase = tmplong & 0xfffc;as bit1 is the enable bit on BayTrail (Intel changes this GPIO base register again for BayTrail, sigh...)
Once I'll be able to use these GPIOs, I will update the entire patch to remove the port from Coreboot as this is not necessary.
>>> #endif /* __CONFIG_H */ >>> --
What is the next step with this patch please? It would be good to apply it to with the changes discussed.
Sorry, actually I was super busy and wasn't able to work on the minnowboard max ... I should have some time this week end. But you can go ahead and drop this patch, I will submit a new one because most of the modification are actually not needed, we can use the generic pch_gpio driver already present in u-boot with some modification/fix.
My only problem at the moment, is to find a solution to declare properly the pin muxing for every pins. I don't want to extend the pch_gpio_set* structures because we have 6 banks on the MNW ... I was thinking to use the device tree but I'm not really fan of the idea to change the pin muxing based on the device tree, it should be only a hardware description. My idea at the moment, is to do the pin muxing through functions named "gpio_set_cfg/gpio_set_pull/..." called in the board file. A little bit like the driver s5p_gpio.c and board file board/sunxi/board.c. What is your though about that?
I think pin mux is part of the hardware description - and each board can do it differently depending on how the circuit works. I believe device tree is exactly the right place.
Is there a suitable binding or do you need to make one up?
Regards, Simon
Agreed.
I was thinking to use the gpioX node and create a binding 'pch,pins', for example:
gpioa { compatible = "intel,ich6-gpio" u-boot,dm-pre-reloc; reg = <0 0x20> bank-name = "A";
pch,pins = < BYT_UART_HD_RSTB MUX_MODE0 >; }
My problem is the pad value/direction for the GPIO are in another register, so I cannot really easily do something like "BYT_UART_HD_RSTB (MUX_MODE0 | GPIO_OUTPUT)" or even "BYT_UART_HD_RSTB (MUX_MODE0 | GPIO_OUTPUT_HIGH)". I was thinking to something like this:
pch,pins = < BYT_UART_HD_RSTB_CFG (MUX_MODE0 | PULL_20K) BYT_UART_HD_RSTB_PAD (GPIO_OUTPUT_HIGH)
BYT_UART_HD_RSTC_CFG (MUX_MODE1) BYT_UART_HD_RSTC_PAD NONE
... >;
But it means that we will need to use 2 lines to defines a pin, even if the second line could define 'nothing', I'm personally fine with that, but not sure if this is the best solution.
Also, I was going to put the code in drivers/gpios/intel_ich6_gpio.c but the GPIO muxing code is called only when we run the command 'gpio', I'll will try to find a better solution as it needs to be call as early as possible.
Regards, Gabriel