| Message ID | 1463541972-19758-3-git-send-email-jonathan.yong@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 05/17/2016 08:26 PM, Yong, Jonathan wrote: > The TCO I/O base is 40h rather than the usual 30h, and the re_reboot > bit is at ACPIBASE+8. > Does this mean that the code never worked, or in other words that it was never tested, or are there now other chips which used the old definitions which no longer work after this patch has been applied ? > Signed-off-by: Yong, Jonathan <jonathan.yong@intel.com> > --- > drivers/platform/x86/intel_pmc_ipc.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c > index 6f497e8..b86e1bc 100644 > --- a/drivers/platform/x86/intel_pmc_ipc.c > +++ b/drivers/platform/x86/intel_pmc_ipc.c > @@ -85,7 +85,7 @@ > * platform device and to export resources for those functions. > */ > #define TCO_DEVICE_NAME "iTCO_wdt" > -#define SMI_EN_OFFSET 0x30 > +#define SMI_EN_OFFSET 0x40 > #define SMI_EN_SIZE 4 > #define TCO_BASE_OFFSET 0x60 > #define TCO_REGS_SIZE 16 > @@ -94,6 +94,8 @@ > #define TELEM_SSRAM_SIZE 240 > #define TELEM_PMC_SSRAM_OFFSET 0x1B00 > #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00 > +#define TCO_PMC_OFFSET 0x8 > +#define TCO_PMC_SIZE 0x4 > > static const int iTCO_version = 3; > > @@ -502,7 +504,7 @@ static struct resource tco_res[] = { > > static struct itco_wdt_platform_data tco_info = { > .name = "Apollo Lake SoC", > - .version = 3, > + .version = 5, > }; > > #define TELEMETRY_RESOURCE_PUNIT_SSRAM 0 > @@ -572,8 +574,8 @@ static int ipc_create_tco_device(void) > res->end = res->start + SMI_EN_SIZE - 1; > > res = tco_res + TCO_RESOURCE_GCR_MEM; > - res->start = ipcdev.gcr_base; > - res->end = res->start + ipcdev.gcr_size - 1; > + res->start = ipcdev.gcr_base + TCO_PMC_OFFSET; > + res->end = res->start + TCO_PMC_SIZE - 1; > > ret = platform_device_add_resources(pdev, tco_res, ARRAY_SIZE(tco_res)); > if (ret) { > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2016 12:03, Guenter Roeck wrote: > On 05/17/2016 08:26 PM, Yong, Jonathan wrote: >> The TCO I/O base is 40h rather than the usual 30h, and the >> re_reboot bit is at ACPIBASE+8. >> > > Does this mean that the code never worked, or in other words that it > was never tested, or are there now other chips which used the old > definitions which no longer work after this patch has been applied ? To my knowledge, it has never worked since 30h is claimed by ACPI GPE0_BLK. With this patch, watchdog ioctl is working properly with WDIOC_SETTIMEOUT, WDIOC_GETTIMEOUT and WDIOC_GETTIMELEFT as expected. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/17/2016 09:48 PM, Yong, Jonathan wrote: > On 05/18/2016 12:03, Guenter Roeck wrote: >> On 05/17/2016 08:26 PM, Yong, Jonathan wrote: >>> The TCO I/O base is 40h rather than the usual 30h, and the >>> re_reboot bit is at ACPIBASE+8. >>> >> >> Does this mean that the code never worked, or in other words that it >> was never tested, or are there now other chips which used the old >> definitions which no longer work after this patch has been applied ? > > To my knowledge, it has never worked since 30h is claimed by ACPI GPE0_BLK. > Hmm.. Darren will have to take that one. I am not in a position to determine if the new patch is correct or not, and/or if it causes some other platform/architecture to fail. I'll Ack the other patch, and would suggest for Darren to take it as well to keep the two patches together. Guenter > With this patch, watchdog ioctl is working properly with WDIOC_SETTIMEOUT, WDIOC_GETTIMEOUT and WDIOC_GETTIMELEFT as expected. > -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 18, 2016 at 07:37:00AM -0700, Guenter Roeck wrote: > On 05/17/2016 09:48 PM, Yong, Jonathan wrote: > >On 05/18/2016 12:03, Guenter Roeck wrote: > >>On 05/17/2016 08:26 PM, Yong, Jonathan wrote: > >>>The TCO I/O base is 40h rather than the usual 30h, and the > >>>re_reboot bit is at ACPIBASE+8. > >>> > >> > >>Does this mean that the code never worked, or in other words that it > >>was never tested, or are there now other chips which used the old > >>definitions which no longer work after this patch has been applied ? > > > >To my knowledge, it has never worked since 30h is claimed by ACPI GPE0_BLK. > > > Hmm.. Darren will have to take that one. I am not in a position to > determine if the new patch is correct or not, and/or if it causes some > other platform/architecture to fail. I'll Ack the other patch, and > would suggest for Darren to take it as well to keep the two patches together. I actually have the same exact question :-) The history of this driver is for Apollo Lake, so the question is if there exist v3 and v5 APL products, or if we only need to concern ourselves with v5 in the upstream driver. Especially given the "I don't know if this is the right way" in the cover letter, I will need an Acked-by at the very least from Qipeng. Jonathan, please remember to Cc ALL the maintainers and all the lists listed by get_maintainers when sending patches: $ scripts/get_maintainer.pl -f drivers/platform/x86/intel_pmc_ipc.c Zha Qipeng <qipeng.zha@intel.com> (maintainer:INTEL PMC/P-Unit IPC DRIVER) Darren Hart <dvhart@infradead.org> (maintainer:X86 PLATFORM DRIVERS) platform-driver-x86@vger.kernel.org (open list:INTEL PMC/P-Unit IPC DRIVER) linux-kernel@vger.kernel.org (open list) In particular, ALL patches MUST cc LKML. > > Guenter > > >With this patch, watchdog ioctl is working properly with WDIOC_SETTIMEOUT, WDIOC_GETTIMEOUT and WDIOC_GETTIMELEFT as expected. > > > > -- > To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 05/25/2016 04:13, Darren Hart wrote: > > The history of this driver is for Apollo Lake, so the question is if there exist > v3 and v5 APL products, or if we only need to concern ourselves with v5 in the > upstream driver. > > Especially given the "I don't know if this is the right way" in the cover > letter, I will need an Acked-by at the very least from Qipeng. > Hi Qipeng, Do you know the history behind drivers/platform/x86/intel_pmc_ipc.c? Were the specs based on earlier prototypes? -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2016 15:55, Yong, Jonathan wrote: > On 05/25/2016 04:13, Darren Hart wrote: >> >> The history of this driver is for Apollo Lake, so the question is if >> there exist >> v3 and v5 APL products, or if we only need to concern ourselves with >> v5 in the >> upstream driver. >> >> Especially given the "I don't know if this is the right way" in the cover >> letter, I will need an Acked-by at the very least from Qipeng. >> > > Hi Qipeng, > > Do you know the history behind drivers/platform/x86/intel_pmc_ipc.c? > Were the specs based on earlier prototypes? > Qipeng, ping? -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 04:28:03PM +0800, Yong, Jonathan wrote: > On 05/25/2016 15:55, Yong, Jonathan wrote: > > On 05/25/2016 04:13, Darren Hart wrote: > > > > > > The history of this driver is for Apollo Lake, so the question is if > > > there exist > > > v3 and v5 APL products, or if we only need to concern ourselves with > > > v5 in the > > > upstream driver. > > > > > > Especially given the "I don't know if this is the right way" in the cover > > > letter, I will need an Acked-by at the very least from Qipeng. > > > > > > > Hi Qipeng, > > > > Do you know the history behind drivers/platform/x86/intel_pmc_ipc.c? > > Were the specs based on earlier prototypes? > > > > Qipeng, ping? > Jonathan, can you follow-up with Qiping (possibly internally) and get back to the list with a definitive response? Getting these into 4.7-rc would be good so we don't have to push it back via stable, but we need to be sure it's the right fix.
On 06/08/2016 06:03, Darren Hart wrote: >>> >>> Hi Qipeng, >>> >>> Do you know the history behind drivers/platform/x86/intel_pmc_ipc.c? >>> Were the specs based on earlier prototypes? >>> >> >> Qipeng, ping? >> > > Jonathan, can you follow-up with Qiping (possibly internally) and get back to > the list with a definitive response? Getting these into 4.7-rc would be good so > we don't have to push it back via stable, but we need to be sure it's the right > fix. > I checked with Qipeng, the 0x30h offset was based on an earlier internal prototype, 0x40h should be OK for Apollo Lake. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c index 6f497e8..b86e1bc 100644 --- a/drivers/platform/x86/intel_pmc_ipc.c +++ b/drivers/platform/x86/intel_pmc_ipc.c @@ -85,7 +85,7 @@ * platform device and to export resources for those functions. */ #define TCO_DEVICE_NAME "iTCO_wdt" -#define SMI_EN_OFFSET 0x30 +#define SMI_EN_OFFSET 0x40 #define SMI_EN_SIZE 4 #define TCO_BASE_OFFSET 0x60 #define TCO_REGS_SIZE 16 @@ -94,6 +94,8 @@ #define TELEM_SSRAM_SIZE 240 #define TELEM_PMC_SSRAM_OFFSET 0x1B00 #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00 +#define TCO_PMC_OFFSET 0x8 +#define TCO_PMC_SIZE 0x4 static const int iTCO_version = 3; @@ -502,7 +504,7 @@ static struct resource tco_res[] = { static struct itco_wdt_platform_data tco_info = { .name = "Apollo Lake SoC", - .version = 3, + .version = 5, }; #define TELEMETRY_RESOURCE_PUNIT_SSRAM 0 @@ -572,8 +574,8 @@ static int ipc_create_tco_device(void) res->end = res->start + SMI_EN_SIZE - 1; res = tco_res + TCO_RESOURCE_GCR_MEM; - res->start = ipcdev.gcr_base; - res->end = res->start + ipcdev.gcr_size - 1; + res->start = ipcdev.gcr_base + TCO_PMC_OFFSET; + res->end = res->start + TCO_PMC_SIZE - 1; ret = platform_device_add_resources(pdev, tco_res, ARRAY_SIZE(tco_res)); if (ret) {
The TCO I/O base is 40h rather than the usual 30h, and the re_reboot bit is at ACPIBASE+8. Signed-off-by: Yong, Jonathan <jonathan.yong@intel.com> --- drivers/platform/x86/intel_pmc_ipc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)