Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751414AbcDKKZq (ORCPT ); Mon, 11 Apr 2016 06:25:46 -0400 Received: from mga11.intel.com ([192.55.52.93]:5252 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750719AbcDKKZp (ORCPT ); Mon, 11 Apr 2016 06:25:45 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,462,1455004800"; d="scan'208";a="684304659" Message-ID: <1460370401.6620.67.camel@linux.intel.com> Subject: Re: [PATCH] IOSF: Add interface for the cases requiring fid From: Andy Shevchenko To: Fei Yang , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "Rafael J. Wysocki" , Boon Leong Ong Cc: linux-kernel@vger.kernel.org, Fei Yang Date: Mon, 11 Apr 2016 13:26:41 +0300 In-Reply-To: <1460149376-101469-1-git-send-email-fei.yang@linux.intel.com> References: <1460149376-101469-1-git-send-email-fei.yang@linux.intel.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4809 Lines: 193 On Fri, 2016-04-08 at 14:02 -0700, Fei Yang wrote: > From: Fei Yang > In subject better to use x86/platform/iosf_mbi prefix. > Some implementations may require an additional step for setting the > FID What FID stands for? > bits to ensure correct transactions over the IOSF side band interface. > Add the FID support accordingly for such implementations > > Change-Id: Ic0227f9e74133a3203aa08e8471939f905d19622 This should not be here. > --- a/arch/x86/include/asm/iosf_mbi.h > +++ b/arch/x86/include/asm/iosf_mbi.h > @@ -88,6 +89,32 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, > u32 mdr); >   */ >  int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 > mask); >   > +/** > + * iosf_mbi_read_with_fid() - MailBox Interface read command > requiring fid > + * @fid: fid bits > + * @port: port indicating subunit being accessed > + * @opcode: port specific read or write opcode > + * @offset: register address offset > + * @mdr: register data to be read > + * > + * Locking is handled by spinlock - cannot sleep. > + * Return: Nonzero on error > + */ > +int iosf_mbi_read_with_fid(u32 fid, u8 port, u8 opcode, u32 offset, > u32 *mdr); > + > +/** > + * iosf_mbi_write_with_fid() - MailBox unmasked write command > requiring fid > + * @fid: fid bits > + * @port: port indicating subunit being accessed > + * @opcode: port specific read or write opcode > + * @offset: register address offset > + * @mdr: register data to be written > + * > + * Locking is handled by spinlock - cannot sleep. > + * Return: Nonzero on error > + */ > +int iosf_mbi_write_with_fid(u32 fid, u8 port, u8 opcode, u32 offset, > u32 mdr); > + >  #else /* CONFIG_IOSF_MBI is not enabled */ >  static inline >  bool iosf_mbi_available(void) > diff --git a/arch/x86/platform/intel/iosf_mbi.c > b/arch/x86/platform/intel/iosf_mbi.c > index edf2c54..af182c1 100644 > --- a/arch/x86/platform/intel/iosf_mbi.c > +++ b/arch/x86/platform/intel/iosf_mbi.c > @@ -98,6 +98,24 @@ fail_write: >   return result; >  } >   > +static int iosf_mbi_pci_write_fid(u32 fid) Function name should continue already used pattern, i.e. …_write_mcrp() > +{ > + int result; > + > + if (!mbi_pdev) > + return -ENODEV; > + > + result = pci_write_config_dword(mbi_pdev, MBI_MCRP_OFFSET, > fid); The function of one line. So, please, inline it directly where it's used. > + if (result < 0) > + goto fail_fid_write; > + > + return 0; > + > +fail_fid_write: > + dev_err(&mbi_pdev->dev, "PCI config access failed with %d\n", > result); > + return result; > +} > + >  int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr) >  { >   u32 mcr, mcrx; > @@ -183,6 +201,61 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 > offset, u32 mdr, u32 mask) >  } >  EXPORT_SYMBOL(iosf_mbi_modify); >   > +/* > + * Some IP blocks require fid to access their registers. Any user? This API doesn't make much sense without user. > + * fid value is programmed through MCRP register, see above function > + * iosf_mbi_pci_write_fid() for details. > + */ > +int iosf_mbi_read_with_fid(u32 fid, u8 port, u8 opcode, u32 offset, > u32 *mdr) Name kinda fuzzy. How user should know which one to choose? Does fid == 0 work for some cases? We have to think about API and naming. > +{ > + u32 mcr, mcrx; > + unsigned long flags; > + int ret; > + > + /*Access to the GFX unit is handled by GPU code */ Spaces. > + if (port == BT_MBI_UNIT_GFX) { > + WARN_ON(1); > + return -EPERM; > + } > + > + mcr = iosf_mbi_form_mcr(opcode, port, offset & MBI_MASK_LO); > + mcrx = offset & MBI_MASK_HI; > + > + spin_lock_irqsave(&iosf_mbi_lock, flags); > + ret = iosf_mbi_pci_write_fid(fid); > + if (!ret) > + ret = iosf_mbi_pci_read_mdr(mcrx, mcr, mdr); > + spin_unlock_irqrestore(&iosf_mbi_lock, flags); > + > + return ret; > +} > +EXPORT_SYMBOL(iosf_mbi_read_with_fid); > + > +int iosf_mbi_write_with_fid(u32 fid, u8 port, u8 opcode, u32 offset, > u32 mdr) > +{ > + u32 mcr, mcrx; > + unsigned long flags; > + int ret; > + > + /*Access to the GFX unit is handled by GPU code */ Ditto. > + if (port == BT_MBI_UNIT_GFX) { > + WARN_ON(1); > + return -EPERM; > + } > + > + mcr = iosf_mbi_form_mcr(opcode, port, offset & MBI_MASK_LO); > + mcrx = offset & MBI_MASK_HI; > + > + spin_lock_irqsave(&iosf_mbi_lock, flags); > + ret = iosf_mbi_pci_write_fid(fid); > + if (!ret) > + ret = iosf_mbi_pci_write_mdr(mcrx, mcr, mdr); > + spin_unlock_irqrestore(&iosf_mbi_lock, flags); Both of them quite similar to already exist _write()/_read(). Is it possible to combine them out? > + > + return ret; > +} > +EXPORT_SYMBOL(iosf_mbi_write_with_fid); > + >  bool iosf_mbi_available(void) >  { >   /* Mbi isn't hot-pluggable. No remove routine is provided */ -- Andy Shevchenko Intel Finland Oy