Re: [PATCH] IOSF: Add interface for the cases requiring fid

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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