RFC: pinctrl: baytrail: Need to fixup mux setting due to broken BIOS/DSDT

RFC: pinctrl: baytrail: Need to fixup mux setting due to broken BIOS/DSDT

[Hans de Goede]

Hi Andy, Mika,

For one of the x86 Android tablets with broken DSDTs which I'm working on
I need to change the mux value of pin 6 of SUS aka INT33FC:02 from 0 to 1,
this changes it from a normal GPIO to outputting the PMC's 32KHz clk.
This is needed for the jack-detection in the audio codec which needs an
external 32KHz clock to work and that is connected to pin 6 of SUS.

On the Windows version of the same tablet (which uses slightly different
hardware, e.g. there is an embedded controller on the board which the
Android version lacks) there is an ACPI call to toggle the mux, since
the firmware does not set it for us.

So the x86-android-tablets.c code for working around all the firmware
challenges on these devices will need a way to toggle the mux and
directly poking it itself is a bad idea because of the need
to serialize all accesses to the GPIO islands on byt, see:

39ce8150a079 ("pinctrl: baytrail: Serialize all register access")

So I see 2 possible options:

1. Add a pingroup for this pin in drivers/pinctrl/intel/pinctrl-baytrail.c
and then mimick the pwm0 pinconf setting code from
drivers/gpu/drm/i915/display/intel_dsi_vbt.c in x86-android-tablets.c
This seems the cleanest, but I'm leaning a bit towards:

2. Do EXPORT_SYMBOL_GPL(byt_lock); in pinctrl-baytrail.c and then just
do a ioremap + raw poke of the pad_conf0 register in x86-android-tablets.c.

This avoids adding an extra pingroup just to workaround around the
buggy firmware on these tablets; and more importantly this gives the
x86-android-tablets.c code a flexible way to fixup various possible
GPIO/pinctrl misconfigurations with needing to push workarounds into
pinctrl-baytrail.c.

To make sure others are not tempted to also take the byt_lock (which
needs a rename before exporting) I plan to not add byt_lock to any header
files. Instead the x86-android-tablets.c code can just do an extern
declaration of it inside the .c file.

###

Typing this out I think that 1 is cleaner and so far on a number of buggy
DSDT Android tablets this is the only case I've found where poking the
GPIO/pinctrl registers "directly" is necessary.

So I guess I should go with solution 1 and if it turns out more workarounds
are necessary then we can reconsider the (somewhat ugly) option 2. 

Before moving forward with either approach I would like to hear your
thoughts on this, so please let me know what you think ?

Regards,

Hans

Re: RFC: pinctrl: baytrail: Need to fixup mux setting due to broken BIOS/DSDT

[Andy Shevchenko]

On Sat, Jan 15, 2022 at 6:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Andy, Mika,
>
> For one of the x86 Android tablets with broken DSDTs which I'm working on
> I need to change the mux value of pin 6 of SUS aka INT33FC:02 from 0 to 1,
> this changes it from a normal GPIO to outputting the PMC's 32KHz clk.
> This is needed for the jack-detection in the audio codec which needs an
> external 32KHz clock to work and that is connected to pin 6 of SUS.
>
> On the Windows version of the same tablet (which uses slightly different
> hardware, e.g. there is an embedded controller on the board which the
> Android version lacks) there is an ACPI call to toggle the mux, since
> the firmware does not set it for us.
>
> So the x86-android-tablets.c code for working around all the firmware
> challenges on these devices will need a way to toggle the mux and
> directly poking it itself is a bad idea because of the need
> to serialize all accesses to the GPIO islands on byt, see:
>
> 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
>
> So I see 2 possible options:
>
> 1. Add a pingroup for this pin in drivers/pinctrl/intel/pinctrl-baytrail.c
> and then mimick the pwm0 pinconf setting code from
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c in x86-android-tablets.c
> This seems the cleanest, but I'm leaning a bit towards:

If you meant
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_dsi_vbt.c#L858
this is definitely the way to go.

> 2. Do EXPORT_SYMBOL_GPL(byt_lock); in pinctrl-baytrail.c and then just
> do a ioremap + raw poke of the pad_conf0 register in x86-android-tablets.c.

Sorry, I do not like the 2. at all. Let's keep that lock private to the driver.



> This avoids adding an extra pingroup just to workaround around the
> buggy firmware on these tablets; and more importantly this gives the
> x86-android-tablets.c code a flexible way to fixup various possible
> GPIO/pinctrl misconfigurations with needing to push workarounds into
> pinctrl-baytrail.c.
>
> To make sure others are not tempted to also take the byt_lock (which
> needs a rename before exporting) I plan to not add byt_lock to any header
> files. Instead the x86-android-tablets.c code can just do an extern
> declaration of it inside the .c file.

This is just a point to be against doing this. We are gaining
(perhaps) more cleanups against W=1 and CONFIG_WERROR=y and this case
might not work well with that (IIRC something similar in arch/x86 when
I tried that combination).

> ###
>
> Typing this out I think that 1 is cleaner and so far on a number of buggy
> DSDT Android tablets this is the only case I've found where poking the
> GPIO/pinctrl registers "directly" is necessary.
>
> So I guess I should go with solution 1 and if it turns out more workarounds
> are necessary then we can reconsider the (somewhat ugly) option 2.
>
> Before moving forward with either approach I would like to hear your
> thoughts on this, so please let me know what you think ?
>
> Regards,
>
> Hans
>
>


-- 
With Best Regards,
Andy Shevchenko

Re: RFC: pinctrl: baytrail: Need to fixup mux setting due to broken BIOS/DSDT

[Hans de Goede]

Hi,

On 1/16/22 22:11, Andy Shevchenko wrote:
> On Sat, Jan 15, 2022 at 6:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Andy, Mika,
>>
>> For one of the x86 Android tablets with broken DSDTs which I'm working on
>> I need to change the mux value of pin 6 of SUS aka INT33FC:02 from 0 to 1,
>> this changes it from a normal GPIO to outputting the PMC's 32KHz clk.
>> This is needed for the jack-detection in the audio codec which needs an
>> external 32KHz clock to work and that is connected to pin 6 of SUS.
>>
>> On the Windows version of the same tablet (which uses slightly different
>> hardware, e.g. there is an embedded controller on the board which the
>> Android version lacks) there is an ACPI call to toggle the mux, since
>> the firmware does not set it for us.
>>
>> So the x86-android-tablets.c code for working around all the firmware
>> challenges on these devices will need a way to toggle the mux and
>> directly poking it itself is a bad idea because of the need
>> to serialize all accesses to the GPIO islands on byt, see:
>>
>> 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
>>
>> So I see 2 possible options:
>>
>> 1. Add a pingroup for this pin in drivers/pinctrl/intel/pinctrl-baytrail.c
>> and then mimick the pwm0 pinconf setting code from
>> drivers/gpu/drm/i915/display/intel_dsi_vbt.c in x86-android-tablets.c
>> This seems the cleanest, but I'm leaning a bit towards:
> 
> If you meant
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_dsi_vbt.c#L858
> this is definitely the way to go.

Ok, I already figured as much, thank you for your input.

Regards,

Hans