Re: [PATCH v1 2/5] fpga: dfl: Move the DFH definitions

[matthew.gerlach@linux.intel.com]

On Sun, 11 Sep 2022, Xu Yilun wrote:

> On 2022-09-06 at 12:04:23 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>>
>> Moving the DFH register offset and register definitions from
>> drivers/fpga/dfl.h to include/linux/dfl.h.  These definitions
>> need to be accessed by dfl drivers that are outside of
>> drivers/fpga.
>>
>> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/dfl.h  | 22 ++--------------------
>>  include/linux/dfl.h | 23 ++++++++++++++++++++++-
>>  2 files changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..d4dfc03a0b61 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -2,7 +2,7 @@
>>  /*
>>   * Driver Header File for FPGA Device Feature List (DFL) Support
>>   *
>> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
>> + * Copyright (C) 2017-2022 Intel Corporation, Inc.
>>   *
>>   * Authors:
>>   *   Kang Luwei <luwei.kang@intel.com>
>> @@ -17,6 +17,7 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/cdev.h>
>>  #include <linux/delay.h>
>> +#include <linux/dfl.h>
>>  #include <linux/eventfd.h>
>>  #include <linux/fs.h>
>>  #include <linux/interrupt.h>
>> @@ -53,28 +54,9 @@
>>  #define PORT_FEATURE_ID_UINT		0x12
>>  #define PORT_FEATURE_ID_STP		0x13
>>
>> -/*
>> - * Device Feature Header Register Set
>> - *
>> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
>> - * For AFUs, they have DFH + GUID as common header registers.
>> - * For private features, they only have DFH register as common header.
>> - */
>> -#define DFH			0x0
>> -#define GUID_L			0x8
>> -#define GUID_H			0x10
>> -#define NEXT_AFU		0x18
>> -
>> -#define DFH_SIZE		0x8
>> -
>>  /* Device Feature Header Register Bitfield */
>> -#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>>  #define DFH_ID_FIU_FME		0
>>  #define DFH_ID_FIU_PORT		1
>> -#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>> -#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>> -#define DFH_EOL			BIT_ULL(40)		/* End of list */
>> -#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>>  #define DFH_TYPE_AFU		1
>>  #define DFH_TYPE_PRIVATE	3
>>  #define DFH_TYPE_FIU		4
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..b5accdcfa368 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -2,7 +2,7 @@
>>  /*
>>   * Header file for DFL driver and device API
>>   *
>> - * Copyright (C) 2020 Intel Corporation, Inc.
>> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>>   */
>>
>>  #ifndef __LINUX_DFL_H
>> @@ -11,6 +11,27 @@
>>  #include <linux/device.h>
>>  #include <linux/mod_devicetable.h>
>>
>> +/*
>> + * Device Feature Header Register Set
>> + *
>> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
>> + * For AFUs, they have DFH + GUID as common header registers.
>> + * For private features, they only have DFH register as common header.
>> + */
>> +#define DFH			0x0
>> +#define GUID_L			0x8
>> +#define GUID_H			0x10
>> +#define NEXT_AFU		0x18
>
> Now these macros are accessible in global kernel, should we add the
> DFL_ or DFH_ prefix for them?
>
> Thanks,
> Yilun

It does make sense to a DFL_ or DFH_ to these globabl macros, but I'll 
look again to see if the ones above really need to be global, where as the 
macros below definitely need to be global.  I also think a marco like 
DFL_DFH might be a little strange.

Thanks,
Matthew Gerlach

8>
>> +
>> +#define DFH_SIZE		0x8
>> +
>> +/* Device Feature Header Register Bitfield */
>> +#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>> +#define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
>> +#define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
>> +#define DFH_EOL			BIT_ULL(40)		/* End of list */
>> +#define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>> +
>>  /**
>>   * enum dfl_id_type - define the DFL FIU types
>>   */
>> --
>> 2.25.1
>>
>