Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access

From: Hans de Goede <hdegoede@redhat.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "russianneuromancer @ ya . ru" <russianneuromancer@ya.ru>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Jarkko Nikula <jarkko.nikula@linux.intel.com>,
	linux-i2c@vger.kernel.org, Len Brown <lenb@kernel.org>
Subject: Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
Date: Fri, 13 Jan 2017 17:06:52 +0100	[thread overview]
Message-ID: <a213718a-b4e9-09a0-a97f-69eb5e892151@redhat.com> (raw)
In-Reply-To: <20170113092644.GD31595@intel.com>

Hi,

On 01/13/2017 10:26 AM, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-01-17 15:12, Ville Syrjälä wrote:
>>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
>>>> The punit on baytrail / cherrytrail systems is not only accessed through
>>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
>>>> the punit against simultaneous accesses and 2 functions to lock / unlock
>>>> this mutex.
>>>
>>> I'm not sure which part of punit you're actually trying to protect
>>> here. Some specific registers?
>>
>> The theory I'm going by is that for certain actions / certain requests
>> we send to the punit, the punit needs to access the (axp288) pmic, to
>> change (or enable / disable) certain voltages.
>
> At least for cpu/display/gt voltages that shouldn't really be the case.
> The vcc/vnn/vgg rails are controlled via svid, not i2c.

Are you sure? The ax288 pmic does not have a svid interface, only
an i2c interface, and AFAICT its buck DCDC converters are used to
feed all of these.

> It also feels quite hand wavy since the punit could do whatever at
> any time AFAIK. Eg. if there's some thermal event or something the
> punit might kick into action. So trying to protect this from the OS
> side might not be able to avoid these problems entirely. It feels like
> there really should be some kind of shared hardware/firmware mutex
> with the punit to arbitrate access to the i2c bus.

Right, and there is such a mutex (which only gets used on systems
with an axp288 pmic...) and we are taking this mutex before starting
an i2c transaction on the pmic i2c bus. But this does not seem to be
enough. It seems the the punit does not check the mutex before
certain OS / host triggered actions. I guess it expects the host to
do this itself.

Please see my new (non RFC) version of this series I've posted.

There are at least 2 problems when relying solely on the punit
pmic i2c bus sempaphore:

1) CPU C1 <-> C6 transations happening while the pmic i2c bus
is being accessed by the host cause the system to hang
2) i915 (runtime) suspend resume fails every other attempt
with timeouts when trying to get a forcewake lock inn i915,
often followed by a system freeze shortly after this.

My non RFC version of this patch-set fixes both.

So summarizing yes you are right that there should be some
hardware mutex (there is, and we are already taking it), but
unfortunately that does not seem to be enough, when explicitly
requesting some power state transation, while another driver
is acccessing the pmic i2c bus bad things happen. Adding
some exclusion mechanism here seems to be necessary.

Note that the i2c acccess vs i915 forcewake issues was
first reported by an user who was trying my patches for
fixing pmic i2c access on cht (fixing the semaphore code on
cht) and then noticed a problem with the i915 driver as soon
as the pmic i2c bus was used. He has also confirmed that the
new non RFC version of my patches fix this.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx