Re: [PATCH v1 1/1] platform/x86: intel_pmc_ipc: fix io mem mapping size

[Rajneesh Bhardwaj]

On Thu, Mar 16, 2017 at 11:50:16AM -0700, sathyanarayanan kuppuswamy wrote:
> Hi,
> 
> 
> On 03/16/2017 07:52 AM, Rajneesh Bhardwaj wrote:
> >On Wed, Mar 15, 2017 at 08:32:53PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >>Mapping entire GCR mem region in this driver creates
> >>mem region request conflict in sub devices that depend
> >>on PMC. This creates driver probe failure in devices like
> >>iTC0_wdt and telemetry device.
> >While this patch might fix the issue for now but IMHO its not taking the
> >right approch. I guess we need some guidance here from the maintainers but
> Agreed. I thought about this problem and I know this solution does not
> scale well. Other way is to expose an API for GCR access and use it on
> PMC dependent drivers. In this case, we should also add GCR register
> address macros to PMC header file and this might need change to PMC header
> file each time when some one wants to add access new register address.
> 
> Since S0ix counter access is only GCR access use case in PMC driver, I
> thought  both solutions has some pros and cons.
> >please do consider the below explaination for why we shoud not take this
> >approch to fix WDT issue. Telemetry driver has no issues while loading since
> >its not using any register in the GCR region.
> >PMC on BXT/APL platforms has contiguous IPC and GCR regions. PMC_IPC driver
> >maps the entire IPC and GCR region. It would be inefficient to map and unmap
> >each time we want to use another register present in IPC or GCR spaces.
> But I am wondering whether there will be any future GCR access
> changes in PMC driver?

You never know!

> If its going to be just S0ix counter access then I don't think we
> need to worry about performance here.

Ditto, its not about performance.

> >
> >iTCO_WDT driver needs to check the BIT4 (NO_REBOOT) of PMC_CFG register
> >(Offset: 0x1008) and this falls in GCR space. The iTCO_WDT driver fails to
> >load because it can't request mem region for the resources already claimed
> >by PMC_IPC driver. However, ioremap would still work here and WDT driver
> >would load just fine.
> >
> >So, IMHO the problem lies elsewhere and we should find a way to handle this
> >better in iTCO_WDT driver.
> >
> >The IPC and GCR resources belong to PMC and should be claimed by the PMC
> >driver rightfully and should not be reclaimed by iTCO_WDT or any other
> >driver.
> iTCO_WDT , Telemtry and Punit are PMC dependent devices right ? And they
> share the resources among them right ?

Resources belong to PMC and hence it should own them, others share. Other
drivers request and map those resources only when PMC driver does not
already do so.

> >
> >
> >>Currently this driver only need memory mapping for
> >>s0ix counter registers. So this patch fixes this issue
> >>by requesting memory mapping for only the s0ix counter mem
> >>region.
> >How about exposing a new API in PMC_IPC driver which can be used for reading
> >the desired GCR register and it can be used by iTCO_WDT instead of
> >requesting mem regions and remapping?
> If you think in future if we might need access to more GCR space in
> PMC driver, then we need to change this solution. 
>
> Please let me know. If yes, I will add an API as you mentioned.

Yes, I think so. Hope Andy and Darren are fine with that?


> >

-- 
Best Regards,
Rajneesh