[PATCH v9 0/6] add DFL bus support to MODULE_DEVICE_TABLE()

[PATCH v9 0/6] add DFL bus support to MODULE_DEVICE_TABLE()

[Xu Yilun]

Patch #1 is a fix of the fields in struct dfl_device & dfl_device_id.

Patch #2, 3, 4 add dfl bus support to MODULE_DEVICE_TABLE(), they also
move the necessary definitions in head file to public folders so that
scatter dfl drivers could use them.

Patch #5 is the dfl driver for N3000 Nios private feature, this is the 
first use case of dfl bus.

Patch #6 is the dfl driver for DFL EMIF private feature.


The patchset version number is jumped because I added the Patch #5 in
this patchset, which has a larger version number.


Main changes from v1: 
- A new patch (Patch #3) to fix the description.
- Rename the dfl-bus.h to dfl.h
- Updated the MAINTAINERS under FPGA DFL DRIVERS.
- Improve comments and minor fixes.

Main changes from v2: 
- Change the bus name from "dfl" to "fpga-dfl", also rename related
  variables, functions ... 
- Changes the data type of fpga_dfl_device_id.type from u8 to u16 
- Explicitly defines the values of enum fpga_dfl_id_type
- Delete the comments for the valid bits of type & feature_id
- changes MODALIAS format for fpga dfl devices

Main changes from v3: 
- Change the bus name back to "dfl".
- Add 2 patches (#5, 6) for dfl drivers.
- Delete the retimer FEC mode configuration via module_parameter for 
  Patch #5
- Merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@huawei.com/T/#t)
  into Patch #5
- Add static prefix for emif attributes macro for Patch #6


Xu Yilun (6):
  fpga: dfl: fix the definitions of type & feature_id for dfl devices
  fpga: dfl: move dfl_device_id to mod_devicetable.h
  fpga: dfl: add dfl bus support to MODULE_DEVICE_TABLE()
  fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h
  fpga: dfl: add support for N3000 Nios private feature
  memory: dfl-emif: add the DFL EMIF private feature driver

 .../ABI/testing/sysfs-bus-dfl-devices-emif         |  25 +
 .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  47 ++
 MAINTAINERS                                        |   3 +-
 drivers/fpga/Kconfig                               |  11 +
 drivers/fpga/Makefile                              |   2 +
 drivers/fpga/dfl-n3000-nios.c                      | 566 +++++++++++++++++++++
 drivers/fpga/dfl.c                                 |   4 +-
 drivers/fpga/dfl.h                                 |  85 +---
 drivers/memory/Kconfig                             |   9 +
 drivers/memory/Makefile                            |   2 +
 drivers/memory/dfl-emif.c                          | 207 ++++++++
 include/linux/fpga/dfl.h                           |  86 ++++
 include/linux/mod_devicetable.h                    |  12 +
 scripts/mod/devicetable-offsets.c                  |   4 +
 scripts/mod/file2alias.c                           |  13 +
 15 files changed, 989 insertions(+), 87 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
 create mode 100644 drivers/fpga/dfl-n3000-nios.c
 create mode 100644 drivers/memory/dfl-emif.c
 create mode 100644 include/linux/fpga/dfl.h

-- 
2.7.4

[PATCH v9 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Xu Yilun]

The value of the field dfl_device.type comes from the 12 bits register
field DFH_ID according to DFL spec. So this patch changes the definition
of the type field to u16.

Also it is not necessary to illustrate the valid bits of the type field
in comments. Instead we should explicitly define the possible values in
the enumeration type for it, because they are shared by hardware spec.
We should not let the compiler decide these values.

Similar changes are also applied to dfl_device.feature_id.

This patch also fixed the MODALIAS format according to the changes
above.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
v9: no change
---
 drivers/fpga/dfl.c |  3 +--
 drivers/fpga/dfl.h | 14 +++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b450870..5a6ba3b 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct dfl_device *ddev = to_dfl_dev(dev);
 
-	/* The type has 4 valid bits and feature_id has 12 valid bits */
-	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
+	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
 			      ddev->type, ddev->feature_id);
 }
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 5dc758f..ac373b1 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
  * enum dfl_id_type - define the DFL FIU types
  */
 enum dfl_id_type {
-	FME_ID,
-	PORT_ID,
+	FME_ID = 0,
+	PORT_ID = 1,
 	DFL_ID_MAX,
 };
 
 /**
  * struct dfl_device_id -  dfl device identifier
- * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
- * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
+ * @type: DFL FIU type of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
  * @driver_data: driver specific data.
  */
 struct dfl_device_id {
-	u8 type;
+	u16 type;
 	u16 feature_id;
 	unsigned long driver_data;
 };
@@ -543,7 +543,7 @@ struct dfl_device_id {
  * @dev: generic device interface.
  * @id: id of the dfl device.
  * @type: type of DFL FIU of the device. See enum dfl_id_type.
- * @feature_id: 16 bits feature identifier local to its DFL FIU type.
+ * @feature_id: feature identifier local to its DFL FIU type.
  * @mmio_res: mmio resource of this dfl device.
  * @irqs: list of Linux IRQ numbers of this dfl device.
  * @num_irqs: number of IRQs supported by this dfl device.
@@ -553,7 +553,7 @@ struct dfl_device_id {
 struct dfl_device {
 	struct device dev;
 	int id;
-	u8 type;
+	u16 type;
 	u16 feature_id;
 	struct resource mmio_res;
 	int *irqs;
-- 
2.7.4

Re: [PATCH v9 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Tom Rix]

On 10/10/20 12:09 AM, Xu Yilun wrote:
> The value of the field dfl_device.type comes from the 12 bits register
> field DFH_ID according to DFL spec. So this patch changes the definition
> of the type field to u16.
>
> Also it is not necessary to illustrate the valid bits of the type field
> in comments. Instead we should explicitly define the possible values in
> the enumeration type for it, because they are shared by hardware spec.
> We should not let the compiler decide these values.
>
> Similar changes are also applied to dfl_device.feature_id.
>
> This patch also fixed the MODALIAS format according to the changes
> above.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
> v9: no change
> ---
>  drivers/fpga/dfl.c |  3 +--
>  drivers/fpga/dfl.h | 14 +++++++-------
>  2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b450870..5a6ba3b 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct dfl_device *ddev = to_dfl_dev(dev);
>  
> -	/* The type has 4 valid bits and feature_id has 12 valid bits */
> -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
>  			      ddev->type, ddev->feature_id);
>  }
>  
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 5dc758f..ac373b1 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>   * enum dfl_id_type - define the DFL FIU types
>   */
>  enum dfl_id_type {
> -	FME_ID,
> -	PORT_ID,
> +	FME_ID = 0,
> +	PORT_ID = 1,

This is redundant, why make this change ?

Tom

>  	DFL_ID_MAX,
>  };
>  
>  /**
>   * struct dfl_device_id -  dfl device identifier
> - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
> - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
> + * @type: DFL FIU type of the device. See enum dfl_id_type.
> + * @feature_id: feature identifier local to its DFL FIU type.
>   * @driver_data: driver specific data.
>   */
>  struct dfl_device_id {
> -	u8 type;
> +	u16 type;
>  	u16 feature_id;
>  	unsigned long driver_data;
>  };
> @@ -543,7 +543,7 @@ struct dfl_device_id {
>   * @dev: generic device interface.
>   * @id: id of the dfl device.
>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
> - * @feature_id: 16 bits feature identifier local to its DFL FIU type.
> + * @feature_id: feature identifier local to its DFL FIU type.
>   * @mmio_res: mmio resource of this dfl device.
>   * @irqs: list of Linux IRQ numbers of this dfl device.
>   * @num_irqs: number of IRQs supported by this dfl device.
> @@ -553,7 +553,7 @@ struct dfl_device_id {
>  struct dfl_device {
>  	struct device dev;
>  	int id;
> -	u8 type;
> +	u16 type;
>  	u16 feature_id;
>  	struct resource mmio_res;
>  	int *irqs;

Re: [PATCH v9 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Xu Yilun]

On Sat, Oct 10, 2020 at 08:07:07AM -0700, Tom Rix wrote:
> 
> On 10/10/20 12:09 AM, Xu Yilun wrote:
> > The value of the field dfl_device.type comes from the 12 bits register
> > field DFH_ID according to DFL spec. So this patch changes the definition
> > of the type field to u16.
> >
> > Also it is not necessary to illustrate the valid bits of the type field
> > in comments. Instead we should explicitly define the possible values in
> > the enumeration type for it, because they are shared by hardware spec.
> > We should not let the compiler decide these values.
> >
> > Similar changes are also applied to dfl_device.feature_id.
> >
> > This patch also fixed the MODALIAS format according to the changes
> > above.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> > v9: no change
> > ---
> >  drivers/fpga/dfl.c |  3 +--
> >  drivers/fpga/dfl.h | 14 +++++++-------
> >  2 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index b450870..5a6ba3b 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >  	struct dfl_device *ddev = to_dfl_dev(dev);
> >  
> > -	/* The type has 4 valid bits and feature_id has 12 valid bits */
> > -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> > +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
> >  			      ddev->type, ddev->feature_id);
> >  }
> >  
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 5dc758f..ac373b1 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> >   * enum dfl_id_type - define the DFL FIU types
> >   */
> >  enum dfl_id_type {
> > -	FME_ID,
> > -	PORT_ID,
> > +	FME_ID = 0,
> > +	PORT_ID = 1,
> 
> This is redundant, why make this change ?

These values are shared by hardware spec, so it is suggested that the
values of the enum type should be explicitly set, otherwise the compiler
is in its right to do whatever it wants with them (within reason...)

Please see the original discussion:
https://lore.kernel.org/linux-fpga/20200923055436.GA2629915@kroah.com/

Thanks,
Yilun

> 
> Tom
> 
> >  	DFL_ID_MAX,
> >  };
> >  
> >  /**
> >   * struct dfl_device_id -  dfl device identifier
> > - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
> > - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
> > + * @type: DFL FIU type of the device. See enum dfl_id_type.
> > + * @feature_id: feature identifier local to its DFL FIU type.
> >   * @driver_data: driver specific data.
> >   */
> >  struct dfl_device_id {
> > -	u8 type;
> > +	u16 type;
> >  	u16 feature_id;
> >  	unsigned long driver_data;
> >  };
> > @@ -543,7 +543,7 @@ struct dfl_device_id {
> >   * @dev: generic device interface.
> >   * @id: id of the dfl device.
> >   * @type: type of DFL FIU of the device. See enum dfl_id_type.
> > - * @feature_id: 16 bits feature identifier local to its DFL FIU type.
> > + * @feature_id: feature identifier local to its DFL FIU type.
> >   * @mmio_res: mmio resource of this dfl device.
> >   * @irqs: list of Linux IRQ numbers of this dfl device.
> >   * @num_irqs: number of IRQs supported by this dfl device.
> > @@ -553,7 +553,7 @@ struct dfl_device_id {
> >  struct dfl_device {
> >  	struct device dev;
> >  	int id;
> > -	u8 type;
> > +	u16 type;
> >  	u16 feature_id;
> >  	struct resource mmio_res;
> >  	int *irqs;

Re: [PATCH v9 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Tom Rix]

On 10/11/20 7:41 PM, Xu Yilun wrote:
> On Sat, Oct 10, 2020 at 08:07:07AM -0700, Tom Rix wrote:
>> On 10/10/20 12:09 AM, Xu Yilun wrote:
>>> The value of the field dfl_device.type comes from the 12 bits register
>>> field DFH_ID according to DFL spec. So this patch changes the definition
>>> of the type field to u16.
>>>
>>> Also it is not necessary to illustrate the valid bits of the type field
>>> in comments. Instead we should explicitly define the possible values in
>>> the enumeration type for it, because they are shared by hardware spec.
>>> We should not let the compiler decide these values.
>>>
>>> Similar changes are also applied to dfl_device.feature_id.
>>>
>>> This patch also fixed the MODALIAS format according to the changes
>>> above.
>>>
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> ---
>>> v9: no change
>>> ---
>>>  drivers/fpga/dfl.c |  3 +--
>>>  drivers/fpga/dfl.h | 14 +++++++-------
>>>  2 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>> index b450870..5a6ba3b 100644
>>> --- a/drivers/fpga/dfl.c
>>> +++ b/drivers/fpga/dfl.c
>>> @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>>>  {
>>>  	struct dfl_device *ddev = to_dfl_dev(dev);
>>>  
>>> -	/* The type has 4 valid bits and feature_id has 12 valid bits */
>>> -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
>>> +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
>>>  			      ddev->type, ddev->feature_id);
>>>  }
>>>  
>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>> index 5dc758f..ac373b1 100644
>>> --- a/drivers/fpga/dfl.h
>>> +++ b/drivers/fpga/dfl.h
>>> @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>>>   * enum dfl_id_type - define the DFL FIU types
>>>   */
>>>  enum dfl_id_type {
>>> -	FME_ID,
>>> -	PORT_ID,
>>> +	FME_ID = 0,
>>> +	PORT_ID = 1,
>> This is redundant, why make this change ?
> These values are shared by hardware spec, so it is suggested that the
> values of the enum type should be explicitly set, otherwise the compiler
> is in its right to do whatever it wants with them (within reason...)
>
> Please see the original discussion:
> https://lore.kernel.org/linux-fpga/20200923055436.GA2629915@kroah.com/

I don't believe this is undefined behavior for the compiler

from c11 6.7.2.2,3

The identifiers in an enumerator list are declared as constants that have type int and may appear wherever such are permitted.127) An enumerator with = defines its enumeration constant as the value of the constant expression. If the first enumerator has no =, the value of its enumeration constant is 0. Each subsequent enumerator with no = defines its enumeration constant as the value of the constant expression obtained by adding 1 to the value of the previous enumeration constant. (The use of enumerators with = may produce enumeration constants with values that duplicate other values in the same enumeration.) The enumerators of an enumeration are also known as its members.

setting them again has some use for documentation so this change is ok if you have strong feeling for it.

Tom

>
> Thanks,
> Yilun
>
>> Tom
>>
>>>  	DFL_ID_MAX,
>>>  };
>>>  
>>>  /**
>>>   * struct dfl_device_id -  dfl device identifier
>>> - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type.
>>> - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type.
>>> + * @type: DFL FIU type of the device. See enum dfl_id_type.
>>> + * @feature_id: feature identifier local to its DFL FIU type.
>>>   * @driver_data: driver specific data.
>>>   */
>>>  struct dfl_device_id {
>>> -	u8 type;
>>> +	u16 type;
>>>  	u16 feature_id;
>>>  	unsigned long driver_data;
>>>  };
>>> @@ -543,7 +543,7 @@ struct dfl_device_id {
>>>   * @dev: generic device interface.
>>>   * @id: id of the dfl device.
>>>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>>> - * @feature_id: 16 bits feature identifier local to its DFL FIU type.
>>> + * @feature_id: feature identifier local to its DFL FIU type.
>>>   * @mmio_res: mmio resource of this dfl device.
>>>   * @irqs: list of Linux IRQ numbers of this dfl device.
>>>   * @num_irqs: number of IRQs supported by this dfl device.
>>> @@ -553,7 +553,7 @@ struct dfl_device_id {
>>>  struct dfl_device {
>>>  	struct device dev;
>>>  	int id;
>>> -	u8 type;
>>> +	u16 type;
>>>  	u16 feature_id;
>>>  	struct resource mmio_res;
>>>  	int *irqs;

Re: [PATCH v9 1/6] fpga: dfl: fix the definitions of type & feature_id for dfl devices

[Greg KH]

On Mon, Oct 12, 2020 at 07:07:55AM -0700, Tom Rix wrote:
> 
> On 10/11/20 7:41 PM, Xu Yilun wrote:
> > On Sat, Oct 10, 2020 at 08:07:07AM -0700, Tom Rix wrote:
> >> On 10/10/20 12:09 AM, Xu Yilun wrote:
> >>> The value of the field dfl_device.type comes from the 12 bits register
> >>> field DFH_ID according to DFL spec. So this patch changes the definition
> >>> of the type field to u16.
> >>>
> >>> Also it is not necessary to illustrate the valid bits of the type field
> >>> in comments. Instead we should explicitly define the possible values in
> >>> the enumeration type for it, because they are shared by hardware spec.
> >>> We should not let the compiler decide these values.
> >>>
> >>> Similar changes are also applied to dfl_device.feature_id.
> >>>
> >>> This patch also fixed the MODALIAS format according to the changes
> >>> above.
> >>>
> >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> >>> ---
> >>> v9: no change
> >>> ---
> >>>  drivers/fpga/dfl.c |  3 +--
> >>>  drivers/fpga/dfl.h | 14 +++++++-------
> >>>  2 files changed, 8 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> >>> index b450870..5a6ba3b 100644
> >>> --- a/drivers/fpga/dfl.c
> >>> +++ b/drivers/fpga/dfl.c
> >>> @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >>>  {
> >>>  	struct dfl_device *ddev = to_dfl_dev(dev);
> >>>  
> >>> -	/* The type has 4 valid bits and feature_id has 12 valid bits */
> >>> -	return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X",
> >>> +	return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X",
> >>>  			      ddev->type, ddev->feature_id);
> >>>  }
> >>>  
> >>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> >>> index 5dc758f..ac373b1 100644
> >>> --- a/drivers/fpga/dfl.h
> >>> +++ b/drivers/fpga/dfl.h
> >>> @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> >>>   * enum dfl_id_type - define the DFL FIU types
> >>>   */
> >>>  enum dfl_id_type {
> >>> -	FME_ID,
> >>> -	PORT_ID,
> >>> +	FME_ID = 0,
> >>> +	PORT_ID = 1,
> >> This is redundant, why make this change ?
> > These values are shared by hardware spec, so it is suggested that the
> > values of the enum type should be explicitly set, otherwise the compiler
> > is in its right to do whatever it wants with them (within reason...)
> >
> > Please see the original discussion:
> > https://lore.kernel.org/linux-fpga/20200923055436.GA2629915@kroah.com/
> 
> I don't believe this is undefined behavior for the compiler
> 
> from c11 6.7.2.2,3
> 
> The identifiers in an enumerator list are declared as constants that have type int and may appear wherever such are permitted.127) An enumerator with = defines its enumeration constant as the value of the constant expression. If the first enumerator has no =, the value of its enumeration constant is 0. Each subsequent enumerator with no = defines its enumeration constant as the value of the constant expression obtained by adding 1 to the value of the previous enumeration constant. (The use of enumerators with = may produce enumeration constants with values that duplicate other values in the same enumeration.) The enumerators of an enumeration are also known as its members.
> 
> setting them again has some use for documentation so this change is ok if you have strong feeling for it.

The kernel developer community has "strong feelings" for this, please be
specific and list the values when they matter.

thanks,

greg k-h

[PATCH v9 2/6] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Xu Yilun]

In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
patch moves struct dfl_device_id to mod_devicetable.h

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
v2: fix the order for the header file
v3: rebase the patch for dfl bus name change
v9: rebase the patch for dfl bus name changes back to "dfl"
---
 drivers/fpga/dfl.h              | 13 +------------
 include/linux/mod_devicetable.h | 12 ++++++++++++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index ac373b1..549c790 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/uuid.h>
@@ -526,18 +527,6 @@ enum dfl_id_type {
 };
 
 /**
- * struct dfl_device_id -  dfl device identifier
- * @type: DFL FIU type of the device. See enum dfl_id_type.
- * @feature_id: feature identifier local to its DFL FIU type.
- * @driver_data: driver specific data.
- */
-struct dfl_device_id {
-	u16 type;
-	u16 feature_id;
-	unsigned long driver_data;
-};
-
-/**
  * struct dfl_device - represent an dfl device on dfl bus
  *
  * @dev: generic device interface.
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a47..66e1405 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -838,4 +838,16 @@ struct mhi_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/**
+ * struct dfl_device_id -  dfl device identifier
+ * @type: DFL FIU type of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
+ * @driver_data: driver specific data.
+ */
+struct dfl_device_id {
+	__u16 type;
+	__u16 feature_id;
+	unsigned long driver_data;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
-- 
2.7.4

Re: [PATCH v9 2/6] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Greg KH]

On Sat, Oct 10, 2020 at 03:09:49PM +0800, Xu Yilun wrote:
> In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
> patch moves struct dfl_device_id to mod_devicetable.h
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Acked-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: fix the order for the header file
> v3: rebase the patch for dfl bus name change
> v9: rebase the patch for dfl bus name changes back to "dfl"
> ---
>  drivers/fpga/dfl.h              | 13 +------------
>  include/linux/mod_devicetable.h | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index ac373b1..549c790 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -22,6 +22,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/uuid.h>
> @@ -526,18 +527,6 @@ enum dfl_id_type {
>  };
>  
>  /**
> - * struct dfl_device_id -  dfl device identifier
> - * @type: DFL FIU type of the device. See enum dfl_id_type.
> - * @feature_id: feature identifier local to its DFL FIU type.
> - * @driver_data: driver specific data.
> - */
> -struct dfl_device_id {
> -	u16 type;
> -	u16 feature_id;
> -	unsigned long driver_data;
> -};
> -
> -/**
>   * struct dfl_device - represent an dfl device on dfl bus
>   *
>   * @dev: generic device interface.
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a47..66e1405 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -838,4 +838,16 @@ struct mhi_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> +/**
> + * struct dfl_device_id -  dfl device identifier

I still have no idea what "dfl" means.  Please at the very least,
explain it here.

thanks,

greg k-h

Re: [PATCH v9 2/6] fpga: dfl: move dfl_device_id to mod_devicetable.h

[Xu Yilun]

On Sun, Oct 11, 2020 at 04:43:51PM +0200, Greg KH wrote:
> On Sat, Oct 10, 2020 at 03:09:49PM +0800, Xu Yilun wrote:
> > In order to support MODULE_DEVICE_TABLE() for dfl device driver, this
> > patch moves struct dfl_device_id to mod_devicetable.h
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Acked-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > v2: fix the order for the header file
> > v3: rebase the patch for dfl bus name change
> > v9: rebase the patch for dfl bus name changes back to "dfl"
> > ---
> >  drivers/fpga/dfl.h              | 13 +------------
> >  include/linux/mod_devicetable.h | 12 ++++++++++++
> >  2 files changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index ac373b1..549c790 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/uuid.h>
> > @@ -526,18 +527,6 @@ enum dfl_id_type {
> >  };
> >  
> >  /**
> > - * struct dfl_device_id -  dfl device identifier
> > - * @type: DFL FIU type of the device. See enum dfl_id_type.
> > - * @feature_id: feature identifier local to its DFL FIU type.
> > - * @driver_data: driver specific data.
> > - */
> > -struct dfl_device_id {
> > -	u16 type;
> > -	u16 feature_id;
> > -	unsigned long driver_data;
> > -};
> > -
> > -/**
> >   * struct dfl_device - represent an dfl device on dfl bus
> >   *
> >   * @dev: generic device interface.
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 5b08a47..66e1405 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -838,4 +838,16 @@ struct mhi_device_id {
> >  	kernel_ulong_t driver_data;
> >  };
> >  
> > +/**
> > + * struct dfl_device_id -  dfl device identifier
> 
> I still have no idea what "dfl" means.  Please at the very least,
> explain it here.

Yes, I will add the comments here.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

[PATCH v9 3/6] fpga: dfl: add dfl bus support to MODULE_DEVICE_TABLE()

[Xu Yilun]

Device Feature List (DFL) is a linked list of feature headers within the
device MMIO space. It is used by FPGA to enumerate multiple sub features
within it. Each feature can be uniquely identified by DFL type and
feature id, which can be read out from feature headers.

A dfl bus helps DFL framework modularize DFL device drivers for
different sub features. The dfl bus matches its devices and drivers by
DFL type and feature id.

This patch adds dfl bus support to MODULE_DEVICE_TABLE() by adding info
about struct dfl_device_id in devicetable-offsets.c and add a dfl entry
point in file2alias.c.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
v2: add comments for the format of modalias
v3: changes the names from dfl_XXX to fpga_dfl_XXX
    delete the comments of valid bits for modalias format
v9: rebase the patch for bus name changes back to "dfl"
---
 scripts/mod/devicetable-offsets.c |  4 ++++
 scripts/mod/file2alias.c          | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 27007c1..d8350ee 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -243,5 +243,9 @@ int main(void)
 	DEVID(mhi_device_id);
 	DEVID_FIELD(mhi_device_id, chan);
 
+	DEVID(dfl_device_id);
+	DEVID_FIELD(dfl_device_id, type);
+	DEVID_FIELD(dfl_device_id, feature_id);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1..8a438c9 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1368,6 +1368,18 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }
 
+/* Looks like: dfl:tNfN */
+static int do_dfl_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, dfl_device_id, type);
+	DEF_FIELD(symval, dfl_device_id, feature_id);
+
+	sprintf(alias, "dfl:t%04Xf%04X", type, feature_id);
+
+	add_wildcard(alias);
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1442,6 +1454,7 @@ static const struct devtable devtable[] = {
 	{"tee", SIZE_tee_client_device_id, do_tee_entry},
 	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
 	{"mhi", SIZE_mhi_device_id, do_mhi_entry},
+	{"dfl", SIZE_dfl_device_id, do_dfl_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.7.4

[PATCH v9 4/6] fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h

[Xu Yilun]

Now the dfl drivers could be made as independent modules and put in
different folders according to their functionalities. In order for
scattered dfl device drivers to include dfl bus APIs, move the
dfl bus APIs to a new header file in the public folder.

[mdf@kernel.org: Fixed up MAINTAINERS entry merge]
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Acked-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
v2: updated the MAINTAINERS under FPGA DFL DRIVERS
    improve the comments
    rename the dfl-bus.h to dfl.h
v3: rebase the patch for previous changes
v9: rebase the patch for bus name changes back to "dfl"
---
 MAINTAINERS              |  1 +
 drivers/fpga/dfl.c       |  1 +
 drivers/fpga/dfl.h       | 72 ----------------------------------------
 include/linux/fpga/dfl.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 72 deletions(-)
 create mode 100644 include/linux/fpga/dfl.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 239ae24..9d46097 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6884,6 +6884,7 @@ S:	Maintained
 F:	Documentation/ABI/testing/sysfs-bus-dfl
 F:	Documentation/fpga/dfl.rst
 F:	drivers/fpga/dfl*
+F:	include/linux/fpga/dfl.h
 F:	include/uapi/linux/fpga-dfl.h
 
 FPGA MANAGER FRAMEWORK
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 5a6ba3b..13ab889 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -11,6 +11,7 @@
  *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
  */
 #include <linux/fpga-dfl.h>
+#include <linux/fpga/dfl.h>
 #include <linux/module.h>
 #include <linux/uaccess.h>
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 549c790..2b82c96 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -517,76 +517,4 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 			       struct dfl_feature *feature,
 			       unsigned long arg);
 
-/**
- * enum dfl_id_type - define the DFL FIU types
- */
-enum dfl_id_type {
-	FME_ID = 0,
-	PORT_ID = 1,
-	DFL_ID_MAX,
-};
-
-/**
- * struct dfl_device - represent an dfl device on dfl bus
- *
- * @dev: generic device interface.
- * @id: id of the dfl device.
- * @type: type of DFL FIU of the device. See enum dfl_id_type.
- * @feature_id: feature identifier local to its DFL FIU type.
- * @mmio_res: mmio resource of this dfl device.
- * @irqs: list of Linux IRQ numbers of this dfl device.
- * @num_irqs: number of IRQs supported by this dfl device.
- * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
- * @id_entry: matched id entry in dfl driver's id table.
- */
-struct dfl_device {
-	struct device dev;
-	int id;
-	u16 type;
-	u16 feature_id;
-	struct resource mmio_res;
-	int *irqs;
-	unsigned int num_irqs;
-	struct dfl_fpga_cdev *cdev;
-	const struct dfl_device_id *id_entry;
-};
-
-/**
- * struct dfl_driver - represent an dfl device driver
- *
- * @drv: driver model structure.
- * @id_table: pointer to table of device IDs the driver is interested in.
- *	      { } member terminated.
- * @probe: mandatory callback for device binding.
- * @remove: callback for device unbinding.
- */
-struct dfl_driver {
-	struct device_driver drv;
-	const struct dfl_device_id *id_table;
-
-	int (*probe)(struct dfl_device *dfl_dev);
-	void (*remove)(struct dfl_device *dfl_dev);
-};
-
-#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
-#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
-
-/*
- * use a macro to avoid include chaining to get THIS_MODULE.
- */
-#define dfl_driver_register(drv) \
-	__dfl_driver_register(drv, THIS_MODULE)
-int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
-void dfl_driver_unregister(struct dfl_driver *dfl_drv);
-
-/*
- * module_dfl_driver() - Helper macro for drivers that don't do
- * anything special in module init/exit.  This eliminates a lot of
- * boilerplate.  Each module may only use this macro once, and
- * calling it replaces module_init() and module_exit().
- */
-#define module_dfl_driver(__dfl_driver) \
-	module_driver(__dfl_driver, dfl_driver_register, \
-		      dfl_driver_unregister)
-
 #endif /* __FPGA_DFL_H */
diff --git a/include/linux/fpga/dfl.h b/include/linux/fpga/dfl.h
new file mode 100644
index 0000000..7affba2f
--- /dev/null
+++ b/include/linux/fpga/dfl.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for DFL driver and device API
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ */
+
+#ifndef __LINUX_FPGA_DFL_H
+#define __LINUX_FPGA_DFL_H
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+
+/**
+ * enum dfl_id_type - define the DFL FIU types
+ */
+enum dfl_id_type {
+	FME_ID = 0,
+	PORT_ID = 1,
+	DFL_ID_MAX,
+};
+
+/**
+ * struct dfl_device - represent an dfl device on dfl bus
+ *
+ * @dev: generic device interface.
+ * @id: id of the dfl device.
+ * @type: type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: feature identifier local to its DFL FIU type.
+ * @mmio_res: mmio resource of this dfl device.
+ * @irqs: list of Linux IRQ numbers of this dfl device.
+ * @num_irqs: number of IRQs supported by this dfl device.
+ * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
+ * @id_entry: matched id entry in dfl driver's id table.
+ */
+struct dfl_device {
+	struct device dev;
+	int id;
+	u16 type;
+	u16 feature_id;
+	struct resource mmio_res;
+	int *irqs;
+	unsigned int num_irqs;
+	struct dfl_fpga_cdev *cdev;
+	const struct dfl_device_id *id_entry;
+};
+
+/**
+ * struct dfl_driver - represent an dfl device driver
+ *
+ * @drv: driver model structure.
+ * @id_table: pointer to table of device IDs the driver is interested in.
+ *	      { } member terminated.
+ * @probe: mandatory callback for device binding.
+ * @remove: callback for device unbinding.
+ */
+struct dfl_driver {
+	struct device_driver drv;
+	const struct dfl_device_id *id_table;
+
+	int (*probe)(struct dfl_device *dfl_dev);
+	void (*remove)(struct dfl_device *dfl_dev);
+};
+
+#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
+#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
+
+/*
+ * use a macro to avoid include chaining to get THIS_MODULE.
+ */
+#define dfl_driver_register(drv) \
+	__dfl_driver_register(drv, THIS_MODULE)
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
+void dfl_driver_unregister(struct dfl_driver *dfl_drv);
+
+/*
+ * module_dfl_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit().
+ */
+#define module_dfl_driver(__dfl_driver) \
+	module_driver(__dfl_driver, dfl_driver_register, \
+		      dfl_driver_unregister)
+
+#endif /* __LINUX_FPGA_DFL_H */
-- 
2.7.4

Re: [PATCH v9 4/6] fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h

[Greg KH]

On Sat, Oct 10, 2020 at 03:09:51PM +0800, Xu Yilun wrote:
> Now the dfl drivers could be made as independent modules and put in
> different folders according to their functionalities. In order for
> scattered dfl device drivers to include dfl bus APIs, move the
> dfl bus APIs to a new header file in the public folder.
> 
> [mdf@kernel.org: Fixed up MAINTAINERS entry merge]
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Acked-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: updated the MAINTAINERS under FPGA DFL DRIVERS
>     improve the comments
>     rename the dfl-bus.h to dfl.h
> v3: rebase the patch for previous changes
> v9: rebase the patch for bus name changes back to "dfl"
> ---
>  MAINTAINERS              |  1 +
>  drivers/fpga/dfl.c       |  1 +
>  drivers/fpga/dfl.h       | 72 ----------------------------------------
>  include/linux/fpga/dfl.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++

Why in the fpga directory?

thanks,

greg k-h

Re: [PATCH v9 4/6] fpga: dfl: move dfl bus related APIs to include/linux/fpga/dfl.h

[Xu Yilun]

On Sun, Oct 11, 2020 at 04:45:22PM +0200, Greg KH wrote:
> On Sat, Oct 10, 2020 at 03:09:51PM +0800, Xu Yilun wrote:
> > Now the dfl drivers could be made as independent modules and put in
> > different folders according to their functionalities. In order for
> > scattered dfl device drivers to include dfl bus APIs, move the
> > dfl bus APIs to a new header file in the public folder.
> > 
> > [mdf@kernel.org: Fixed up MAINTAINERS entry merge]
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > Acked-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > v2: updated the MAINTAINERS under FPGA DFL DRIVERS
> >     improve the comments
> >     rename the dfl-bus.h to dfl.h
> > v3: rebase the patch for previous changes
> > v9: rebase the patch for bus name changes back to "dfl"
> > ---
> >  MAINTAINERS              |  1 +
> >  drivers/fpga/dfl.c       |  1 +
> >  drivers/fpga/dfl.h       | 72 ----------------------------------------
> >  include/linux/fpga/dfl.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Why in the fpga directory?

OK, since the dfl could be independent to fpga, I think it could be put in include/linux.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

[PATCH v9 5/6] fpga: dfl: add support for N3000 Nios private feature

[Xu Yilun]

This patch adds support for the Nios handshake private feature on Intel
PAC (Programmable Acceleration Card) N3000.

The Nios is the embedded processor on the FPGA card. This private feature
provides a handshake interface to FPGA Nios firmware, which receives
retimer configuration command from host and executes via an internal SPI
master (spi-altera). When Nios finishes the configuration, host takes over
the ownership of the SPI master to control an Intel MAX10 BMC (Board
Management Controller) Chip on the SPI bus.

For Nios firmware handshake part, this driver requests the retimer
configuration for Nios firmware on probe, and adds some sysfs nodes for
user to query the onboard retimer's working mode and Nios firmware
version.

For SPI part, this driver adds a spi-altera platform device as well as
the MAX10 BMC spi slave info. A spi-altera driver will be matched to
handle the following SPI work.

[mdf@kernel.org: Fixed up MAINTAINERS file to include added ABI doc]
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v3: add the doc for this driver
    minor fixes for comments from Tom
v4: move the err log in regmap implementation, and delete
     n3000_nios_writel/readl(), they have nothing to wrapper now.
    some minor fixes and comments improvement.
v5: fix the output of fec_mode sysfs inf to "no" on 10G configuration,
     cause no FEC mode could be configured for 10G.
    rename the dfl_n3000_nios_* to n3000_nios_*
    improves comments.
v6: fix the output of fec_mode sysfs inf to "not supported" if in 10G,
     or the firmware version major < 3.
    minor fixes and improves comments.
v7: improves comments.
v8: add sysfs interfaces for retimer mode, also doc update.
    delete duplicated sysfs interfaces description in doc.
    minor fixes.
v9: delete the retimer FEC mode configuration via module_parameter.
    update the kernel version of the sysfs interfaces in Doc.
    merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@huawei.com/T/#t)
    remove the tags of Maintainers, this implementation is changed.
---
 .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  47 ++
 MAINTAINERS                                        |   2 +-
 drivers/fpga/Kconfig                               |  11 +
 drivers/fpga/Makefile                              |   2 +
 drivers/fpga/dfl-n3000-nios.c                      | 566 +++++++++++++++++++++
 5 files changed, 627 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
 create mode 100644 drivers/fpga/dfl-n3000-nios.c

diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
new file mode 100644
index 0000000..f2f6e5e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
@@ -0,0 +1,47 @@
+What:		/sys/bus/dfl/devices/dfl_dev.X/fec_mode
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns the FEC mode of the 25G links of the
+		ethernet retimers configured by Nios firmware. "rs" for Reed
+		Solomon FEC, "kr" for Fire Code FEC, "no" for NO FEC.
+		"not supported" if the FEC mode setting is not supported, this
+		happens when the Nios firmware version major < 3, or no link is
+		configured to 25G.
+		Format: string
+
+What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_A_mode
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns the enumeration value of the working mode
+		of the retimer A configured by the Nios firmware. The value is
+		read out from shared registers filled by the Nios firmware. Now
+		the values could be:
+
+		- "0": Reset
+		- "1": 4x10G
+		- "2": 4x25G
+		- "3": 2x25G
+		- "4": 2x25G+2x10G
+		- "5": 1x25G
+
+		If the Nios firmware is updated in future to support more
+		retimer modes, more enumeration value is expected.
+		Format: 0x%x
+
+What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_B_mode
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns the enumeration value of the working mode
+		of the retimer B configured by the Nios firmware. The value
+		format is the same as retimer_A_mode.
+
+What:		/sys/bus/dfl/devices/dfl_dev.X/nios_fw_version
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns the version of the Nios firmware in FPGA.
+		Its format is "major.minor.patch".
+		Format: %x.%x.%x
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d46097..3a81cac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6881,7 +6881,7 @@ M:	Wu Hao <hao.wu@intel.com>
 R:	Tom Rix <trix@redhat.com>
 L:	linux-fpga@vger.kernel.org
 S:	Maintained
-F:	Documentation/ABI/testing/sysfs-bus-dfl
+F:	Documentation/ABI/testing/sysfs-bus-dfl*
 F:	Documentation/fpga/dfl.rst
 F:	drivers/fpga/dfl*
 F:	include/linux/fpga/dfl.h
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 7cd5a29..38c7130 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -191,6 +191,17 @@ config FPGA_DFL_AFU
 	  to the FPGA infrastructure via a Port. There may be more than one
 	  Port/AFU per DFL based FPGA device.
 
+config FPGA_DFL_NIOS_INTEL_PAC_N3000
+        tristate "FPGA DFL NIOS Driver for Intel PAC N3000"
+        depends on FPGA_DFL
+        select REGMAP
+        help
+	  This is the driver for the N3000 Nios private feature on Intel
+	  PAC (Programmable Acceleration Card) N3000. It communicates
+	  with the embedded Nios processor to configure the retimers on
+	  the card. It also instantiates the SPI master (spi-altera) for
+	  the card's BMC (Board Management Controller) control.
+
 config FPGA_DFL_PCI
 	tristate "FPGA DFL PCIe Device Driver"
 	depends on PCI && FPGA_DFL
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index d8e21df..18dc9885 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o
 dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
 dfl-afu-objs += dfl-afu-error.o
 
+obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
+
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
new file mode 100644
index 0000000..e7c3862
--- /dev/null
+++ b/drivers/fpga/dfl-n3000-nios.c
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DFL device driver for Nios private feature on Intel PAC (Programmable
+ * Acceleration Card) N3000
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Wu Hao <hao.wu@intel.com>
+ *   Xu Yilun <yilun.xu@intel.com>
+ */
+#include <linux/bitfield.h>
+#include <linux/errno.h>
+#include <linux/fpga/dfl.h>
+#include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/stddef.h>
+#include <linux/spi/altera.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+
+/* N3000 Nios private feature registers */
+#define NIOS_SPI_PARAM			0x8
+#define PARAM_SHIFT_MODE_MSK		BIT_ULL(1)
+#define PARAM_SHIFT_MODE_MSB		0
+#define PARAM_SHIFT_MODE_LSB		1
+#define PARAM_DATA_WIDTH		GENMASK_ULL(7, 2)
+#define PARAM_NUM_CS			GENMASK_ULL(13, 8)
+#define PARAM_CLK_POL			BIT_ULL(14)
+#define PARAM_CLK_PHASE			BIT_ULL(15)
+#define PARAM_PERIPHERAL_ID		GENMASK_ULL(47, 32)
+
+#define NIOS_SPI_CTRL			0x10
+#define CTRL_WR_DATA			GENMASK_ULL(31, 0)
+#define CTRL_ADDR			GENMASK_ULL(44, 32)
+#define CTRL_CMD_MSK			GENMASK_ULL(63, 62)
+#define CTRL_CMD_NOP			0
+#define CTRL_CMD_RD			1
+#define CTRL_CMD_WR			2
+
+#define NIOS_SPI_STAT			0x18
+#define STAT_RD_DATA			GENMASK_ULL(31, 0)
+#define STAT_RW_VAL			BIT_ULL(32)
+
+/* Nios handshake registers, indirect access */
+#define NIOS_INIT			0x1000
+#define NIOS_INIT_DONE			BIT(0)
+#define NIOS_INIT_START			BIT(1)
+/* Mode for retimer A, link 0, the same below */
+#define REQ_FEC_MODE_A0_MSK		GENMASK(9, 8)
+#define REQ_FEC_MODE_A1_MSK		GENMASK(11, 10)
+#define REQ_FEC_MODE_A2_MSK		GENMASK(13, 12)
+#define REQ_FEC_MODE_A3_MSK		GENMASK(15, 14)
+#define REQ_FEC_MODE_B0_MSK		GENMASK(17, 16)
+#define REQ_FEC_MODE_B1_MSK		GENMASK(19, 18)
+#define REQ_FEC_MODE_B2_MSK		GENMASK(21, 20)
+#define REQ_FEC_MODE_B3_MSK		GENMASK(23, 22)
+#define REQ_FEC_MODE_NO			0x0
+#define REQ_FEC_MODE_KR			0x1
+#define REQ_FEC_MODE_RS			0x2
+
+#define NIOS_FW_VERSION			0x1004
+#define NIOS_FW_VERSION_PATCH		GENMASK(23, 20)
+#define NIOS_FW_VERSION_MINOR		GENMASK(27, 24)
+#define NIOS_FW_VERSION_MAJOR		GENMASK(31, 28)
+
+/* The retimers we use on Intel PAC N3000 is Parkvale, abbreviated to PKVL */
+#define PKVL_A_MODE_STS			0x1020
+#define PKVL_B_MODE_STS			0x1024
+#define PKVL_MODE_STS_GROUP_MSK		GENMASK(15, 8)
+#define PKVL_MODE_STS_GROUP_OK		0x0
+#define PKVL_MODE_STS_ID_MSK		GENMASK(7, 0)
+/* When GROUP MASK field == GROUP_OK  */
+#define PKVL_MODE_ID_RESET		0x0
+#define PKVL_MODE_ID_4X10G		0x1
+#define PKVL_MODE_ID_4X25G		0x2
+#define PKVL_MODE_ID_2X25G		0x3
+#define PKVL_MODE_ID_2X25G_2X10G	0x4
+#define PKVL_MODE_ID_1X25G		0x5
+
+#define NS_REGBUS_WAIT_TIMEOUT		10000		/* loop count */
+#define NIOS_INIT_TIMEOUT		10000000	/* usec */
+#define NIOS_INIT_TIME_INTV		100000		/* usec */
+
+struct n3000_nios {
+	void __iomem *base;
+	struct regmap *regmap;
+	struct device *dev;
+	struct platform_device *altera_spi;
+};
+
+static ssize_t nios_fw_version_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct n3000_nios *ns = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%x.%x.%x\n",
+		       (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val),
+		       (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val),
+		       (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val));
+}
+static DEVICE_ATTR_RO(nios_fw_version);
+
+#define IS_MODE_STATUS_OK(mode_stat)				\
+	(FIELD_GET(PKVL_MODE_STS_GROUP_MSK, (mode_stat)) ==	\
+	 PKVL_MODE_STS_GROUP_OK)
+
+#define IS_RETIMER_FEC_SUPPORTED(retimer_mode)	\
+	((retimer_mode) != PKVL_MODE_ID_RESET &&	\
+	 (retimer_mode) != PKVL_MODE_ID_4X10G)
+
+static int get_retimer_mode(struct n3000_nios *ns, unsigned int mode_stat_reg,
+			    unsigned int *retimer_mode)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(ns->regmap, mode_stat_reg, &val);
+	if (ret)
+		return ret;
+
+	if (!IS_MODE_STATUS_OK(val))
+		return -EFAULT;
+
+	*retimer_mode = FIELD_GET(PKVL_MODE_STS_ID_MSK, val);
+
+	return 0;
+}
+
+static ssize_t retimer_A_mode_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct n3000_nios *ns = dev_get_drvdata(dev);
+	unsigned int mode;
+	int ret;
+
+	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &mode);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", mode);
+}
+static DEVICE_ATTR_RO(retimer_A_mode);
+
+static ssize_t retimer_B_mode_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct n3000_nios *ns = dev_get_drvdata(dev);
+	unsigned int mode;
+	int ret;
+
+	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &mode);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", mode);
+}
+static DEVICE_ATTR_RO(retimer_B_mode);
+
+static ssize_t fec_mode_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
+	struct n3000_nios *ns = dev_get_drvdata(dev);
+	int ret;
+
+	/* FEC mode setting is not supported in early FW versions */
+	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) < 3)
+		return sprintf(buf, "not supported\n");
+
+	/* If no 25G links, FEC mode setting is not supported either */
+	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &retimer_a_mode);
+	if (ret)
+		return ret;
+
+	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &retimer_b_mode);
+	if (ret)
+		return ret;
+
+	if (!IS_RETIMER_FEC_SUPPORTED(retimer_a_mode) &&
+	    !IS_RETIMER_FEC_SUPPORTED(retimer_b_mode))
+		return sprintf(buf, "not supported\n");
+
+	/* get the valid FEC mode for 25G links */
+	ret = regmap_read(ns->regmap, NIOS_INIT, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * FEC mode should always be the same for all links, as we set them
+	 * in this way.
+	 */
+	fec_mode = FIELD_GET(REQ_FEC_MODE_A0_MSK, val);
+	if (fec_mode != FIELD_GET(REQ_FEC_MODE_A1_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_A2_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_A3_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_B0_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_B1_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_B2_MSK, val) ||
+	    fec_mode != FIELD_GET(REQ_FEC_MODE_B3_MSK, val))
+		return -EFAULT;
+
+	switch (fec_mode) {
+	case REQ_FEC_MODE_NO:
+		return sprintf(buf, "no\n");
+	case REQ_FEC_MODE_KR:
+		return sprintf(buf, "kr\n");
+	case REQ_FEC_MODE_RS:
+		return sprintf(buf, "rs\n");
+	}
+
+	return -EFAULT;
+}
+static DEVICE_ATTR_RO(fec_mode);
+
+static struct attribute *n3000_nios_attrs[] = {
+	&dev_attr_nios_fw_version.attr,
+	&dev_attr_retimer_A_mode.attr,
+	&dev_attr_retimer_B_mode.attr,
+	&dev_attr_fec_mode.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(n3000_nios);
+
+static bool init_error_detected(struct n3000_nios *ns)
+{
+	unsigned int val;
+
+	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
+		return true;
+
+	if (!IS_MODE_STATUS_OK(val))
+		return true;
+
+	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
+		return true;
+
+	if (!IS_MODE_STATUS_OK(val))
+		return true;
+
+	return false;
+}
+
+static void dump_error_stat(struct n3000_nios *ns)
+{
+	unsigned int val;
+
+	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
+		return;
+
+	dev_err(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
+
+	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
+		return;
+
+	dev_err(ns->dev, "PKVL_B_MODE_STS 0x%x\n", val);
+}
+
+static int n3000_nios_init_done_check(struct n3000_nios *ns)
+{
+	struct device *dev = ns->dev;
+	unsigned int val, mode;
+	int ret;
+
+	/*
+	 * this SPI is shared by Nios core inside FPGA, Nios will use this SPI
+	 * master to do some one time initialization after power up, and then
+	 * release the control to OS. driver needs to poll on INIT_DONE to
+	 * see when driver could take the control.
+	 *
+	 * Please note that after Nios firmware version 3.0.0, INIT_START is
+	 * introduced, so driver needs to trigger START firstly and then check
+	 * INIT_DONE.
+	 */
+
+	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * If Nios version register is totally uninitialized(== 0x0), then the
+	 * Nios firmware is missing. So host could take control of SPI master
+	 * safely, but initialization work for Nios is not done. To restore the
+	 * card, we need to reprogram a new Nios firmware via the BMC chip on
+	 * SPI bus. So the driver doesn't error out, it continues to create the
+	 * spi controller device and spi_board_info for BMC.
+	 */
+	if (val == 0) {
+		dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n",
+			val);
+		return 0;
+	}
+
+	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
+		/* read NIOS_INIT to check if retimer initialization is done */
+		ret = regmap_read(ns->regmap, NIOS_INIT, &val);
+		if (ret)
+			return ret;
+
+		/* check if retimers are initialized already */
+		if (val & NIOS_INIT_DONE || val & NIOS_INIT_START)
+			goto nios_init_done;
+
+		/* configure FEC mode per module param */
+		val = NIOS_INIT_START;
+
+		/*
+		 * When the retimer is to be set to 10G mode, there is no FEC
+		 * mode setting, so the REQ_FEC_MODE field will be ignored by
+		 * Nios firmware in this case. But we should still fill the FEC
+		 * mode field cause host could not get the retimer working mode
+		 * until the Nios init is done.
+		 *
+		 * For now the driver doesn't support the retimer FEC mode
+		 * switching per user's request. It is always set to Reed
+		 * Solomon FEC.
+		 */
+		mode = REQ_FEC_MODE_RS;
+
+		/* set the same FEC mode for all links */
+		val |= FIELD_PREP(REQ_FEC_MODE_A0_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_A1_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_A2_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_A3_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_B0_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_B1_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_B2_MSK, mode) |
+		       FIELD_PREP(REQ_FEC_MODE_B3_MSK, mode);
+
+		ret = regmap_write(ns->regmap, NIOS_INIT, val);
+		if (ret)
+			return ret;
+	}
+
+nios_init_done:
+	/* polls on NIOS_INIT_DONE */
+	ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val,
+				       val & NIOS_INIT_DONE,
+				       NIOS_INIT_TIME_INTV,
+				       NIOS_INIT_TIMEOUT);
+	if (ret) {
+		dev_err(dev, "NIOS_INIT_DONE %s\n",
+			(ret == -ETIMEDOUT) ? "timed out" : "check error");
+		goto dump_sts;
+	}
+
+	/*
+	 * After INIT_DONE is detected, it still needs to check if any error
+	 * detected.
+	 */
+	if (init_error_detected(ns)) {
+		/*
+		 * If the retimer configuration is failed, the Nios firmware
+		 * will still release the spi controller for host to
+		 * communicate with the BMC. It makes possible for people to
+		 * reprogram a new Nios firmware and restore the card. So the
+		 * driver doesn't error out, it continues to create the spi
+		 * controller device and spi_board_info for BMC.
+		 */
+		dev_err(dev, "NIOS_INIT_DONE OK, but err found during init\n");
+		goto dump_sts;
+	}
+	return 0;
+
+dump_sts:
+	dump_error_stat(ns);
+
+	return ret;
+}
+
+static struct spi_board_info m10_n3000_info = {
+	.modalias = "m10-n3000",
+	.max_speed_hz = 12500000,
+	.bus_num = 0,
+	.chip_select = 0,
+};
+
+static int create_altera_spi_controller(struct n3000_nios *ns)
+{
+	struct altera_spi_platform_data pdata = { 0 };
+	struct platform_device_info pdevinfo = { 0 };
+	void __iomem *base = ns->base;
+	u64 v;
+
+	v = readq(base + NIOS_SPI_PARAM);
+
+	pdata.mode_bits = SPI_CS_HIGH;
+	if (FIELD_GET(PARAM_CLK_POL, v))
+		pdata.mode_bits |= SPI_CPOL;
+	if (FIELD_GET(PARAM_CLK_PHASE, v))
+		pdata.mode_bits |= SPI_CPHA;
+
+	pdata.num_chipselect = FIELD_GET(PARAM_NUM_CS, v);
+	pdata.bits_per_word_mask =
+		SPI_BPW_RANGE_MASK(1, FIELD_GET(PARAM_DATA_WIDTH, v));
+
+	pdata.num_devices = 1;
+	pdata.devices = &m10_n3000_info;
+
+	dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
+		pdata.num_chipselect, pdata.bits_per_word_mask,
+		pdata.mode_bits);
+
+	pdevinfo.name = "subdev_spi_altera";
+	pdevinfo.id = PLATFORM_DEVID_AUTO;
+	pdevinfo.parent = ns->dev;
+	pdevinfo.data = &pdata;
+	pdevinfo.size_data = sizeof(pdata);
+
+	ns->altera_spi = platform_device_register_full(&pdevinfo);
+	return PTR_ERR_OR_ZERO(ns->altera_spi);
+}
+
+static void destroy_altera_spi_controller(struct n3000_nios *ns)
+{
+	platform_device_unregister(ns->altera_spi);
+}
+
+/* ns is the abbreviation of nios_spi */
+static int ns_poll_stat_timeout(void __iomem *base, u64 *v)
+{
+	int loops = NS_REGBUS_WAIT_TIMEOUT;
+
+	/*
+	 * We don't use the time based timeout here for performance.
+	 *
+	 * The regbus read/write is on the critical path of Intel PAC N3000
+	 * image programing. The time based timeout checking will add too much
+	 * overhead on it. Usually the state changes in 1 or 2 loops on the
+	 * test server, and we set 10000 times loop here for safety.
+	 */
+	do {
+		*v = readq(base + NIOS_SPI_STAT);
+		if (*v & STAT_RW_VAL)
+			break;
+		cpu_relax();
+	} while (--loops);
+
+	return loops ? 0 : -ETIMEDOUT;
+}
+
+static int ns_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct n3000_nios *ns = context;
+	u64 v = 0;
+	int ret;
+
+	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_WR);
+	v |= FIELD_PREP(CTRL_ADDR, reg);
+	v |= FIELD_PREP(CTRL_WR_DATA, val);
+	writeq(v, ns->base + NIOS_SPI_CTRL);
+
+	ret = ns_poll_stat_timeout(ns->base, &v);
+	if (ret)
+		dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
+			reg, val, ret);
+
+	return ret;
+}
+
+static int ns_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct n3000_nios *ns = context;
+	u64 v = 0;
+	int ret;
+
+	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_RD);
+	v |= FIELD_PREP(CTRL_ADDR, reg);
+	writeq(v, ns->base + NIOS_SPI_CTRL);
+
+	ret = ns_poll_stat_timeout(ns->base, &v);
+	if (ret)
+		dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
+	else
+		*val = FIELD_GET(STAT_RD_DATA, v);
+
+	return ret;
+}
+
+static const struct regmap_config ns_regbus_cfg = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+
+	.reg_write = ns_reg_write,
+	.reg_read = ns_reg_read,
+};
+
+static int n3000_nios_probe(struct dfl_device *ddev)
+{
+	struct device *dev = &ddev->dev;
+	struct n3000_nios *ns;
+	int ret;
+
+	ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return -ENOMEM;
+
+	dev_set_drvdata(&ddev->dev, ns);
+
+	ns->dev = dev;
+
+	ns->base = devm_ioremap_resource(&ddev->dev, &ddev->mmio_res);
+	if (IS_ERR(ns->base))
+		return PTR_ERR(ns->base);
+
+	ns->regmap = devm_regmap_init(dev, NULL, ns, &ns_regbus_cfg);
+	if (IS_ERR(ns->regmap))
+		return PTR_ERR(ns->regmap);
+
+	ret = n3000_nios_init_done_check(ns);
+	if (ret)
+		return ret;
+
+	ret = create_altera_spi_controller(ns);
+	if (ret)
+		dev_err(dev, "altera spi controller create failed: %d\n", ret);
+
+	return ret;
+}
+
+static void n3000_nios_remove(struct dfl_device *ddev)
+{
+	struct n3000_nios *ns = dev_get_drvdata(&ddev->dev);
+
+	destroy_altera_spi_controller(ns);
+}
+
+#define FME_FEATURE_ID_N3000_NIOS	0xd
+
+static const struct dfl_device_id n3000_nios_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_N3000_NIOS },
+	{ }
+};
+
+static struct dfl_driver n3000_nios_driver = {
+	.drv	= {
+		.name       = "n3000-nios",
+		.dev_groups = n3000_nios_groups,
+	},
+	.id_table = n3000_nios_ids,
+	.probe   = n3000_nios_probe,
+	.remove  = n3000_nios_remove,
+};
+
+module_dfl_driver(n3000_nios_driver);
+
+MODULE_DEVICE_TABLE(dfl, n3000_nios_ids);
+MODULE_DESCRIPTION("Driver for Nios private feature on Intel PAC N3000");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v9 5/6] fpga: dfl: add support for N3000 Nios private feature

[Tom Rix]

On 10/10/20 12:09 AM, Xu Yilun wrote:
> This patch adds support for the Nios handshake private feature on Intel
> PAC (Programmable Acceleration Card) N3000.
>
> The Nios is the embedded processor on the FPGA card. This private feature
> provides a handshake interface to FPGA Nios firmware, which receives
> retimer configuration command from host and executes via an internal SPI
> master (spi-altera). When Nios finishes the configuration, host takes over
> the ownership of the SPI master to control an Intel MAX10 BMC (Board
> Management Controller) Chip on the SPI bus.
>
> For Nios firmware handshake part, this driver requests the retimer
> configuration for Nios firmware on probe, and adds some sysfs nodes for
> user to query the onboard retimer's working mode and Nios firmware
> version.
>
> For SPI part, this driver adds a spi-altera platform device as well as
> the MAX10 BMC spi slave info. A spi-altera driver will be matched to
> handle the following SPI work.
>
> [mdf@kernel.org: Fixed up MAINTAINERS file to include added ABI doc]
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v3: add the doc for this driver
>     minor fixes for comments from Tom
> v4: move the err log in regmap implementation, and delete
>      n3000_nios_writel/readl(), they have nothing to wrapper now.
>     some minor fixes and comments improvement.
> v5: fix the output of fec_mode sysfs inf to "no" on 10G configuration,
>      cause no FEC mode could be configured for 10G.
>     rename the dfl_n3000_nios_* to n3000_nios_*
>     improves comments.
> v6: fix the output of fec_mode sysfs inf to "not supported" if in 10G,
>      or the firmware version major < 3.
>     minor fixes and improves comments.
> v7: improves comments.
> v8: add sysfs interfaces for retimer mode, also doc update.
>     delete duplicated sysfs interfaces description in doc.
>     minor fixes.
> v9: delete the retimer FEC mode configuration via module_parameter.
>     update the kernel version of the sysfs interfaces in Doc.
>     merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@huawei.com/T/#t)
>     remove the tags of Maintainers, this implementation is changed.
> ---
>  .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  47 ++
>  MAINTAINERS                                        |   2 +-
>  drivers/fpga/Kconfig                               |  11 +
>  drivers/fpga/Makefile                              |   2 +
>  drivers/fpga/dfl-n3000-nios.c                      | 566 +++++++++++++++++++++
>  5 files changed, 627 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>  create mode 100644 drivers/fpga/dfl-n3000-nios.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> new file mode 100644
> index 0000000..f2f6e5e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> @@ -0,0 +1,47 @@
> +What:		/sys/bus/dfl/devices/dfl_dev.X/fec_mode
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns the FEC mode of the 25G links of the
Could drop the 'It' and start with 'Returns'  here and similar
> +		ethernet retimers configured by Nios firmware. "rs" for Reed
> +		Solomon FEC, "kr" for Fire Code FEC, "no" for NO FEC.
> +		"not supported" if the FEC mode setting is not supported, this
setting ? the description says 'Read-only'
> +		happens when the Nios firmware version major < 3, or no link is
> +		configured to 25G.
> +		Format: string
> +
> +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_A_mode
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns the enumeration value of the working mode
> +		of the retimer A configured by the Nios firmware. The value is
> +		read out from shared registers filled by the Nios firmware. Now
> +		the values could be:
> +
> +		- "0": Reset
> +		- "1": 4x10G
> +		- "2": 4x25G
> +		- "3": 2x25G
> +		- "4": 2x25G+2x10G
> +		- "5": 1x25G
> +
> +		If the Nios firmware is updated in future to support more
> +		retimer modes, more enumeration value is expected. 
> +		Format: 0x%x
> +
> +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_B_mode
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns the enumeration value of the working mode
> +		of the retimer B configured by the Nios firmware. The value
> +		format is the same as retimer_A_mode.
> +
> +What:		/sys/bus/dfl/devices/dfl_dev.X/nios_fw_version
> +Date:		Oct 2020
> +KernelVersion:	5.11
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns the version of the Nios firmware in FPGA.
Can drop 'in FPGA'.
> +		Its format is "major.minor.patch".
> +		Format: %x.%x.%x
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d46097..3a81cac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6881,7 +6881,7 @@ M:	Wu Hao <hao.wu@intel.com>
>  R:	Tom Rix <trix@redhat.com>
>  L:	linux-fpga@vger.kernel.org
>  S:	Maintained
> -F:	Documentation/ABI/testing/sysfs-bus-dfl
> +F:	Documentation/ABI/testing/sysfs-bus-dfl*
>  F:	Documentation/fpga/dfl.rst
>  F:	drivers/fpga/dfl*
>  F:	include/linux/fpga/dfl.h
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 7cd5a29..38c7130 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -191,6 +191,17 @@ config FPGA_DFL_AFU
>  	  to the FPGA infrastructure via a Port. There may be more than one
>  	  Port/AFU per DFL based FPGA device.
>  
> +config FPGA_DFL_NIOS_INTEL_PAC_N3000
> +        tristate "FPGA DFL NIOS Driver for Intel PAC N3000"
> +        depends on FPGA_DFL
Would 'depends on FPGA_DFL_PCI' be better ?
> +        select REGMAP
> +        help
Last 3 lines have whitespace issues, need to convert spaces to tabs
> +	  This is the driver for the N3000 Nios private feature on Intel
> +	  PAC (Programmable Acceleration Card) N3000. It communicates
> +	  with the embedded Nios processor to configure the retimers on
> +	  the card. It also instantiates the SPI master (spi-altera) for
> +	  the card's BMC (Board Management Controller) control.

can drop 'control'

If you read out 'card's board mangement controller control' it sounds awkward.

> +
>  config FPGA_DFL_PCI
>  	tristate "FPGA DFL PCIe Device Driver"
>  	depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index d8e21df..18dc9885 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o
>  dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>  dfl-afu-objs += dfl-afu-error.o
>  
> +obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> +
>  # Drivers for FPGAs which implement DFL
>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
> new file mode 100644
> index 0000000..e7c3862
> --- /dev/null
> +++ b/drivers/fpga/dfl-n3000-nios.c
> @@ -0,0 +1,566 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DFL device driver for Nios private feature on Intel PAC (Programmable
> + * Acceleration Card) N3000
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Wu Hao <hao.wu@intel.com>
> + *   Xu Yilun <yilun.xu@intel.com>
> + */
> +#include <linux/bitfield.h>
> +#include <linux/errno.h>
> +#include <linux/fpga/dfl.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/stddef.h>
> +#include <linux/spi/altera.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +
> +/* N3000 Nios private feature registers */
> +#define NIOS_SPI_PARAM			0x8

The #defines should have a consistent prefix that is unlikely to conflict.

Maybe N3000_NIOS_

> +#define PARAM_SHIFT_MODE_MSK		BIT_ULL(1)
> +#define PARAM_SHIFT_MODE_MSB		0
> +#define PARAM_SHIFT_MODE_LSB		1
> +#define PARAM_DATA_WIDTH		GENMASK_ULL(7, 2)
> +#define PARAM_NUM_CS			GENMASK_ULL(13, 8)
> +#define PARAM_CLK_POL			BIT_ULL(14)
> +#define PARAM_CLK_PHASE			BIT_ULL(15)
> +#define PARAM_PERIPHERAL_ID		GENMASK_ULL(47, 32)
> +
> +#define NIOS_SPI_CTRL			0x10
> +#define CTRL_WR_DATA			GENMASK_ULL(31, 0)
> +#define CTRL_ADDR			GENMASK_ULL(44, 32)
> +#define CTRL_CMD_MSK			GENMASK_ULL(63, 62)
> +#define CTRL_CMD_NOP			0
> +#define CTRL_CMD_RD			1
> +#define CTRL_CMD_WR			2
> +
> +#define NIOS_SPI_STAT			0x18
> +#define STAT_RD_DATA			GENMASK_ULL(31, 0)
> +#define STAT_RW_VAL			BIT_ULL(32)
> +
> +/* Nios handshake registers, indirect access */
> +#define NIOS_INIT			0x1000
> +#define NIOS_INIT_DONE			BIT(0)
> +#define NIOS_INIT_START			BIT(1)
> +/* Mode for retimer A, link 0, the same below */
> +#define REQ_FEC_MODE_A0_MSK		GENMASK(9, 8)
> +#define REQ_FEC_MODE_A1_MSK		GENMASK(11, 10)
> +#define REQ_FEC_MODE_A2_MSK		GENMASK(13, 12)
> +#define REQ_FEC_MODE_A3_MSK		GENMASK(15, 14)
> +#define REQ_FEC_MODE_B0_MSK		GENMASK(17, 16)
> +#define REQ_FEC_MODE_B1_MSK		GENMASK(19, 18)
> +#define REQ_FEC_MODE_B2_MSK		GENMASK(21, 20)
> +#define REQ_FEC_MODE_B3_MSK		GENMASK(23, 22)
> +#define REQ_FEC_MODE_NO			0x0
> +#define REQ_FEC_MODE_KR			0x1
> +#define REQ_FEC_MODE_RS			0x2
> +
> +#define NIOS_FW_VERSION			0x1004
> +#define NIOS_FW_VERSION_PATCH		GENMASK(23, 20)
> +#define NIOS_FW_VERSION_MINOR		GENMASK(27, 24)
> +#define NIOS_FW_VERSION_MAJOR		GENMASK(31, 28)
> +
> +/* The retimers we use on Intel PAC N3000 is Parkvale, abbreviated to PKVL */
Parkvale is codename, why not just RETIMER_ ?
> +#define PKVL_A_MODE_STS			0x1020
> +#define PKVL_B_MODE_STS			0x1024
> +#define PKVL_MODE_STS_GROUP_MSK		GENMASK(15, 8)
> +#define PKVL_MODE_STS_GROUP_OK		0x0
> +#define PKVL_MODE_STS_ID_MSK		GENMASK(7, 0)
> +/* When GROUP MASK field == GROUP_OK  */
> +#define PKVL_MODE_ID_RESET		0x0
> +#define PKVL_MODE_ID_4X10G		0x1
> +#define PKVL_MODE_ID_4X25G		0x2
> +#define PKVL_MODE_ID_2X25G		0x3
> +#define PKVL_MODE_ID_2X25G_2X10G	0x4
> +#define PKVL_MODE_ID_1X25G		0x5
> +
> +#define NS_REGBUS_WAIT_TIMEOUT		10000		/* loop count */

NS ? Expand.

'WAIT_TIMEOUT' should be changed to 'RETRY_COUNT' to reflect its use

> +#define NIOS_INIT_TIMEOUT		10000000	/* usec */
> +#define NIOS_INIT_TIME_INTV		100000		/* usec */
> +
> +struct n3000_nios {
> +	void __iomem *base;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct platform_device *altera_spi;
> +};
> +
> +static ssize_t nios_fw_version_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct n3000_nios *ns = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%x.%x.%x\n",
> +		       (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val),
> +		       (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val),
> +		       (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val));
> +}
> +static DEVICE_ATTR_RO(nios_fw_version);
> +
> +#define IS_MODE_STATUS_OK(mode_stat)				\
> +	(FIELD_GET(PKVL_MODE_STS_GROUP_MSK, (mode_stat)) ==	\
> +	 PKVL_MODE_STS_GROUP_OK)
> +
> +#define IS_RETIMER_FEC_SUPPORTED(retimer_mode)	\
> +	((retimer_mode) != PKVL_MODE_ID_RESET &&	\
> +	 (retimer_mode) != PKVL_MODE_ID_4X10G)
> +
> +static int get_retimer_mode(struct n3000_nios *ns, unsigned int mode_stat_reg,
> +			    unsigned int *retimer_mode)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(ns->regmap, mode_stat_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (!IS_MODE_STATUS_OK(val))
> +		return -EFAULT;
> +
> +	*retimer_mode = FIELD_GET(PKVL_MODE_STS_ID_MSK, val);
> +
> +	return 0;
> +}
> +
> +static ssize_t retimer_A_mode_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct n3000_nios *ns = dev_get_drvdata(dev);
> +	unsigned int mode;
> +	int ret;
> +
> +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &mode);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", mode);
> +}
> +static DEVICE_ATTR_RO(retimer_A_mode);
> +
> +static ssize_t retimer_B_mode_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct n3000_nios *ns = dev_get_drvdata(dev);
> +	unsigned int mode;
> +	int ret;
> +
> +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &mode);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", mode);
> +}
> +static DEVICE_ATTR_RO(retimer_B_mode);
> +
> +static ssize_t fec_mode_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
> +	struct n3000_nios *ns = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* FEC mode setting is not supported in early FW versions */
> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) < 3)
> +		return sprintf(buf, "not supported\n");
> +
> +	/* If no 25G links, FEC mode setting is not supported either */
> +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &retimer_a_mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &retimer_b_mode);
> +	if (ret)
> +		return ret;
> +
> +	if (!IS_RETIMER_FEC_SUPPORTED(retimer_a_mode) &&
> +	    !IS_RETIMER_FEC_SUPPORTED(retimer_b_mode))
> +		return sprintf(buf, "not supported\n");
> +
> +	/* get the valid FEC mode for 25G links */
> +	ret = regmap_read(ns->regmap, NIOS_INIT, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * FEC mode should always be the same for all links, as we set them
> +	 * in this way.
> +	 */
> +	fec_mode = FIELD_GET(REQ_FEC_MODE_A0_MSK, val);
> +	if (fec_mode != FIELD_GET(REQ_FEC_MODE_A1_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A2_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A3_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B0_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B1_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B2_MSK, val) ||
> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B3_MSK, val))
> +		return -EFAULT;

a precomputed table setting the fields would reduce the this to a lookup and 1 check.

similar table could be used below for the setting.

> +
> +	switch (fec_mode) {
> +	case REQ_FEC_MODE_NO:
> +		return sprintf(buf, "no\n");
> +	case REQ_FEC_MODE_KR:
> +		return sprintf(buf, "kr\n");
> +	case REQ_FEC_MODE_RS:
> +		return sprintf(buf, "rs\n");
> +	}
> +
> +	return -EFAULT;
> +}
> +static DEVICE_ATTR_RO(fec_mode);
> +
> +static struct attribute *n3000_nios_attrs[] = {
> +	&dev_attr_nios_fw_version.attr,
> +	&dev_attr_retimer_A_mode.attr,
> +	&dev_attr_retimer_B_mode.attr,
> +	&dev_attr_fec_mode.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(n3000_nios);
> +
> +static bool init_error_detected(struct n3000_nios *ns)
> +{
> +	unsigned int val;
> +
> +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
> +		return true;
> +
> +	if (!IS_MODE_STATUS_OK(val))
> +		return true;
> +
> +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
> +		return true;
> +
> +	if (!IS_MODE_STATUS_OK(val))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void dump_error_stat(struct n3000_nios *ns)
> +{
> +	unsigned int val;
> +
> +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
> +		return;
> +
> +	dev_err(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
> +
> +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
> +		return;
> +
> +	dev_err(ns->dev, "PKVL_B_MODE_STS 0x%x\n", val);
> +}
> +
> +static int n3000_nios_init_done_check(struct n3000_nios *ns)
> +{
> +	struct device *dev = ns->dev;
> +	unsigned int val, mode;
> +	int ret;
> +
> +	/*
> +	 * this SPI is shared by Nios core inside FPGA, Nios will use this SPI
'by the Nios core inside the FPGA ... '
> +	 * master to do some one time initialization after power up, and then
> +	 * release the control to OS. driver needs to poll on INIT_DONE to
'The driver..'
> +	 * see when driver could take the control.
> +	 *
> +	 * Please note that after Nios firmware version 3.0.0, INIT_START is
> +	 * introduced, so driver needs to trigger START firstly and then check
> +	 * INIT_DONE.
> +	 */
> +
> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If Nios version register is totally uninitialized(== 0x0), then the
> +	 * Nios firmware is missing. So host could take control of SPI master
> +	 * safely, but initialization work for Nios is not done. To restore the
> +	 * card, we need to reprogram a new Nios firmware via the BMC chip on
> +	 * SPI bus. So the driver doesn't error out, it continues to create the
> +	 * spi controller device and spi_board_info for BMC.
> +	 */
> +	if (val == 0) {
> +		dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n",
> +			val);
> +		return 0;
> +	}
> +
> +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
> +		/* read NIOS_INIT to check if retimer initialization is done */
> +		ret = regmap_read(ns->regmap, NIOS_INIT, &val);
> +		if (ret)
> +			return ret;
> +
> +		/* check if retimers are initialized already */
> +		if (val & NIOS_INIT_DONE || val & NIOS_INIT_START)
val & (NIOS_INIT_DONE | NIOS_INIT_START)
> +			goto nios_init_done;
> +
> +		/* configure FEC mode per module param */
> +		val = NIOS_INIT_START;
> +
> +		/*
> +		 * When the retimer is to be set to 10G mode, there is no FEC
> +		 * mode setting, so the REQ_FEC_MODE field will be ignored by
> +		 * Nios firmware in this case. But we should still fill the FEC
> +		 * mode field cause host could not get the retimer working mode
> +		 * until the Nios init is done.
> +		 *
> +		 * For now the driver doesn't support the retimer FEC mode
> +		 * switching per user's request. It is always set to Reed
> +		 * Solomon FEC.
> +		 */
> +		mode = REQ_FEC_MODE_RS;
> +
> +		/* set the same FEC mode for all links */
> +		val |= FIELD_PREP(REQ_FEC_MODE_A0_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_A1_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_A2_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_A3_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_B0_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_B1_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_B2_MSK, mode) |
> +		       FIELD_PREP(REQ_FEC_MODE_B3_MSK, mode);
> +
> +		ret = regmap_write(ns->regmap, NIOS_INIT, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +nios_init_done:
> +	/* polls on NIOS_INIT_DONE */
> +	ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val,
> +				       val & NIOS_INIT_DONE,
> +				       NIOS_INIT_TIME_INTV,
> +				       NIOS_INIT_TIMEOUT);
> +	if (ret) {
> +		dev_err(dev, "NIOS_INIT_DONE %s\n",
> +			(ret == -ETIMEDOUT) ? "timed out" : "check error");
> +		goto dump_sts;
> +	}
> +
> +	/*
> +	 * After INIT_DONE is detected, it still needs to check if any error
> +	 * detected.
> +	 */
> +	if (init_error_detected(ns)) {
> +		/*
> +		 * If the retimer configuration is failed, the Nios firmware
> +		 * will still release the spi controller for host to
> +		 * communicate with the BMC. It makes possible for people to
> +		 * reprogram a new Nios firmware and restore the card. So the
> +		 * driver doesn't error out, it continues to create the spi
> +		 * controller device and spi_board_info for BMC.
> +		 */
> +		dev_err(dev, "NIOS_INIT_DONE OK, but err found during init\n");
> +		goto dump_sts;
> +	}
> +	return 0;
> +
> +dump_sts:
> +	dump_error_stat(ns);
Could init_error_detected and dump_error_stat be combined ?
> +
> +	return ret;
> +}
> +
> +static struct spi_board_info m10_n3000_info = {
> +	.modalias = "m10-n3000",
> +	.max_speed_hz = 12500000,
> +	.bus_num = 0,
> +	.chip_select = 0,
> +};
> +
> +static int create_altera_spi_controller(struct n3000_nios *ns)
> +{
> +	struct altera_spi_platform_data pdata = { 0 };
> +	struct platform_device_info pdevinfo = { 0 };
> +	void __iomem *base = ns->base;
> +	u64 v;
> +
> +	v = readq(base + NIOS_SPI_PARAM);
> +
> +	pdata.mode_bits = SPI_CS_HIGH;
> +	if (FIELD_GET(PARAM_CLK_POL, v))
> +		pdata.mode_bits |= SPI_CPOL;
> +	if (FIELD_GET(PARAM_CLK_PHASE, v))
> +		pdata.mode_bits |= SPI_CPHA;
> +
> +	pdata.num_chipselect = FIELD_GET(PARAM_NUM_CS, v);
> +	pdata.bits_per_word_mask =
> +		SPI_BPW_RANGE_MASK(1, FIELD_GET(PARAM_DATA_WIDTH, v));
> +
> +	pdata.num_devices = 1;
> +	pdata.devices = &m10_n3000_info;
> +
> +	dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> +		pdata.num_chipselect, pdata.bits_per_word_mask,
> +		pdata.mode_bits);
> +
> +	pdevinfo.name = "subdev_spi_altera";
> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> +	pdevinfo.parent = ns->dev;
> +	pdevinfo.data = &pdata;
> +	pdevinfo.size_data = sizeof(pdata);
> +
> +	ns->altera_spi = platform_device_register_full(&pdevinfo);
> +	return PTR_ERR_OR_ZERO(ns->altera_spi);
> +}
> +
> +static void destroy_altera_spi_controller(struct n3000_nios *ns)
> +{
> +	platform_device_unregister(ns->altera_spi);
> +}
> +
> +/* ns is the abbreviation of nios_spi */
> +static int ns_poll_stat_timeout(void __iomem *base, u64 *v)
> +{
> +	int loops = NS_REGBUS_WAIT_TIMEOUT;
> +
> +	/*
> +	 * We don't use the time based timeout here for performance.
> +	 *
> +	 * The regbus read/write is on the critical path of Intel PAC N3000
> +	 * image programing. The time based timeout checking will add too much
> +	 * overhead on it. Usually the state changes in 1 or 2 loops on the
> +	 * test server, and we set 10000 times loop here for safety.
> +	 */
> +	do {
> +		*v = readq(base + NIOS_SPI_STAT);
> +		if (*v & STAT_RW_VAL)
> +			break;
> +		cpu_relax();
> +	} while (--loops);
This becomes an infinite loop if loops <= 0
> +
> +	return loops ? 0 : -ETIMEDOUT;
> +}
> +
> +static int ns_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct n3000_nios *ns = context;
> +	u64 v = 0;
> +	int ret;
> +
> +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_WR);
> +	v |= FIELD_PREP(CTRL_ADDR, reg);
> +	v |= FIELD_PREP(CTRL_WR_DATA, val);
> +	writeq(v, ns->base + NIOS_SPI_CTRL);
> +
> +	ret = ns_poll_stat_timeout(ns->base, &v);
> +	if (ret)
> +		dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
> +			reg, val, ret);
> +
> +	return ret;
> +}
> +
> +static int ns_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct n3000_nios *ns = context;
> +	u64 v = 0;
> +	int ret;
> +
> +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_RD);

v = , and save the intialiation

or v = ( ... | ... )

> +	v |= FIELD_PREP(CTRL_ADDR, reg);
> +	writeq(v, ns->base + NIOS_SPI_CTRL);
> +
> +	ret = ns_poll_stat_timeout(ns->base, &v);
> +	if (ret)
> +		dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
> +	else
> +		*val = FIELD_GET(STAT_RD_DATA, v);
> +
> +	return ret;
> +}
> +
> +static const struct regmap_config ns_regbus_cfg = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +
> +	.reg_write = ns_reg_write,
> +	.reg_read = ns_reg_read,
> +};
> +
> +static int n3000_nios_probe(struct dfl_device *ddev)
> +{
> +	struct device *dev = &ddev->dev;
> +	struct n3000_nios *ns;
> +	int ret;
> +
> +	ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
> +	if (!ns)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&ddev->dev, ns);
> +
> +	ns->dev = dev;
> +
> +	ns->base = devm_ioremap_resource(&ddev->dev, &ddev->mmio_res);
> +	if (IS_ERR(ns->base))
> +		return PTR_ERR(ns->base);
> +
> +	ns->regmap = devm_regmap_init(dev, NULL, ns, &ns_regbus_cfg);
> +	if (IS_ERR(ns->regmap))
> +		return PTR_ERR(ns->regmap);
> +
> +	ret = n3000_nios_init_done_check(ns);
> +	if (ret)
> +		return ret;
> +
> +	ret = create_altera_spi_controller(ns);
> +	if (ret)
> +		dev_err(dev, "altera spi controller create failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void n3000_nios_remove(struct dfl_device *ddev)
> +{
> +	struct n3000_nios *ns = dev_get_drvdata(&ddev->dev);
> +
> +	destroy_altera_spi_controller(ns);
> +}
> +
> +#define FME_FEATURE_ID_N3000_NIOS	0xd
> +
> +static const struct dfl_device_id n3000_nios_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_N3000_NIOS },
> +	{ }
> +};
> +
> +static struct dfl_driver n3000_nios_driver = {
> +	.drv	= {
> +		.name       = "n3000-nios",

"dfl-n3000-nios" ?

Tom

> +		.dev_groups = n3000_nios_groups,
> +	},
> +	.id_table = n3000_nios_ids,
> +	.probe   = n3000_nios_probe,
> +	.remove  = n3000_nios_remove,
> +};
> +
> +module_dfl_driver(n3000_nios_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, n3000_nios_ids);
> +MODULE_DESCRIPTION("Driver for Nios private feature on Intel PAC N3000");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v9 5/6] fpga: dfl: add support for N3000 Nios private feature

[Xu Yilun]

On Sat, Oct 10, 2020 at 02:21:45PM -0700, Tom Rix wrote:
> 
> On 10/10/20 12:09 AM, Xu Yilun wrote:
> > This patch adds support for the Nios handshake private feature on Intel
> > PAC (Programmable Acceleration Card) N3000.
> >
> > The Nios is the embedded processor on the FPGA card. This private feature
> > provides a handshake interface to FPGA Nios firmware, which receives
> > retimer configuration command from host and executes via an internal SPI
> > master (spi-altera). When Nios finishes the configuration, host takes over
> > the ownership of the SPI master to control an Intel MAX10 BMC (Board
> > Management Controller) Chip on the SPI bus.
> >
> > For Nios firmware handshake part, this driver requests the retimer
> > configuration for Nios firmware on probe, and adds some sysfs nodes for
> > user to query the onboard retimer's working mode and Nios firmware
> > version.
> >
> > For SPI part, this driver adds a spi-altera platform device as well as
> > the MAX10 BMC spi slave info. A spi-altera driver will be matched to
> > handle the following SPI work.
> >
> > [mdf@kernel.org: Fixed up MAINTAINERS file to include added ABI doc]
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> > v3: add the doc for this driver
> >     minor fixes for comments from Tom
> > v4: move the err log in regmap implementation, and delete
> >      n3000_nios_writel/readl(), they have nothing to wrapper now.
> >     some minor fixes and comments improvement.
> > v5: fix the output of fec_mode sysfs inf to "no" on 10G configuration,
> >      cause no FEC mode could be configured for 10G.
> >     rename the dfl_n3000_nios_* to n3000_nios_*
> >     improves comments.
> > v6: fix the output of fec_mode sysfs inf to "not supported" if in 10G,
> >      or the firmware version major < 3.
> >     minor fixes and improves comments.
> > v7: improves comments.
> > v8: add sysfs interfaces for retimer mode, also doc update.
> >     delete duplicated sysfs interfaces description in doc.
> >     minor fixes.
> > v9: delete the retimer FEC mode configuration via module_parameter.
> >     update the kernel version of the sysfs interfaces in Doc.
> >     merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@huawei.com/T/#t)
> >     remove the tags of Maintainers, this implementation is changed.
> > ---
> >  .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  47 ++
> >  MAINTAINERS                                        |   2 +-
> >  drivers/fpga/Kconfig                               |  11 +
> >  drivers/fpga/Makefile                              |   2 +
> >  drivers/fpga/dfl-n3000-nios.c                      | 566 +++++++++++++++++++++
> >  5 files changed, 627 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> >  create mode 100644 drivers/fpga/dfl-n3000-nios.c
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> > new file mode 100644
> > index 0000000..f2f6e5e
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> > @@ -0,0 +1,47 @@
> > +What:		/sys/bus/dfl/devices/dfl_dev.X/fec_mode
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns the FEC mode of the 25G links of the
> Could drop the 'It' and start with 'Returns'  here and similar

Yes, I can change it.

> > +		ethernet retimers configured by Nios firmware. "rs" for Reed
> > +		Solomon FEC, "kr" for Fire Code FEC, "no" for NO FEC.
> > +		"not supported" if the FEC mode setting is not supported, this
> setting ? the description says 'Read-only'

The setting is done by Nios firmware, host cannot change it at runtime
so it is 'Read-only'. It is previously described in this section. Is it
good?

> > +		happens when the Nios firmware version major < 3, or no link is
> > +		configured to 25G.
> > +		Format: string
> > +
> > +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_A_mode
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns the enumeration value of the working mode
> > +		of the retimer A configured by the Nios firmware. The value is
> > +		read out from shared registers filled by the Nios firmware. Now
> > +		the values could be:
> > +
> > +		- "0": Reset
> > +		- "1": 4x10G
> > +		- "2": 4x25G
> > +		- "3": 2x25G
> > +		- "4": 2x25G+2x10G
> > +		- "5": 1x25G
> > +
> > +		If the Nios firmware is updated in future to support more
> > +		retimer modes, more enumeration value is expected. 
> > +		Format: 0x%x
> > +
> > +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_B_mode
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns the enumeration value of the working mode
> > +		of the retimer B configured by the Nios firmware. The value
> > +		format is the same as retimer_A_mode.
> > +
> > +What:		/sys/bus/dfl/devices/dfl_dev.X/nios_fw_version
> > +Date:		Oct 2020
> > +KernelVersion:	5.11
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns the version of the Nios firmware in FPGA.
> Can drop 'in FPGA'.

The PAC N3000 card has 2 Nios firmwares. The one for this driver is
embedded in A10 FPGA. Another one is embedded in MAX10 BMC chip.

So to be clear, I think we may keep the 'in FPGA'.

> > +		Its format is "major.minor.patch".
> > +		Format: %x.%x.%x
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9d46097..3a81cac 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6881,7 +6881,7 @@ M:	Wu Hao <hao.wu@intel.com>
> >  R:	Tom Rix <trix@redhat.com>
> >  L:	linux-fpga@vger.kernel.org
> >  S:	Maintained
> > -F:	Documentation/ABI/testing/sysfs-bus-dfl
> > +F:	Documentation/ABI/testing/sysfs-bus-dfl*
> >  F:	Documentation/fpga/dfl.rst
> >  F:	drivers/fpga/dfl*
> >  F:	include/linux/fpga/dfl.h
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 7cd5a29..38c7130 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -191,6 +191,17 @@ config FPGA_DFL_AFU
> >  	  to the FPGA infrastructure via a Port. There may be more than one
> >  	  Port/AFU per DFL based FPGA device.
> >  
> > +config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > +        tristate "FPGA DFL NIOS Driver for Intel PAC N3000"
> > +        depends on FPGA_DFL
> Would 'depends on FPGA_DFL_PCI' be better ?

The relationship between the 2 config is, the n3000 nios private feature
now only appears on PAC N3000 which is a PCIe card.

But the code has no dependency to FPGA_DFL_PCI. Is it necessary we add the
'depends on'?

> > +        select REGMAP
> > +        help
> Last 3 lines have whitespace issues, need to convert spaces to tabs

Thanks for catching this.

> > +	  This is the driver for the N3000 Nios private feature on Intel
> > +	  PAC (Programmable Acceleration Card) N3000. It communicates
> > +	  with the embedded Nios processor to configure the retimers on
> > +	  the card. It also instantiates the SPI master (spi-altera) for
> > +	  the card's BMC (Board Management Controller) control.
> 
> can drop 'control'
> 
> If you read out 'card's board mangement controller control' it sounds awkward.

It's true. I'll drop it.

> 
> > +
> >  config FPGA_DFL_PCI
> >  	tristate "FPGA DFL PCIe Device Driver"
> >  	depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index d8e21df..18dc9885 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o
> >  dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> >  dfl-afu-objs += dfl-afu-error.o
> >  
> > +obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> > +
> >  # Drivers for FPGAs which implement DFL
> >  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
> > new file mode 100644
> > index 0000000..e7c3862
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-n3000-nios.c
> > @@ -0,0 +1,566 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DFL device driver for Nios private feature on Intel PAC (Programmable
> > + * Acceleration Card) N3000
> > + *
> > + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + *   Wu Hao <hao.wu@intel.com>
> > + *   Xu Yilun <yilun.xu@intel.com>
> > + */
> > +#include <linux/bitfield.h>
> > +#include <linux/errno.h>
> > +#include <linux/fpga/dfl.h>
> > +#include <linux/io.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/stddef.h>
> > +#include <linux/spi/altera.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/types.h>
> > +
> > +/* N3000 Nios private feature registers */
> > +#define NIOS_SPI_PARAM			0x8
> 
> The #defines should have a consistent prefix that is unlikely to conflict.
> 
> Maybe N3000_NIOS_

Yes, I'll try to refactor the macro names.
The register names in spec are NIOS_SPI_xx, how about we compress the prefixes,
'N3000_NS' & 'n3000_ns' for 'N3000 NIOS SPI'

> 
> > +#define PARAM_SHIFT_MODE_MSK		BIT_ULL(1)
> > +#define PARAM_SHIFT_MODE_MSB		0
> > +#define PARAM_SHIFT_MODE_LSB		1
> > +#define PARAM_DATA_WIDTH		GENMASK_ULL(7, 2)
> > +#define PARAM_NUM_CS			GENMASK_ULL(13, 8)
> > +#define PARAM_CLK_POL			BIT_ULL(14)
> > +#define PARAM_CLK_PHASE			BIT_ULL(15)
> > +#define PARAM_PERIPHERAL_ID		GENMASK_ULL(47, 32)
> > +
> > +#define NIOS_SPI_CTRL			0x10
> > +#define CTRL_WR_DATA			GENMASK_ULL(31, 0)
> > +#define CTRL_ADDR			GENMASK_ULL(44, 32)
> > +#define CTRL_CMD_MSK			GENMASK_ULL(63, 62)
> > +#define CTRL_CMD_NOP			0
> > +#define CTRL_CMD_RD			1
> > +#define CTRL_CMD_WR			2
> > +
> > +#define NIOS_SPI_STAT			0x18
> > +#define STAT_RD_DATA			GENMASK_ULL(31, 0)
> > +#define STAT_RW_VAL			BIT_ULL(32)
> > +
> > +/* Nios handshake registers, indirect access */
> > +#define NIOS_INIT			0x1000
> > +#define NIOS_INIT_DONE			BIT(0)
> > +#define NIOS_INIT_START			BIT(1)
> > +/* Mode for retimer A, link 0, the same below */
> > +#define REQ_FEC_MODE_A0_MSK		GENMASK(9, 8)
> > +#define REQ_FEC_MODE_A1_MSK		GENMASK(11, 10)
> > +#define REQ_FEC_MODE_A2_MSK		GENMASK(13, 12)
> > +#define REQ_FEC_MODE_A3_MSK		GENMASK(15, 14)
> > +#define REQ_FEC_MODE_B0_MSK		GENMASK(17, 16)
> > +#define REQ_FEC_MODE_B1_MSK		GENMASK(19, 18)
> > +#define REQ_FEC_MODE_B2_MSK		GENMASK(21, 20)
> > +#define REQ_FEC_MODE_B3_MSK		GENMASK(23, 22)
> > +#define REQ_FEC_MODE_NO			0x0
> > +#define REQ_FEC_MODE_KR			0x1
> > +#define REQ_FEC_MODE_RS			0x2
> > +
> > +#define NIOS_FW_VERSION			0x1004
> > +#define NIOS_FW_VERSION_PATCH		GENMASK(23, 20)
> > +#define NIOS_FW_VERSION_MINOR		GENMASK(27, 24)
> > +#define NIOS_FW_VERSION_MAJOR		GENMASK(31, 28)
> > +
> > +/* The retimers we use on Intel PAC N3000 is Parkvale, abbreviated to PKVL */
> Parkvale is codename, why not just RETIMER_ ?

The register fields names in Hardware spec is PKVL_, so I mean to keep
align with the spec.

> > +#define PKVL_A_MODE_STS			0x1020
> > +#define PKVL_B_MODE_STS			0x1024
> > +#define PKVL_MODE_STS_GROUP_MSK		GENMASK(15, 8)
> > +#define PKVL_MODE_STS_GROUP_OK		0x0
> > +#define PKVL_MODE_STS_ID_MSK		GENMASK(7, 0)
> > +/* When GROUP MASK field == GROUP_OK  */
> > +#define PKVL_MODE_ID_RESET		0x0
> > +#define PKVL_MODE_ID_4X10G		0x1
> > +#define PKVL_MODE_ID_4X25G		0x2
> > +#define PKVL_MODE_ID_2X25G		0x3
> > +#define PKVL_MODE_ID_2X25G_2X10G	0x4
> > +#define PKVL_MODE_ID_1X25G		0x5
> > +
> > +#define NS_REGBUS_WAIT_TIMEOUT		10000		/* loop count */
> 
> NS ? Expand.

I could explain the NS - NIOS SPI in comments, and is N3000_NS_REGBUS_RETRY_COUNT good?

> 
> 'WAIT_TIMEOUT' should be changed to 'RETRY_COUNT' to reflect its use

It makes sense.

> 
> > +#define NIOS_INIT_TIMEOUT		10000000	/* usec */
> > +#define NIOS_INIT_TIME_INTV		100000		/* usec */
> > +
> > +struct n3000_nios {
> > +	void __iomem *base;
> > +	struct regmap *regmap;
> > +	struct device *dev;
> > +	struct platform_device *altera_spi;
> > +};
> > +
> > +static ssize_t nios_fw_version_show(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct n3000_nios *ns = dev_get_drvdata(dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "%x.%x.%x\n",
> > +		       (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val),
> > +		       (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val),
> > +		       (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val));
> > +}
> > +static DEVICE_ATTR_RO(nios_fw_version);
> > +
> > +#define IS_MODE_STATUS_OK(mode_stat)				\
> > +	(FIELD_GET(PKVL_MODE_STS_GROUP_MSK, (mode_stat)) ==	\
> > +	 PKVL_MODE_STS_GROUP_OK)
> > +
> > +#define IS_RETIMER_FEC_SUPPORTED(retimer_mode)	\
> > +	((retimer_mode) != PKVL_MODE_ID_RESET &&	\
> > +	 (retimer_mode) != PKVL_MODE_ID_4X10G)
> > +
> > +static int get_retimer_mode(struct n3000_nios *ns, unsigned int mode_stat_reg,
> > +			    unsigned int *retimer_mode)
> > +{
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(ns->regmap, mode_stat_reg, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!IS_MODE_STATUS_OK(val))
> > +		return -EFAULT;
> > +
> > +	*retimer_mode = FIELD_GET(PKVL_MODE_STS_ID_MSK, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t retimer_A_mode_show(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct n3000_nios *ns = dev_get_drvdata(dev);
> > +	unsigned int mode;
> > +	int ret;
> > +
> > +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", mode);
> > +}
> > +static DEVICE_ATTR_RO(retimer_A_mode);
> > +
> > +static ssize_t retimer_B_mode_show(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct n3000_nios *ns = dev_get_drvdata(dev);
> > +	unsigned int mode;
> > +	int ret;
> > +
> > +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", mode);
> > +}
> > +static DEVICE_ATTR_RO(retimer_B_mode);
> > +
> > +static ssize_t fec_mode_show(struct device *dev,
> > +			     struct device_attribute *attr, char *buf)
> > +{
> > +	unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
> > +	struct n3000_nios *ns = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	/* FEC mode setting is not supported in early FW versions */
> > +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) < 3)
> > +		return sprintf(buf, "not supported\n");
> > +
> > +	/* If no 25G links, FEC mode setting is not supported either */
> > +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &retimer_a_mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &retimer_b_mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!IS_RETIMER_FEC_SUPPORTED(retimer_a_mode) &&
> > +	    !IS_RETIMER_FEC_SUPPORTED(retimer_b_mode))
> > +		return sprintf(buf, "not supported\n");
> > +
> > +	/* get the valid FEC mode for 25G links */
> > +	ret = regmap_read(ns->regmap, NIOS_INIT, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * FEC mode should always be the same for all links, as we set them
> > +	 * in this way.
> > +	 */
> > +	fec_mode = FIELD_GET(REQ_FEC_MODE_A0_MSK, val);
> > +	if (fec_mode != FIELD_GET(REQ_FEC_MODE_A1_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A2_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A3_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B0_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B1_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B2_MSK, val) ||
> > +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B3_MSK, val))
> > +		return -EFAULT;
> 
> a precomputed table setting the fields would reduce the this to a lookup and 1 check.
> 
> similar table could be used below for the setting.

Yes, I can do it.

> 
> > +
> > +	switch (fec_mode) {
> > +	case REQ_FEC_MODE_NO:
> > +		return sprintf(buf, "no\n");
> > +	case REQ_FEC_MODE_KR:
> > +		return sprintf(buf, "kr\n");
> > +	case REQ_FEC_MODE_RS:
> > +		return sprintf(buf, "rs\n");
> > +	}
> > +
> > +	return -EFAULT;
> > +}
> > +static DEVICE_ATTR_RO(fec_mode);
> > +
> > +static struct attribute *n3000_nios_attrs[] = {
> > +	&dev_attr_nios_fw_version.attr,
> > +	&dev_attr_retimer_A_mode.attr,
> > +	&dev_attr_retimer_B_mode.attr,
> > +	&dev_attr_fec_mode.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(n3000_nios);
> > +
> > +static bool init_error_detected(struct n3000_nios *ns)
> > +{
> > +	unsigned int val;
> > +
> > +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
> > +		return true;
> > +
> > +	if (!IS_MODE_STATUS_OK(val))
> > +		return true;
> > +
> > +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
> > +		return true;
> > +
> > +	if (!IS_MODE_STATUS_OK(val))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +static void dump_error_stat(struct n3000_nios *ns)
> > +{
> > +	unsigned int val;
> > +
> > +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
> > +		return;
> > +
> > +	dev_err(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
> > +
> > +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
> > +		return;
> > +
> > +	dev_err(ns->dev, "PKVL_B_MODE_STS 0x%x\n", val);
> > +}
> > +
> > +static int n3000_nios_init_done_check(struct n3000_nios *ns)
> > +{
> > +	struct device *dev = ns->dev;
> > +	unsigned int val, mode;
> > +	int ret;
> > +
> > +	/*
> > +	 * this SPI is shared by Nios core inside FPGA, Nios will use this SPI
> 'by the Nios core inside the FPGA ... '

Yes.

> > +	 * master to do some one time initialization after power up, and then
> > +	 * release the control to OS. driver needs to poll on INIT_DONE to
> 'The driver..'

Yes.

> > +	 * see when driver could take the control.
> > +	 *
> > +	 * Please note that after Nios firmware version 3.0.0, INIT_START is
> > +	 * introduced, so driver needs to trigger START firstly and then check
> > +	 * INIT_DONE.
> > +	 */
> > +
> > +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * If Nios version register is totally uninitialized(== 0x0), then the
> > +	 * Nios firmware is missing. So host could take control of SPI master
> > +	 * safely, but initialization work for Nios is not done. To restore the
> > +	 * card, we need to reprogram a new Nios firmware via the BMC chip on
> > +	 * SPI bus. So the driver doesn't error out, it continues to create the
> > +	 * spi controller device and spi_board_info for BMC.
> > +	 */
> > +	if (val == 0) {
> > +		dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n",
> > +			val);
> > +		return 0;
> > +	}
> > +
> > +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
> > +		/* read NIOS_INIT to check if retimer initialization is done */
> > +		ret = regmap_read(ns->regmap, NIOS_INIT, &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* check if retimers are initialized already */
> > +		if (val & NIOS_INIT_DONE || val & NIOS_INIT_START)
> val & (NIOS_INIT_DONE | NIOS_INIT_START)

Yes.

> > +			goto nios_init_done;
> > +
> > +		/* configure FEC mode per module param */
> > +		val = NIOS_INIT_START;
> > +
> > +		/*
> > +		 * When the retimer is to be set to 10G mode, there is no FEC
> > +		 * mode setting, so the REQ_FEC_MODE field will be ignored by
> > +		 * Nios firmware in this case. But we should still fill the FEC
> > +		 * mode field cause host could not get the retimer working mode
> > +		 * until the Nios init is done.
> > +		 *
> > +		 * For now the driver doesn't support the retimer FEC mode
> > +		 * switching per user's request. It is always set to Reed
> > +		 * Solomon FEC.
> > +		 */
> > +		mode = REQ_FEC_MODE_RS;
> > +
> > +		/* set the same FEC mode for all links */
> > +		val |= FIELD_PREP(REQ_FEC_MODE_A0_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_A1_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_A2_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_A3_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_B0_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_B1_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_B2_MSK, mode) |
> > +		       FIELD_PREP(REQ_FEC_MODE_B3_MSK, mode);

I'll also optimize the code here.

> > +
> > +		ret = regmap_write(ns->regmap, NIOS_INIT, val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +nios_init_done:
> > +	/* polls on NIOS_INIT_DONE */
> > +	ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val,
> > +				       val & NIOS_INIT_DONE,
> > +				       NIOS_INIT_TIME_INTV,
> > +				       NIOS_INIT_TIMEOUT);
> > +	if (ret) {
> > +		dev_err(dev, "NIOS_INIT_DONE %s\n",
> > +			(ret == -ETIMEDOUT) ? "timed out" : "check error");
> > +		goto dump_sts;
> > +	}
> > +
> > +	/*
> > +	 * After INIT_DONE is detected, it still needs to check if any error
> > +	 * detected.
> > +	 */
> > +	if (init_error_detected(ns)) {
> > +		/*
> > +		 * If the retimer configuration is failed, the Nios firmware
> > +		 * will still release the spi controller for host to
> > +		 * communicate with the BMC. It makes possible for people to
> > +		 * reprogram a new Nios firmware and restore the card. So the
> > +		 * driver doesn't error out, it continues to create the spi
> > +		 * controller device and spi_board_info for BMC.
> > +		 */
> > +		dev_err(dev, "NIOS_INIT_DONE OK, but err found during init\n");
> > +		goto dump_sts;
> > +	}
> > +	return 0;
> > +
> > +dump_sts:
> > +	dump_error_stat(ns);
> Could init_error_detected and dump_error_stat be combined ?

Yes, I could try to refactor it.

> > +
> > +	return ret;
> > +}
> > +
> > +static struct spi_board_info m10_n3000_info = {
> > +	.modalias = "m10-n3000",
> > +	.max_speed_hz = 12500000,
> > +	.bus_num = 0,
> > +	.chip_select = 0,
> > +};
> > +
> > +static int create_altera_spi_controller(struct n3000_nios *ns)
> > +{
> > +	struct altera_spi_platform_data pdata = { 0 };
> > +	struct platform_device_info pdevinfo = { 0 };
> > +	void __iomem *base = ns->base;
> > +	u64 v;
> > +
> > +	v = readq(base + NIOS_SPI_PARAM);
> > +
> > +	pdata.mode_bits = SPI_CS_HIGH;
> > +	if (FIELD_GET(PARAM_CLK_POL, v))
> > +		pdata.mode_bits |= SPI_CPOL;
> > +	if (FIELD_GET(PARAM_CLK_PHASE, v))
> > +		pdata.mode_bits |= SPI_CPHA;
> > +
> > +	pdata.num_chipselect = FIELD_GET(PARAM_NUM_CS, v);
> > +	pdata.bits_per_word_mask =
> > +		SPI_BPW_RANGE_MASK(1, FIELD_GET(PARAM_DATA_WIDTH, v));
> > +
> > +	pdata.num_devices = 1;
> > +	pdata.devices = &m10_n3000_info;
> > +
> > +	dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > +		pdata.num_chipselect, pdata.bits_per_word_mask,
> > +		pdata.mode_bits);
> > +
> > +	pdevinfo.name = "subdev_spi_altera";
> > +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > +	pdevinfo.parent = ns->dev;
> > +	pdevinfo.data = &pdata;
> > +	pdevinfo.size_data = sizeof(pdata);
> > +
> > +	ns->altera_spi = platform_device_register_full(&pdevinfo);
> > +	return PTR_ERR_OR_ZERO(ns->altera_spi);
> > +}
> > +
> > +static void destroy_altera_spi_controller(struct n3000_nios *ns)
> > +{
> > +	platform_device_unregister(ns->altera_spi);
> > +}
> > +
> > +/* ns is the abbreviation of nios_spi */
> > +static int ns_poll_stat_timeout(void __iomem *base, u64 *v)
> > +{
> > +	int loops = NS_REGBUS_WAIT_TIMEOUT;
> > +
> > +	/*
> > +	 * We don't use the time based timeout here for performance.
> > +	 *
> > +	 * The regbus read/write is on the critical path of Intel PAC N3000
> > +	 * image programing. The time based timeout checking will add too much
> > +	 * overhead on it. Usually the state changes in 1 or 2 loops on the
> > +	 * test server, and we set 10000 times loop here for safety.
> > +	 */
> > +	do {
> > +		*v = readq(base + NIOS_SPI_STAT);
> > +		if (*v & STAT_RW_VAL)
> > +			break;
> > +		cpu_relax();
> > +	} while (--loops);
> This becomes an infinite loop if loops <= 0

The loops count is always initialized at the start of the function.
Do you mean we should lower the impact when the developer inputs the
wrong loop count?

Is it better we change to:

	unsigned int loops = NS_REGBUS_WAIT_TIMEOUT;

	while (loops--) {
		
	}

> > +
> > +	return loops ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int ns_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > +	struct n3000_nios *ns = context;
> > +	u64 v = 0;
> > +	int ret;
> > +
> > +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_WR);
> > +	v |= FIELD_PREP(CTRL_ADDR, reg);
> > +	v |= FIELD_PREP(CTRL_WR_DATA, val);
> > +	writeq(v, ns->base + NIOS_SPI_CTRL);
> > +
> > +	ret = ns_poll_stat_timeout(ns->base, &v);
> > +	if (ret)
> > +		dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
> > +			reg, val, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ns_reg_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > +	struct n3000_nios *ns = context;
> > +	u64 v = 0;
> > +	int ret;
> > +
> > +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_RD);
> 
> v = , and save the intialiation
> 
> or v = ( ... | ... )

Yes.

> 
> > +	v |= FIELD_PREP(CTRL_ADDR, reg);
> > +	writeq(v, ns->base + NIOS_SPI_CTRL);
> > +
> > +	ret = ns_poll_stat_timeout(ns->base, &v);
> > +	if (ret)
> > +		dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
> > +	else
> > +		*val = FIELD_GET(STAT_RD_DATA, v);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct regmap_config ns_regbus_cfg = {
> > +	.reg_bits = 32,
> > +	.reg_stride = 4,
> > +	.val_bits = 32,
> > +	.fast_io = true,
> > +
> > +	.reg_write = ns_reg_write,
> > +	.reg_read = ns_reg_read,
> > +};
> > +
> > +static int n3000_nios_probe(struct dfl_device *ddev)
> > +{
> > +	struct device *dev = &ddev->dev;
> > +	struct n3000_nios *ns;
> > +	int ret;
> > +
> > +	ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
> > +	if (!ns)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(&ddev->dev, ns);
> > +
> > +	ns->dev = dev;
> > +
> > +	ns->base = devm_ioremap_resource(&ddev->dev, &ddev->mmio_res);
> > +	if (IS_ERR(ns->base))
> > +		return PTR_ERR(ns->base);
> > +
> > +	ns->regmap = devm_regmap_init(dev, NULL, ns, &ns_regbus_cfg);
> > +	if (IS_ERR(ns->regmap))
> > +		return PTR_ERR(ns->regmap);
> > +
> > +	ret = n3000_nios_init_done_check(ns);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = create_altera_spi_controller(ns);
> > +	if (ret)
> > +		dev_err(dev, "altera spi controller create failed: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static void n3000_nios_remove(struct dfl_device *ddev)
> > +{
> > +	struct n3000_nios *ns = dev_get_drvdata(&ddev->dev);
> > +
> > +	destroy_altera_spi_controller(ns);
> > +}
> > +
> > +#define FME_FEATURE_ID_N3000_NIOS	0xd
> > +
> > +static const struct dfl_device_id n3000_nios_ids[] = {
> > +	{ FME_ID, FME_FEATURE_ID_N3000_NIOS },
> > +	{ }
> > +};
> > +
> > +static struct dfl_driver n3000_nios_driver = {
> > +	.drv	= {
> > +		.name       = "n3000-nios",
> 
> "dfl-n3000-nios" ?

Yes. I think it is better.

Thanks,
Yilun

> 
> Tom
> 
> > +		.dev_groups = n3000_nios_groups,
> > +	},
> > +	.id_table = n3000_nios_ids,
> > +	.probe   = n3000_nios_probe,
> > +	.remove  = n3000_nios_remove,
> > +};
> > +
> > +module_dfl_driver(n3000_nios_driver);
> > +
> > +MODULE_DEVICE_TABLE(dfl, n3000_nios_ids);
> > +MODULE_DESCRIPTION("Driver for Nios private feature on Intel PAC N3000");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");

Re: [PATCH v9 5/6] fpga: dfl: add support for N3000 Nios private feature

[Tom Rix]

On 10/11/20 11:51 PM, Xu Yilun wrote:
> On Sat, Oct 10, 2020 at 02:21:45PM -0700, Tom Rix wrote:
>> On 10/10/20 12:09 AM, Xu Yilun wrote:
>>> This patch adds support for the Nios handshake private feature on Intel
>>> PAC (Programmable Acceleration Card) N3000.
>>>
>>> The Nios is the embedded processor on the FPGA card. This private feature
>>> provides a handshake interface to FPGA Nios firmware, which receives
>>> retimer configuration command from host and executes via an internal SPI
>>> master (spi-altera). When Nios finishes the configuration, host takes over
>>> the ownership of the SPI master to control an Intel MAX10 BMC (Board
>>> Management Controller) Chip on the SPI bus.
>>>
>>> For Nios firmware handshake part, this driver requests the retimer
>>> configuration for Nios firmware on probe, and adds some sysfs nodes for
>>> user to query the onboard retimer's working mode and Nios firmware
>>> version.
>>>
>>> For SPI part, this driver adds a spi-altera platform device as well as
>>> the MAX10 BMC spi slave info. A spi-altera driver will be matched to
>>> handle the following SPI work.
>>>
>>> [mdf@kernel.org: Fixed up MAINTAINERS file to include added ABI doc]
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> Signed-off-by: Wu Hao <hao.wu@intel.com>
>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> ---
>>> v3: add the doc for this driver
>>>     minor fixes for comments from Tom
>>> v4: move the err log in regmap implementation, and delete
>>>      n3000_nios_writel/readl(), they have nothing to wrapper now.
>>>     some minor fixes and comments improvement.
>>> v5: fix the output of fec_mode sysfs inf to "no" on 10G configuration,
>>>      cause no FEC mode could be configured for 10G.
>>>     rename the dfl_n3000_nios_* to n3000_nios_*
>>>     improves comments.
>>> v6: fix the output of fec_mode sysfs inf to "not supported" if in 10G,
>>>      or the firmware version major < 3.
>>>     minor fixes and improves comments.
>>> v7: improves comments.
>>> v8: add sysfs interfaces for retimer mode, also doc update.
>>>     delete duplicated sysfs interfaces description in doc.
>>>     minor fixes.
>>> v9: delete the retimer FEC mode configuration via module_parameter.
>>>     update the kernel version of the sysfs interfaces in Doc.
>>>     merge the patch "Make m10_n3000_info static" (https://lore.kernel.org/linux-fpga/52d8411e-13d8-1e91-756d-131802f5f445@huawei.com/T/#t)
>>>     remove the tags of Maintainers, this implementation is changed.
>>> ---
>>>  .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios   |  47 ++
>>>  MAINTAINERS                                        |   2 +-
>>>  drivers/fpga/Kconfig                               |  11 +
>>>  drivers/fpga/Makefile                              |   2 +
>>>  drivers/fpga/dfl-n3000-nios.c                      | 566 +++++++++++++++++++++
>>>  5 files changed, 627 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>>  create mode 100644 drivers/fpga/dfl-n3000-nios.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> new file mode 100644
>>> index 0000000..f2f6e5e
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> @@ -0,0 +1,47 @@
>>> +What:		/sys/bus/dfl/devices/dfl_dev.X/fec_mode
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>> +Contact:	Xu Yilun <yilun.xu@intel.com>
>>> +Description:	Read-only. It returns the FEC mode of the 25G links of the
>> Could drop the 'It' and start with 'Returns'  here and similar
> Yes, I can change it.
>
>>> +		ethernet retimers configured by Nios firmware. "rs" for Reed
>>> +		Solomon FEC, "kr" for Fire Code FEC, "no" for NO FEC.
>>> +		"not supported" if the FEC mode setting is not supported, this
>> setting ? the description says 'Read-only'
> The setting is done by Nios firmware, host cannot change it at runtime
> so it is 'Read-only'. It is previously described in this section. Is it
> good?
ok
>
>>> +		happens when the Nios firmware version major < 3, or no link is
>>> +		configured to 25G.
>>> +		Format: string
>>> +
>>> +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_A_mode
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>> +Contact:	Xu Yilun <yilun.xu@intel.com>
>>> +Description:	Read-only. It returns the enumeration value of the working mode
>>> +		of the retimer A configured by the Nios firmware. The value is
>>> +		read out from shared registers filled by the Nios firmware. Now
>>> +		the values could be:
>>> +
>>> +		- "0": Reset
>>> +		- "1": 4x10G
>>> +		- "2": 4x25G
>>> +		- "3": 2x25G
>>> +		- "4": 2x25G+2x10G
>>> +		- "5": 1x25G
>>> +
>>> +		If the Nios firmware is updated in future to support more
>>> +		retimer modes, more enumeration value is expected. 
>>> +		Format: 0x%x
>>> +
>>> +What:		/sys/bus/dfl/devices/dfl_dev.X/retimer_B_mode
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>> +Contact:	Xu Yilun <yilun.xu@intel.com>
>>> +Description:	Read-only. It returns the enumeration value of the working mode
>>> +		of the retimer B configured by the Nios firmware. The value
>>> +		format is the same as retimer_A_mode.
>>> +
>>> +What:		/sys/bus/dfl/devices/dfl_dev.X/nios_fw_version
>>> +Date:		Oct 2020
>>> +KernelVersion:	5.11
>>> +Contact:	Xu Yilun <yilun.xu@intel.com>
>>> +Description:	Read-only. It returns the version of the Nios firmware in FPGA.
>> Can drop 'in FPGA'.
> The PAC N3000 card has 2 Nios firmwares. The one for this driver is
> embedded in A10 FPGA. Another one is embedded in MAX10 BMC chip.
>
> So to be clear, I think we may keep the 'in FPGA'.

ok, then change to

'in the FPGA'

>
>>> +		Its format is "major.minor.patch".
>>> +		Format: %x.%x.%x
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 9d46097..3a81cac 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6881,7 +6881,7 @@ M:	Wu Hao <hao.wu@intel.com>
>>>  R:	Tom Rix <trix@redhat.com>
>>>  L:	linux-fpga@vger.kernel.org
>>>  S:	Maintained
>>> -F:	Documentation/ABI/testing/sysfs-bus-dfl
>>> +F:	Documentation/ABI/testing/sysfs-bus-dfl*
>>>  F:	Documentation/fpga/dfl.rst
>>>  F:	drivers/fpga/dfl*
>>>  F:	include/linux/fpga/dfl.h
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 7cd5a29..38c7130 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -191,6 +191,17 @@ config FPGA_DFL_AFU
>>>  	  to the FPGA infrastructure via a Port. There may be more than one
>>>  	  Port/AFU per DFL based FPGA device.
>>>  
>>> +config FPGA_DFL_NIOS_INTEL_PAC_N3000
>>> +        tristate "FPGA DFL NIOS Driver for Intel PAC N3000"
>>> +        depends on FPGA_DFL
>> Would 'depends on FPGA_DFL_PCI' be better ?
> The relationship between the 2 config is, the n3000 nios private feature
> now only appears on PAC N3000 which is a PCIe card.
>
> But the code has no dependency to FPGA_DFL_PCI. Is it necessary we add the
> 'depends on'?

as-is is fine

i am thinking a card specific config is needed but that is another topic.

>
>>> +        select REGMAP
>>> +        help
>> Last 3 lines have whitespace issues, need to convert spaces to tabs
> Thanks for catching this.
>
>>> +	  This is the driver for the N3000 Nios private feature on Intel
>>> +	  PAC (Programmable Acceleration Card) N3000. It communicates
>>> +	  with the embedded Nios processor to configure the retimers on
>>> +	  the card. It also instantiates the SPI master (spi-altera) for
>>> +	  the card's BMC (Board Management Controller) control.
>> can drop 'control'
>>
>> If you read out 'card's board mangement controller control' it sounds awkward.
> It's true. I'll drop it.
>
>>> +
>>>  config FPGA_DFL_PCI
>>>  	tristate "FPGA DFL PCIe Device Driver"
>>>  	depends on PCI && FPGA_DFL
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>> index d8e21df..18dc9885 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o
>>>  dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>>>  dfl-afu-objs += dfl-afu-error.o
>>>  
>>> +obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
>>> +
>>>  # Drivers for FPGAs which implement DFL
>>>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
>>> diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c
>>> new file mode 100644
>>> index 0000000..e7c3862
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-n3000-nios.c
>>> @@ -0,0 +1,566 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * DFL device driver for Nios private feature on Intel PAC (Programmable
>>> + * Acceleration Card) N3000
>>> + *
>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>> + *
>>> + * Authors:
>>> + *   Wu Hao <hao.wu@intel.com>
>>> + *   Xu Yilun <yilun.xu@intel.com>
>>> + */
>>> +#include <linux/bitfield.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/fpga/dfl.h>
>>> +#include <linux/io.h>
>>> +#include <linux/io-64-nonatomic-lo-hi.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/spi/altera.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/types.h>
>>> +
>>> +/* N3000 Nios private feature registers */
>>> +#define NIOS_SPI_PARAM			0x8
>> The #defines should have a consistent prefix that is unlikely to conflict.
>>
>> Maybe N3000_NIOS_
> Yes, I'll try to refactor the macro names.
> The register names in spec are NIOS_SPI_xx, how about we compress the prefixes,
> 'N3000_NS' & 'n3000_ns' for 'N3000 NIOS SPI'
ok for n3000_ns.  just move the comment for 'ns' up this first use.
>
>>> +#define PARAM_SHIFT_MODE_MSK		BIT_ULL(1)
>>> +#define PARAM_SHIFT_MODE_MSB		0
>>> +#define PARAM_SHIFT_MODE_LSB		1
>>> +#define PARAM_DATA_WIDTH		GENMASK_ULL(7, 2)
>>> +#define PARAM_NUM_CS			GENMASK_ULL(13, 8)
>>> +#define PARAM_CLK_POL			BIT_ULL(14)
>>> +#define PARAM_CLK_PHASE			BIT_ULL(15)
>>> +#define PARAM_PERIPHERAL_ID		GENMASK_ULL(47, 32)
>>> +
>>> +#define NIOS_SPI_CTRL			0x10
>>> +#define CTRL_WR_DATA			GENMASK_ULL(31, 0)
>>> +#define CTRL_ADDR			GENMASK_ULL(44, 32)
>>> +#define CTRL_CMD_MSK			GENMASK_ULL(63, 62)
>>> +#define CTRL_CMD_NOP			0
>>> +#define CTRL_CMD_RD			1
>>> +#define CTRL_CMD_WR			2
>>> +
>>> +#define NIOS_SPI_STAT			0x18
>>> +#define STAT_RD_DATA			GENMASK_ULL(31, 0)
>>> +#define STAT_RW_VAL			BIT_ULL(32)
>>> +
>>> +/* Nios handshake registers, indirect access */
>>> +#define NIOS_INIT			0x1000
>>> +#define NIOS_INIT_DONE			BIT(0)
>>> +#define NIOS_INIT_START			BIT(1)
>>> +/* Mode for retimer A, link 0, the same below */
>>> +#define REQ_FEC_MODE_A0_MSK		GENMASK(9, 8)
>>> +#define REQ_FEC_MODE_A1_MSK		GENMASK(11, 10)
>>> +#define REQ_FEC_MODE_A2_MSK		GENMASK(13, 12)
>>> +#define REQ_FEC_MODE_A3_MSK		GENMASK(15, 14)
>>> +#define REQ_FEC_MODE_B0_MSK		GENMASK(17, 16)
>>> +#define REQ_FEC_MODE_B1_MSK		GENMASK(19, 18)
>>> +#define REQ_FEC_MODE_B2_MSK		GENMASK(21, 20)
>>> +#define REQ_FEC_MODE_B3_MSK		GENMASK(23, 22)
>>> +#define REQ_FEC_MODE_NO			0x0
>>> +#define REQ_FEC_MODE_KR			0x1
>>> +#define REQ_FEC_MODE_RS			0x2
>>> +
>>> +#define NIOS_FW_VERSION			0x1004
>>> +#define NIOS_FW_VERSION_PATCH		GENMASK(23, 20)
>>> +#define NIOS_FW_VERSION_MINOR		GENMASK(27, 24)
>>> +#define NIOS_FW_VERSION_MAJOR		GENMASK(31, 28)
>>> +
>>> +/* The retimers we use on Intel PAC N3000 is Parkvale, abbreviated to PKVL */
>> Parkvale is codename, why not just RETIMER_ ?
> The register fields names in Hardware spec is PKVL_, so I mean to keep
> align with the spec.
ok
>
>>> +#define PKVL_A_MODE_STS			0x1020
>>> +#define PKVL_B_MODE_STS			0x1024
>>> +#define PKVL_MODE_STS_GROUP_MSK		GENMASK(15, 8)
>>> +#define PKVL_MODE_STS_GROUP_OK		0x0
>>> +#define PKVL_MODE_STS_ID_MSK		GENMASK(7, 0)
>>> +/* When GROUP MASK field == GROUP_OK  */
>>> +#define PKVL_MODE_ID_RESET		0x0
>>> +#define PKVL_MODE_ID_4X10G		0x1
>>> +#define PKVL_MODE_ID_4X25G		0x2
>>> +#define PKVL_MODE_ID_2X25G		0x3
>>> +#define PKVL_MODE_ID_2X25G_2X10G	0x4
>>> +#define PKVL_MODE_ID_1X25G		0x5
>>> +
>>> +#define NS_REGBUS_WAIT_TIMEOUT		10000		/* loop count */
>> NS ? Expand.
> I could explain the NS - NIOS SPI in comments, and is N3000_NS_REGBUS_RETRY_COUNT good?
yes this is fine. for use of 'ns' which has a more common meaning of 'nano second' need to take care when meaning could be confused with 'nios spi'
>
>> 'WAIT_TIMEOUT' should be changed to 'RETRY_COUNT' to reflect its use
> It makes sense.
>
>>> +#define NIOS_INIT_TIMEOUT		10000000	/* usec */
>>> +#define NIOS_INIT_TIME_INTV		100000		/* usec */
>>> +
>>> +struct n3000_nios {
>>> +	void __iomem *base;
>>> +	struct regmap *regmap;
>>> +	struct device *dev;
>>> +	struct platform_device *altera_spi;
>>> +};
>>> +
>>> +static ssize_t nios_fw_version_show(struct device *dev,
>>> +				    struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct n3000_nios *ns = dev_get_drvdata(dev);
>>> +	unsigned int val;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "%x.%x.%x\n",
>>> +		       (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val),
>>> +		       (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val),
>>> +		       (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val));
>>> +}
>>> +static DEVICE_ATTR_RO(nios_fw_version);
>>> +
>>> +#define IS_MODE_STATUS_OK(mode_stat)				\
>>> +	(FIELD_GET(PKVL_MODE_STS_GROUP_MSK, (mode_stat)) ==	\
>>> +	 PKVL_MODE_STS_GROUP_OK)
>>> +
>>> +#define IS_RETIMER_FEC_SUPPORTED(retimer_mode)	\
>>> +	((retimer_mode) != PKVL_MODE_ID_RESET &&	\
>>> +	 (retimer_mode) != PKVL_MODE_ID_4X10G)
>>> +
>>> +static int get_retimer_mode(struct n3000_nios *ns, unsigned int mode_stat_reg,
>>> +			    unsigned int *retimer_mode)
>>> +{
>>> +	unsigned int val;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(ns->regmap, mode_stat_reg, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!IS_MODE_STATUS_OK(val))
>>> +		return -EFAULT;
>>> +
>>> +	*retimer_mode = FIELD_GET(PKVL_MODE_STS_ID_MSK, val);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static ssize_t retimer_A_mode_show(struct device *dev,
>>> +				   struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct n3000_nios *ns = dev_get_drvdata(dev);
>>> +	unsigned int mode;
>>> +	int ret;
>>> +
>>> +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &mode);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "0x%x\n", mode);
>>> +}
>>> +static DEVICE_ATTR_RO(retimer_A_mode);
>>> +
>>> +static ssize_t retimer_B_mode_show(struct device *dev,
>>> +				   struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct n3000_nios *ns = dev_get_drvdata(dev);
>>> +	unsigned int mode;
>>> +	int ret;
>>> +
>>> +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &mode);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "0x%x\n", mode);
>>> +}
>>> +static DEVICE_ATTR_RO(retimer_B_mode);
>>> +
>>> +static ssize_t fec_mode_show(struct device *dev,
>>> +			     struct device_attribute *attr, char *buf)
>>> +{
>>> +	unsigned int val, retimer_a_mode, retimer_b_mode, fec_mode;
>>> +	struct n3000_nios *ns = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	/* FEC mode setting is not supported in early FW versions */
>>> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) < 3)
>>> +		return sprintf(buf, "not supported\n");
>>> +
>>> +	/* If no 25G links, FEC mode setting is not supported either */
>>> +	ret = get_retimer_mode(ns, PKVL_A_MODE_STS, &retimer_a_mode);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = get_retimer_mode(ns, PKVL_B_MODE_STS, &retimer_b_mode);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!IS_RETIMER_FEC_SUPPORTED(retimer_a_mode) &&
>>> +	    !IS_RETIMER_FEC_SUPPORTED(retimer_b_mode))
>>> +		return sprintf(buf, "not supported\n");
>>> +
>>> +	/* get the valid FEC mode for 25G links */
>>> +	ret = regmap_read(ns->regmap, NIOS_INIT, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * FEC mode should always be the same for all links, as we set them
>>> +	 * in this way.
>>> +	 */
>>> +	fec_mode = FIELD_GET(REQ_FEC_MODE_A0_MSK, val);
>>> +	if (fec_mode != FIELD_GET(REQ_FEC_MODE_A1_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A2_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_A3_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B0_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B1_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B2_MSK, val) ||
>>> +	    fec_mode != FIELD_GET(REQ_FEC_MODE_B3_MSK, val))
>>> +		return -EFAULT;
>> a precomputed table setting the fields would reduce the this to a lookup and 1 check.
>>
>> similar table could be used below for the setting.
> Yes, I can do it.
>
>>> +
>>> +	switch (fec_mode) {
>>> +	case REQ_FEC_MODE_NO:
>>> +		return sprintf(buf, "no\n");
>>> +	case REQ_FEC_MODE_KR:
>>> +		return sprintf(buf, "kr\n");
>>> +	case REQ_FEC_MODE_RS:
>>> +		return sprintf(buf, "rs\n");
>>> +	}
>>> +
>>> +	return -EFAULT;
>>> +}
>>> +static DEVICE_ATTR_RO(fec_mode);
>>> +
>>> +static struct attribute *n3000_nios_attrs[] = {
>>> +	&dev_attr_nios_fw_version.attr,
>>> +	&dev_attr_retimer_A_mode.attr,
>>> +	&dev_attr_retimer_B_mode.attr,
>>> +	&dev_attr_fec_mode.attr,
>>> +	NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(n3000_nios);
>>> +
>>> +static bool init_error_detected(struct n3000_nios *ns)
>>> +{
>>> +	unsigned int val;
>>> +
>>> +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
>>> +		return true;
>>> +
>>> +	if (!IS_MODE_STATUS_OK(val))
>>> +		return true;
>>> +
>>> +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
>>> +		return true;
>>> +
>>> +	if (!IS_MODE_STATUS_OK(val))
>>> +		return true;
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +static void dump_error_stat(struct n3000_nios *ns)
>>> +{
>>> +	unsigned int val;
>>> +
>>> +	if (regmap_read(ns->regmap, PKVL_A_MODE_STS, &val))
>>> +		return;
>>> +
>>> +	dev_err(ns->dev, "PKVL_A_MODE_STS 0x%x\n", val);
>>> +
>>> +	if (regmap_read(ns->regmap, PKVL_B_MODE_STS, &val))
>>> +		return;
>>> +
>>> +	dev_err(ns->dev, "PKVL_B_MODE_STS 0x%x\n", val);
>>> +}
>>> +
>>> +static int n3000_nios_init_done_check(struct n3000_nios *ns)
>>> +{
>>> +	struct device *dev = ns->dev;
>>> +	unsigned int val, mode;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * this SPI is shared by Nios core inside FPGA, Nios will use this SPI
>> 'by the Nios core inside the FPGA ... '
> Yes.
>
>>> +	 * master to do some one time initialization after power up, and then
>>> +	 * release the control to OS. driver needs to poll on INIT_DONE to
>> 'The driver..'
> Yes.
>
>>> +	 * see when driver could take the control.
>>> +	 *
>>> +	 * Please note that after Nios firmware version 3.0.0, INIT_START is
>>> +	 * introduced, so driver needs to trigger START firstly and then check
>>> +	 * INIT_DONE.
>>> +	 */
>>> +
>>> +	ret = regmap_read(ns->regmap, NIOS_FW_VERSION, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * If Nios version register is totally uninitialized(== 0x0), then the
>>> +	 * Nios firmware is missing. So host could take control of SPI master
>>> +	 * safely, but initialization work for Nios is not done. To restore the
>>> +	 * card, we need to reprogram a new Nios firmware via the BMC chip on
>>> +	 * SPI bus. So the driver doesn't error out, it continues to create the
>>> +	 * spi controller device and spi_board_info for BMC.
>>> +	 */
>>> +	if (val == 0) {
>>> +		dev_err(dev, "Nios version reg = 0x%x, skip INIT_DONE check, but the retimer may be uninitialized\n",
>>> +			val);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
>>> +		/* read NIOS_INIT to check if retimer initialization is done */
>>> +		ret = regmap_read(ns->regmap, NIOS_INIT, &val);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		/* check if retimers are initialized already */
>>> +		if (val & NIOS_INIT_DONE || val & NIOS_INIT_START)
>> val & (NIOS_INIT_DONE | NIOS_INIT_START)
> Yes.
>
>>> +			goto nios_init_done;
>>> +
>>> +		/* configure FEC mode per module param */
>>> +		val = NIOS_INIT_START;
>>> +
>>> +		/*
>>> +		 * When the retimer is to be set to 10G mode, there is no FEC
>>> +		 * mode setting, so the REQ_FEC_MODE field will be ignored by
>>> +		 * Nios firmware in this case. But we should still fill the FEC
>>> +		 * mode field cause host could not get the retimer working mode
>>> +		 * until the Nios init is done.
>>> +		 *
>>> +		 * For now the driver doesn't support the retimer FEC mode
>>> +		 * switching per user's request. It is always set to Reed
>>> +		 * Solomon FEC.
>>> +		 */
>>> +		mode = REQ_FEC_MODE_RS;
>>> +
>>> +		/* set the same FEC mode for all links */
>>> +		val |= FIELD_PREP(REQ_FEC_MODE_A0_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_A1_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_A2_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_A3_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_B0_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_B1_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_B2_MSK, mode) |
>>> +		       FIELD_PREP(REQ_FEC_MODE_B3_MSK, mode);
> I'll also optimize the code here.
>
>>> +
>>> +		ret = regmap_write(ns->regmap, NIOS_INIT, val);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +nios_init_done:
>>> +	/* polls on NIOS_INIT_DONE */
>>> +	ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val,
>>> +				       val & NIOS_INIT_DONE,
>>> +				       NIOS_INIT_TIME_INTV,
>>> +				       NIOS_INIT_TIMEOUT);
>>> +	if (ret) {
>>> +		dev_err(dev, "NIOS_INIT_DONE %s\n",
>>> +			(ret == -ETIMEDOUT) ? "timed out" : "check error");
>>> +		goto dump_sts;
>>> +	}
>>> +
>>> +	/*
>>> +	 * After INIT_DONE is detected, it still needs to check if any error
>>> +	 * detected.
>>> +	 */
>>> +	if (init_error_detected(ns)) {
>>> +		/*
>>> +		 * If the retimer configuration is failed, the Nios firmware
>>> +		 * will still release the spi controller for host to
>>> +		 * communicate with the BMC. It makes possible for people to
>>> +		 * reprogram a new Nios firmware and restore the card. So the
>>> +		 * driver doesn't error out, it continues to create the spi
>>> +		 * controller device and spi_board_info for BMC.
>>> +		 */
>>> +		dev_err(dev, "NIOS_INIT_DONE OK, but err found during init\n");
>>> +		goto dump_sts;
>>> +	}
>>> +	return 0;
>>> +
>>> +dump_sts:
>>> +	dump_error_stat(ns);
>> Could init_error_detected and dump_error_stat be combined ?
> Yes, I could try to refactor it.
>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static struct spi_board_info m10_n3000_info = {
>>> +	.modalias = "m10-n3000",
>>> +	.max_speed_hz = 12500000,
>>> +	.bus_num = 0,
>>> +	.chip_select = 0,
>>> +};
>>> +
>>> +static int create_altera_spi_controller(struct n3000_nios *ns)
>>> +{
>>> +	struct altera_spi_platform_data pdata = { 0 };
>>> +	struct platform_device_info pdevinfo = { 0 };
>>> +	void __iomem *base = ns->base;
>>> +	u64 v;
>>> +
>>> +	v = readq(base + NIOS_SPI_PARAM);
>>> +
>>> +	pdata.mode_bits = SPI_CS_HIGH;
>>> +	if (FIELD_GET(PARAM_CLK_POL, v))
>>> +		pdata.mode_bits |= SPI_CPOL;
>>> +	if (FIELD_GET(PARAM_CLK_PHASE, v))
>>> +		pdata.mode_bits |= SPI_CPHA;
>>> +
>>> +	pdata.num_chipselect = FIELD_GET(PARAM_NUM_CS, v);
>>> +	pdata.bits_per_word_mask =
>>> +		SPI_BPW_RANGE_MASK(1, FIELD_GET(PARAM_DATA_WIDTH, v));
>>> +
>>> +	pdata.num_devices = 1;
>>> +	pdata.devices = &m10_n3000_info;
>>> +
>>> +	dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
>>> +		pdata.num_chipselect, pdata.bits_per_word_mask,
>>> +		pdata.mode_bits);
>>> +
>>> +	pdevinfo.name = "subdev_spi_altera";
>>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
>>> +	pdevinfo.parent = ns->dev;
>>> +	pdevinfo.data = &pdata;
>>> +	pdevinfo.size_data = sizeof(pdata);
>>> +
>>> +	ns->altera_spi = platform_device_register_full(&pdevinfo);
>>> +	return PTR_ERR_OR_ZERO(ns->altera_spi);
>>> +}
>>> +
>>> +static void destroy_altera_spi_controller(struct n3000_nios *ns)
>>> +{
>>> +	platform_device_unregister(ns->altera_spi);
>>> +}
>>> +
>>> +/* ns is the abbreviation of nios_spi */
>>> +static int ns_poll_stat_timeout(void __iomem *base, u64 *v)
>>> +{
>>> +	int loops = NS_REGBUS_WAIT_TIMEOUT;
>>> +
>>> +	/*
>>> +	 * We don't use the time based timeout here for performance.
>>> +	 *
>>> +	 * The regbus read/write is on the critical path of Intel PAC N3000
>>> +	 * image programing. The time based timeout checking will add too much
>>> +	 * overhead on it. Usually the state changes in 1 or 2 loops on the
>>> +	 * test server, and we set 10000 times loop here for safety.
>>> +	 */
>>> +	do {
>>> +		*v = readq(base + NIOS_SPI_STAT);
>>> +		if (*v & STAT_RW_VAL)
>>> +			break;
>>> +		cpu_relax();
>>> +	} while (--loops);
>> This becomes an infinite loop if loops <= 0
> The loops count is always initialized at the start of the function.
> Do you mean we should lower the impact when the developer inputs the
> wrong loop count?
>
> Is it better we change to:
>
> 	unsigned int loops = NS_REGBUS_WAIT_TIMEOUT;
>
> 	while (loops--) {
> 		
> 	}
yes
>>> +
>>> +	return loops ? 0 : -ETIMEDOUT;
>>> +}
>>> +
>>> +static int ns_reg_write(void *context, unsigned int reg, unsigned int val)
>>> +{
>>> +	struct n3000_nios *ns = context;
>>> +	u64 v = 0;
>>> +	int ret;
>>> +
>>> +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_WR);
>>> +	v |= FIELD_PREP(CTRL_ADDR, reg);
>>> +	v |= FIELD_PREP(CTRL_WR_DATA, val);
>>> +	writeq(v, ns->base + NIOS_SPI_CTRL);
>>> +
>>> +	ret = ns_poll_stat_timeout(ns->base, &v);
>>> +	if (ret)
>>> +		dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
>>> +			reg, val, ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int ns_reg_read(void *context, unsigned int reg, unsigned int *val)
>>> +{
>>> +	struct n3000_nios *ns = context;
>>> +	u64 v = 0;
>>> +	int ret;
>>> +
>>> +	v |= FIELD_PREP(CTRL_CMD_MSK, CTRL_CMD_RD);
>> v = , and save the intialiation
>>
>> or v = ( ... | ... )
> Yes.
>
>>> +	v |= FIELD_PREP(CTRL_ADDR, reg);
>>> +	writeq(v, ns->base + NIOS_SPI_CTRL);
>>> +
>>> +	ret = ns_poll_stat_timeout(ns->base, &v);
>>> +	if (ret)
>>> +		dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
>>> +	else
>>> +		*val = FIELD_GET(STAT_RD_DATA, v);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct regmap_config ns_regbus_cfg = {
>>> +	.reg_bits = 32,
>>> +	.reg_stride = 4,
>>> +	.val_bits = 32,
>>> +	.fast_io = true,
>>> +
>>> +	.reg_write = ns_reg_write,
>>> +	.reg_read = ns_reg_read,
>>> +};
>>> +
>>> +static int n3000_nios_probe(struct dfl_device *ddev)
>>> +{
>>> +	struct device *dev = &ddev->dev;
>>> +	struct n3000_nios *ns;
>>> +	int ret;
>>> +
>>> +	ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
>>> +	if (!ns)
>>> +		return -ENOMEM;
>>> +
>>> +	dev_set_drvdata(&ddev->dev, ns);
>>> +
>>> +	ns->dev = dev;
>>> +
>>> +	ns->base = devm_ioremap_resource(&ddev->dev, &ddev->mmio_res);
>>> +	if (IS_ERR(ns->base))
>>> +		return PTR_ERR(ns->base);
>>> +
>>> +	ns->regmap = devm_regmap_init(dev, NULL, ns, &ns_regbus_cfg);
>>> +	if (IS_ERR(ns->regmap))
>>> +		return PTR_ERR(ns->regmap);
>>> +
>>> +	ret = n3000_nios_init_done_check(ns);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = create_altera_spi_controller(ns);
>>> +	if (ret)
>>> +		dev_err(dev, "altera spi controller create failed: %d\n", ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void n3000_nios_remove(struct dfl_device *ddev)
>>> +{
>>> +	struct n3000_nios *ns = dev_get_drvdata(&ddev->dev);
>>> +
>>> +	destroy_altera_spi_controller(ns);
>>> +}
>>> +
>>> +#define FME_FEATURE_ID_N3000_NIOS	0xd
>>> +
>>> +static const struct dfl_device_id n3000_nios_ids[] = {
>>> +	{ FME_ID, FME_FEATURE_ID_N3000_NIOS },
>>> +	{ }
>>> +};
>>> +
>>> +static struct dfl_driver n3000_nios_driver = {
>>> +	.drv	= {
>>> +		.name       = "n3000-nios",
>> "dfl-n3000-nios" ?
> Yes. I think it is better.
>
> Thanks,
> Yilun

Tom

>> Tom
>>
>>> +		.dev_groups = n3000_nios_groups,
>>> +	},
>>> +	.id_table = n3000_nios_ids,
>>> +	.probe   = n3000_nios_probe,
>>> +	.remove  = n3000_nios_remove,
>>> +};
>>> +
>>> +module_dfl_driver(n3000_nios_driver);
>>> +
>>> +MODULE_DEVICE_TABLE(dfl, n3000_nios_ids);
>>> +MODULE_DESCRIPTION("Driver for Nios private feature on Intel PAC N3000");
>>> +MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_LICENSE("GPL v2");

[PATCH v9 6/6] memory: dfl-emif: add the DFL EMIF private feature driver

[Xu Yilun]

This driver is for the EMIF private feature implemented under FPGA
Device Feature List (DFL) framework. It is used to expose memory
interface status information as well as memory clearing control.

The purpose of memory clearing block is to zero out all private memory
when FPGA is to be reprogrammed. This gives users a reliable method to
prevent potential data leakage.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
---
v2: Adjust the position of this driver in Kconfig.
    Improves the name of the Kconfig option.
    Change the include dfl-bus.h to dfl.h, cause the previous patchset
     renames the file.
    Some minor fixes and comment improvement.
v3: Adjust the position of the driver in Makefile.
v9: Add static prefix for emif attributes macro
    Update the kernel version of the sysfs interfaces in Doc.
---
 .../ABI/testing/sysfs-bus-dfl-devices-emif         |  25 +++
 drivers/memory/Kconfig                             |   9 +
 drivers/memory/Makefile                            |   2 +
 drivers/memory/dfl-emif.c                          | 207 +++++++++++++++++++++
 4 files changed, 243 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
 create mode 100644 drivers/memory/dfl-emif.c

diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
new file mode 100644
index 0000000..1ae8ebd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
@@ -0,0 +1,25 @@
+What:		/sys/bus/dfl/devices/dfl_dev.X/infX_cal_fail
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It indicates if the calibration failed on this
+		memory interface. "1" for calibration failure, "0" for OK.
+		Format: %u
+
+What:		/sys/bus/dfl/devices/dfl_dev.X/infX_init_done
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It indicates if the initialization completed on
+		this memory interface. "1" for initialization complete, "0"
+		for not yet.
+		Format: %u
+
+What:		/sys/bus/dfl/devices/dfl_dev.X/infX_clear
+Date:		Oct 2020
+KernelVersion:	5.11
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Write-only. Writing "1" to this file will zero out all memory
+		data in this memory interface. Writing of other values is
+		invalid.
+		Format: %u
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 00e013b..2495bc4 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -137,6 +137,15 @@ config TI_EMIF_SRAM
 	  sequence so this driver provides several relocatable PM functions
 	  for the SoC PM code to use.
 
+config FPGA_DFL_EMIF
+	tristate "FPGA DFL EMIF Driver"
+	depends on FPGA_DFL && HAS_IOMEM
+	help
+	  This driver is for the EMIF private feature implemented under
+	  FPGA Device Feature List (DFL) framework. It is used to expose
+	  memory interface status information as well as memory clearing
+	  control.
+
 config MVEBU_DEVBUS
 	bool "Marvell EBU Device Bus Controller"
 	default y if PLAT_ORION
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index e71cf7b..bc7663e 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -28,6 +28,8 @@ obj-$(CONFIG_STM32_FMC2_EBI)	+= stm32-fmc2-ebi.o
 obj-$(CONFIG_SAMSUNG_MC)	+= samsung/
 obj-$(CONFIG_TEGRA_MC)		+= tegra/
 obj-$(CONFIG_TI_EMIF_SRAM)	+= ti-emif-sram.o
+obj-$(CONFIG_FPGA_DFL_EMIF)	+= dfl-emif.o
+
 ti-emif-sram-objs		:= ti-emif-pm.o ti-emif-sram-pm.o
 
 AFLAGS_ti-emif-sram-pm.o	:=-Wa,-march=armv7-a
diff --git a/drivers/memory/dfl-emif.c b/drivers/memory/dfl-emif.c
new file mode 100644
index 0000000..dcbc741e
--- /dev/null
+++ b/drivers/memory/dfl-emif.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DFL device driver for EMIF private feature
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/errno.h>
+#include <linux/fpga/dfl.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#define FME_FEATURE_ID_EMIF		0x9
+
+#define EMIF_STAT			0x8
+#define EMIF_STAT_INIT_DONE_SFT		0
+#define EMIF_STAT_CALC_FAIL_SFT		8
+#define EMIF_STAT_CLEAR_BUSY_SFT	16
+#define EMIF_CTRL			0x10
+#define EMIF_CTRL_CLEAR_EN_SFT		0
+#define EMIF_CTRL_CLEAR_EN_MSK		GENMASK_ULL(3, 0)
+
+#define EMIF_POLL_INVL			10000 /* us */
+#define EMIF_POLL_TIMEOUT		5000000 /* us */
+
+struct dfl_emif {
+	struct device *dev;
+	void __iomem *base;
+	spinlock_t lock;	/* Serialises access to EMIF_CTRL reg */
+};
+
+struct emif_attr {
+	struct device_attribute attr;
+	u32 shift;
+	u32 index;
+};
+
+#define to_emif_attr(dev_attr) \
+	container_of(dev_attr, struct emif_attr, attr)
+
+static ssize_t emif_state_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct emif_attr *eattr = to_emif_attr(attr);
+	struct dfl_emif *de = dev_get_drvdata(dev);
+	u64 val;
+
+	val = readq(de->base + EMIF_STAT);
+
+	return sprintf(buf, "%u\n",
+		       !!(val & BIT_ULL(eattr->shift + eattr->index)));
+}
+
+static ssize_t emif_clear_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct emif_attr *eattr = to_emif_attr(attr);
+	struct dfl_emif *de = dev_get_drvdata(dev);
+	u64 clear_busy_msk, clear_en_msk, val;
+	void __iomem *base = de->base;
+
+	if (!sysfs_streq(buf, "1"))
+		return -EINVAL;
+
+	clear_busy_msk = BIT_ULL(EMIF_STAT_CLEAR_BUSY_SFT + eattr->index);
+	clear_en_msk = BIT_ULL(EMIF_CTRL_CLEAR_EN_SFT + eattr->index);
+
+	spin_lock(&de->lock);
+	/* The CLEAR_EN field is WO, but other fields are RW */
+	val = readq(base + EMIF_CTRL);
+	val &= ~EMIF_CTRL_CLEAR_EN_MSK;
+	val |= clear_en_msk;
+	writeq(val, base + EMIF_CTRL);
+	spin_unlock(&de->lock);
+
+	if (readq_poll_timeout(base + EMIF_STAT, val,
+			       !(val & clear_busy_msk),
+			       EMIF_POLL_INVL, EMIF_POLL_TIMEOUT)) {
+		dev_err(de->dev, "timeout, fail to clear\n");
+		return -ETIMEDOUT;
+	}
+
+	return count;
+}
+
+#define emif_state_attr(_name, _shift, _index)				\
+	static struct emif_attr emif_attr_##inf##_index##_##_name =	\
+		{ .attr = __ATTR(inf##_index##_##_name, 0444,		\
+				 emif_state_show, NULL),		\
+		  .shift = (_shift), .index = (_index) }
+
+#define emif_clear_attr(_index)						\
+	static struct emif_attr emif_attr_##inf##_index##_clear =	\
+		{ .attr = __ATTR(inf##_index##_clear, 0200,		\
+				 NULL, emif_clear_store),		\
+		  .index = (_index) }
+
+emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 0);
+emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 1);
+emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 2);
+emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 3);
+
+emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 0);
+emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 1);
+emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 2);
+emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 3);
+
+emif_clear_attr(0);
+emif_clear_attr(1);
+emif_clear_attr(2);
+emif_clear_attr(3);
+
+static struct attribute *dfl_emif_attrs[] = {
+	&emif_attr_inf0_init_done.attr.attr,
+	&emif_attr_inf0_cal_fail.attr.attr,
+	&emif_attr_inf0_clear.attr.attr,
+
+	&emif_attr_inf1_init_done.attr.attr,
+	&emif_attr_inf1_cal_fail.attr.attr,
+	&emif_attr_inf1_clear.attr.attr,
+
+	&emif_attr_inf2_init_done.attr.attr,
+	&emif_attr_inf2_cal_fail.attr.attr,
+	&emif_attr_inf2_clear.attr.attr,
+
+	&emif_attr_inf3_init_done.attr.attr,
+	&emif_attr_inf3_cal_fail.attr.attr,
+	&emif_attr_inf3_clear.attr.attr,
+
+	NULL,
+};
+
+static umode_t dfl_emif_visible(struct kobject *kobj,
+				struct attribute *attr, int n)
+{
+	struct dfl_emif *de = dev_get_drvdata(kobj_to_dev(kobj));
+	struct emif_attr *eattr = container_of(attr, struct emif_attr,
+					       attr.attr);
+	u64 val;
+
+	/*
+	 * This device supports upto 4 memory interfaces, but not all
+	 * interfaces are used on different platforms. The read out value of
+	 * CLEAN_EN field (which is a bitmap) could tell how many interfaces
+	 * are available.
+	 */
+	val = FIELD_GET(EMIF_CTRL_CLEAR_EN_MSK, readq(de->base + EMIF_CTRL));
+
+	return (val & BIT_ULL(eattr->index)) ? attr->mode : 0;
+}
+
+static const struct attribute_group dfl_emif_group = {
+	.is_visible = dfl_emif_visible,
+	.attrs = dfl_emif_attrs,
+};
+
+static const struct attribute_group *dfl_emif_groups[] = {
+	&dfl_emif_group,
+	NULL,
+};
+
+static int dfl_emif_probe(struct dfl_device *ddev)
+{
+	struct device *dev = &ddev->dev;
+	struct dfl_emif *de;
+
+	de = devm_kzalloc(dev, sizeof(*de), GFP_KERNEL);
+	if (!de)
+		return -ENOMEM;
+
+	de->base = devm_ioremap_resource(dev, &ddev->mmio_res);
+	if (IS_ERR(de->base))
+		return PTR_ERR(de->base);
+
+	de->dev = dev;
+	spin_lock_init(&de->lock);
+	dev_set_drvdata(dev, de);
+
+	return 0;
+}
+
+static const struct dfl_device_id dfl_emif_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_EMIF },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_emif_ids);
+
+static struct dfl_driver dfl_emif_driver = {
+	.drv	= {
+		.name       = "dfl-emif",
+		.dev_groups = dfl_emif_groups,
+	},
+	.id_table = dfl_emif_ids,
+	.probe   = dfl_emif_probe,
+};
+module_dfl_driver(dfl_emif_driver);
+
+MODULE_DESCRIPTION("DFL EMIF driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v9 6/6] memory: dfl-emif: add the DFL EMIF private feature driver

[Krzysztof Kozlowski]

On Sat, 10 Oct 2020 at 09:15, Xu Yilun <yilun.xu@intel.com> wrote:
>
> This driver is for the EMIF private feature implemented under FPGA
> Device Feature List (DFL) framework. It is used to expose memory
> interface status information as well as memory clearing control.
>
> The purpose of memory clearing block is to zero out all private memory
> when FPGA is to be reprogrammed. This gives users a reliable method to
> prevent potential data leakage.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> v2: Adjust the position of this driver in Kconfig.
>     Improves the name of the Kconfig option.
>     Change the include dfl-bus.h to dfl.h, cause the previous patchset
>      renames the file.
>     Some minor fixes and comment improvement.
> v3: Adjust the position of the driver in Makefile.
> v9: Add static prefix for emif attributes macro
>     Update the kernel version of the sysfs interfaces in Doc.
> ---
>  .../ABI/testing/sysfs-bus-dfl-devices-emif         |  25 +++
>  drivers/memory/Kconfig                             |   9 +
>  drivers/memory/Makefile                            |   2 +
>  drivers/memory/dfl-emif.c                          | 207 +++++++++++++++++++++
>  4 files changed, 243 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
>  create mode 100644 drivers/memory/dfl-emif.c
>

I am confused now. This was already taken by Moritz, wasn't it? And
the dependencies were already taken, weren't they? Previously it was
depending on "Modularization of DFL private feature drivers" and "add
dfl bus support to MODULE_DEVICE_TABLE()"... now this is here so did
the dependencies change? What is the reason to include this patch
here?

My ack was for the purpose of taking it via Moritz tree, because of
the dependencies. If this is not the case, then probably better to
take it via memory controllers tree to avoid any conflicts (it's not a
small change).

Best regards,
Krzysztof

Re: [PATCH v9 6/6] memory: dfl-emif: add the DFL EMIF private feature driver

[Moritz Fischer]

Hi Krzysztof,

On Mon, Oct 12, 2020 at 06:40:46PM +0200, Krzysztof Kozlowski wrote:
> On Sat, 10 Oct 2020 at 09:15, Xu Yilun <yilun.xu@intel.com> wrote:
> >
> > This driver is for the EMIF private feature implemented under FPGA
> > Device Feature List (DFL) framework. It is used to expose memory
> > interface status information as well as memory clearing control.
> >
> > The purpose of memory clearing block is to zero out all private memory
> > when FPGA is to be reprogrammed. This gives users a reliable method to
> > prevent potential data leakage.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> > v2: Adjust the position of this driver in Kconfig.
> >     Improves the name of the Kconfig option.
> >     Change the include dfl-bus.h to dfl.h, cause the previous patchset
> >      renames the file.
> >     Some minor fixes and comment improvement.
> > v3: Adjust the position of the driver in Makefile.
> > v9: Add static prefix for emif attributes macro
> >     Update the kernel version of the sysfs interfaces in Doc.
> > ---
> >  .../ABI/testing/sysfs-bus-dfl-devices-emif         |  25 +++
> >  drivers/memory/Kconfig                             |   9 +
> >  drivers/memory/Makefile                            |   2 +
> >  drivers/memory/dfl-emif.c                          | 207 +++++++++++++++++++++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> >  create mode 100644 drivers/memory/dfl-emif.c
> >
> 
> I am confused now. This was already taken by Moritz, wasn't it? And
> the dependencies were already taken, weren't they? Previously it was
> depending on "Modularization of DFL private feature drivers" and "add
> dfl bus support to MODULE_DEVICE_TABLE()"... now this is here so did
> the dependencies change? What is the reason to include this patch
> here?

It is confusing. Basically Greg had comments on the patch after I had
applied it. It's going through anothe round of review.

> 
> My ack was for the purpose of taking it via Moritz tree, because of
> the dependencies. If this is not the case, then probably better to
> take it via memory controllers tree to avoid any conflicts (it's not a
> small change).

Once it's ok I can put it on a branch with a stable tag and you can pull
that in and take the patch through your tree.

Does that work?

Sorry for the mess,

Moritz

Re: [PATCH v9 6/6] memory: dfl-emif: add the DFL EMIF private feature driver

[Krzysztof Kozlowski]

On Mon, Oct 12, 2020 at 09:58:03AM -0700, Moritz Fischer wrote:
> Hi Krzysztof,
> 
> On Mon, Oct 12, 2020 at 06:40:46PM +0200, Krzysztof Kozlowski wrote:
> > On Sat, 10 Oct 2020 at 09:15, Xu Yilun <yilun.xu@intel.com> wrote:
> > >
> > > This driver is for the EMIF private feature implemented under FPGA
> > > Device Feature List (DFL) framework. It is used to expose memory
> > > interface status information as well as memory clearing control.
> > >
> > > The purpose of memory clearing block is to zero out all private memory
> > > when FPGA is to be reprogrammed. This gives users a reliable method to
> > > prevent potential data leakage.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > ---
> > > v2: Adjust the position of this driver in Kconfig.
> > >     Improves the name of the Kconfig option.
> > >     Change the include dfl-bus.h to dfl.h, cause the previous patchset
> > >      renames the file.
> > >     Some minor fixes and comment improvement.
> > > v3: Adjust the position of the driver in Makefile.
> > > v9: Add static prefix for emif attributes macro
> > >     Update the kernel version of the sysfs interfaces in Doc.
> > > ---
> > >  .../ABI/testing/sysfs-bus-dfl-devices-emif         |  25 +++
> > >  drivers/memory/Kconfig                             |   9 +
> > >  drivers/memory/Makefile                            |   2 +
> > >  drivers/memory/dfl-emif.c                          | 207 +++++++++++++++++++++
> > >  4 files changed, 243 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-emif
> > >  create mode 100644 drivers/memory/dfl-emif.c
> > >
> > 
> > I am confused now. This was already taken by Moritz, wasn't it? And
> > the dependencies were already taken, weren't they? Previously it was
> > depending on "Modularization of DFL private feature drivers" and "add
> > dfl bus support to MODULE_DEVICE_TABLE()"... now this is here so did
> > the dependencies change? What is the reason to include this patch
> > here?
> 
> It is confusing. Basically Greg had comments on the patch after I had
> applied it. It's going through anothe round of review.
> 
> > 
> > My ack was for the purpose of taking it via Moritz tree, because of
> > the dependencies. If this is not the case, then probably better to
> > take it via memory controllers tree to avoid any conflicts (it's not a
> > small change).
> 
> Once it's ok I can put it on a branch with a stable tag and you can pull
> that in and take the patch through your tree.

I am not going to apply any new patches before end of merge window, so
if you can still push it to Greg for v5.10, then please take it. However
I think Greg also does not take patches during merge window, so most
likely this will wait for the next cycle. In such case let's do it via
stable tag for DFL parts.

Best regards,
Krzysztof