Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1601914 > unrolled thread
| Started by | Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| First post | 2017-03-16 04:40 +0100 |
| Last post | 2017-03-17 18:20 +0100 |
| Articles | 9 on this page of 29 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-16 04:40 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-16 16:00 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-16 17:10 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-16 19:20 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-16 21:20 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-16 22:20 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-16 20:00 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-16 20:30 +0100
Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-16 22:30 +0100
[PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 01:50 +0100
Re: [PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-17 12:30 +0100
Re: [PATCH v2 2/4] platform/x86: intel_pmc_ipc: Add pmc gcr read/write api's sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 18:40 +0100
[PATCH v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 01:50 +0100
Re: [PATCH v2 4/4] platform/x86: intel_pmc_ipc: remove iTCO GCR mem resource Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-17 12:50 +0100
[PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 01:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-17 12:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Guenter Roeck <linux@roeck-us.net> - 2017-03-17 14:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-17 15:10 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-17 15:30 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 18:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-17 19:40 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 20:00 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure Guenter Roeck <linux@roeck-us.net> - 2017-03-17 19:00 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 19:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 21:50 +0100
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 18:30 +0100
[PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 02:00 +0100
Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> - 2017-03-17 12:20 +0100
Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> - 2017-03-17 18:20 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-03-17 19:40 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm4X7-2fG-15@gated-at.bofh.it> |
| In reply to | #1603497 |
On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > On 03/17/2017 07:25 AM, Andy Shevchenko wrote: >> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote: >>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote: >>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan >>>> wrote: >> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU >> IPC. >> (PMC IPC how it's called is actually just a [main] part of SCU in newer >> SoCs). >> >> Rajneesh, Kuppuswamy, >> please pay attention on the below. >> >> We have two libraries doing almost the same (basics) one for old >> platforms, one for new. >> >> My vision what should be done before we go further is: >> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some >> library. > > I think we should create MFD driver for PMC and remove the redundant > resource and platform device creation codes. > Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc > code. This needs be modularized. > > I can work on it and send a RFC patch for this cleanup. But it could take > more time for merging this cleanup patch. > So I think, in the mean time, we should merge this watchdog fix first to > remove iTCO watchdog device probe issue. I have heard already such excuses. Let's consider this as a "Last Chinese Warning". So, we consider reviewing applying *already floating around* patches in exchange to looking forward for clean up next. Do we have a deal? Before you are going to implement anything in the code, please, share a document (architectural point of view) how you would see things should be done. Also consider to address PMC (Atom drivers) and P-Unit drivers which are related to SCU / IPC to have some structure. >> 2. Move headers to linux/platform_data/x86 for sharing with drivers >> that are supporting non-Intel / not-newest-Intel hardware. >> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_() >> helpers where it makes sense, no use of global variables, etc) > > Agreed. >> >> >> On top of that >> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details) >> >> [1] Oops, it happened on internal mailing list Jan 27. And mentioned >> publicly after in a review on some patch here. >> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html -- With Best Regards, Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 20:00 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm5gv-2nQ-63@gated-at.bofh.it> |
| In reply to | #1603541 |
Hi Andy, On 03/17/2017 11:38 AM, Andy Shevchenko wrote: > On Fri, Mar 17, 2017 at 7:37 PM, sathyanarayanan kuppuswamy > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> On 03/17/2017 07:25 AM, Andy Shevchenko wrote: >>> On Fri, Mar 17, 2017 at 3:40 PM, Guenter Roeck <linux@roeck-us.net> wrote: >>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote: >>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan >>>>> wrote: >>> I already asked once [1] to fix up the mess we have in PDx86 regarding SCU >>> IPC. >>> (PMC IPC how it's called is actually just a [main] part of SCU in newer >>> SoCs). >>> >>> Rajneesh, Kuppuswamy, >>> please pay attention on the below. >>> >>> We have two libraries doing almost the same (basics) one for old >>> platforms, one for new. >>> >>> My vision what should be done before we go further is: >>> 1. Split out common part from intel_scu_ipc and intel_pmc_ipc to some >>> library. >> I think we should create MFD driver for PMC and remove the redundant >> resource and platform device creation codes. >> Yes, there is common code in IPC implementation between scu_ipc and pmc_ipc >> code. This needs be modularized. >> >> I can work on it and send a RFC patch for this cleanup. But it could take >> more time for merging this cleanup patch. >> So I think, in the mean time, we should merge this watchdog fix first to >> remove iTCO watchdog device probe issue. > I have heard already such excuses. Let's consider this as a "Last > Chinese Warning". > > So, we consider reviewing applying *already floating around* patches > in exchange to looking forward for clean up next. > Do we have a deal? Deal. > > Before you are going to implement anything in the code, please, share > a document (architectural point of view) how you would see things > should be done. > Also consider to address PMC (Atom drivers) and P-Unit drivers which > are related to SCU / IPC to have some structure. Will send out the design document summarizing the issues we want to solve and a proposed design model. > >>> 2. Move headers to linux/platform_data/x86 for sharing with drivers >>> that are supporting non-Intel / not-newest-Intel hardware. >>> 3. Fix the mess inside the intel_pmc_ipc code (like use devm_() >>> helpers where it makes sense, no use of global variables, etc) >> Agreed. >>> >>> On top of that >>> 4. Fix up Whiskey Cove PMIC code (See Hans' message [2] for the details) >>> >>> [1] Oops, it happened on internal mailing list Jan 27. And mentioned >>> publicly after in a review on some patch here. >>> [2] http://lkml.iu.edu/hypermail/linux/kernel/1702.3/01408.html -- Sathyanarayanan Kuppuswamy Android kernel developer
[toc] | [prev] | [next] | [standalone]
| From | Guenter Roeck <linux@roeck-us.net> |
|---|---|
| Date | 2017-03-17 19:00 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm4kq-1GF-29@gated-at.bofh.it> |
| In reply to | #1603318 |
On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
>
>
> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> >On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> >>On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
> >>wrote:
> >>>Currently, iTCO watchdog driver uses memory map to access
> >>>PMC_CFG GCR register. But the entire GCR address space is
> >>>already mapped in intel_scu_ipc driver. So remapping the
> >>
> >>intel_pmc_ipc driver.
> >>
> >>>GCR register in this driver causes the mem request failure in
> >>>iTCO_wdt probe function. This patch fixes this issue by
> >>>using PMC GCR read/write API's to access PMC_CFG register.
> >>>
> >>>Signed-off-by: Kuppuswamy Sathyanarayanan
> >>><sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>---
> >>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
> >>> 1 file changed, 7 insertions(+), 24 deletions(-)
> >>>
> >>>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> >>>index 3d0abc0..31abfc5 100644
> >>>--- a/drivers/watchdog/iTCO_wdt.c
> >>>+++ b/drivers/watchdog/iTCO_wdt.c
> >>>@@ -68,6 +68,8 @@
> >>> #include <linux/io.h> /* For inb/outb/... */
> >>> #include <linux/platform_data/itco_wdt.h>
> >>>
> >>>+#include <asm/intel_pmc_ipc.h>
> >>>+
> >>> #include "iTCO_vendor.h"
> >>>
> >>> /* Address definitions for the TCO */
> >>>@@ -94,12 +96,6 @@ struct iTCO_wdt_private {
> >>> unsigned int iTCO_version;
> >>> struct resource *tco_res;
> >>> struct resource *smi_res;
> >>>- /*
> >>>- * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
> >>>version 2),
> >>>- * or memory-mapped PMC register bit 4 (TCO version 3).
> >>>- */
> >>
> >>Better to retain this comment elsewhere.
> >>
> >>>- struct resource *gcs_pmc_res;
> >>>- unsigned long __iomem *gcs_pmc;
> >>> /* the lock for io operations */
> >>> spinlock_t io_lock;
> >>> /* the PCI-device */
> >>>@@ -176,9 +172,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct
> >>>iTCO_wdt_private *p)
> >>>
> >>> /* Set the NO_REBOOT bit: this disables reboots */
> >>> if (p->iTCO_version >= 2) {
> >>>- val32 = readl(p->gcs_pmc);
> >>>+ val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> >>
> >>better to have protection and error handling, discussed in v2, 2/4.
> >>
> >>compiled and tested this on APL and i see iTCO_WDT driver loads fine.
> >>Since
> >>it impacts core WDT functionality, need to be thoroughly tested on
> >>various
> >>platforms.
> >>
> >
> >I don't think I (or the watchdog mailing list) was copied on the original
> >patch.
> Sorry. Its my mistake. I will fix it in next series update.
> >Major immediate concern is that this introduces a dependency on external
> >code.
> >The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> >machines". I don't know where the function is introduced, but I hope this
> >change
> >does not require the pmc_ipc code to be present on such machines for the
> >watchdog
> >to work. It would be bad if it does. If it doesn't, it appears that the
> >function
> >should not be declared in asm/intel_pmc_ipc.h.
> It should not create any compile time dependency with INTEL_PMC_IPC config
> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
> empty definitions for these calls defined in asm/intel_pmc_ipc.h
>
So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
is not defined ? And that is supposed to be acceptable ?
> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
> its version iTCO_version >= 2.
>
Unless I am missing something, there is no explicit dependency. AFAICS
the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
and/or if it is built as module and the module is not loaded.
Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
driver is loaded. That would be a bug, not a dependency.
Guenter
[toc] | [prev] | [next] | [standalone]
| From | sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 19:50 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm56O-2jb-19@gated-at.bofh.it> |
| In reply to | #1603513 |
On 03/17/2017 10:50 AM, Guenter Roeck wrote:
> On Fri, Mar 17, 2017 at 10:24:35AM -0700, sathyanarayanan kuppuswamy wrote:
>>
>> On 03/17/2017 06:40 AM, Guenter Roeck wrote:
>>> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>>>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>>>> wrote:
>>>>> Currently, iTCO watchdog driver uses memory map to access
>>>>> PMC_CFG GCR register. But the entire GCR address space is
>>>>> already mapped in intel_scu_ipc driver. So remapping the
>>>> intel_pmc_ipc driver.
>>>>
>>>>> GCR register in this driver causes the mem request failure in
>>>>> iTCO_wdt probe function. This patch fixes this issue by
>>>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>>>
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> ---
>>>>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>>>> 1 file changed, 7 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>>>> index 3d0abc0..31abfc5 100644
>>>>> --- a/drivers/watchdog/iTCO_wdt.c
>>>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>>>> @@ -68,6 +68,8 @@
>>>>> #include <linux/io.h> /* For inb/outb/... */
>>>>> #include <linux/platform_data/itco_wdt.h>
>>>>>
>>>>> +#include <asm/intel_pmc_ipc.h>
>>>>> +
>>>>> #include "iTCO_vendor.h"
>>>>>
>>>>> /* Address definitions for the TCO */
>>>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>>>> unsigned int iTCO_version;
>>>>> struct resource *tco_res;
>>>>> struct resource *smi_res;
>>>>> - /*
>>>>> - * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
>>>>> version 2),
>>>>> - * or memory-mapped PMC register bit 4 (TCO version 3).
>>>>> - */
>>>> Better to retain this comment elsewhere.
>>>>
>>>>> - struct resource *gcs_pmc_res;
>>>>> - unsigned long __iomem *gcs_pmc;
>>>>> /* the lock for io operations */
>>>>> spinlock_t io_lock;
>>>>> /* the PCI-device */
>>>>> @@ -176,9 +172,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct
>>>>> iTCO_wdt_private *p)
>>>>>
>>>>> /* Set the NO_REBOOT bit: this disables reboots */
>>>>> if (p->iTCO_version >= 2) {
>>>>> - val32 = readl(p->gcs_pmc);
>>>>> + val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>>> better to have protection and error handling, discussed in v2, 2/4.
>>>>
>>>> compiled and tested this on APL and i see iTCO_WDT driver loads fine.
>>>> Since
>>>> it impacts core WDT functionality, need to be thoroughly tested on
>>>> various
>>>> platforms.
>>>>
>>> I don't think I (or the watchdog mailing list) was copied on the original
>>> patch.
>> Sorry. Its my mistake. I will fix it in next series update.
>>> Major immediate concern is that this introduces a dependency on external
>>> code.
>>> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
>>> machines". I don't know where the function is introduced, but I hope this
>>> change
>>> does not require the pmc_ipc code to be present on such machines for the
>>> watchdog
>>> to work. It would be bad if it does. If it doesn't, it appears that the
>>> function
>>> should not be declared in asm/intel_pmc_ipc.h.
>> It should not create any compile time dependency with INTEL_PMC_IPC config
>> option. If INTEL_PMC_IPC_CONFIG is disabled, we use
>> empty definitions for these calls defined in asm/intel_pmc_ipc.h
>>
> So the watchdog driver would get an error if CONFIG_INTEL_PMC_IPC
> is not defined ? And that is supposed to be acceptable ?
Sorry, It looks like I missed your point in your previous email. I
thought that gcs_pmc mem resource will be used only if watchdog device
is enumerated by intel_pmc_ipc.c
After reviewing the code again, I found out this iTCO_wdt driver can
also be enumerated by drivers/mfd/lpc_ich.c and
drivers/i2c/busses/i2c-i801.c. Both these drivers pass the GCS as memory
resource. So using intel_pmc_ipc specific calls will break watchdog
functionality if its enumerated by any device other than intel_pmic_ipc.c
May be I should add a flag for ipc case in itco_wdt_platform_data and
handle this as a special case.
>
>> But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC if
>> its version iTCO_version >= 2.
>>
> Unless I am missing something, there is no explicit dependency. AFAICS
> the watchdog driver works just fine if INTEL_PMIC_IPC is not enabled,
> and/or if it is built as module and the module is not loaded.
>
> Maybe you mean that the watchdog driver doesn't load if the INTEL_PMIC_IPC
> driver is loaded. That would be a bug, not a dependency.
>
> Guenter
>
--
Sathyanarayanan Kuppuswamy
Android kernel developer
[toc] | [prev] | [next] | [standalone]
| From | sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 21:50 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm4kq-1GF-31@gated-at.bofh.it> |
| In reply to | #1603318 |
On 03/17/2017 06:40 AM, Guenter Roeck wrote:
> On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
>> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan
>> wrote:
>>> Currently, iTCO watchdog driver uses memory map to access
>>> PMC_CFG GCR register. But the entire GCR address space is
>>> already mapped in intel_scu_ipc driver. So remapping the
>>
>> intel_pmc_ipc driver.
>>
>>> GCR register in this driver causes the mem request failure in
>>> iTCO_wdt probe function. This patch fixes this issue by
>>> using PMC GCR read/write API's to access PMC_CFG register.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>>> 1 file changed, 7 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>>> index 3d0abc0..31abfc5 100644
>>> --- a/drivers/watchdog/iTCO_wdt.c
>>> +++ b/drivers/watchdog/iTCO_wdt.c
>>> @@ -68,6 +68,8 @@
>>> #include <linux/io.h> /* For inb/outb/... */
>>> #include <linux/platform_data/itco_wdt.h>
>>>
>>> +#include <asm/intel_pmc_ipc.h>
>>> +
>>> #include "iTCO_vendor.h"
>>>
>>> /* Address definitions for the TCO */
>>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>>> unsigned int iTCO_version;
>>> struct resource *tco_res;
>>> struct resource *smi_res;
>>> - /*
>>> - * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO
>>> version 2),
>>> - * or memory-mapped PMC register bit 4 (TCO version 3).
>>> - */
>>
>> Better to retain this comment elsewhere.
>>
>>> - struct resource *gcs_pmc_res;
>>> - unsigned long __iomem *gcs_pmc;
>>> /* the lock for io operations */
>>> spinlock_t io_lock;
>>> /* the PCI-device */
>>> @@ -176,9 +172,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct
>>> iTCO_wdt_private *p)
>>>
>>> /* Set the NO_REBOOT bit: this disables reboots */
>>> if (p->iTCO_version >= 2) {
>>> - val32 = readl(p->gcs_pmc);
>>> + val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
>>
>> better to have protection and error handling, discussed in v2, 2/4.
>>
>> compiled and tested this on APL and i see iTCO_WDT driver loads fine.
>> Since
>> it impacts core WDT functionality, need to be thoroughly tested on
>> various
>> platforms.
>>
>
> I don't think I (or the watchdog mailing list) was copied on the
> original patch.
Sorry. Its my mistake. I will fix it in next series update.
> Major immediate concern is that this introduces a dependency on
> external code.
> The pmc_ipc driver's Kconfig entry states "This is not needed for PC-type
> machines". I don't know where the function is introduced, but I hope
> this change
> does not require the pmc_ipc code to be present on such machines for
> the watchdog
> to work. It would be bad if it does. If it doesn't, it appears that
> the function
> should not be declared in asm/intel_pmc_ipc.h.
It should not create any compile time dependency with INTEL_PMC_IPC
config option. If INTEL_PMC_IPC_CONFIG is disabled, we use
empty definitions for these calls defined in asm/intel_pmc_ipc.h
But iTCO_wdt driver already has runtime dependency with INTEL_PMIC_IPC
if its version iTCO_version >= 2.
>
> Guenter
>
>
--
Sathyanarayanan Kuppuswamy
Android kernel developer
[toc] | [prev] | [next] | [standalone]
| From | sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 18:30 +0100 |
| Subject | Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure |
| Message-ID | <tm3Ro-1t5-27@gated-at.bofh.it> |
| In reply to | #1603236 |
Hi Rajneesh,
On 03/17/2017 04:43 AM, Rajneesh Bhardwaj wrote:
> On Thu, Mar 16, 2017 at 05:41:35PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Currently, iTCO watchdog driver uses memory map to access
>> PMC_CFG GCR register. But the entire GCR address space is
>> already mapped in intel_scu_ipc driver. So remapping the
> intel_pmc_ipc driver.
Will fix it in next series.
>
>> GCR register in this driver causes the mem request failure in
>> iTCO_wdt probe function. This patch fixes this issue by
>> using PMC GCR read/write API's to access PMC_CFG register.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/watchdog/iTCO_wdt.c | 31 +++++++------------------------
>> 1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
>> index 3d0abc0..31abfc5 100644
>> --- a/drivers/watchdog/iTCO_wdt.c
>> +++ b/drivers/watchdog/iTCO_wdt.c
>> @@ -68,6 +68,8 @@
>> #include <linux/io.h> /* For inb/outb/... */
>> #include <linux/platform_data/itco_wdt.h>
>>
>> +#include <asm/intel_pmc_ipc.h>
>> +
>> #include "iTCO_vendor.h"
>>
>> /* Address definitions for the TCO */
>> @@ -94,12 +96,6 @@ struct iTCO_wdt_private {
>> unsigned int iTCO_version;
>> struct resource *tco_res;
>> struct resource *smi_res;
>> - /*
>> - * NO_REBOOT flag is Memory-Mapped GCS register bit 5 (TCO version 2),
>> - * or memory-mapped PMC register bit 4 (TCO version 3).
>> - */
> Better to retain this comment elsewhere.
no_reboot_bit() function might be better place for this comment.
>
>> - struct resource *gcs_pmc_res;
>> - unsigned long __iomem *gcs_pmc;
>> /* the lock for io operations */
>> spinlock_t io_lock;
>> /* the PCI-device */
>> @@ -176,9 +172,9 @@ static void iTCO_wdt_set_NO_REBOOT_bit(struct iTCO_wdt_private *p)
>>
>> /* Set the NO_REBOOT bit: this disables reboots */
>> if (p->iTCO_version >= 2) {
>> - val32 = readl(p->gcs_pmc);
>> + val32 = intel_pmc_gcr_read(PMC_GCR_PMC_CFG_REG);
> better to have protection and error handling, discussed in v2, 2/4.
>
> compiled and tested this on APL and i see iTCO_WDT driver loads fine. Since
> it impacts core WDT functionality, need to be thoroughly tested on various
> platforms.
I have tested it in Joule and it works fine. it would be nice if some
one can verify it in non-atom platforms.
>
>> --
>> 2.7.4
>>
--
Sathyanarayanan Kuppuswamy
Android kernel developer
[toc] | [prev] | [next] | [standalone]
| From | Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 02:00 +0100 |
| Subject | [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset |
| Message-ID | <tlOpk-6qV-11@gated-at.bofh.it> |
| In reply to | #1602801 |
According to the PMC spec, gcr offset from ipc mem region is 0x1000(4K). But currently this driver uses 0x1008 as gcr offset. This patch fixes this issue. Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- drivers/platform/x86/intel_pmc_ipc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index 0651d47..0a33592 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -82,7 +82,7 @@ /* exported resources from IFWI */ #define PLAT_RESOURCE_IPC_INDEX 0 #define PLAT_RESOURCE_IPC_SIZE 0x1000 -#define PLAT_RESOURCE_GCR_OFFSET 0x1008 +#define PLAT_RESOURCE_GCR_OFFSET 0x1000 #define PLAT_RESOURCE_GCR_SIZE 0x1000 #define PLAT_RESOURCE_BIOS_DATA_INDEX 1 #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2 -- 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com> |
|---|---|
| Date | 2017-03-17 12:20 +0100 |
| Subject | Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset |
| Message-ID | <tlY5j-5AZ-9@gated-at.bofh.it> |
| In reply to | #1602959 |
On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote: > According to the PMC spec, gcr offset from ipc mem > region is 0x1000(4K). But currently this driver uses > 0x1008 as gcr offset. This patch fixes this issue. > This one is fine and was one of the WIP patches. This now enables further cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base. CC: Shanth Murthy <shanth.murthy@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > drivers/platform/x86/intel_pmc_ipc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c > index 0651d47..0a33592 100644 > --- a/drivers/platform/x86/intel_pmc_ipc.c > +++ b/drivers/platform/x86/intel_pmc_ipc.c > @@ -82,7 +82,7 @@ > /* exported resources from IFWI */ > #define PLAT_RESOURCE_IPC_INDEX 0 > #define PLAT_RESOURCE_IPC_SIZE 0x1000 > -#define PLAT_RESOURCE_GCR_OFFSET 0x1008 > +#define PLAT_RESOURCE_GCR_OFFSET 0x1000 > #define PLAT_RESOURCE_GCR_SIZE 0x1000 > #define PLAT_RESOURCE_BIOS_DATA_INDEX 1 > #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2 > -- > 2.7.4 > -- Best Regards, Rajneesh
[toc] | [prev] | [next] | [standalone]
| From | sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> |
|---|---|
| Date | 2017-03-17 18:20 +0100 |
| Subject | Re: [PATCH v2 1/4] platform/x86: intel_pmc_ipc: fix gcr offset |
| Message-ID | <tm3HI-1mJ-23@gated-at.bofh.it> |
| In reply to | #1603205 |
On 03/17/2017 04:13 AM, Rajneesh Bhardwaj wrote: > On Thu, Mar 16, 2017 at 05:41:33PM -0700, Kuppuswamy Sathyanarayanan wrote: >> According to the PMC spec, gcr offset from ipc mem >> region is 0x1000(4K). But currently this driver uses >> 0x1008 as gcr offset. This patch fixes this issue. >> > This one is fine and was one of the WIP patches. This now enables further > cleanup and we should re-align GCR_TELEM_DEEP_S0IX_OFFSET from gcr_base. Since S0IX_OFFSET currently does not use GCR_OFFSET as base, I think that change is irrelevant to this fix. I can submit another patch for S0IX_OFFSET cleanup. > > CC: Shanth Murthy <shanth.murthy@intel.com> > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> drivers/platform/x86/intel_pmc_ipc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c >> index 0651d47..0a33592 100644 >> --- a/drivers/platform/x86/intel_pmc_ipc.c >> +++ b/drivers/platform/x86/intel_pmc_ipc.c >> @@ -82,7 +82,7 @@ >> /* exported resources from IFWI */ >> #define PLAT_RESOURCE_IPC_INDEX 0 >> #define PLAT_RESOURCE_IPC_SIZE 0x1000 >> -#define PLAT_RESOURCE_GCR_OFFSET 0x1008 >> +#define PLAT_RESOURCE_GCR_OFFSET 0x1000 >> #define PLAT_RESOURCE_GCR_SIZE 0x1000 >> #define PLAT_RESOURCE_BIOS_DATA_INDEX 1 >> #define PLAT_RESOURCE_BIOS_IFACE_INDEX 2 >> -- >> 2.7.4 >> -- Sathyanarayanan Kuppuswamy Android kernel developer
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web