[PATCH 1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

[PATCH 1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

[Hans de Goede]

For all the "score" pin-groups all the intel_pingroup-s to select
the non GPIO function are re-used for byt_score_gpio_groups[].

But this is incorrect since a pin-group includes the mode setting,
which for the non GPIO functions generally is 1, where as to select
the GPIO function mode must be set to 0.

So the GPIO function needs separate intel_pingroup-s with their own mode
value of 0.

Add foo_gpio entries for each function to byt_score_groups[] and make all
the byt_score_gpio_groups[] entries point to these instead to fix this.

The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
baytrail: Add pinconf group + function for the pmu_clk") added support for
the pmu_clk pins following the broken "score" model.

Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
in byt_sus_gpio_groups[] to fix this.

Fixes: 2f46d7f7e959 ("pinctrl: baytrail: Add pinconf group + function for the pmu_clk")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 45 ++++++++++++++++++++----
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 7865ef587788..019a886a37ae 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -279,32 +279,59 @@ static const unsigned int byt_score_smbus_pins[] = { 51, 52, 53 };
 
 static const struct intel_pingroup byt_score_groups[] = {
 	PIN_GROUP("uart1_grp", byt_score_uart1_pins, 1),
+	PIN_GROUP("uart1_grp_gpio", byt_score_uart1_pins, 0),
 	PIN_GROUP("uart2_grp", byt_score_uart2_pins, 1),
+	PIN_GROUP("uart2_grp_gpio", byt_score_uart2_pins, 0),
 	PIN_GROUP("pwm0_grp", byt_score_pwm0_pins, 1),
+	PIN_GROUP("pwm0_grp_gpio", byt_score_pwm0_pins, 0),
 	PIN_GROUP("pwm1_grp", byt_score_pwm1_pins, 1),
+	PIN_GROUP("pwm1_grp_gpio", byt_score_pwm1_pins, 0),
 	PIN_GROUP("ssp2_grp", byt_score_ssp2_pins, 1),
+	PIN_GROUP("ssp2_grp_gpio", byt_score_ssp2_pins, 0),
 	PIN_GROUP("sio_spi_grp", byt_score_sio_spi_pins, 1),
+	PIN_GROUP("sio_spi_grp_gpio", byt_score_sio_spi_pins, 0),
 	PIN_GROUP("i2c5_grp", byt_score_i2c5_pins, 1),
+	PIN_GROUP("i2c5_grp_gpio", byt_score_i2c5_pins, 0),
 	PIN_GROUP("i2c6_grp", byt_score_i2c6_pins, 1),
+	PIN_GROUP("i2c6_grp_gpio", byt_score_i2c6_pins, 0),
 	PIN_GROUP("i2c4_grp", byt_score_i2c4_pins, 1),
+	PIN_GROUP("i2c4_grp_gpio", byt_score_i2c4_pins, 0),
 	PIN_GROUP("i2c3_grp", byt_score_i2c3_pins, 1),
+	PIN_GROUP("i2c3_grp_gpio", byt_score_i2c3_pins, 0),
 	PIN_GROUP("i2c2_grp", byt_score_i2c2_pins, 1),
+	PIN_GROUP("i2c2_grp_gpio", byt_score_i2c2_pins, 0),
 	PIN_GROUP("i2c1_grp", byt_score_i2c1_pins, 1),
+	PIN_GROUP("i2c1_grp_gpio", byt_score_i2c1_pins, 0),
 	PIN_GROUP("i2c0_grp", byt_score_i2c0_pins, 1),
+	PIN_GROUP("i2c0_grp_gpio", byt_score_i2c0_pins, 0),
 	PIN_GROUP("ssp0_grp", byt_score_ssp0_pins, 1),
+	PIN_GROUP("ssp0_grp_gpio", byt_score_ssp0_pins, 0),
 	PIN_GROUP("ssp1_grp", byt_score_ssp1_pins, 1),
+	PIN_GROUP("ssp1_grp_gpio", byt_score_ssp1_pins, 0),
 	PIN_GROUP("sdcard_grp", byt_score_sdcard_pins, byt_score_sdcard_mux_values),
+	PIN_GROUP("sdcard_grp_gpio", byt_score_sdcard_pins, 0),
 	PIN_GROUP("sdio_grp", byt_score_sdio_pins, 1),
+	PIN_GROUP("sdio_grp_gpio", byt_score_sdio_pins, 0),
 	PIN_GROUP("emmc_grp", byt_score_emmc_pins, 1),
+	PIN_GROUP("emmc_grp_gpio", byt_score_emmc_pins, 0),
 	PIN_GROUP("lpc_grp", byt_score_ilb_lpc_pins, 1),
+	PIN_GROUP("lpc_grp_gpio", byt_score_ilb_lpc_pins, 0),
 	PIN_GROUP("sata_grp", byt_score_sata_pins, 1),
+	PIN_GROUP("sata_grp_gpio", byt_score_sata_pins, 0),
 	PIN_GROUP("plt_clk0_grp", byt_score_plt_clk0_pins, 1),
+	PIN_GROUP("plt_clk0_grp_gpio", byt_score_plt_clk0_pins, 0),
 	PIN_GROUP("plt_clk1_grp", byt_score_plt_clk1_pins, 1),
+	PIN_GROUP("plt_clk1_grp_gpio", byt_score_plt_clk1_pins, 0),
 	PIN_GROUP("plt_clk2_grp", byt_score_plt_clk2_pins, 1),
+	PIN_GROUP("plt_clk2_grp_gpio", byt_score_plt_clk2_pins, 0),
 	PIN_GROUP("plt_clk3_grp", byt_score_plt_clk3_pins, 1),
+	PIN_GROUP("plt_clk3_grp_gpio", byt_score_plt_clk3_pins, 0),
 	PIN_GROUP("plt_clk4_grp", byt_score_plt_clk4_pins, 1),
+	PIN_GROUP("plt_clk4_grp_gpio", byt_score_plt_clk4_pins, 0),
 	PIN_GROUP("plt_clk5_grp", byt_score_plt_clk5_pins, 1),
+	PIN_GROUP("plt_clk5_grp_gpio", byt_score_plt_clk5_pins, 0),
 	PIN_GROUP("smbus_grp", byt_score_smbus_pins, 1),
+	PIN_GROUP("smbus_grp_gpio", byt_score_smbus_pins, 0),
 };
 
 static const char * const byt_score_uart_groups[] = {
@@ -332,12 +359,14 @@ static const char * const byt_score_plt_clk_groups[] = {
 };
 static const char * const byt_score_smbus_groups[] = { "smbus_grp" };
 static const char * const byt_score_gpio_groups[] = {
-	"uart1_grp", "uart2_grp", "pwm0_grp", "pwm1_grp", "ssp0_grp",
-	"ssp1_grp", "ssp2_grp", "sio_spi_grp", "i2c0_grp", "i2c1_grp",
-	"i2c2_grp", "i2c3_grp", "i2c4_grp", "i2c5_grp", "i2c6_grp",
-	"sdcard_grp", "sdio_grp", "emmc_grp", "lpc_grp", "sata_grp",
-	"plt_clk0_grp", "plt_clk1_grp", "plt_clk2_grp", "plt_clk3_grp",
-	"plt_clk4_grp", "plt_clk5_grp", "smbus_grp",
+	"uart1_grp_gpio", "uart2_grp_gpio", "pwm0_grp_gpio",
+	"pwm1_grp_gpio", "ssp0_grp_gpio", "ssp1_grp_gpio", "ssp2_grp_gpio",
+	"sio_spi_grp_gpio", "i2c0_grp_gpio", "i2c1_grp_gpio", "i2c2_grp_gpio",
+	"i2c3_grp_gpio", "i2c4_grp_gpio", "i2c5_grp_gpio", "i2c6_grp_gpio",
+	"sdcard_grp_gpio", "sdio_grp_gpio", "emmc_grp_gpio", "lpc_grp_gpio",
+	"sata_grp_gpio", "plt_clk0_grp_gpio", "plt_clk1_grp_gpio",
+	"plt_clk2_grp_gpio", "plt_clk3_grp_gpio", "plt_clk4_grp_gpio",
+	"plt_clk5_grp_gpio", "smbus_grp_gpio",
 };
 
 static const struct intel_function byt_score_functions[] = {
@@ -457,7 +486,9 @@ static const struct intel_pingroup byt_sus_groups[] = {
 	PIN_GROUP("usb_ulpi_grp_gpio", byt_sus_usb_ulpi_pins, byt_sus_usb_ulpi_gpio_mode_values),
 	PIN_GROUP("pcu_spi_grp_gpio", byt_sus_pcu_spi_pins, byt_sus_pcu_spi_gpio_mode_values),
 	PIN_GROUP("pmu_clk1_grp", byt_sus_pmu_clk1_pins, 1),
+	PIN_GROUP("pmu_clk1_grp_gpio", byt_sus_pmu_clk1_pins, 0),
 	PIN_GROUP("pmu_clk2_grp", byt_sus_pmu_clk2_pins, 1),
+	PIN_GROUP("pmu_clk2_grp_gpio", byt_sus_pmu_clk2_pins, 0),
 };
 
 static const char * const byt_sus_usb_groups[] = {
@@ -469,7 +500,7 @@ static const char * const byt_sus_pmu_clk_groups[] = {
 };
 static const char * const byt_sus_gpio_groups[] = {
 	"usb_oc_grp_gpio", "usb_ulpi_grp_gpio", "pcu_spi_grp_gpio",
-	"pmu_clk1_grp", "pmu_clk2_grp",
+	"pmu_clk1_grp_gpio", "pmu_clk2_grp_gpio",
 };
 
 static const struct intel_function byt_sus_functions[] = {
-- 
2.44.0

[PATCH 2/2] pinctrl: baytrail: Add pinconf group for uart3

[Hans de Goede]

GPIO_S0_SC57 / GPIO_S0_SC61 can be muxed to PCU_UART_TXD / PCU_UART_RXD,
add a pinconf group for this.

On Bay Trail board schematics using these pins as UART these are
called UART3_TXD / UART3_RXD, name the pinconf group "uart3_grp"
to be consistent with the schematics.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 019a886a37ae..bd2363355a60 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -231,6 +231,7 @@ static const unsigned int byt_score_pins_map[BYT_NGPIO_SCORE] = {
 /* SCORE groups */
 static const unsigned int byt_score_uart1_pins[] = { 70, 71, 72, 73 };
 static const unsigned int byt_score_uart2_pins[] = { 74, 75, 76, 77 };
+static const unsigned int byt_score_uart3_pins[] = { 57, 61 };
 
 static const unsigned int byt_score_pwm0_pins[] = { 94 };
 static const unsigned int byt_score_pwm1_pins[] = { 95 };
@@ -282,6 +283,8 @@ static const struct intel_pingroup byt_score_groups[] = {
 	PIN_GROUP("uart1_grp_gpio", byt_score_uart1_pins, 0),
 	PIN_GROUP("uart2_grp", byt_score_uart2_pins, 1),
 	PIN_GROUP("uart2_grp_gpio", byt_score_uart2_pins, 0),
+	PIN_GROUP("uart3_grp", byt_score_uart3_pins, 1),
+	PIN_GROUP("uart3_grp_gpio", byt_score_uart3_pins, 0),
 	PIN_GROUP("pwm0_grp", byt_score_pwm0_pins, 1),
 	PIN_GROUP("pwm0_grp_gpio", byt_score_pwm0_pins, 0),
 	PIN_GROUP("pwm1_grp", byt_score_pwm1_pins, 1),
@@ -335,7 +338,7 @@ static const struct intel_pingroup byt_score_groups[] = {
 };
 
 static const char * const byt_score_uart_groups[] = {
-	"uart1_grp", "uart2_grp",
+	"uart1_grp", "uart2_grp", "uart3_grp",
 };
 static const char * const byt_score_pwm_groups[] = {
 	"pwm0_grp", "pwm1_grp",
@@ -359,7 +362,7 @@ static const char * const byt_score_plt_clk_groups[] = {
 };
 static const char * const byt_score_smbus_groups[] = { "smbus_grp" };
 static const char * const byt_score_gpio_groups[] = {
-	"uart1_grp_gpio", "uart2_grp_gpio", "pwm0_grp_gpio",
+	"uart1_grp_gpio", "uart2_grp_gpio", "uart3_grp_gpio", "pwm0_grp_gpio",
 	"pwm1_grp_gpio", "ssp0_grp_gpio", "ssp1_grp_gpio", "ssp2_grp_gpio",
 	"sio_spi_grp_gpio", "i2c0_grp_gpio", "i2c1_grp_gpio", "i2c2_grp_gpio",
 	"i2c3_grp_gpio", "i2c4_grp_gpio", "i2c5_grp_gpio", "i2c6_grp_gpio",
-- 
2.44.0

Re: [PATCH 1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

[Andy Shevchenko]

On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> For all the "score" pin-groups all the intel_pingroup-s to select
> the non GPIO function are re-used for byt_score_gpio_groups[].
>
> But this is incorrect since a pin-group includes the mode setting,
> which for the non GPIO functions generally is 1, where as to select
> the GPIO function mode must be set to 0.
>
> So the GPIO function needs separate intel_pingroup-s with their own mode
> value of 0.
>
> Add foo_gpio entries for each function to byt_score_groups[] and make all
> the byt_score_gpio_groups[] entries point to these instead to fix this.
>
> The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
> baytrail: Add pinconf group + function for the pmu_clk") added support for
> the pmu_clk pins following the broken "score" model.
>
> Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
> in byt_sus_gpio_groups[] to fix this.

I'm wondering if it's possible to add some code to imply all these. I
mean to have a comparator to the _gpio in the naming and generate them
at runtime and add. In this case if we add / modify the original one
the rest (for _gpio cases) will be done automatically.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pinctrl: baytrail: Add pinconf group for uart3

[Andy Shevchenko]

On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> GPIO_S0_SC57 / GPIO_S0_SC61 can be muxed to PCU_UART_TXD / PCU_UART_RXD,
> add a pinconf group for this.
>
> On Bay Trail board schematics using these pins as UART these are
> called UART3_TXD / UART3_RXD, name the pinconf group "uart3_grp"
> to be consistent with the schematics.

...

> +       PIN_GROUP("uart3_grp_gpio", byt_score_uart3_pins, 0),

>  static const char * const byt_score_gpio_groups[] = {
> -       "uart1_grp_gpio", "uart2_grp_gpio", "pwm0_grp_gpio",
> +       "uart1_grp_gpio", "uart2_grp_gpio", "uart3_grp_gpio", "pwm0_grp_gpio",
>         "pwm1_grp_gpio", "ssp0_grp_gpio", "ssp1_grp_gpio", "ssp2_grp_gpio",
>         "sio_spi_grp_gpio", "i2c0_grp_gpio", "i2c1_grp_gpio", "i2c2_grp_gpio",
>         "i2c3_grp_gpio", "i2c4_grp_gpio", "i2c5_grp_gpio", "i2c6_grp_gpio",

The above parts won't be needed if we have automatic _gpio generation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

[Hans de Goede]

Hi,

On 4/6/24 3:52 PM, Andy Shevchenko wrote:
> On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> For all the "score" pin-groups all the intel_pingroup-s to select
>> the non GPIO function are re-used for byt_score_gpio_groups[].
>>
>> But this is incorrect since a pin-group includes the mode setting,
>> which for the non GPIO functions generally is 1, where as to select
>> the GPIO function mode must be set to 0.
>>
>> So the GPIO function needs separate intel_pingroup-s with their own mode
>> value of 0.
>>
>> Add foo_gpio entries for each function to byt_score_groups[] and make all
>> the byt_score_gpio_groups[] entries point to these instead to fix this.
>>
>> The "sus" pin-groups got this correct until commit 2f46d7f7e959 ("pinctrl:
>> baytrail: Add pinconf group + function for the pmu_clk") added support for
>> the pmu_clk pins following the broken "score" model.
>>
>> Add pmu_clk?_grp_gpio entries to byt_sus_groups[] and point to those
>> in byt_sus_gpio_groups[] to fix this.
> 
> I'm wondering if it's possible to add some code to imply all these. I
> mean to have a comparator to the _gpio in the naming and generate them
> at runtime and add. In this case if we add / modify the original one
> the rest (for _gpio cases) will be done automatically.

Yes some better solution for this would be nice but I don't have time
to work on this, so I suggest to just move forward with this fix for now.

Regards,

Hans

Re: [PATCH 1/2] pinctrl: baytrail: Fix selecting gpio pinctrl state

[Andy Shevchenko]

On Sat, Apr 6, 2024 at 5:09 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 4/6/24 3:52 PM, Andy Shevchenko wrote:
> > On Sat, Apr 6, 2024 at 3:37 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > I'm wondering if it's possible to add some code to imply all these. I
> > mean to have a comparator to the _gpio in the naming and generate them
> > at runtime and add. In this case if we add / modify the original one
> > the rest (for _gpio cases) will be done automatically.
>
> Yes some better solution for this would be nice but I don't have time
> to work on this, so I suggest to just move forward with this fix for now.

I'm worrying that the _temporary_ solution becomes a permanent
solution, which is not good.

I'm okay to move forward, but we need to evaluate how harder it is to
implement the better one.

-- 
With Best Regards,
Andy Shevchenko