| Message ID | 20181004153321.34757-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] pinctrl: geminilake: Update pin list for B0 stepping | expand |
On Thu, Oct 04, 2018 at 06:33:19PM +0300, Andy Shevchenko wrote: > Intel Gemini Lake provides vGPIO pins which are now missed from the list. > Add them here. I don't think we should expose these unless there is some real reason to do so. These pins are pretty much internal to the SoC in Gemini Lake.
On Thu, Oct 4, 2018 at 5:33 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Intel Gemini Lake provides vGPIO pins which are now missed from the list. > Add them here. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> What does the "v" in "vGPIO" stand for? Nice to expand on that in the commit message and maybe even a comment in the code. They look like they are all Bluetooth and GPS related. Mika writes: > I don't think we should expose these unless there is some real reason to > do so. These pins are pretty much internal to the SoC in Gemini Lake. In what sense are they "internal"? There are many, many precedents of SoCs exposing pins and pin groups for Bluetooth and GPS in the pin control subsystem. If people build systems, knowing how hardware engineers think, and since pins are a scarcity, they will in short time do the following: + PINCTRL_PIN(88, "vCNV_BT_UART_CTS_B"), + PINCTRL_PIN(89, "vCNV_BT_UART_RTS_B"), (...) + PINCTRL_PIN(92, "vCNV_MFUART1_CTS_B"), + PINCTRL_PIN(93, "vCNV_MFUART1_RTS_B"), (...) + PINCTRL_PIN(96, "vCNV_GNSS_UART_CTS_B"), + PINCTRL_PIN(97, "vCNV_GNSS_UART_RTS_B"), (...) + PINCTRL_PIN(100, "vLPSS_UART0_CTS_B"), + PINCTRL_PIN(101, "vLPSS_UART0_RTS_B"), (...) + PINCTRL_PIN(104, "vLPSS_UART1_CTS_B"), + PINCTRL_PIN(105, "vLPSS_UART1_RTS_B"), "Oh we don't use those CTS/RTS pins, let's reuse them for LEDs and buttons" That is what hardware engineers to 9 times out of 10. Yours, Linus Walleij
On Tue, Oct 16, 2018 at 09:25:02AM +0200, Linus Walleij wrote: > On Thu, Oct 4, 2018 at 5:33 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > Intel Gemini Lake provides vGPIO pins which are now missed from the list. > > Add them here. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > What does the "v" in "vGPIO" stand for? It stands for "virtual". > Nice to expand on that in the commit message and maybe even > a comment in the code. They look like they are all Bluetooth > and GPS related. > > Mika writes: > > > I don't think we should expose these unless there is some real reason to > > do so. These pins are pretty much internal to the SoC in Gemini Lake. > > In what sense are they "internal"? > > There are many, many precedents of SoCs exposing pins and pin groups > for Bluetooth and GPS in the pin control subsystem. If people build > systems, knowing how hardware engineers think, and since pins > are a scarcity, they will in short time do the following: These pins are not exposed externally (e.g there is no physical pin for thse), just internal so there is no way to connect anything to them. > + PINCTRL_PIN(88, "vCNV_BT_UART_CTS_B"), > + PINCTRL_PIN(89, "vCNV_BT_UART_RTS_B"), > (...) > + PINCTRL_PIN(92, "vCNV_MFUART1_CTS_B"), > + PINCTRL_PIN(93, "vCNV_MFUART1_RTS_B"), > (...) > + PINCTRL_PIN(96, "vCNV_GNSS_UART_CTS_B"), > + PINCTRL_PIN(97, "vCNV_GNSS_UART_RTS_B"), > (...) > + PINCTRL_PIN(100, "vLPSS_UART0_CTS_B"), > + PINCTRL_PIN(101, "vLPSS_UART0_RTS_B"), > (...) > + PINCTRL_PIN(104, "vLPSS_UART1_CTS_B"), > + PINCTRL_PIN(105, "vLPSS_UART1_RTS_B"), > > "Oh we don't use those CTS/RTS pins, let's reuse them for LEDs > and buttons" > > That is what hardware engineers to 9 times out of 10. I agree but in this case it is really not possible to connect anything to those pins ;-)
On Tue, Oct 16, 2018 at 9:28 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Oct 16, 2018 at 09:25:02AM +0200, Linus Walleij wrote: > > On Thu, Oct 4, 2018 at 5:33 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > Intel Gemini Lake provides vGPIO pins which are now missed from the list. > > > Add them here. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > What does the "v" in "vGPIO" stand for? > > It stands for "virtual". (...) > These pins are not exposed externally (e.g there is no physical pin for > thse), just internal so there is no way to connect anything to them. Ah but it is really pin control, because what you say is that on Gemini Lake there are several chips inside the package, and this pertains to pads bonded directly over to another chip, right? Or even a line of polysilicon over to another part of the same chip die, but I doubt that since BT and GPS would give interesting radio interference with the CPU parts (no idea how smart your silicon people are though, I think they are pretty smart so who knows!) In either case, none of that is "virtual" it's pretty physical. The electrons still pass through a medium. Virtual is a bad name for this. I guess I would still add them if they had some interesting electronic properties that you have to change though, like setting drive strength, that BlueTooth of GPS needs. I would assume that they are set up by either hardware defaults or ROM/BIOS, but you know: someone writing the ROM/BIOS will get it wrong and it needs to be reconfigured. Or reconfigured just to save power for some usecase. This always happens in my experience, I would not be surprised if some Intel customer has code for doing exactly that in their vendor tree (because I have seen so much of that stuff before). But until proven wrong, I can agree to keep this out of tree. I just wanted to give Andy some credit for exposing this, in most cases it actually makes sense to just expose all buttons. Yours, Linus Walleij
On Tue, Oct 16, 2018 at 10:12:22AM +0200, Linus Walleij wrote: > On Tue, Oct 16, 2018 at 9:28 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Oct 16, 2018 at 09:25:02AM +0200, Linus Walleij wrote: > > > On Thu, Oct 4, 2018 at 5:33 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Intel Gemini Lake provides vGPIO pins which are now missed from the list. > > > > Add them here. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > What does the "v" in "vGPIO" stand for? > > > > It stands for "virtual". > (...) > > These pins are not exposed externally (e.g there is no physical pin for > > thse), just internal so there is no way to connect anything to them. > > Ah but it is really pin control, because what you say is that on > Gemini Lake there are several chips inside the package, and this > pertains to pads bonded directly over to another chip, right? > > Or even a line of polysilicon over to another part of the same chip > die, but I doubt that since BT and GPS would give interesting > radio interference with the CPU parts (no idea how smart your > silicon people are though, I think they are pretty smart so who > knows!) I don't know how it is implemented inside the chip but I think probably something like the above. > In either case, none of that is "virtual" it's pretty physical. The > electrons still pass through a medium. Virtual is a bad name > for this. Well, I did not invent the name ;-) > I guess I would still add them if they had some interesting electronic > properties that you have to change though, like setting drive strength, > that BlueTooth of GPS needs. Yes, in that case they would be needed but there is currently no evidence that it is needed in Gemini Lake. > I would assume that they are set up by either hardware defaults > or ROM/BIOS, but you know: someone writing the ROM/BIOS > will get it wrong and it needs to be reconfigured. Or reconfigured > just to save power for some usecase. This always happens in > my experience, I would not be surprised if some Intel customer > has code for doing exactly that in their vendor tree (because I > have seen so much of that stuff before). > > But until proven wrong, I can agree to keep this out of tree. OK. > I just wanted to give Andy some credit for exposing this, in > most cases it actually makes sense to just expose all buttons. IMHO if there is a real need to expose those pins (say some pin is misconfigured by the BIOS and need to be worked around in the kernel) we should expose those. As of now there are no bug reports regarding this so I think we should keep them unexposed.
diff --git a/drivers/pinctrl/intel/pinctrl-geminilake.c b/drivers/pinctrl/intel/pinctrl-geminilake.c index 105881e5c042..189527bee47e 100644 --- a/drivers/pinctrl/intel/pinctrl-geminilake.c +++ b/drivers/pinctrl/intel/pinctrl-geminilake.c @@ -112,6 +112,37 @@ static const struct pinctrl_pin_desc glk_northwest_pins[] = { PINCTRL_PIN(77, "GPIO_212"), PINCTRL_PIN(78, "GPIO_213"), PINCTRL_PIN(79, "GPIO_214"), + PINCTRL_PIN(80, "vCNV_BTEN"), + PINCTRL_PIN(81, "vCNV_GNEN"), + PINCTRL_PIN(82, "vCNV_WFEN"), + PINCTRL_PIN(83, "vCNV_WCEN"), + PINCTRL_PIN(84, "vCNV_BT_HOST_WAKEB"), + PINCTRL_PIN(85, "vCNV_BT_IF_SELECT"), + PINCTRL_PIN(86, "vCNV_BT_UART_TXD"), + PINCTRL_PIN(87, "vCNV_BT_UART_RXD"), + PINCTRL_PIN(88, "vCNV_BT_UART_CTS_B"), + PINCTRL_PIN(89, "vCNV_BT_UART_RTS_B"), + PINCTRL_PIN(90, "vCNV_MFUART1_TXD"), + PINCTRL_PIN(91, "vCNV_MFUART1_RXD"), + PINCTRL_PIN(92, "vCNV_MFUART1_CTS_B"), + PINCTRL_PIN(93, "vCNV_MFUART1_RTS_B"), + PINCTRL_PIN(94, "vCNV_GNSS_UART_TXD"), + PINCTRL_PIN(95, "vCNV_GNSS_UART_RXD"), + PINCTRL_PIN(96, "vCNV_GNSS_UART_CTS_B"), + PINCTRL_PIN(97, "vCNV_GNSS_UART_RTS_B"), + PINCTRL_PIN(98, "vLPSS_UART0_TXD"), + PINCTRL_PIN(99, "vLPSS_UART0_RXD"), + PINCTRL_PIN(100, "vLPSS_UART0_CTS_B"), + PINCTRL_PIN(101, "vLPSS_UART0_RTS_B"), + PINCTRL_PIN(102, "vLPSS_UART1_TXD"), + PINCTRL_PIN(103, "vLPSS_UART1_RXD"), + PINCTRL_PIN(104, "vLPSS_UART1_CTS_B"), + PINCTRL_PIN(105, "vLPSS_UART1_RTS_B"), + PINCTRL_PIN(106, "vLPSS_UART2_TXD"), + PINCTRL_PIN(107, "vLPSS_UART2_RXD"), + PINCTRL_PIN(108, "vLPSS_UART2_CTS_B"), + PINCTRL_PIN(109, "vLPSS_UART2_RTS_B"), + PINCTRL_PIN(110, "vCNV_GNSS_HOST_WAKEB"), }; static const unsigned int glk_northwest_uart1_pins[] = { 26, 27, 28, 29 }; @@ -171,7 +202,7 @@ static const struct intel_function glk_northwest_functions[] = { }; static const struct intel_community glk_northwest_communities[] = { - GLK_COMMUNITY(0, 79), + GLK_COMMUNITY(0, 110), }; static const struct intel_pinctrl_soc_data glk_northwest_soc_data = { @@ -340,10 +371,18 @@ static const struct pinctrl_pin_desc glk_audio_pins[] = { PINCTRL_PIN(17, "AVS_M_DATA_1"), PINCTRL_PIN(18, "AVS_M_CLK_AB2"), PINCTRL_PIN(19, "AVS_M_DATA_2"), + PINCTRL_PIN(20, "vCNV_BT_I2S_BCLK"), + PINCTRL_PIN(21, "vCNV_BT_I2S_WS_SYNC"), + PINCTRL_PIN(22, "vCNV_BT_I2S_SDO"), + PINCTRL_PIN(23, "vCNV_BT_I2S_SDI"), + PINCTRL_PIN(24, "vAVS_I2S0_BCLK"), + PINCTRL_PIN(25, "vAVS_I2S0_WS_SYNC"), + PINCTRL_PIN(26, "vAVS_I2S0_SDO"), + PINCTRL_PIN(27, "vAVS_I2S0_SDI"), }; static const struct intel_community glk_audio_communities[] = { - GLK_COMMUNITY(0, 19), + GLK_COMMUNITY(0, 27), }; static const struct intel_pinctrl_soc_data glk_audio_soc_data = {
Intel Gemini Lake provides vGPIO pins which are now missed from the list. Add them here. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-geminilake.c | 43 +++++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-)