Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754557AbdDDVgz (ORCPT ); Tue, 4 Apr 2017 17:36:55 -0400 Received: from mga14.intel.com ([192.55.52.115]:1629 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754281AbdDDVgx (ORCPT ); Tue, 4 Apr 2017 17:36:53 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,275,1486454400"; d="scan'208";a="841890519" Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH v4 1/5] platform/x86: intel_pmc_ipc: fix gcr offset References: <20170331133728.GA23725@rajaneesh-OptiPlex-9010> To: Andy Shevchenko , Sathyanarayanan Kuppuswamy Natarajan Cc: Andy Shevchenko , Zha Qipeng , "dvhart@infradead.org" , Guenter Roeck , Wim Van Sebroeck , David Box , Rajneesh Bhardwaj , Platform Driver , "linux-kernel@vger.kernel.org" , linux-watchdog@vger.kernel.org From: sathyanarayanan kuppuswamy Organization: Intel Message-ID: <3e7ca77c-2be5-5f89-f51f-faaca9f2d9ac@linux.intel.com> Date: Tue, 4 Apr 2017 14:32:50 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2764 Lines: 77 On 04/04/2017 06:25 AM, Andy Shevchenko wrote: > Please, STOP top-posting. > > On Mon, Apr 3, 2017 at 4:51 AM, Sathyanarayanan Kuppuswamy Natarajan > wrote: >> Yes, just applying this patch will fix the existing offset issue. > Then the question how it was tested in both cases (before this change and now) ? iTCO_wdt is the only driver which uses this offset when calculating the address of PMC_CFG for setting/unsetting the no-reboot bit. Without this change, instead of modifying the PMC_CFG(offset:0x1008) register, iTCO_wdt was modifying the address with offset 0x1010. According to spec, offset 0x1010 is unused memory address. So modifying that area did not create any errors, but its not functionally correct. So with or without this change you will not see any error message. But its creating the functional issue of not modifying the no-reboot bit. Currently, I tested this fix by duping the GCR registers and verified whether the no-reboot bit is modified or not. > >> On Sun, Apr 2, 2017 at 7:11 AM, Andy Shevchenko >> wrote: >>> On Sat, Apr 1, 2017 at 2:27 AM, Kuppuswamy Sathyanarayanan >>> wrote: >>>> According to Broxton APL PMC spec, gcr mem region starts >>>> at offset 0x1000 from ipc mem base address. In this driver, >>>> PLAT_RESOURCE_GCR_OFFSET macro defines the offset of GCR >>>> memory region from IPC mem region. So we should use 0x1000(4K) >>>> as GCR offset. But currently this driver uses 0x1008 as GCT >>>> offset.This patch fixes this issue. >>> >>> So, if I apply this one independently, would it fix an existin issue? >>> >>>> Signed-off-by: Kuppuswamy Sathyanarayanan >>>> --- >>>> drivers/platform/x86/intel_pmc_ipc.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> Changes since v3: >>>> * Updated the commit history >>>> >>>> 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 >>>> >>> >>> >>> -- >>> With Best Regards, >>> Andy Shevchenko >> >> >> -- >> -- >> >> Sathya > > -- Sathyanarayanan Kuppuswamy Android kernel developer