On Fri, 2016-04-08 at 14:02 -0700, Fei Yang wrote:
> From: Fei Yang <fei.y...@intel.com>
>
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 <andriy.shevche...@linux.intel.com>
Intel Finland Oy
