Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> > For the record: once the designware maintainers are okay with this > > change, I am also okay with it going via the x86 platform tree. > > > Acked-by: Jarkko Nikula > Tested-by: Jarkko Nikula Acked-by: Wolfram Sang signature.asc Description: PGP signature
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> > For the record: once the designware maintainers are okay with this > > change, I am also okay with it going via the x86 platform tree. > > > Acked-by: Jarkko Nikula > Tested-by: Jarkko Nikula Acked-by: Wolfram Sang signature.asc Description: PGP signature
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/14/2018 04:17 PM, Wolfram Sang wrote: On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On 10/14/2018 04:17 PM, Wolfram Sang wrote: On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. Acked-by: Jarkko Nikula Tested-by: Jarkko Nikula
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: > On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the > kernel. The P-Unit has a semaphore for the PMIC bus which we can take to > block it from accessing the shared bus while the kernel wants to access it. > > Currently we have the I2C-controller driver acquiring and releasing the > semaphore around each I2C transfer. There are 2 problems with this: > > 1) PMIC accesses often come in the form of a read-modify-write on one of > the PMIC registers, we currently release the P-Unit's PMIC bus semaphore > between the read and the write. If the P-Unit modifies the register during > this window?, then we end up overwriting the P-Unit's changes. > I believe that this is mostly an academic problem, but I'm not sure. > > 2) To safely access the shared I2C bus, we need to do 3 things: > a) Notify the GPU driver that we are starting a window in which it may not > access the P-Unit, since the P-Unit seems to ignore the semaphore for > explicit power-level requests made by the GPU driver > b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering > C6/C7 while we hold the semaphore hangs the SoC > c) Finally take the P-Unit's PMIC bus semaphore > All 3 these steps together are somewhat expensive, so ideally if we have > a bunch of i2c transfers grouped together we only do this once for the > entire group. > > Taking the read-modify-write on a PMIC register as example then ideally we > would only do all 3 steps once at the beginning and undo all 3 steps once > at the end. > > For this we need to be able to take the semaphore from within e.g. the PMIC > opregion driver, yet we do not want to remove the taking of the semaphore > from the I2C-controller driver, as that is still necessary to protect many > other code-paths leading to accessing the shared I2C bus. > > This means that we first have the PMIC driver acquire the semaphore and > then have the I2C controller driver trying to acquire it again. > > To make this possible this commit does the following: > > 1) Move the semaphore code from being private to the I2C controller driver > into the generic iosf_mbi code, which already has other code to deal with > the shared bus so that it can be accessed outside of the I2C bus driver. > > 2) Rework the code so that it can be called multiple times nested, while > still blocking I2C accesses while e.g. the GPU driver has indicated the > P-Unit needs the bus through a iosf_mbi_punit_acquire() call. > > Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. signature.asc Description: PGP signature
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On Thu, Oct 11, 2018 at 04:29:09PM +0200, Hans de Goede wrote: > On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the > kernel. The P-Unit has a semaphore for the PMIC bus which we can take to > block it from accessing the shared bus while the kernel wants to access it. > > Currently we have the I2C-controller driver acquiring and releasing the > semaphore around each I2C transfer. There are 2 problems with this: > > 1) PMIC accesses often come in the form of a read-modify-write on one of > the PMIC registers, we currently release the P-Unit's PMIC bus semaphore > between the read and the write. If the P-Unit modifies the register during > this window?, then we end up overwriting the P-Unit's changes. > I believe that this is mostly an academic problem, but I'm not sure. > > 2) To safely access the shared I2C bus, we need to do 3 things: > a) Notify the GPU driver that we are starting a window in which it may not > access the P-Unit, since the P-Unit seems to ignore the semaphore for > explicit power-level requests made by the GPU driver > b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering > C6/C7 while we hold the semaphore hangs the SoC > c) Finally take the P-Unit's PMIC bus semaphore > All 3 these steps together are somewhat expensive, so ideally if we have > a bunch of i2c transfers grouped together we only do this once for the > entire group. > > Taking the read-modify-write on a PMIC register as example then ideally we > would only do all 3 steps once at the beginning and undo all 3 steps once > at the end. > > For this we need to be able to take the semaphore from within e.g. the PMIC > opregion driver, yet we do not want to remove the taking of the semaphore > from the I2C-controller driver, as that is still necessary to protect many > other code-paths leading to accessing the shared I2C bus. > > This means that we first have the PMIC driver acquire the semaphore and > then have the I2C controller driver trying to acquire it again. > > To make this possible this commit does the following: > > 1) Move the semaphore code from being private to the I2C controller driver > into the generic iosf_mbi code, which already has other code to deal with > the shared bus so that it can be accessed outside of the I2C bus driver. > > 2) Rework the code so that it can be called multiple times nested, while > still blocking I2C accesses while e.g. the GPU driver has indicated the > P-Unit needs the bus through a iosf_mbi_punit_acquire() call. > > Signed-off-by: Hans de Goede For the record: once the designware maintainers are okay with this change, I am also okay with it going via the x86 platform tree. signature.asc Description: PGP signature
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> > It should be. > > You mean that the problem should be purely academic, IOW that registers > touched > by the P-Unit are never touched through ACPI Opregions / power-resources? As far as I am aware. Holding the lock over both is definitely better regardless > >> 2) To safely access the shared I2C bus, we need to do 3 things: > >> a) Notify the GPU driver that we are starting a window in which it may not > >> access the P-Unit, since the P-Unit seems to ignore the semaphore for > >> explicit power-level requests made by the GPU driver > > > > That's not what happens. It's more a problem of > > > > We take the SEM > > The GPU driver pokes the GPU > > The GPU decides it wants to change the power situation > > The GPU asks > > It blocks on the SEM > > > > and the system deadlocks. > > That may be, but why does it deadlock? As I understand it because the CPU is stuck waiting for the GPU which is waiting for the SEM which the CPU is holding. This isn't purely software remember. > I can understand that you are reluctant to change this code, but this > commit is not changing the logic, it mostly just moves the code around > and I do believe that overall doing this is worthwhile. Fair enough Alan
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> > It should be. > > You mean that the problem should be purely academic, IOW that registers > touched > by the P-Unit are never touched through ACPI Opregions / power-resources? As far as I am aware. Holding the lock over both is definitely better regardless > >> 2) To safely access the shared I2C bus, we need to do 3 things: > >> a) Notify the GPU driver that we are starting a window in which it may not > >> access the P-Unit, since the P-Unit seems to ignore the semaphore for > >> explicit power-level requests made by the GPU driver > > > > That's not what happens. It's more a problem of > > > > We take the SEM > > The GPU driver pokes the GPU > > The GPU decides it wants to change the power situation > > The GPU asks > > It blocks on the SEM > > > > and the system deadlocks. > > That may be, but why does it deadlock? As I understand it because the CPU is stuck waiting for the GPU which is waiting for the SEM which the CPU is holding. This isn't purely software remember. > I can understand that you are reluctant to change this code, but this > commit is not changing the logic, it mostly just moves the code around > and I do believe that overall doing this is worthwhile. Fair enough Alan
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
Hi, On 11-10-18 22:35, Alan Cox wrote: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. It should be. You mean that the problem should be purely academic, IOW that registers touched by the P-Unit are never touched through ACPI Opregions / power-resources? 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver That's not what happens. It's more a problem of We take the SEM The GPU driver pokes the GPU The GPU decides it wants to change the power situation The GPU asks It blocks on the SEM and the system deadlocks. That may be, but why does it deadlock? It should just wait for the I2C transfer to finish, the GPU driver does wait for the P-Unit to report back that it has done its job, IIRC it even has a timeout on the wait, yet we get a consistent freeze. While nothing should stop the I2C transfer to simply complete at this point, release the semaphore and everything then can continue normally. b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC Not just C6/C7 necessarily. We need to stop assorted transitions. Could be, it may be that the pm_qos request results in the CPU never leaving C0. The code for this originally comes from the Android-x86 kernel patchset: https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches and IIRC the commit there talks about avoiding C6 and C7. Given how horrible this lot was to debug originally do you have any meaningful test data and performance numbers to justify it ? No, my mean reason for re-visiting this (I wrote most of the original code to deal with this) was that I thought I was seeing a case where the AML was modifying a PMIC register which was also being touched by the P-Unit. Eventually I figured out I was not actually seeing this, but then the patch was already written. As an ahem 'feature' it's gone away in modern chips so is it worth the attention ? I know and good riddance. But Cherry Trail SoCs with AXP288 PMICs are still very common and are being sold 1 at a time by Endless with Linux pre-installed. This change mostly just moves a bunch of code around, the only new feature is the ability to nest calls to the iosf_mbi_block_punit_i2c_access() function and not deadlock then. This does result in a nice cleanup in the form of putting all the code dealing with this together in arch/x86/platform/intel/iosf_mbi.c instead of having it split over iosf_mbi.c and i2c-designware code. And this will allow making the AXP288 fuel-gauge driver do all the steps to claim the i2c-bus once before reading multiple registers to get the battery status, something which has been on my TODO list for a while, since as mentioned taking all these steps together is not cheap, esp. if you also take into account all the work the GPU driver does when notified that it will be unable to access the P-Unit for a while and currently we do all the setup and teardown like 5 times or so just to get the battery status once. But sorry no numbers, it is sort of hard to measure this and I do think that the impact will not be that big since the battery status is not checked that often. Which is also why I've not spend time on this so far. I can understand that you are reluctant to change this code, but this commit is not changing the logic, it mostly just moves the code around and I do believe that overall doing this is worthwhile. Regards, Hans p.s. There also is this infamous bug which we really really need to fix: https://bugzilla.kernel.org/show_bug.cgi?id=109051 But mostly (only?) seems to happen on systems with a Crystal Cove PMIC where the I2C bus is not shared. I know that some work was being done on this recently what is the status of this?
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
Hi, On 11-10-18 22:35, Alan Cox wrote: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. It should be. You mean that the problem should be purely academic, IOW that registers touched by the P-Unit are never touched through ACPI Opregions / power-resources? 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver That's not what happens. It's more a problem of We take the SEM The GPU driver pokes the GPU The GPU decides it wants to change the power situation The GPU asks It blocks on the SEM and the system deadlocks. That may be, but why does it deadlock? It should just wait for the I2C transfer to finish, the GPU driver does wait for the P-Unit to report back that it has done its job, IIRC it even has a timeout on the wait, yet we get a consistent freeze. While nothing should stop the I2C transfer to simply complete at this point, release the semaphore and everything then can continue normally. b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC Not just C6/C7 necessarily. We need to stop assorted transitions. Could be, it may be that the pm_qos request results in the CPU never leaving C0. The code for this originally comes from the Android-x86 kernel patchset: https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches and IIRC the commit there talks about avoiding C6 and C7. Given how horrible this lot was to debug originally do you have any meaningful test data and performance numbers to justify it ? No, my mean reason for re-visiting this (I wrote most of the original code to deal with this) was that I thought I was seeing a case where the AML was modifying a PMIC register which was also being touched by the P-Unit. Eventually I figured out I was not actually seeing this, but then the patch was already written. As an ahem 'feature' it's gone away in modern chips so is it worth the attention ? I know and good riddance. But Cherry Trail SoCs with AXP288 PMICs are still very common and are being sold 1 at a time by Endless with Linux pre-installed. This change mostly just moves a bunch of code around, the only new feature is the ability to nest calls to the iosf_mbi_block_punit_i2c_access() function and not deadlock then. This does result in a nice cleanup in the form of putting all the code dealing with this together in arch/x86/platform/intel/iosf_mbi.c instead of having it split over iosf_mbi.c and i2c-designware code. And this will allow making the AXP288 fuel-gauge driver do all the steps to claim the i2c-bus once before reading multiple registers to get the battery status, something which has been on my TODO list for a while, since as mentioned taking all these steps together is not cheap, esp. if you also take into account all the work the GPU driver does when notified that it will be unable to access the P-Unit for a while and currently we do all the setup and teardown like 5 times or so just to get the battery status once. But sorry no numbers, it is sort of hard to measure this and I do think that the impact will not be that big since the battery status is not checked that often. Which is also why I've not spend time on this so far. I can understand that you are reluctant to change this code, but this commit is not changing the logic, it mostly just moves the code around and I do believe that overall doing this is worthwhile. Regards, Hans p.s. There also is this infamous bug which we really really need to fix: https://bugzilla.kernel.org/show_bug.cgi?id=109051 But mostly (only?) seems to happen on systems with a Crystal Cove PMIC where the I2C bus is not shared. I know that some work was being done on this recently what is the status of this?
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> 1) PMIC accesses often come in the form of a read-modify-write on one of > the PMIC registers, we currently release the P-Unit's PMIC bus semaphore > between the read and the write. If the P-Unit modifies the register during > this window?, then we end up overwriting the P-Unit's changes. > I believe that this is mostly an academic problem, but I'm not sure. It should be. > 2) To safely access the shared I2C bus, we need to do 3 things: > a) Notify the GPU driver that we are starting a window in which it may not > access the P-Unit, since the P-Unit seems to ignore the semaphore for > explicit power-level requests made by the GPU driver That's not what happens. It's more a problem of We take the SEM The GPU driver pokes the GPU The GPU decides it wants to change the power situation The GPU asks It blocks on the SEM and the system deadlocks. > b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering > C6/C7 while we hold the semaphore hangs the SoC Not just C6/C7 necessarily. We need to stop assorted transitions. Given how horrible this lot was to debug originally do you have any meaningful test data and performance numbers to justify it ? As an ahem 'feature' it's gone away in modern chips so is it worth the attention ? Alan
Re: [PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
> 1) PMIC accesses often come in the form of a read-modify-write on one of > the PMIC registers, we currently release the P-Unit's PMIC bus semaphore > between the read and the write. If the P-Unit modifies the register during > this window?, then we end up overwriting the P-Unit's changes. > I believe that this is mostly an academic problem, but I'm not sure. It should be. > 2) To safely access the shared I2C bus, we need to do 3 things: > a) Notify the GPU driver that we are starting a window in which it may not > access the P-Unit, since the P-Unit seems to ignore the semaphore for > explicit power-level requests made by the GPU driver That's not what happens. It's more a problem of We take the SEM The GPU driver pokes the GPU The GPU decides it wants to change the power situation The GPU asks It blocks on the SEM and the system deadlocks. > b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering > C6/C7 while we hold the semaphore hangs the SoC Not just C6/C7 necessarily. We need to stop assorted transitions. Given how horrible this lot was to debug originally do you have any meaningful test data and performance numbers to justify it ? As an ahem 'feature' it's gone away in modern chips so is it worth the attention ? Alan
[PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede --- Note this commit deliberately limits the i2c-designware changes to only touch i2c-designware-baytrail.c, deliberately not doing some cleanups which become possible after removing the semaphore code from the i2c-designmware code. This is done so that this commit can be merged through the x86 tree without causing conflicts in the i2c tree. The cleanups to the i2c-designware tree will be done in a follow up patch which can be merged once this commit is in place. --- Changes in v3: -Fix iosf_mbi_punit_mutex not being unlocked in error exit path -Add a big comment describing the what-and-why of iosf_mbi_block_punit_i2c_access() Changes in v2: -Move mutex_unlock(_mbi_punit_mutex); to the callers of iosf_mbi_reset_semaphore() -Use PCI_DEVICE_DATA() to pass the driver_data --- arch/x86/include/asm/iosf_mbi.h | 39 +++- arch/x86/platform/intel/iosf_mbi.c | 217 +-- drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-- 3 files changed, 230 insertions(+), 151 deletions(-) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index 3de0489deade..5270ff39b9af 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); * the PMIC bus while another driver is also accessing the PMIC bus various bad * things happen. * - * To avoid these problems this function must be called before accessing the - * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means. + * Call this function before sending requests to the P-Unit which may make it + * access the PMIC, be it through iosf_mbi* functions or through other means. + * This function will block all kernel access to the PMIC I2C bus, so that the + * P-Unit can safely access the PMIC over the shared I2C bus. * * Note on these systems the i2c-bus driver will request a sempahore from the * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing @@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void); */ void iosf_mbi_punit_release(void); +/** + * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus + * + * Call this function to block P-Unit access to the PMIC I2C bus, so that the + * kernel can safely access the PMIC over the shared I2C bus. + * + * This function acquires the P-Unit bus semaphore and notifies + * pmic_bus_access_notifier listeners that they may no longer access the + * P-Unit in a way which may cause it to access the shared I2C bus.
[PATCH v3 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code
On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the kernel. The P-Unit has a semaphore for the PMIC bus which we can take to block it from accessing the shared bus while the kernel wants to access it. Currently we have the I2C-controller driver acquiring and releasing the semaphore around each I2C transfer. There are 2 problems with this: 1) PMIC accesses often come in the form of a read-modify-write on one of the PMIC registers, we currently release the P-Unit's PMIC bus semaphore between the read and the write. If the P-Unit modifies the register during this window?, then we end up overwriting the P-Unit's changes. I believe that this is mostly an academic problem, but I'm not sure. 2) To safely access the shared I2C bus, we need to do 3 things: a) Notify the GPU driver that we are starting a window in which it may not access the P-Unit, since the P-Unit seems to ignore the semaphore for explicit power-level requests made by the GPU driver b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering C6/C7 while we hold the semaphore hangs the SoC c) Finally take the P-Unit's PMIC bus semaphore All 3 these steps together are somewhat expensive, so ideally if we have a bunch of i2c transfers grouped together we only do this once for the entire group. Taking the read-modify-write on a PMIC register as example then ideally we would only do all 3 steps once at the beginning and undo all 3 steps once at the end. For this we need to be able to take the semaphore from within e.g. the PMIC opregion driver, yet we do not want to remove the taking of the semaphore from the I2C-controller driver, as that is still necessary to protect many other code-paths leading to accessing the shared I2C bus. This means that we first have the PMIC driver acquire the semaphore and then have the I2C controller driver trying to acquire it again. To make this possible this commit does the following: 1) Move the semaphore code from being private to the I2C controller driver into the generic iosf_mbi code, which already has other code to deal with the shared bus so that it can be accessed outside of the I2C bus driver. 2) Rework the code so that it can be called multiple times nested, while still blocking I2C accesses while e.g. the GPU driver has indicated the P-Unit needs the bus through a iosf_mbi_punit_acquire() call. Signed-off-by: Hans de Goede --- Note this commit deliberately limits the i2c-designware changes to only touch i2c-designware-baytrail.c, deliberately not doing some cleanups which become possible after removing the semaphore code from the i2c-designmware code. This is done so that this commit can be merged through the x86 tree without causing conflicts in the i2c tree. The cleanups to the i2c-designware tree will be done in a follow up patch which can be merged once this commit is in place. --- Changes in v3: -Fix iosf_mbi_punit_mutex not being unlocked in error exit path -Add a big comment describing the what-and-why of iosf_mbi_block_punit_i2c_access() Changes in v2: -Move mutex_unlock(_mbi_punit_mutex); to the callers of iosf_mbi_reset_semaphore() -Use PCI_DEVICE_DATA() to pass the driver_data --- arch/x86/include/asm/iosf_mbi.h | 39 +++- arch/x86/platform/intel/iosf_mbi.c | 217 +-- drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-- 3 files changed, 230 insertions(+), 151 deletions(-) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index 3de0489deade..5270ff39b9af 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); * the PMIC bus while another driver is also accessing the PMIC bus various bad * things happen. * - * To avoid these problems this function must be called before accessing the - * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means. + * Call this function before sending requests to the P-Unit which may make it + * access the PMIC, be it through iosf_mbi* functions or through other means. + * This function will block all kernel access to the PMIC I2C bus, so that the + * P-Unit can safely access the PMIC over the shared I2C bus. * * Note on these systems the i2c-bus driver will request a sempahore from the * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing @@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void); */ void iosf_mbi_punit_release(void); +/** + * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus + * + * Call this function to block P-Unit access to the PMIC I2C bus, so that the + * kernel can safely access the PMIC over the shared I2C bus. + * + * This function acquires the P-Unit bus semaphore and notifies + * pmic_bus_access_notifier listeners that they may no longer access the + * P-Unit in a way which may cause it to access the shared I2C bus.
