* [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16
2020-07-24 2:09 [PATCH v2 0/4] Modularization of DFL private feature drivers Xu Yilun
@ 2020-07-24 2:09 ` Xu Yilun
2020-07-25 13:29 ` Tom Rix
2020-07-24 2:09 ` [PATCH v2 2/4] fpga: dfl: map feature mmio resources in their own feature drivers Xu Yilun
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-24 2:09 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel; +Cc: trix, lgoncalv, yilun.xu
The feature id is stored in a 12 bit field in DFH. So a u16 variable is
enough for feature id.
This patch changes all feature id related places to fit u16.
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
drivers/fpga/dfl-fme-perf.c | 2 +-
drivers/fpga/dfl.c | 29 +++++++++++++++--------------
drivers/fpga/dfl.h | 10 +++++-----
3 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/fpga/dfl-fme-perf.c b/drivers/fpga/dfl-fme-perf.c
index 6ce1ed2..5312662 100644
--- a/drivers/fpga/dfl-fme-perf.c
+++ b/drivers/fpga/dfl-fme-perf.c
@@ -148,7 +148,7 @@ struct fme_perf_priv {
struct device *dev;
void __iomem *ioaddr;
struct pmu pmu;
- u64 id;
+ u16 id;
u32 fab_users;
u32 fab_port_id;
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b51db80..9bca22e 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -58,7 +58,7 @@ static const char *dfl_pdata_key_strings[DFL_ID_MAX] = {
*/
struct dfl_dev_info {
const char *name;
- u32 dfh_id;
+ u16 dfh_id;
struct idr id;
enum dfl_fpga_devt_type devt_type;
};
@@ -134,7 +134,7 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
return DFL_ID_MAX;
}
-static enum dfl_id_type dfh_id_to_type(u32 id)
+static enum dfl_id_type dfh_id_to_type(u16 id)
{
int i;
@@ -454,7 +454,7 @@ struct build_feature_devs_info {
* @nr_irqs: number of irqs of this sub feature.
*/
struct dfl_feature_info {
- u64 fid;
+ u16 fid;
struct resource mmio_res;
void __iomem *ioaddr;
struct list_head node;
@@ -650,7 +650,7 @@ static inline u32 feature_size(void __iomem *start)
return ofst ? ofst : 4096;
}
-static u64 feature_id(void __iomem *start)
+static u16 feature_id(void __iomem *start)
{
u64 v = readq(start + DFH);
u16 id = FIELD_GET(DFH_ID, v);
@@ -668,7 +668,7 @@ static u64 feature_id(void __iomem *start)
}
static int parse_feature_irqs(struct build_feature_devs_info *binfo,
- resource_size_t ofst, u64 fid,
+ resource_size_t ofst, u16 fid,
unsigned int *irq_base, unsigned int *nr_irqs)
{
void __iomem *base = binfo->ioaddr + ofst;
@@ -714,12 +714,12 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
return 0;
}
- dev_dbg(binfo->dev, "feature: 0x%llx, irq_base: %u, nr_irqs: %u\n",
+ dev_dbg(binfo->dev, "feature: 0x%x, irq_base: %u, nr_irqs: %u\n",
fid, ibase, inr);
if (ibase + inr > binfo->nr_irqs) {
dev_err(binfo->dev,
- "Invalid interrupt number in feature 0x%llx\n", fid);
+ "Invalid interrupt number in feature 0x%x\n", fid);
return -EINVAL;
}
@@ -727,7 +727,7 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
virq = binfo->irq_table[ibase + i];
if (virq < 0 || virq > NR_IRQS) {
dev_err(binfo->dev,
- "Invalid irq table entry for feature 0x%llx\n",
+ "Invalid irq table entry for feature 0x%x\n",
fid);
return -EINVAL;
}
@@ -749,7 +749,7 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
static int
create_feature_instance(struct build_feature_devs_info *binfo,
struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
- resource_size_t size, u64 fid)
+ resource_size_t size, u16 fid)
{
unsigned int irq_base, nr_irqs;
struct dfl_feature_info *finfo;
@@ -820,9 +820,10 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
struct dfl_fpga_enum_dfl *dfl,
resource_size_t ofst)
{
- u32 id, offset;
- u64 v;
int ret = 0;
+ u32 offset;
+ u16 id;
+ u64 v;
v = readq(dfl->ioaddr + ofst + DFH);
id = FIELD_GET(DFH_ID, v);
@@ -856,8 +857,8 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
resource_size_t ofst)
{
if (!binfo->feature_dev) {
- dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
- (unsigned long long)feature_id(dfl->ioaddr + ofst));
+ dev_err(binfo->dev, "the private feature 0x%x does not belong to any AFU.\n",
+ feature_id(dfl->ioaddr + ofst));
return -EINVAL;
}
@@ -1425,7 +1426,7 @@ static int do_set_irq_trigger(struct dfl_feature *feature, unsigned int idx,
return 0;
feature->irq_ctx[idx].name =
- kasprintf(GFP_KERNEL, "fpga-irq[%u](%s-%llx)", idx,
+ kasprintf(GFP_KERNEL, "fpga-irq[%u](%s-%x)", idx,
dev_name(&pdev->dev), feature->id);
if (!feature->irq_ctx[idx].name)
return -ENOMEM;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 632e733..b848e3c 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -197,7 +197,7 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id);
* @id: unique dfl private feature id.
*/
struct dfl_feature_id {
- u64 id;
+ u16 id;
};
/**
@@ -240,7 +240,7 @@ struct dfl_feature_irq_ctx {
*/
struct dfl_feature {
struct platform_device *dev;
- u64 id;
+ u16 id;
int resource_index;
void __iomem *ioaddr;
struct dfl_feature_irq_ctx *irq_ctx;
@@ -371,7 +371,7 @@ struct platform_device *dfl_fpga_inode_to_feature_dev(struct inode *inode)
(feature) < (pdata)->features + (pdata)->num; (feature)++)
static inline
-struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
+struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u16 id)
{
struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
struct dfl_feature *feature;
@@ -384,7 +384,7 @@ struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
}
static inline
-void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
+void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u16 id)
{
struct dfl_feature *feature = dfl_get_feature_by_id(dev, id);
@@ -395,7 +395,7 @@ void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
return NULL;
}
-static inline bool is_dfl_feature_present(struct device *dev, u64 id)
+static inline bool is_dfl_feature_present(struct device *dev, u16 id)
{
return !!dfl_get_feature_ioaddr_by_id(dev, id);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16
2020-07-24 2:09 ` [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16 Xu Yilun
@ 2020-07-25 13:29 ` Tom Rix
2020-07-31 7:48 ` Xu Yilun
0 siblings, 1 reply; 15+ messages in thread
From: Tom Rix @ 2020-07-25 13:29 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel; +Cc: lgoncalv
It would be good if the variable or element for the feature id had a consistent name.
> @@ -197,7 +197,7 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id);
> * @id: unique dfl private feature id.
> */
> struct dfl_feature_id {
> - u64 id;
> + u16 id;
> };
Is this structure needed ?
Here is how it could be changed to
struct dfl_feature_driver {
- const dfl_feature_id *
+ const u16 *id_table;
...
Tom
>
> /**
> @@ -240,7 +240,7 @@ struct dfl_feature_irq_ctx {
> */
> struct dfl_feature {
> struct platform_device *dev;
> - u64 id;
> + u16 id;
> int resource_index;
> void __iomem *ioaddr;
> struct dfl_feature_irq_ctx *irq_ctx;
> @@ -371,7 +371,7 @@ struct platform_device *dfl_fpga_inode_to_feature_dev(struct inode *inode)
> (feature) < (pdata)->features + (pdata)->num; (feature)++)
>
> static inline
> -struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
> +struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u16 id)
> {
> struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> struct dfl_feature *feature;
> @@ -384,7 +384,7 @@ struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
> }
>
> static inline
> -void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> +void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u16 id)
> {
> struct dfl_feature *feature = dfl_get_feature_by_id(dev, id);
>
> @@ -395,7 +395,7 @@ void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> return NULL;
> }
>
> -static inline bool is_dfl_feature_present(struct device *dev, u64 id)
> +static inline bool is_dfl_feature_present(struct device *dev, u16 id)
> {
> return !!dfl_get_feature_ioaddr_by_id(dev, id);
> }
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16
2020-07-25 13:29 ` Tom Rix
@ 2020-07-31 7:48 ` Xu Yilun
2020-08-01 15:40 ` Tom Rix
0 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-31 7:48 UTC (permalink / raw)
To: Tom Rix; +Cc: mdf, linux-fpga, linux-kernel, lgoncalv
On Sat, Jul 25, 2020 at 06:29:53AM -0700, Tom Rix wrote:
> It would be good if the variable or element for the feature id had a consistent name.
>
>
> > @@ -197,7 +197,7 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id);
> > * @id: unique dfl private feature id.
> > */
> > struct dfl_feature_id {
> > - u64 id;
> > + u16 id;
> > };
>
> Is this structure needed ?
>
> Here is how it could be changed to
>
> struct dfl_feature_driver {
>
> - const dfl_feature_id *
>
> + const u16 *id_table;
This structure is to represent an id type, which is used to match
fme/port owned features. It could be extended if some feature drivers
needs driver_data.
Actually I see some example of device_ids with similar structure, like:
struct mips_cdmm_device_id {
__u8 type;
};
struct tee_client_device_id {
uuid_t uuid;
};
Thanks,
Yilun.
>
> ...
>
> Tom
>
>
> >
> > /**
> > @@ -240,7 +240,7 @@ struct dfl_feature_irq_ctx {
> > */
> > struct dfl_feature {
> > struct platform_device *dev;
> > - u64 id;
> > + u16 id;
> > int resource_index;
> > void __iomem *ioaddr;
> > struct dfl_feature_irq_ctx *irq_ctx;
> > @@ -371,7 +371,7 @@ struct platform_device *dfl_fpga_inode_to_feature_dev(struct inode *inode)
> > (feature) < (pdata)->features + (pdata)->num; (feature)++)
> >
> > static inline
> > -struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
> > +struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u16 id)
> > {
> > struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > struct dfl_feature *feature;
> > @@ -384,7 +384,7 @@ struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
> > }
> >
> > static inline
> > -void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> > +void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u16 id)
> > {
> > struct dfl_feature *feature = dfl_get_feature_by_id(dev, id);
> >
> > @@ -395,7 +395,7 @@ void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
> > return NULL;
> > }
> >
> > -static inline bool is_dfl_feature_present(struct device *dev, u64 id)
> > +static inline bool is_dfl_feature_present(struct device *dev, u16 id)
> > {
> > return !!dfl_get_feature_ioaddr_by_id(dev, id);
> > }
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16
2020-07-31 7:48 ` Xu Yilun
@ 2020-08-01 15:40 ` Tom Rix
0 siblings, 0 replies; 15+ messages in thread
From: Tom Rix @ 2020-08-01 15:40 UTC (permalink / raw)
To: Xu Yilun; +Cc: mdf, linux-fpga, linux-kernel, lgoncalv
On 7/31/20 12:48 AM, Xu Yilun wrote:
> On Sat, Jul 25, 2020 at 06:29:53AM -0700, Tom Rix wrote:
>> It would be good if the variable or element for the feature id had a consistent name.
>>
>>
>>> @@ -197,7 +197,7 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id);
>>> * @id: unique dfl private feature id.
>>> */
>>> struct dfl_feature_id {
>>> - u64 id;
>>> + u16 id;
>>> };
>> Is this structure needed ?
>>
>> Here is how it could be changed to
>>
>> struct dfl_feature_driver {
>>
>> - const dfl_feature_id *
>>
>> + const u16 *id_table;
> This structure is to represent an id type, which is used to match
> fme/port owned features. It could be extended if some feature drivers
> needs driver_data.
>
> Actually I see some example of device_ids with similar structure, like:
>
> struct mips_cdmm_device_id {
> __u8 type;
> };
>
> struct tee_client_device_id {
> uuid_t uuid;
> };
>
Ok.
Reviewed-by: Tom Rix <trix@redhat.com>
> Thanks,
> Yilun.
>
>> ...
>>
>> Tom
>>
>>
>>>
>>> /**
>>> @@ -240,7 +240,7 @@ struct dfl_feature_irq_ctx {
>>> */
>>> struct dfl_feature {
>>> struct platform_device *dev;
>>> - u64 id;
>>> + u16 id;
>>> int resource_index;
>>> void __iomem *ioaddr;
>>> struct dfl_feature_irq_ctx *irq_ctx;
>>> @@ -371,7 +371,7 @@ struct platform_device *dfl_fpga_inode_to_feature_dev(struct inode *inode)
>>> (feature) < (pdata)->features + (pdata)->num; (feature)++)
>>>
>>> static inline
>>> -struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
>>> +struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u16 id)
>>> {
>>> struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
>>> struct dfl_feature *feature;
>>> @@ -384,7 +384,7 @@ struct dfl_feature *dfl_get_feature_by_id(struct device *dev, u64 id)
>>> }
>>>
>>> static inline
>>> -void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
>>> +void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u16 id)
>>> {
>>> struct dfl_feature *feature = dfl_get_feature_by_id(dev, id);
>>>
>>> @@ -395,7 +395,7 @@ void __iomem *dfl_get_feature_ioaddr_by_id(struct device *dev, u64 id)
>>> return NULL;
>>> }
>>>
>>> -static inline bool is_dfl_feature_present(struct device *dev, u64 id)
>>> +static inline bool is_dfl_feature_present(struct device *dev, u16 id)
>>> {
>>> return !!dfl_get_feature_ioaddr_by_id(dev, id);
>>> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-24 2:09 [PATCH v2 0/4] Modularization of DFL private feature drivers Xu Yilun
2020-07-24 2:09 ` [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16 Xu Yilun
@ 2020-07-24 2:09 ` Xu Yilun
2020-08-01 15:43 ` Tom Rix
2020-07-24 2:09 ` [PATCH v2 3/4] fpga: dfl: create a dfl bus type to support DFL devices Xu Yilun
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-24 2:09 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, Wu Hao, Matthew Gerlach, Russ Weight
This patch makes preparation for modularization of DFL sub feature
drivers.
Currently, if we need to support a new DFL sub feature, an entry should
be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
to re-compile the whole DFL modules. That make the DFL drivers hard to be
extended.
Another consideration is that DFL may contain some IP blocks which are
already supported by kernel, most of them are supported by platform
device drivers. We could create platform devices for these IP blocks and
get them supported by these drivers.
An important issue is that platform device drivers usually requests mmio
resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
dfl-pci) as a whole region. Then platform device drivers for sub features
can't request their own mmio resources again. This is what the patch
trying to resolve.
This patch changes the DFL enumeration. DFL bus driver will unmap mmio
resources after first step enumeration and pass enumeration info to DFL
framework. Then DFL framework will map the mmio resources again, do 2nd
step enumeration, and also unmap the mmio resources. In this way, sub
feature drivers could then request their own mmio resources as needed.
An exception is that mmio resource of FIU headers are still mapped in dfl
bus driver. The FIU headers have some fundamental functions (sriov set,
port enable/disable) needed for dfl bus devices and other sub features.
They should not be unmapped as long as dfl bus device is alive.
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>
Signed-off-by: Tom Rix <trix@redhat.com>
----
v2: delete dfl_binfo_shift() cause no shift is possible for FIU parsing.
Only map bar0 for first phase enumeration in dfl-pci, we can find
all dfl mmio resource info in bar0.
Some minor fixes for comments from Hao & Tom.
---
drivers/fpga/dfl-pci.c | 24 +++----
drivers/fpga/dfl.c | 185 ++++++++++++++++++++++++++++++++++---------------
drivers/fpga/dfl.h | 7 +-
3 files changed, 140 insertions(+), 76 deletions(-)
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index e220bec..a2203d0 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -31,12 +31,12 @@ struct cci_drvdata {
struct dfl_fpga_cdev *cdev; /* container device */
};
-static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
+static void __iomem *cci_pci_ioremap_bar0(struct pci_dev *pcidev)
{
- if (pcim_iomap_regions(pcidev, BIT(bar), DRV_NAME))
+ if (pcim_iomap_regions(pcidev, BIT(0), DRV_NAME))
return NULL;
- return pcim_iomap_table(pcidev)[bar];
+ return pcim_iomap_table(pcidev)[0];
}
static int cci_pci_alloc_irq(struct pci_dev *pcidev)
@@ -156,8 +156,8 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
goto irq_free_exit;
}
- /* start to find Device Feature List from Bar 0 */
- base = cci_pci_ioremap_bar(pcidev, 0);
+ /* start to find Device Feature List in Bar 0 */
+ base = cci_pci_ioremap_bar0(pcidev);
if (!base) {
ret = -ENOMEM;
goto irq_free_exit;
@@ -172,7 +172,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
start = pci_resource_start(pcidev, 0);
len = pci_resource_len(pcidev, 0);
- dfl_fpga_enum_info_add_dfl(info, start, len, base);
+ dfl_fpga_enum_info_add_dfl(info, start, len);
/*
* find more Device Feature Lists (e.g. Ports) per information
@@ -196,26 +196,24 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
*/
bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
- base = cci_pci_ioremap_bar(pcidev, bar);
- if (!base)
- continue;
-
start = pci_resource_start(pcidev, bar) + offset;
len = pci_resource_len(pcidev, bar) - offset;
- dfl_fpga_enum_info_add_dfl(info, start, len,
- base + offset);
+ dfl_fpga_enum_info_add_dfl(info, start, len);
}
} else if (dfl_feature_is_port(base)) {
start = pci_resource_start(pcidev, 0);
len = pci_resource_len(pcidev, 0);
- dfl_fpga_enum_info_add_dfl(info, start, len, base);
+ dfl_fpga_enum_info_add_dfl(info, start, len);
} else {
ret = -ENODEV;
goto irq_free_exit;
}
+ /* release I/O mappings for next step enumeration */
+ pcim_iounmap_regions(pcidev, BIT(0));
+
/* start enumeration with prepared enumeration information */
cdev = dfl_fpga_feature_devs_enumerate(info);
if (IS_ERR(cdev)) {
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 9bca22e..b675957 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -250,6 +250,8 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
}
EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
+#define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
+
/**
* dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
* @pdev: feature device.
@@ -273,8 +275,22 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
struct dfl_feature *feature,
struct dfl_feature_driver *drv)
{
+ void __iomem *base;
int ret = 0;
+ if (!is_header_feature(feature)) {
+ base = devm_platform_ioremap_resource(pdev,
+ feature->resource_index);
+ if (IS_ERR(base)) {
+ dev_err(&pdev->dev,
+ "ioremap failed for feature 0x%x!\n",
+ feature->id);
+ return PTR_ERR(base);
+ }
+
+ feature->ioaddr = base;
+ }
+
if (drv->ops->init) {
ret = drv->ops->init(pdev, feature);
if (ret)
@@ -427,7 +443,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
* @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
* this device.
* @feature_dev: current feature device.
- * @ioaddr: header register region address of feature device in enumeration.
+ * @ioaddr: header register region address of current FIU in enumeration.
+ * @start: register resource start of current FIU.
+ * @len: max register resource length of current FIU.
* @sub_features: a sub features linked list for feature device in enumeration.
* @feature_num: number of sub features for feature device in enumeration.
*/
@@ -439,6 +457,8 @@ struct build_feature_devs_info {
struct platform_device *feature_dev;
void __iomem *ioaddr;
+ resource_size_t start;
+ resource_size_t len;
struct list_head sub_features;
int feature_num;
};
@@ -484,10 +504,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
struct dfl_feature_platform_data *pdata;
struct dfl_feature_info *finfo, *p;
enum dfl_id_type type;
- int ret, index = 0;
-
- if (!fdev)
- return 0;
+ int ret, index = 0, res_idx = 0;
type = feature_dev_id_type(fdev);
if (WARN_ON_ONCE(type >= DFL_ID_MAX))
@@ -531,16 +548,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
/* fill features and resource information for feature dev */
list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
- struct dfl_feature *feature = &pdata->features[index];
+ struct dfl_feature *feature = &pdata->features[index++];
struct dfl_feature_irq_ctx *ctx;
unsigned int i;
/* save resource information for each feature */
feature->dev = fdev;
feature->id = finfo->fid;
- feature->resource_index = index;
- feature->ioaddr = finfo->ioaddr;
- fdev->resource[index++] = finfo->mmio_res;
+
+ /*
+ * map header resource for dfl bus device. Don't add header
+ * resource to feature devices, or the resource tree will be
+ * disordered and cause warning on resource release
+ */
+ if (is_header_feature(feature)) {
+ feature->resource_index = -1;
+ feature->ioaddr =
+ devm_ioremap_resource(binfo->dev,
+ &finfo->mmio_res);
+ if (IS_ERR(feature->ioaddr))
+ return PTR_ERR(feature->ioaddr);
+ } else {
+ feature->resource_index = res_idx;
+ fdev->resource[res_idx++] = finfo->mmio_res;
+ }
if (finfo->nr_irqs) {
ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
@@ -583,19 +614,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
static int
build_info_create_dev(struct build_feature_devs_info *binfo,
- enum dfl_id_type type, void __iomem *ioaddr)
+ enum dfl_id_type type)
{
struct platform_device *fdev;
- int ret;
if (type >= DFL_ID_MAX)
return -EINVAL;
- /* we will create a new device, commit current device first */
- ret = build_info_commit_dev(binfo);
- if (ret)
- return ret;
-
/*
* we use -ENODEV as the initialization indicator which indicates
* whether the id need to be reclaimed
@@ -606,7 +631,7 @@ build_info_create_dev(struct build_feature_devs_info *binfo,
binfo->feature_dev = fdev;
binfo->feature_num = 0;
- binfo->ioaddr = ioaddr;
+
INIT_LIST_HEAD(&binfo->sub_features);
fdev->id = dfl_id_alloc(type, &fdev->dev);
@@ -748,18 +773,17 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
*/
static int
create_feature_instance(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
- resource_size_t size, u16 fid)
+ resource_size_t ofst, resource_size_t size, u16 fid)
{
unsigned int irq_base, nr_irqs;
struct dfl_feature_info *finfo;
int ret;
/* read feature size and id if inputs are invalid */
- size = size ? size : feature_size(dfl->ioaddr + ofst);
- fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
+ size = size ? size : feature_size(binfo->ioaddr + ofst);
+ fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
- if (dfl->len - ofst < size)
+ if (binfo->len - ofst < size)
return -EINVAL;
ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
@@ -771,12 +795,11 @@ create_feature_instance(struct build_feature_devs_info *binfo,
return -ENOMEM;
finfo->fid = fid;
- finfo->mmio_res.start = dfl->start + ofst;
+ finfo->mmio_res.start = binfo->start + ofst;
finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
finfo->mmio_res.flags = IORESOURCE_MEM;
finfo->irq_base = irq_base;
finfo->nr_irqs = nr_irqs;
- finfo->ioaddr = dfl->ioaddr + ofst;
list_add_tail(&finfo->node, &binfo->sub_features);
binfo->feature_num++;
@@ -785,7 +808,6 @@ create_feature_instance(struct build_feature_devs_info *binfo,
}
static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl,
resource_size_t ofst)
{
u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
@@ -793,21 +815,22 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
WARN_ON(!size);
- return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
+ return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
}
+#define is_feature_dev_detected(binfo) (!!(binfo)->feature_dev)
+
static int parse_feature_afu(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl,
resource_size_t ofst)
{
- if (!binfo->feature_dev) {
+ if (!is_feature_dev_detected(binfo)) {
dev_err(binfo->dev, "this AFU does not belong to any FIU.\n");
return -EINVAL;
}
switch (feature_dev_id_type(binfo->feature_dev)) {
case PORT_ID:
- return parse_feature_port_afu(binfo, dfl, ofst);
+ return parse_feature_port_afu(binfo, ofst);
default:
dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
binfo->feature_dev->name);
@@ -816,8 +839,40 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
return 0;
}
+static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
+ resource_size_t start, resource_size_t len)
+{
+ struct device *dev = binfo->dev;
+ void __iomem *ioaddr;
+
+ if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
+ dev_err(dev, "request region fail, start:%pa, len:%pa\n",
+ &start, &len);
+ return -EBUSY;
+ }
+
+ ioaddr = devm_ioremap(dev, start, len);
+ if (!ioaddr) {
+ dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
+ &start, &len);
+ devm_release_mem_region(dev, start, len);
+ return -ENOMEM;
+ }
+
+ binfo->start = start;
+ binfo->len = len;
+ binfo->ioaddr = ioaddr;
+
+ return 0;
+}
+
+static void dfl_binfo_complete(struct build_feature_devs_info *binfo)
+{
+ devm_iounmap(binfo->dev, binfo->ioaddr);
+ devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
+}
+
static int parse_feature_fiu(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl,
resource_size_t ofst)
{
int ret = 0;
@@ -825,27 +880,39 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
u16 id;
u64 v;
- v = readq(dfl->ioaddr + ofst + DFH);
+ if (is_feature_dev_detected(binfo)) {
+ dfl_binfo_complete(binfo);
+
+ ret = build_info_commit_dev(binfo);
+ if (ret)
+ return ret;
+
+ ret = dfl_binfo_prepare(binfo, binfo->start + ofst,
+ binfo->len - ofst);
+ if (ret)
+ return ret;
+ }
+
+ v = readq(binfo->ioaddr + DFH);
id = FIELD_GET(DFH_ID, v);
/* create platform device for dfl feature dev */
- ret = build_info_create_dev(binfo, dfh_id_to_type(id),
- dfl->ioaddr + ofst);
+ ret = build_info_create_dev(binfo, dfh_id_to_type(id));
if (ret)
return ret;
- ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
+ ret = create_feature_instance(binfo, 0, 0, 0);
if (ret)
return ret;
/*
* find and parse FIU's child AFU via its NEXT_AFU register.
* please note that only Port has valid NEXT_AFU pointer per spec.
*/
- v = readq(dfl->ioaddr + ofst + NEXT_AFU);
+ v = readq(binfo->ioaddr + NEXT_AFU);
offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
if (offset)
- return parse_feature_afu(binfo, dfl, ofst + offset);
+ return parse_feature_afu(binfo, offset);
dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
@@ -853,16 +920,15 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
}
static int parse_feature_private(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl,
resource_size_t ofst)
{
- if (!binfo->feature_dev) {
+ if (!is_feature_dev_detected(binfo)) {
dev_err(binfo->dev, "the private feature 0x%x does not belong to any AFU.\n",
- feature_id(dfl->ioaddr + ofst));
+ feature_id(binfo->ioaddr + ofst));
return -EINVAL;
}
- return create_feature_instance(binfo, dfl, ofst, 0, 0);
+ return create_feature_instance(binfo, ofst, 0, 0);
}
/**
@@ -870,24 +936,24 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
*
* @binfo: build feature devices information.
* @dfl: device feature list to parse
- * @ofst: offset to feature header on this device feature list
+ * @ofst: offset to current FIU header
*/
static int parse_feature(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
+ resource_size_t ofst)
{
u64 v;
u32 type;
- v = readq(dfl->ioaddr + ofst + DFH);
+ v = readq(binfo->ioaddr + ofst + DFH);
type = FIELD_GET(DFH_TYPE, v);
switch (type) {
case DFH_TYPE_AFU:
- return parse_feature_afu(binfo, dfl, ofst);
+ return parse_feature_afu(binfo, ofst);
case DFH_TYPE_PRIVATE:
- return parse_feature_private(binfo, dfl, ofst);
+ return parse_feature_private(binfo, ofst);
case DFH_TYPE_FIU:
- return parse_feature_fiu(binfo, dfl, ofst);
+ return parse_feature_fiu(binfo, ofst);
default:
dev_info(binfo->dev,
"Feature Type %x is not supported.\n", type);
@@ -897,14 +963,17 @@ static int parse_feature(struct build_feature_devs_info *binfo,
}
static int parse_feature_list(struct build_feature_devs_info *binfo,
- struct dfl_fpga_enum_dfl *dfl)
+ resource_size_t start, resource_size_t len)
{
- void __iomem *start = dfl->ioaddr;
- void __iomem *end = dfl->ioaddr + dfl->len;
+ resource_size_t end = start + len;
int ret = 0;
u32 ofst = 0;
u64 v;
+ ret = dfl_binfo_prepare(binfo, start, len);
+ if (ret)
+ return ret;
+
/* walk through the device feature list via DFH's next DFH pointer. */
for (; start < end; start += ofst) {
if (end - start < DFH_SIZE) {
@@ -912,11 +981,11 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
return -EINVAL;
}
- ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
+ ret = parse_feature(binfo, start - binfo->start);
if (ret)
return ret;
- v = readq(start + DFH);
+ v = readq(binfo->ioaddr + start - binfo->start + DFH);
ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
/* stop parsing if EOL(End of List) is set or offset is 0 */
@@ -925,7 +994,12 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
}
/* commit current feature device when reach the end of list */
- return build_info_commit_dev(binfo);
+ dfl_binfo_complete(binfo);
+
+ if (is_feature_dev_detected(binfo))
+ ret = build_info_commit_dev(binfo);
+
+ return ret;
}
struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev)
@@ -978,7 +1052,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
* @info: ptr to dfl_fpga_enum_info
* @start: mmio resource address of the device feature list.
* @len: mmio resource length of the device feature list.
- * @ioaddr: mapped mmio resource address of the device feature list.
*
* One FPGA device may have one or more Device Feature Lists (DFLs), use this
* function to add information of each DFL to common data structure for next
@@ -987,8 +1060,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
* Return: 0 on success, negative error code otherwise.
*/
int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
- resource_size_t start, resource_size_t len,
- void __iomem *ioaddr)
+ resource_size_t start, resource_size_t len)
{
struct dfl_fpga_enum_dfl *dfl;
@@ -998,7 +1070,6 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
dfl->start = start;
dfl->len = len;
- dfl->ioaddr = ioaddr;
list_add_tail(&dfl->node, &info->dfls);
@@ -1121,7 +1192,7 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
* Lists.
*/
list_for_each_entry(dfl, &info->dfls, node) {
- ret = parse_feature_list(binfo, dfl);
+ ret = parse_feature_list(binfo, dfl->start, dfl->len);
if (ret) {
remove_feature_devs(cdev);
build_info_free(binfo);
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index b848e3c..704efec 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -447,22 +447,17 @@ struct dfl_fpga_enum_info {
*
* @start: base address of this device feature list.
* @len: size of this device feature list.
- * @ioaddr: mapped base address of this device feature list.
* @node: node in list of device feature lists.
*/
struct dfl_fpga_enum_dfl {
resource_size_t start;
resource_size_t len;
-
- void __iomem *ioaddr;
-
struct list_head node;
};
struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
- resource_size_t start, resource_size_t len,
- void __iomem *ioaddr);
+ resource_size_t start, resource_size_t len);
int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
unsigned int nr_irqs, int *irq_table);
void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/4] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-24 2:09 ` [PATCH v2 2/4] fpga: dfl: map feature mmio resources in their own feature drivers Xu Yilun
@ 2020-08-01 15:43 ` Tom Rix
0 siblings, 0 replies; 15+ messages in thread
From: Tom Rix @ 2020-08-01 15:43 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel
Cc: lgoncalv, Wu Hao, Matthew Gerlach, Russ Weight
Should change my Signed-off-by to
Reviewed-by: Tom Rix <trix@redhat.com>
On 7/23/20 7:09 PM, Xu Yilun wrote:
> This patch makes preparation for modularization of DFL sub feature
> drivers.
>
> Currently, if we need to support a new DFL sub feature, an entry should
> be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
> to re-compile the whole DFL modules. That make the DFL drivers hard to be
> extended.
>
> Another consideration is that DFL may contain some IP blocks which are
> already supported by kernel, most of them are supported by platform
> device drivers. We could create platform devices for these IP blocks and
> get them supported by these drivers.
>
> An important issue is that platform device drivers usually requests mmio
> resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
> dfl-pci) as a whole region. Then platform device drivers for sub features
> can't request their own mmio resources again. This is what the patch
> trying to resolve.
>
> This patch changes the DFL enumeration. DFL bus driver will unmap mmio
> resources after first step enumeration and pass enumeration info to DFL
> framework. Then DFL framework will map the mmio resources again, do 2nd
> step enumeration, and also unmap the mmio resources. In this way, sub
> feature drivers could then request their own mmio resources as needed.
>
> An exception is that mmio resource of FIU headers are still mapped in dfl
> bus driver. The FIU headers have some fundamental functions (sriov set,
> port enable/disable) needed for dfl bus devices and other sub features.
> They should not be unmapped as long as dfl bus device is alive.
>
> 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>
> Signed-off-by: Tom Rix <trix@redhat.com>
> ----
> v2: delete dfl_binfo_shift() cause no shift is possible for FIU parsing.
> Only map bar0 for first phase enumeration in dfl-pci, we can find
> all dfl mmio resource info in bar0.
> Some minor fixes for comments from Hao & Tom.
> ---
> drivers/fpga/dfl-pci.c | 24 +++----
> drivers/fpga/dfl.c | 185 ++++++++++++++++++++++++++++++++++---------------
> drivers/fpga/dfl.h | 7 +-
> 3 files changed, 140 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..a2203d0 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -31,12 +31,12 @@ struct cci_drvdata {
> struct dfl_fpga_cdev *cdev; /* container device */
> };
>
> -static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> +static void __iomem *cci_pci_ioremap_bar0(struct pci_dev *pcidev)
> {
> - if (pcim_iomap_regions(pcidev, BIT(bar), DRV_NAME))
> + if (pcim_iomap_regions(pcidev, BIT(0), DRV_NAME))
> return NULL;
>
> - return pcim_iomap_table(pcidev)[bar];
> + return pcim_iomap_table(pcidev)[0];
> }
>
> static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> @@ -156,8 +156,8 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> goto irq_free_exit;
> }
>
> - /* start to find Device Feature List from Bar 0 */
> - base = cci_pci_ioremap_bar(pcidev, 0);
> + /* start to find Device Feature List in Bar 0 */
> + base = cci_pci_ioremap_bar0(pcidev);
> if (!base) {
> ret = -ENOMEM;
> goto irq_free_exit;
> @@ -172,7 +172,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> start = pci_resource_start(pcidev, 0);
> len = pci_resource_len(pcidev, 0);
>
> - dfl_fpga_enum_info_add_dfl(info, start, len, base);
> + dfl_fpga_enum_info_add_dfl(info, start, len);
>
> /*
> * find more Device Feature Lists (e.g. Ports) per information
> @@ -196,26 +196,24 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> */
> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> - base = cci_pci_ioremap_bar(pcidev, bar);
> - if (!base)
> - continue;
> -
> start = pci_resource_start(pcidev, bar) + offset;
> len = pci_resource_len(pcidev, bar) - offset;
>
> - dfl_fpga_enum_info_add_dfl(info, start, len,
> - base + offset);
> + dfl_fpga_enum_info_add_dfl(info, start, len);
> }
> } else if (dfl_feature_is_port(base)) {
> start = pci_resource_start(pcidev, 0);
> len = pci_resource_len(pcidev, 0);
>
> - dfl_fpga_enum_info_add_dfl(info, start, len, base);
> + dfl_fpga_enum_info_add_dfl(info, start, len);
> } else {
> ret = -ENODEV;
> goto irq_free_exit;
> }
>
> + /* release I/O mappings for next step enumeration */
> + pcim_iounmap_regions(pcidev, BIT(0));
> +
> /* start enumeration with prepared enumeration information */
> cdev = dfl_fpga_feature_devs_enumerate(info);
> if (IS_ERR(cdev)) {
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 9bca22e..b675957 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -250,6 +250,8 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
>
> +#define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
> +
> /**
> * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> * @pdev: feature device.
> @@ -273,8 +275,22 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
> struct dfl_feature *feature,
> struct dfl_feature_driver *drv)
> {
> + void __iomem *base;
> int ret = 0;
>
> + if (!is_header_feature(feature)) {
> + base = devm_platform_ioremap_resource(pdev,
> + feature->resource_index);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev,
> + "ioremap failed for feature 0x%x!\n",
> + feature->id);
> + return PTR_ERR(base);
> + }
> +
> + feature->ioaddr = base;
> + }
> +
> if (drv->ops->init) {
> ret = drv->ops->init(pdev, feature);
> if (ret)
> @@ -427,7 +443,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> * this device.
> * @feature_dev: current feature device.
> - * @ioaddr: header register region address of feature device in enumeration.
> + * @ioaddr: header register region address of current FIU in enumeration.
> + * @start: register resource start of current FIU.
> + * @len: max register resource length of current FIU.
> * @sub_features: a sub features linked list for feature device in enumeration.
> * @feature_num: number of sub features for feature device in enumeration.
> */
> @@ -439,6 +457,8 @@ struct build_feature_devs_info {
>
> struct platform_device *feature_dev;
> void __iomem *ioaddr;
> + resource_size_t start;
> + resource_size_t len;
> struct list_head sub_features;
> int feature_num;
> };
> @@ -484,10 +504,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> struct dfl_feature_platform_data *pdata;
> struct dfl_feature_info *finfo, *p;
> enum dfl_id_type type;
> - int ret, index = 0;
> -
> - if (!fdev)
> - return 0;
> + int ret, index = 0, res_idx = 0;
>
> type = feature_dev_id_type(fdev);
> if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> @@ -531,16 +548,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>
> /* fill features and resource information for feature dev */
> list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> - struct dfl_feature *feature = &pdata->features[index];
> + struct dfl_feature *feature = &pdata->features[index++];
> struct dfl_feature_irq_ctx *ctx;
> unsigned int i;
>
> /* save resource information for each feature */
> feature->dev = fdev;
> feature->id = finfo->fid;
> - feature->resource_index = index;
> - feature->ioaddr = finfo->ioaddr;
> - fdev->resource[index++] = finfo->mmio_res;
> +
> + /*
> + * map header resource for dfl bus device. Don't add header
> + * resource to feature devices, or the resource tree will be
> + * disordered and cause warning on resource release
> + */
> + if (is_header_feature(feature)) {
> + feature->resource_index = -1;
> + feature->ioaddr =
> + devm_ioremap_resource(binfo->dev,
> + &finfo->mmio_res);
> + if (IS_ERR(feature->ioaddr))
> + return PTR_ERR(feature->ioaddr);
> + } else {
> + feature->resource_index = res_idx;
> + fdev->resource[res_idx++] = finfo->mmio_res;
> + }
>
> if (finfo->nr_irqs) {
> ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> @@ -583,19 +614,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>
> static int
> build_info_create_dev(struct build_feature_devs_info *binfo,
> - enum dfl_id_type type, void __iomem *ioaddr)
> + enum dfl_id_type type)
> {
> struct platform_device *fdev;
> - int ret;
>
> if (type >= DFL_ID_MAX)
> return -EINVAL;
>
> - /* we will create a new device, commit current device first */
> - ret = build_info_commit_dev(binfo);
> - if (ret)
> - return ret;
> -
> /*
> * we use -ENODEV as the initialization indicator which indicates
> * whether the id need to be reclaimed
> @@ -606,7 +631,7 @@ build_info_create_dev(struct build_feature_devs_info *binfo,
>
> binfo->feature_dev = fdev;
> binfo->feature_num = 0;
> - binfo->ioaddr = ioaddr;
> +
> INIT_LIST_HEAD(&binfo->sub_features);
>
> fdev->id = dfl_id_alloc(type, &fdev->dev);
> @@ -748,18 +773,17 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> */
> static int
> create_feature_instance(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> - resource_size_t size, u16 fid)
> + resource_size_t ofst, resource_size_t size, u16 fid)
> {
> unsigned int irq_base, nr_irqs;
> struct dfl_feature_info *finfo;
> int ret;
>
> /* read feature size and id if inputs are invalid */
> - size = size ? size : feature_size(dfl->ioaddr + ofst);
> - fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> + size = size ? size : feature_size(binfo->ioaddr + ofst);
> + fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
>
> - if (dfl->len - ofst < size)
> + if (binfo->len - ofst < size)
> return -EINVAL;
>
> ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> @@ -771,12 +795,11 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> return -ENOMEM;
>
> finfo->fid = fid;
> - finfo->mmio_res.start = dfl->start + ofst;
> + finfo->mmio_res.start = binfo->start + ofst;
> finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> finfo->mmio_res.flags = IORESOURCE_MEM;
> finfo->irq_base = irq_base;
> finfo->nr_irqs = nr_irqs;
> - finfo->ioaddr = dfl->ioaddr + ofst;
>
> list_add_tail(&finfo->node, &binfo->sub_features);
> binfo->feature_num++;
> @@ -785,7 +808,6 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> }
>
> static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl,
> resource_size_t ofst)
> {
> u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> @@ -793,21 +815,22 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
>
> WARN_ON(!size);
>
> - return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> + return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
> }
>
> +#define is_feature_dev_detected(binfo) (!!(binfo)->feature_dev)
> +
> static int parse_feature_afu(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl,
> resource_size_t ofst)
> {
> - if (!binfo->feature_dev) {
> + if (!is_feature_dev_detected(binfo)) {
> dev_err(binfo->dev, "this AFU does not belong to any FIU.\n");
> return -EINVAL;
> }
>
> switch (feature_dev_id_type(binfo->feature_dev)) {
> case PORT_ID:
> - return parse_feature_port_afu(binfo, dfl, ofst);
> + return parse_feature_port_afu(binfo, ofst);
> default:
> dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
> binfo->feature_dev->name);
> @@ -816,8 +839,40 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
> return 0;
> }
>
> +static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
> + resource_size_t start, resource_size_t len)
> +{
> + struct device *dev = binfo->dev;
> + void __iomem *ioaddr;
> +
> + if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
> + dev_err(dev, "request region fail, start:%pa, len:%pa\n",
> + &start, &len);
> + return -EBUSY;
> + }
> +
> + ioaddr = devm_ioremap(dev, start, len);
> + if (!ioaddr) {
> + dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
> + &start, &len);
> + devm_release_mem_region(dev, start, len);
> + return -ENOMEM;
> + }
> +
> + binfo->start = start;
> + binfo->len = len;
> + binfo->ioaddr = ioaddr;
> +
> + return 0;
> +}
> +
> +static void dfl_binfo_complete(struct build_feature_devs_info *binfo)
> +{
> + devm_iounmap(binfo->dev, binfo->ioaddr);
> + devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
> +}
> +
> static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl,
> resource_size_t ofst)
> {
> int ret = 0;
> @@ -825,27 +880,39 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> u16 id;
> u64 v;
>
> - v = readq(dfl->ioaddr + ofst + DFH);
> + if (is_feature_dev_detected(binfo)) {
> + dfl_binfo_complete(binfo);
> +
> + ret = build_info_commit_dev(binfo);
> + if (ret)
> + return ret;
> +
> + ret = dfl_binfo_prepare(binfo, binfo->start + ofst,
> + binfo->len - ofst);
> + if (ret)
> + return ret;
> + }
> +
> + v = readq(binfo->ioaddr + DFH);
> id = FIELD_GET(DFH_ID, v);
>
> /* create platform device for dfl feature dev */
> - ret = build_info_create_dev(binfo, dfh_id_to_type(id),
> - dfl->ioaddr + ofst);
> + ret = build_info_create_dev(binfo, dfh_id_to_type(id));
> if (ret)
> return ret;
>
> - ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> + ret = create_feature_instance(binfo, 0, 0, 0);
> if (ret)
> return ret;
> /*
> * find and parse FIU's child AFU via its NEXT_AFU register.
> * please note that only Port has valid NEXT_AFU pointer per spec.
> */
> - v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> + v = readq(binfo->ioaddr + NEXT_AFU);
>
> offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> if (offset)
> - return parse_feature_afu(binfo, dfl, ofst + offset);
> + return parse_feature_afu(binfo, offset);
>
> dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
>
> @@ -853,16 +920,15 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> }
>
> static int parse_feature_private(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl,
> resource_size_t ofst)
> {
> - if (!binfo->feature_dev) {
> + if (!is_feature_dev_detected(binfo)) {
> dev_err(binfo->dev, "the private feature 0x%x does not belong to any AFU.\n",
> - feature_id(dfl->ioaddr + ofst));
> + feature_id(binfo->ioaddr + ofst));
> return -EINVAL;
> }
>
> - return create_feature_instance(binfo, dfl, ofst, 0, 0);
> + return create_feature_instance(binfo, ofst, 0, 0);
> }
>
> /**
> @@ -870,24 +936,24 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
> *
> * @binfo: build feature devices information.
> * @dfl: device feature list to parse
> - * @ofst: offset to feature header on this device feature list
> + * @ofst: offset to current FIU header
> */
> static int parse_feature(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> + resource_size_t ofst)
> {
> u64 v;
> u32 type;
>
> - v = readq(dfl->ioaddr + ofst + DFH);
> + v = readq(binfo->ioaddr + ofst + DFH);
> type = FIELD_GET(DFH_TYPE, v);
>
> switch (type) {
> case DFH_TYPE_AFU:
> - return parse_feature_afu(binfo, dfl, ofst);
> + return parse_feature_afu(binfo, ofst);
> case DFH_TYPE_PRIVATE:
> - return parse_feature_private(binfo, dfl, ofst);
> + return parse_feature_private(binfo, ofst);
> case DFH_TYPE_FIU:
> - return parse_feature_fiu(binfo, dfl, ofst);
> + return parse_feature_fiu(binfo, ofst);
> default:
> dev_info(binfo->dev,
> "Feature Type %x is not supported.\n", type);
> @@ -897,14 +963,17 @@ static int parse_feature(struct build_feature_devs_info *binfo,
> }
>
> static int parse_feature_list(struct build_feature_devs_info *binfo,
> - struct dfl_fpga_enum_dfl *dfl)
> + resource_size_t start, resource_size_t len)
> {
> - void __iomem *start = dfl->ioaddr;
> - void __iomem *end = dfl->ioaddr + dfl->len;
> + resource_size_t end = start + len;
> int ret = 0;
> u32 ofst = 0;
> u64 v;
>
> + ret = dfl_binfo_prepare(binfo, start, len);
> + if (ret)
> + return ret;
> +
> /* walk through the device feature list via DFH's next DFH pointer. */
> for (; start < end; start += ofst) {
> if (end - start < DFH_SIZE) {
> @@ -912,11 +981,11 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
> return -EINVAL;
> }
>
> - ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> + ret = parse_feature(binfo, start - binfo->start);
> if (ret)
> return ret;
>
> - v = readq(start + DFH);
> + v = readq(binfo->ioaddr + start - binfo->start + DFH);
> ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
>
> /* stop parsing if EOL(End of List) is set or offset is 0 */
> @@ -925,7 +994,12 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
> }
>
> /* commit current feature device when reach the end of list */
> - return build_info_commit_dev(binfo);
> + dfl_binfo_complete(binfo);
> +
> + if (is_feature_dev_detected(binfo))
> + ret = build_info_commit_dev(binfo);
> +
> + return ret;
> }
>
> struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev)
> @@ -978,7 +1052,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> * @info: ptr to dfl_fpga_enum_info
> * @start: mmio resource address of the device feature list.
> * @len: mmio resource length of the device feature list.
> - * @ioaddr: mapped mmio resource address of the device feature list.
> *
> * One FPGA device may have one or more Device Feature Lists (DFLs), use this
> * function to add information of each DFL to common data structure for next
> @@ -987,8 +1060,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> * Return: 0 on success, negative error code otherwise.
> */
> int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> - resource_size_t start, resource_size_t len,
> - void __iomem *ioaddr)
> + resource_size_t start, resource_size_t len)
> {
> struct dfl_fpga_enum_dfl *dfl;
>
> @@ -998,7 +1070,6 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
>
> dfl->start = start;
> dfl->len = len;
> - dfl->ioaddr = ioaddr;
>
> list_add_tail(&dfl->node, &info->dfls);
>
> @@ -1121,7 +1192,7 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
> * Lists.
> */
> list_for_each_entry(dfl, &info->dfls, node) {
> - ret = parse_feature_list(binfo, dfl);
> + ret = parse_feature_list(binfo, dfl->start, dfl->len);
> if (ret) {
> remove_feature_devs(cdev);
> build_info_free(binfo);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index b848e3c..704efec 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -447,22 +447,17 @@ struct dfl_fpga_enum_info {
> *
> * @start: base address of this device feature list.
> * @len: size of this device feature list.
> - * @ioaddr: mapped base address of this device feature list.
> * @node: node in list of device feature lists.
> */
> struct dfl_fpga_enum_dfl {
> resource_size_t start;
> resource_size_t len;
> -
> - void __iomem *ioaddr;
> -
> struct list_head node;
> };
>
> struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> - resource_size_t start, resource_size_t len,
> - void __iomem *ioaddr);
> + resource_size_t start, resource_size_t len);
> int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> unsigned int nr_irqs, int *irq_table);
> void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] fpga: dfl: create a dfl bus type to support DFL devices
2020-07-24 2:09 [PATCH v2 0/4] Modularization of DFL private feature drivers Xu Yilun
2020-07-24 2:09 ` [PATCH v2 1/4] fpga: dfl: change data type of feature id to u16 Xu Yilun
2020-07-24 2:09 ` [PATCH v2 2/4] fpga: dfl: map feature mmio resources in their own feature drivers Xu Yilun
@ 2020-07-24 2:09 ` Xu Yilun
2020-08-01 15:47 ` Tom Rix
2020-07-24 2:09 ` [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature Xu Yilun
2020-07-24 3:03 ` [PATCH v2 0/4] Modularization of DFL private feature drivers Randy Dunlap
4 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-24 2:09 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, Wu Hao, Matthew Gerlach, Russ Weight
A new bus type "dfl" is introduced for private features which are not
initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
private features could be handled by separate driver modules.
DFL feature drivers (dfl-fme, dfl-port) will create DFL devices on
enumeration. DFL drivers could be registered on this bus to match these
DFL devices. They are matched by dfl type & feature_id.
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>
----
v2: change the bus uevent format.
change the dfl device's sysfs name format.
refactor dfl_dev_add().
minor fixes for comments from Hao and Tom.
---
Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
drivers/fpga/dfl.c | 254 +++++++++++++++++++++++++++++++-
drivers/fpga/dfl.h | 84 +++++++++++
3 files changed, 345 insertions(+), 8 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
new file mode 100644
index 0000000..b1eea30
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl
@@ -0,0 +1,15 @@
+What: /sys/bus/dfl/devices/.../type
+Date: July 2020
+KernelVersion: 5.9
+Contact: Xu Yilun <yilun.xu@intel.com>
+Description: Read-only. It returns type of DFL FIU of the device. Now DFL
+ supports 2 FIU types, 0 for FME, 1 for PORT.
+ Format: 0x%x
+
+What: /sys/bus/dfl/devices/.../feature_id
+Date: July 2020
+KernelVersion: 5.9
+Contact: Xu Yilun <yilun.xu@intel.com>
+Description: Read-only. It returns feature identifier local to its DFL FIU
+ type.
+ Format: 0x%x
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b675957..c437053 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
* index to dfl_chardevs table. If no chardev support just set devt_type
* as one invalid index (DFL_FPGA_DEVT_MAX).
*/
-enum dfl_id_type {
- FME_ID, /* fme id allocation and mapping */
- PORT_ID, /* port id allocation and mapping */
- DFL_ID_MAX,
-};
-
enum dfl_fpga_devt_type {
DFL_FPGA_DEVT_FME,
DFL_FPGA_DEVT_PORT,
@@ -250,6 +244,236 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
}
EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
+static DEFINE_IDA(dfl_device_ida);
+
+static const struct dfl_device_id *
+dfl_match_one_device(const struct dfl_device_id *id, struct dfl_device *ddev)
+{
+ if (id->type == ddev->type && id->feature_id == ddev->feature_id)
+ return id;
+
+ return NULL;
+}
+
+static int dfl_bus_match(struct device *dev, struct device_driver *drv)
+{
+ struct dfl_device *ddev = to_dfl_dev(dev);
+ struct dfl_driver *ddrv = to_dfl_drv(drv);
+ const struct dfl_device_id *id_entry = ddrv->id_table;
+
+ if (id_entry) {
+ while (id_entry->feature_id) {
+ if (dfl_match_one_device(id_entry, ddev)) {
+ ddev->id_entry = id_entry;
+ return 1;
+ }
+ id_entry++;
+ }
+ }
+
+ return 0;
+}
+
+static int dfl_bus_probe(struct device *dev)
+{
+ struct dfl_device *ddev = to_dfl_dev(dev);
+ struct dfl_driver *ddrv = to_dfl_drv(dev->driver);
+
+ return ddrv->probe(ddev);
+}
+
+static int dfl_bus_remove(struct device *dev)
+{
+ struct dfl_device *ddev = to_dfl_dev(dev);
+ struct dfl_driver *ddrv = to_dfl_drv(dev->driver);
+
+ if (ddrv->remove)
+ ddrv->remove(ddev);
+
+ return 0;
+}
+
+static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+ struct dfl_device *ddev = to_dfl_dev(dev);
+
+ return add_uevent_var(env, "MODALIAS=dfl:t%08Xf%04X",
+ ddev->type, ddev->feature_id);
+}
+
+/* show dfl info fields */
+#define dfl_info_attr(field, format_string) \
+static ssize_t \
+field##_show(struct device *dev, struct device_attribute *attr, \
+ char *buf) \
+{ \
+ struct dfl_device *ddev = to_dfl_dev(dev); \
+ \
+ return sprintf(buf, format_string, ddev->field); \
+} \
+static DEVICE_ATTR_RO(field)
+
+dfl_info_attr(type, "0x%x\n");
+dfl_info_attr(feature_id, "0x%x\n");
+
+static struct attribute *dfl_dev_attrs[] = {
+ &dev_attr_type.attr,
+ &dev_attr_feature_id.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(dfl_dev);
+
+static struct bus_type dfl_bus_type = {
+ .name = "dfl",
+ .match = dfl_bus_match,
+ .probe = dfl_bus_probe,
+ .remove = dfl_bus_remove,
+ .uevent = dfl_bus_uevent,
+ .dev_groups = dfl_dev_groups,
+};
+
+static void release_dfl_dev(struct device *dev)
+{
+ struct dfl_device *ddev = to_dfl_dev(dev);
+
+ if (ddev->mmio_res.parent)
+ release_resource(&ddev->mmio_res);
+
+ ida_simple_remove(&dfl_device_ida, ddev->id);
+ kfree(ddev->irqs);
+ kfree(ddev);
+}
+
+static struct dfl_device *
+dfl_dev_add(struct dfl_feature_platform_data *pdata,
+ struct dfl_feature *feature)
+{
+ struct platform_device *pdev = pdata->dev;
+ struct resource *parent_res;
+ struct dfl_device *ddev;
+ int id, i, ret;
+
+ ddev = kzalloc(sizeof(*ddev), GFP_KERNEL);
+ if (!ddev)
+ return ERR_PTR(-ENOMEM);
+
+ id = ida_simple_get(&dfl_device_ida, 0, 0, GFP_KERNEL);
+ if (id < 0) {
+ dev_err(&pdev->dev, "unable to get id\n");
+ kfree(ddev);
+ return ERR_PTR(id);
+ }
+
+ /* freeing resources by put_device() after device_initialize() */
+ device_initialize(&ddev->dev);
+ ddev->dev.parent = &pdev->dev;
+ ddev->dev.bus = &dfl_bus_type;
+ ddev->dev.release = release_dfl_dev;
+ ddev->id = id;
+ ret = dev_set_name(&ddev->dev, "dfl_dev.%d", id);
+ if (ret)
+ goto put_dev;
+
+ ddev->type = feature_dev_id_type(pdev);
+ ddev->feature_id = feature->id;
+ ddev->cdev = pdata->dfl_cdev;
+
+ /* add mmio resource */
+ parent_res = &pdev->resource[feature->resource_index];
+ ddev->mmio_res.flags = IORESOURCE_MEM;
+ ddev->mmio_res.start = parent_res->start;
+ ddev->mmio_res.end = parent_res->end;
+ ddev->mmio_res.name = dev_name(&ddev->dev);
+ ret = insert_resource(parent_res, &ddev->mmio_res);
+ if (ret) {
+ dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
+ dev_name(&ddev->dev), &ddev->mmio_res);
+ goto put_dev;
+ }
+
+ /* then add irq resource */
+ if (feature->nr_irqs) {
+ ddev->irqs = kcalloc(feature->nr_irqs,
+ sizeof(*ddev->irqs), GFP_KERNEL);
+ if (!ddev->irqs) {
+ ret = -ENOMEM;
+ goto put_dev;
+ }
+
+ for (i = 0; i < feature->nr_irqs; i++)
+ ddev->irqs[i] = feature->irq_ctx[i].irq;
+
+ ddev->num_irqs = feature->nr_irqs;
+ }
+
+ ret = device_add(&ddev->dev);
+ if (ret)
+ goto put_dev;
+
+ dev_info(&pdev->dev, "add dfl_dev: %s\n", dev_name(&ddev->dev));
+ return ddev;
+
+put_dev:
+ /* calls release_dfl_dev() which does the clean up */
+ put_device(&ddev->dev);
+ return ERR_PTR(ret);
+}
+
+static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
+{
+ struct dfl_device *ddev;
+ struct dfl_feature *feature;
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
+ if (!feature->ioaddr && feature->priv) {
+ ddev = feature->priv;
+ device_unregister(&ddev->dev);
+ feature->priv = NULL;
+ }
+ }
+}
+
+static int dfl_devs_init(struct platform_device *pdev)
+{
+ struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ struct dfl_feature *feature;
+ struct dfl_device *ddev;
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
+ if (feature->ioaddr || feature->priv)
+ continue;
+
+ ddev = dfl_dev_add(pdata, feature);
+ if (IS_ERR(ddev)) {
+ dfl_devs_uinit(pdata);
+ return PTR_ERR(ddev);
+ }
+
+ feature->priv = ddev;
+ }
+
+ return 0;
+}
+
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
+{
+ if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
+ return -EINVAL;
+
+ dfl_drv->drv.owner = owner;
+ dfl_drv->drv.bus = &dfl_bus_type;
+
+ return driver_register(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(__dfl_driver_register);
+
+void dfl_driver_unregister(struct dfl_driver *dfl_drv)
+{
+ driver_unregister(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(dfl_driver_unregister);
+
#define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
/**
@@ -261,12 +485,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct dfl_feature *feature;
- dfl_fpga_dev_for_each_feature(pdata, feature)
+ dfl_devs_uinit(pdata);
+
+ dfl_fpga_dev_for_each_feature(pdata, feature) {
if (feature->ops) {
if (feature->ops->uinit)
feature->ops->uinit(pdev, feature);
feature->ops = NULL;
}
+ }
}
EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
@@ -347,6 +574,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
drv++;
}
+ ret = dfl_devs_init(pdev);
+ if (ret)
+ goto exit;
+
return 0;
exit:
dfl_fpga_dev_feature_uinit(pdev);
@@ -1285,11 +1516,17 @@ static int __init dfl_fpga_init(void)
{
int ret;
+ ret = bus_register(&dfl_bus_type);
+ if (ret)
+ return ret;
+
dfl_ids_init();
ret = dfl_chardev_init();
- if (ret)
+ if (ret) {
dfl_ids_destroy();
+ bus_unregister(&dfl_bus_type);
+ }
return ret;
}
@@ -1627,6 +1864,7 @@ static void __exit dfl_fpga_exit(void)
{
dfl_chardev_uinit();
dfl_ids_destroy();
+ bus_unregister(&dfl_bus_type);
}
module_init(dfl_fpga_init);
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 704efec..7b196867 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -520,4 +520,88 @@ 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,
+ PORT_ID,
+ DFL_ID_MAX,
+};
+
+/**
+ * struct dfl_device_id - dfl device identifier
+ * @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.
+ * @driver_data: Driver specific data
+ */
+struct dfl_device_id {
+ unsigned int type;
+ u16 feature_id;
+ unsigned long driver_data;
+};
+
+/**
+ * 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: 16 bits 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;
+ unsigned int 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: 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 */
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/4] fpga: dfl: create a dfl bus type to support DFL devices
2020-07-24 2:09 ` [PATCH v2 3/4] fpga: dfl: create a dfl bus type to support DFL devices Xu Yilun
@ 2020-08-01 15:47 ` Tom Rix
0 siblings, 0 replies; 15+ messages in thread
From: Tom Rix @ 2020-08-01 15:47 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel
Cc: lgoncalv, Wu Hao, Matthew Gerlach, Russ Weight
This patchset is fine.
Please add.
Reviewed-by: Tom Rix <trix@redhat.com>
Thanks
Tom
On 7/23/20 7:09 PM, Xu Yilun wrote:
> A new bus type "dfl" is introduced for private features which are not
> initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> private features could be handled by separate driver modules.
>
> DFL feature drivers (dfl-fme, dfl-port) will create DFL devices on
> enumeration. DFL drivers could be registered on this bus to match these
> DFL devices. They are matched by dfl type & feature_id.
>
> 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>
> ----
> v2: change the bus uevent format.
> change the dfl device's sysfs name format.
> refactor dfl_dev_add().
> minor fixes for comments from Hao and Tom.
> ---
> Documentation/ABI/testing/sysfs-bus-dfl | 15 ++
> drivers/fpga/dfl.c | 254 +++++++++++++++++++++++++++++++-
> drivers/fpga/dfl.h | 84 +++++++++++
> 3 files changed, 345 insertions(+), 8 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> new file mode 100644
> index 0000000..b1eea30
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -0,0 +1,15 @@
> +What: /sys/bus/dfl/devices/.../type
> +Date: July 2020
> +KernelVersion: 5.9
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It returns type of DFL FIU of the device. Now DFL
> + supports 2 FIU types, 0 for FME, 1 for PORT.
> + Format: 0x%x
> +
> +What: /sys/bus/dfl/devices/.../feature_id
> +Date: July 2020
> +KernelVersion: 5.9
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It returns feature identifier local to its DFL FIU
> + type.
> + Format: 0x%x
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b675957..c437053 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> * index to dfl_chardevs table. If no chardev support just set devt_type
> * as one invalid index (DFL_FPGA_DEVT_MAX).
> */
> -enum dfl_id_type {
> - FME_ID, /* fme id allocation and mapping */
> - PORT_ID, /* port id allocation and mapping */
> - DFL_ID_MAX,
> -};
> -
> enum dfl_fpga_devt_type {
> DFL_FPGA_DEVT_FME,
> DFL_FPGA_DEVT_PORT,
> @@ -250,6 +244,236 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
>
> +static DEFINE_IDA(dfl_device_ida);
> +
> +static const struct dfl_device_id *
> +dfl_match_one_device(const struct dfl_device_id *id, struct dfl_device *ddev)
> +{
> + if (id->type == ddev->type && id->feature_id == ddev->feature_id)
> + return id;
> +
> + return NULL;
> +}
> +
> +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> +{
> + struct dfl_device *ddev = to_dfl_dev(dev);
> + struct dfl_driver *ddrv = to_dfl_drv(drv);
> + const struct dfl_device_id *id_entry = ddrv->id_table;
> +
> + if (id_entry) {
> + while (id_entry->feature_id) {
> + if (dfl_match_one_device(id_entry, ddev)) {
> + ddev->id_entry = id_entry;
> + return 1;
> + }
> + id_entry++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int dfl_bus_probe(struct device *dev)
> +{
> + struct dfl_device *ddev = to_dfl_dev(dev);
> + struct dfl_driver *ddrv = to_dfl_drv(dev->driver);
> +
> + return ddrv->probe(ddev);
> +}
> +
> +static int dfl_bus_remove(struct device *dev)
> +{
> + struct dfl_device *ddev = to_dfl_dev(dev);
> + struct dfl_driver *ddrv = to_dfl_drv(dev->driver);
> +
> + if (ddrv->remove)
> + ddrv->remove(ddev);
> +
> + return 0;
> +}
> +
> +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct dfl_device *ddev = to_dfl_dev(dev);
> +
> + return add_uevent_var(env, "MODALIAS=dfl:t%08Xf%04X",
> + ddev->type, ddev->feature_id);
> +}
> +
> +/* show dfl info fields */
> +#define dfl_info_attr(field, format_string) \
> +static ssize_t \
> +field##_show(struct device *dev, struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + struct dfl_device *ddev = to_dfl_dev(dev); \
> + \
> + return sprintf(buf, format_string, ddev->field); \
> +} \
> +static DEVICE_ATTR_RO(field)
> +
> +dfl_info_attr(type, "0x%x\n");
> +dfl_info_attr(feature_id, "0x%x\n");
> +
> +static struct attribute *dfl_dev_attrs[] = {
> + &dev_attr_type.attr,
> + &dev_attr_feature_id.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dfl_dev);
> +
> +static struct bus_type dfl_bus_type = {
> + .name = "dfl",
> + .match = dfl_bus_match,
> + .probe = dfl_bus_probe,
> + .remove = dfl_bus_remove,
> + .uevent = dfl_bus_uevent,
> + .dev_groups = dfl_dev_groups,
> +};
> +
> +static void release_dfl_dev(struct device *dev)
> +{
> + struct dfl_device *ddev = to_dfl_dev(dev);
> +
> + if (ddev->mmio_res.parent)
> + release_resource(&ddev->mmio_res);
> +
> + ida_simple_remove(&dfl_device_ida, ddev->id);
> + kfree(ddev->irqs);
> + kfree(ddev);
> +}
> +
> +static struct dfl_device *
> +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> + struct dfl_feature *feature)
> +{
> + struct platform_device *pdev = pdata->dev;
> + struct resource *parent_res;
> + struct dfl_device *ddev;
> + int id, i, ret;
> +
> + ddev = kzalloc(sizeof(*ddev), GFP_KERNEL);
> + if (!ddev)
> + return ERR_PTR(-ENOMEM);
> +
> + id = ida_simple_get(&dfl_device_ida, 0, 0, GFP_KERNEL);
> + if (id < 0) {
> + dev_err(&pdev->dev, "unable to get id\n");
> + kfree(ddev);
> + return ERR_PTR(id);
> + }
> +
> + /* freeing resources by put_device() after device_initialize() */
> + device_initialize(&ddev->dev);
> + ddev->dev.parent = &pdev->dev;
> + ddev->dev.bus = &dfl_bus_type;
> + ddev->dev.release = release_dfl_dev;
> + ddev->id = id;
> + ret = dev_set_name(&ddev->dev, "dfl_dev.%d", id);
> + if (ret)
> + goto put_dev;
> +
> + ddev->type = feature_dev_id_type(pdev);
> + ddev->feature_id = feature->id;
> + ddev->cdev = pdata->dfl_cdev;
> +
> + /* add mmio resource */
> + parent_res = &pdev->resource[feature->resource_index];
> + ddev->mmio_res.flags = IORESOURCE_MEM;
> + ddev->mmio_res.start = parent_res->start;
> + ddev->mmio_res.end = parent_res->end;
> + ddev->mmio_res.name = dev_name(&ddev->dev);
> + ret = insert_resource(parent_res, &ddev->mmio_res);
> + if (ret) {
> + dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> + dev_name(&ddev->dev), &ddev->mmio_res);
> + goto put_dev;
> + }
> +
> + /* then add irq resource */
> + if (feature->nr_irqs) {
> + ddev->irqs = kcalloc(feature->nr_irqs,
> + sizeof(*ddev->irqs), GFP_KERNEL);
> + if (!ddev->irqs) {
> + ret = -ENOMEM;
> + goto put_dev;
> + }
> +
> + for (i = 0; i < feature->nr_irqs; i++)
> + ddev->irqs[i] = feature->irq_ctx[i].irq;
> +
> + ddev->num_irqs = feature->nr_irqs;
> + }
> +
> + ret = device_add(&ddev->dev);
> + if (ret)
> + goto put_dev;
> +
> + dev_info(&pdev->dev, "add dfl_dev: %s\n", dev_name(&ddev->dev));
> + return ddev;
> +
> +put_dev:
> + /* calls release_dfl_dev() which does the clean up */
> + put_device(&ddev->dev);
> + return ERR_PTR(ret);
> +}
> +
> +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> +{
> + struct dfl_device *ddev;
> + struct dfl_feature *feature;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (!feature->ioaddr && feature->priv) {
> + ddev = feature->priv;
> + device_unregister(&ddev->dev);
> + feature->priv = NULL;
> + }
> + }
> +}
> +
> +static int dfl_devs_init(struct platform_device *pdev)
> +{
> + struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + struct dfl_feature *feature;
> + struct dfl_device *ddev;
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> + if (feature->ioaddr || feature->priv)
> + continue;
> +
> + ddev = dfl_dev_add(pdata, feature);
> + if (IS_ERR(ddev)) {
> + dfl_devs_uinit(pdata);
> + return PTR_ERR(ddev);
> + }
> +
> + feature->priv = ddev;
> + }
> +
> + return 0;
> +}
> +
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> +{
> + if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> + return -EINVAL;
> +
> + dfl_drv->drv.owner = owner;
> + dfl_drv->drv.bus = &dfl_bus_type;
> +
> + return driver_register(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(__dfl_driver_register);
> +
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> +{
> + driver_unregister(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(dfl_driver_unregister);
> +
> #define is_header_feature(feature) ((feature)->id == FEATURE_ID_FIU_HEADER)
>
> /**
> @@ -261,12 +485,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> struct dfl_feature *feature;
>
> - dfl_fpga_dev_for_each_feature(pdata, feature)
> + dfl_devs_uinit(pdata);
> +
> + dfl_fpga_dev_for_each_feature(pdata, feature) {
> if (feature->ops) {
> if (feature->ops->uinit)
> feature->ops->uinit(pdev, feature);
> feature->ops = NULL;
> }
> + }
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
>
> @@ -347,6 +574,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> drv++;
> }
>
> + ret = dfl_devs_init(pdev);
> + if (ret)
> + goto exit;
> +
> return 0;
> exit:
> dfl_fpga_dev_feature_uinit(pdev);
> @@ -1285,11 +1516,17 @@ static int __init dfl_fpga_init(void)
> {
> int ret;
>
> + ret = bus_register(&dfl_bus_type);
> + if (ret)
> + return ret;
> +
> dfl_ids_init();
>
> ret = dfl_chardev_init();
> - if (ret)
> + if (ret) {
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> + }
>
> return ret;
> }
> @@ -1627,6 +1864,7 @@ static void __exit dfl_fpga_exit(void)
> {
> dfl_chardev_uinit();
> dfl_ids_destroy();
> + bus_unregister(&dfl_bus_type);
> }
>
> module_init(dfl_fpga_init);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 704efec..7b196867 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -520,4 +520,88 @@ 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,
> + PORT_ID,
> + DFL_ID_MAX,
> +};
> +
> +/**
> + * struct dfl_device_id - dfl device identifier
> + * @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.
> + * @driver_data: Driver specific data
> + */
> +struct dfl_device_id {
> + unsigned int type;
> + u16 feature_id;
> + unsigned long driver_data;
> +};
> +
> +/**
> + * 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: 16 bits 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;
> + unsigned int 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: 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 */
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature
2020-07-24 2:09 [PATCH v2 0/4] Modularization of DFL private feature drivers Xu Yilun
` (2 preceding siblings ...)
2020-07-24 2:09 ` [PATCH v2 3/4] fpga: dfl: create a dfl bus type to support DFL devices Xu Yilun
@ 2020-07-24 2:09 ` Xu Yilun
2020-07-25 14:53 ` Tom Rix
2020-07-24 3:03 ` [PATCH v2 0/4] Modularization of DFL private feature drivers Randy Dunlap
4 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-24 2:09 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, Wu Hao, Matthew Gerlach, Russ Weight
This patch adds support for the nios handshake private feature on Intel
N3000 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. When nios finished the
configuration, host takes over the ownership of the SPI master to control
an Intel MAX10 BMC Chip on the SPI bus.
For NIOS firmware handshake part, this driver requests the retimer
configuration for NIOS with parameters from module param, and adds some
sysfs nodes for user to query NIOS state.
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 following the SPI work.
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>
---
.../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 16 +
drivers/fpga/Kconfig | 12 +
drivers/fpga/Makefile | 2 +
drivers/fpga/dfl-n3000-nios.c | 483 +++++++++++++++++++++
4 files changed, 513 insertions(+)
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..8346b74
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
@@ -0,0 +1,16 @@
+What: /sys/bus/dfl/devices/dfl-fme.X.X/fec_mode
+Date: July 2020
+KernelVersion: 5.9
+Contact: Xu Yilun <yilun.xu@intel.com>
+Description: Read-only. It returns the FEC mode of the ethernet retimer
+ configured by NIOS firmware. "rs" for RS FEC mode, "kr" for
+ KR FEC mode, "no" FOR NO FEC mode.
+ Format: string
+
+What: /sys/bus/dfl/devices/dfl-fme.X.X/nios_fw_version
+Date: July 2020
+KernelVersion: 5.9
+Contact: Xu Yilun <yilun.xu@intel.com>
+Description: Read-only. It returns the NIOS firmware version in FPGA. Its
+ format is "major.minor.patch".
+ Format: %x.%x.%x
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index b2408a7..fa5f3ab 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -191,6 +191,18 @@ 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_N3000_NIOS
+ tristate "FPGA DFL N3000 NIOS Driver"
+ depends on FPGA_DFL
+ select REGMAP
+ help
+ This is the driver for the nios handshake private feature on Intel
+ N3000 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. When nios finished
+ the configuration, host takes over the ownership of the SPI master to
+ control an Intel MAX10 BMC Chip on the SPI bus.
+
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..27f20f2 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_N3000_NIOS) += 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..d098a10
--- /dev/null
+++ b/drivers/fpga/dfl-n3000-nios.c
@@ -0,0 +1,483 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for DFL N3000 NIOS private feature
+ *
+ * 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/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>
+
+#include "dfl.h"
+
+static char *fec_mode = "rs";
+module_param(fec_mode, charp, 0444);
+MODULE_PARM_DESC(child, "FEC mode");
+
+/* N3000 NIOS private feature registers */
+#define NIOS_SPI_PARAM 0x8
+#define SHIFT_MODE BIT_ULL(1)
+#define SHIFT_MODE_MSB 0
+#define SHIFT_MODE_LSB 1
+#define DATA_WIDTH GENMASK_ULL(7, 2)
+#define NUM_CHIPSELECT GENMASK_ULL(13, 8)
+#define CLK_POLARITY BIT_ULL(14)
+#define CLK_PHASE BIT_ULL(15)
+#define 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 GENMASK_ULL(63, 62)
+#define CMD_NOP 0
+#define CMD_RD 1
+#define 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 PKVL A, link 0, the same below */
+#define REQ_FEC_MODE_A0 GENMASK(9, 8)
+#define REQ_FEC_MODE_A1 GENMASK(11, 10)
+#define REQ_FEC_MODE_A2 GENMASK(13, 12)
+#define REQ_FEC_MODE_A3 GENMASK(15, 14)
+#define REQ_FEC_MODE_B0 GENMASK(17, 16)
+#define REQ_FEC_MODE_B1 GENMASK(19, 18)
+#define REQ_FEC_MODE_B2 GENMASK(21, 20)
+#define REQ_FEC_MODE_B3 GENMASK(23, 22)
+#define FEC_MODE_NO 0x0
+#define FEC_MODE_KR 0x1
+#define 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)
+
+#define PKVL_A_MODE_STS 0x1020
+#define PKVL_B_MODE_STS 0x1024
+
+#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */
+#define NIOS_INIT_TIMEOUT 10000000 /* usec */
+#define NIOS_INIT_TIME_INTV 100000 /* usec */
+
+struct dfl_n3000_nios {
+ void __iomem *base;
+ struct regmap *regmap;
+ struct device *dev;
+ struct platform_device *altr_spi;
+};
+
+static int n3000_nios_writel(struct dfl_n3000_nios *ns, unsigned int reg,
+ unsigned int val)
+{
+ int ret;
+
+ ret = regmap_write(ns->regmap, reg, val);
+ if (ret)
+ dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
+ reg, val, ret);
+
+ return ret;
+}
+
+static int n3000_nios_readl(struct dfl_n3000_nios *ns, unsigned int reg,
+ unsigned int *val)
+{
+ int ret;
+
+ ret = regmap_read(ns->regmap, reg, val);
+ if (ret)
+ dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
+
+ return ret;
+}
+
+static ssize_t nios_fw_version_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
+ unsigned int val;
+ int ret;
+
+ ret = n3000_nios_readl(ns, 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);
+
+static ssize_t fec_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
+ unsigned int val, mode;
+ int ret;
+
+ ret = n3000_nios_readl(ns, NIOS_INIT, &val);
+ if (ret)
+ return ret;
+
+ /*
+ * FEC mode should always be the same for all links, as we set them
+ * in this way.
+ */
+ mode = FIELD_GET(REQ_FEC_MODE_A0, val);
+ if (mode != FIELD_GET(REQ_FEC_MODE_A1, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_A2, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_A3, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_B0, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_B1, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_B2, val) ||
+ mode != FIELD_GET(REQ_FEC_MODE_B3, val))
+ return -EFAULT;
+
+ switch (mode) {
+ case FEC_MODE_NO:
+ return sprintf(buf, "no\n");
+ case FEC_MODE_KR:
+ return sprintf(buf, "kr\n");
+ case 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_fec_mode.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(n3000_nios);
+
+static int init_error_detected(struct dfl_n3000_nios *ns)
+{
+ unsigned int val;
+
+ if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
+ return true;
+
+ if (val >= 0x100)
+ return true;
+
+ if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
+ return true;
+
+ if (val >= 0x100)
+ return true;
+
+ return false;
+}
+
+static void dump_error_stat(struct dfl_n3000_nios *ns)
+{
+ unsigned int val;
+
+ if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
+ return;
+
+ dev_info(ns->dev, "PKVL_A_MODE_STS %x\n", val);
+
+ if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
+ return;
+
+ dev_info(ns->dev, "PKVL_B_MODE_STS %x\n", val);
+}
+
+static int n3000_nios_init_done_check(struct dfl_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 3.x.x version, INIT_START is introduced, so
+ * driver needs to trigger START firstly and then check INIT_DONE.
+ */
+
+ ret = n3000_nios_readl(ns, 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. This is an
+ * issue of FPGA image. We didn't error out because we need SPI master
+ * to reprogram a new image.
+ */
+ if (val == 0) {
+ dev_warn(dev, "NIOS version reg = 0x%x, skip INIT_DONE check, but PKVL may be uninitialized\n",
+ val);
+ return 0;
+ }
+
+ if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
+ /* read NIOS_INIT to check if PKVL INIT done or not */
+ ret = n3000_nios_readl(ns, NIOS_INIT, &val);
+ if (ret)
+ return ret;
+
+ /* check if PKVLs 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;
+
+ /* FEC mode will be ignored by hardware in 10G mode */
+ if (!strcmp(fec_mode, "no"))
+ mode = FEC_MODE_NO;
+ else if (!strcmp(fec_mode, "kr"))
+ mode = FEC_MODE_KR;
+ else if (!strcmp(fec_mode, "rs"))
+ mode = FEC_MODE_RS;
+ else
+ return -EINVAL;
+
+ /* set the same FEC mode for all links */
+ val |= FIELD_PREP(REQ_FEC_MODE_A0, mode) |
+ FIELD_PREP(REQ_FEC_MODE_A1, mode) |
+ FIELD_PREP(REQ_FEC_MODE_A2, mode) |
+ FIELD_PREP(REQ_FEC_MODE_A3, mode) |
+ FIELD_PREP(REQ_FEC_MODE_B0, mode) |
+ FIELD_PREP(REQ_FEC_MODE_B1, mode) |
+ FIELD_PREP(REQ_FEC_MODE_B2, mode) |
+ FIELD_PREP(REQ_FEC_MODE_B3, mode);
+
+ ret = n3000_nios_writel(ns, 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 ERR
+ * detected.
+ * We won't error out here even if error detected. Nios will release
+ * spi controller when INIT_DONE is set, so driver could continue to
+ * initialize spi controller device.
+ */
+ if (init_error_detected(ns)) {
+ dev_warn(dev, "NIOS_INIT_DONE OK, but err found during init\n");
+ goto dump_sts;
+ }
+ return 0;
+
+dump_sts:
+ dump_error_stat(ns);
+
+ return ret;
+}
+
+struct spi_board_info m10_n3000_info = {
+ .modalias = "m10-n3000",
+ .max_speed_hz = 12500000,
+ .bus_num = 0,
+ .chip_select = 0,
+};
+
+static int create_altr_spi_controller(struct dfl_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(CLK_POLARITY, v))
+ pdata.mode_bits |= SPI_CPOL;
+ if (FIELD_GET(CLK_PHASE, v))
+ pdata.mode_bits |= SPI_CPHA;
+
+ pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
+ pdata.bits_per_word_mask =
+ SPI_BPW_RANGE_MASK(1, FIELD_GET(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->altr_spi = platform_device_register_full(&pdevinfo);
+ return PTR_ERR_OR_ZERO(ns->altr_spi);
+}
+
+static void destroy_altr_spi_controller(struct dfl_n3000_nios *ns)
+{
+ platform_device_unregister(ns->altr_spi);
+}
+
+static int ns_bus_poll_stat_timeout(void __iomem *base, u64 *v)
+{
+ int loops = NS_REGBUS_WAIT_TIMEOUT;
+
+ /*
+ * Usually the state changes in few loops, so we try to be simple here
+ * for performance.
+ */
+ do {
+ *v = readq(base + NIOS_SPI_STAT);
+ if (*v & STAT_RW_VAL)
+ break;
+ cpu_relax();
+ } while (--loops);
+
+ return loops ? 0 : -ETIMEDOUT;
+}
+
+static int ns_bus_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+ void __iomem *base = context;
+ u64 v = 0;
+
+ v |= FIELD_PREP(CTRL_CMD, CMD_WR);
+ v |= FIELD_PREP(CTRL_ADDR, reg);
+ v |= FIELD_PREP(CTRL_WR_DATA, val);
+ writeq(v, base + NIOS_SPI_CTRL);
+
+ return ns_bus_poll_stat_timeout(base, &v);
+}
+
+static int ns_bus_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+ void __iomem *base = context;
+ u64 v = 0;
+ int ret;
+
+ v |= FIELD_PREP(CTRL_CMD, CMD_RD);
+ v |= FIELD_PREP(CTRL_ADDR, reg);
+ writeq(v, base + NIOS_SPI_CTRL);
+
+ ret = ns_bus_poll_stat_timeout(base, &v);
+ if (!ret)
+ *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_bus_reg_write,
+ .reg_read = ns_bus_reg_read,
+};
+
+static int dfl_n3000_nios_probe(struct dfl_device *dfl_dev)
+{
+ struct device *dev = &dfl_dev->dev;
+ struct dfl_n3000_nios *ns;
+ int ret;
+
+ ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
+ if (!ns)
+ return -ENOMEM;
+
+ dev_set_drvdata(&dfl_dev->dev, ns);
+
+ ns->dev = dev;
+
+ ns->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
+ if (IS_ERR(ns->base)) {
+ dev_err(dev, "get mem resource fail!\n");
+ return PTR_ERR(ns->base);
+ }
+
+ ns->regmap = devm_regmap_init(dev, NULL, ns->base, &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_altr_spi_controller(ns);
+ if (ret)
+ dev_err(dev, "altr spi controller create failed: %d\n", ret);
+
+ return ret;
+}
+
+static void dfl_n3000_nios_remove(struct dfl_device *dfl_dev)
+{
+ struct dfl_n3000_nios *ns = dev_get_drvdata(&dfl_dev->dev);
+
+ destroy_altr_spi_controller(ns);
+}
+
+#define FME_FEATURE_ID_N3000_NIOS 0xd
+
+static const struct dfl_device_id dfl_n3000_nios_ids[] = {
+ { FME_ID, FME_FEATURE_ID_N3000_NIOS },
+ { }
+};
+
+static struct dfl_driver dfl_n3000_nios_driver = {
+ .drv = {
+ .name = "dfl-n3000-nios",
+ .dev_groups = n3000_nios_groups,
+ },
+ .id_table = dfl_n3000_nios_ids,
+ .probe = dfl_n3000_nios_probe,
+ .remove = dfl_n3000_nios_remove,
+};
+
+module_dfl_driver(dfl_n3000_nios_driver);
+
+MODULE_DEVICE_TABLE(dfl, dfl_n3000_nios_ids);
+MODULE_DESCRIPTION("DFL N3000 NIOS driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature
2020-07-24 2:09 ` [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature Xu Yilun
@ 2020-07-25 14:53 ` Tom Rix
2020-07-31 8:56 ` Xu Yilun
0 siblings, 1 reply; 15+ messages in thread
From: Tom Rix @ 2020-07-25 14:53 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel
Cc: lgoncalv, Wu Hao, Matthew Gerlach, Russ Weight
Mostly little stuff.
The polling on the int in ns_bus_poll_stat_timeout should be refactored.
Tom
On 7/23/20 7:09 PM, Xu Yilun wrote:
> This patch adds support for the nios handshake private feature on Intel
> N3000 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. When nios finished the
> configuration, host takes over the ownership of the SPI master to control
> an Intel MAX10 BMC Chip on the SPI bus.
>
> For NIOS firmware handshake part, this driver requests the retimer
> configuration for NIOS with parameters from module param, and adds some
> sysfs nodes for user to query NIOS state.
>
> 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 following the SPI work.
>
> 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>
> ---
> .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 16 +
> drivers/fpga/Kconfig | 12 +
> drivers/fpga/Makefile | 2 +
> drivers/fpga/dfl-n3000-nios.c | 483 +++++++++++++++++++++
> 4 files changed, 513 insertions(+)
> 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..8346b74
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> @@ -0,0 +1,16 @@
> +What: /sys/bus/dfl/devices/dfl-fme.X.X/fec_mode
> +Date: July 2020
> +KernelVersion: 5.9
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It returns the FEC mode of the ethernet retimer
> + configured by NIOS firmware. "rs" for RS FEC mode, "kr" for
> + KR FEC mode, "no" FOR NO FEC mode.
> + Format: string
> +
> +What: /sys/bus/dfl/devices/dfl-fme.X.X/nios_fw_version
> +Date: July 2020
> +KernelVersion: 5.9
> +Contact: Xu Yilun <yilun.xu@intel.com>
> +Description: Read-only. It returns the NIOS firmware version in FPGA. Its
> + format is "major.minor.patch".
> + Format: %x.%x.%x
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index b2408a7..fa5f3ab 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -191,6 +191,18 @@ 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_N3000_NIOS
> + tristate "FPGA DFL N3000 NIOS Driver"
> + depends on FPGA_DFL
> + select REGMAP
depends on SPI_MASTER ?
> + help
> + This is the driver for the nios handshake private feature on Intel
> + N3000 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. When nios finished
> + the configuration, host takes over the ownership of the SPI master to
> + control an Intel MAX10 BMC Chip on the SPI bus.
> +
> 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..27f20f2 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_N3000_NIOS) += 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..d098a10
> --- /dev/null
> +++ b/drivers/fpga/dfl-n3000-nios.c
> @@ -0,0 +1,483 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for DFL N3000 NIOS private feature
> + *
> + * 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/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>
> +
> +#include "dfl.h"
> +
> +static char *fec_mode = "rs";
> +module_param(fec_mode, charp, 0444);
> +MODULE_PARM_DESC(child, "FEC mode");
Should document what the valid values are and what they mean.
ex/ what is 'rs' ?
Maybe append something in the sysfs doc about fec_mode being settable.
> +
> +/* N3000 NIOS private feature registers */
> +#define NIOS_SPI_PARAM 0x8
> +#define SHIFT_MODE BIT_ULL(1)
> +#define SHIFT_MODE_MSB 0
> +#define SHIFT_MODE_LSB 1
> +#define DATA_WIDTH GENMASK_ULL(7, 2)
> +#define NUM_CHIPSELECT GENMASK_ULL(13, 8)
> +#define CLK_POLARITY BIT_ULL(14)
> +#define CLK_PHASE BIT_ULL(15)
> +#define 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 GENMASK_ULL(63, 62)
> +#define CMD_NOP 0
> +#define CMD_RD 1
> +#define CMD_WR 2
These are fairly generic #define names, consider adding a prefix.
> +
> +#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 PKVL A, link 0, the same below */
> +#define REQ_FEC_MODE_A0 GENMASK(9, 8)
> +#define REQ_FEC_MODE_A1 GENMASK(11, 10)
> +#define REQ_FEC_MODE_A2 GENMASK(13, 12)
> +#define REQ_FEC_MODE_A3 GENMASK(15, 14)
> +#define REQ_FEC_MODE_B0 GENMASK(17, 16)
> +#define REQ_FEC_MODE_B1 GENMASK(19, 18)
> +#define REQ_FEC_MODE_B2 GENMASK(21, 20)
> +#define REQ_FEC_MODE_B3 GENMASK(23, 22)
> +#define FEC_MODE_NO 0x0
> +#define FEC_MODE_KR 0x1
> +#define 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)
> +
> +#define PKVL_A_MODE_STS 0x1020
> +#define PKVL_B_MODE_STS 0x1024
> +
> +#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */
> +#define NIOS_INIT_TIMEOUT 10000000 /* usec */
> +#define NIOS_INIT_TIME_INTV 100000 /* usec */
> +
> +struct dfl_n3000_nios {
> + void __iomem *base;
> + struct regmap *regmap;
> + struct device *dev;
> + struct platform_device *altr_spi;
Make this element more meaningful, expand to 'altera_spi'
> +};
> +
> +static int n3000_nios_writel(struct dfl_n3000_nios *ns, unsigned int reg,
> + unsigned int val)
> +{
> + int ret;
> +
> + ret = regmap_write(ns->regmap, reg, val);
> + if (ret)
> + dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
> + reg, val, ret);
> +
> + return ret;
> +}
> +
> +static int n3000_nios_readl(struct dfl_n3000_nios *ns, unsigned int reg,
> + unsigned int *val)
> +{
> + int ret;
> +
> + ret = regmap_read(ns->regmap, reg, val);
> + if (ret)
> + dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
> +
> + return ret;
> +}
> +
> +static ssize_t nios_fw_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
This and similar could be
ssize_t ret.
> +
> + ret = n3000_nios_readl(ns, 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);
> +
> +static ssize_t fec_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
> + unsigned int val, mode;
> + int ret;
> +
> + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
> + if (ret)
> + return ret;
> +
> + /*
> + * FEC mode should always be the same for all links, as we set them
> + * in this way.
> + */
> + mode = FIELD_GET(REQ_FEC_MODE_A0, val);
> + if (mode != FIELD_GET(REQ_FEC_MODE_A1, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_A2, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_A3, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_B0, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_B1, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_B2, val) ||
> + mode != FIELD_GET(REQ_FEC_MODE_B3, val))
> + return -EFAULT;
> +
> + switch (mode) {
> + case FEC_MODE_NO:
> + return sprintf(buf, "no\n");
> + case FEC_MODE_KR:
> + return sprintf(buf, "kr\n");
> + case 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_fec_mode.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(n3000_nios);
> +
> +static int init_error_detected(struct dfl_n3000_nios *ns)
returning true/false, the return type should be boolean
> +{
> + unsigned int val;
> +
> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
> + return true;
> +
> + if (val >= 0x100)
A magic number like 0x100 should be #define-ed.
similar below
> + return true;
> +
> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
> + return true;
> +
> + if (val >= 0x100)
> + return true;
> +
> + return false;
> +}
> +
> +static void dump_error_stat(struct dfl_n3000_nios *ns)
> +{
> + unsigned int val;
> +
> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
> + return;
Not being able to read the register is itself and error.
Should add something like 'Could not read register PKVL_A_MODE_STS'
similar below
> +
> + dev_info(ns->dev, "PKVL_A_MODE_STS %x\n", val);
> +
> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
> + return;
> +
> + dev_info(ns->dev, "PKVL_B_MODE_STS %x\n", val);
> +}
> +
> +static int n3000_nios_init_done_check(struct dfl_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 3.x.x version, INIT_START is introduced, so
> + * driver needs to trigger START firstly and then check INIT_DONE.
> + */
> +
> + ret = n3000_nios_readl(ns, 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. This is an
> + * issue of FPGA image. We didn't error out because we need SPI master
> + * to reprogram a new image.
> + */
> + if (val == 0) {
> + dev_warn(dev, "NIOS version reg = 0x%x, skip INIT_DONE check, but PKVL may be uninitialized\n",
> + val);
> + return 0;
> + }
> +
> + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
> + /* read NIOS_INIT to check if PKVL INIT done or not */
> + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
> + if (ret)
> + return ret;
> +
> + /* check if PKVLs 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;
> +
> + /* FEC mode will be ignored by hardware in 10G mode */
> + if (!strcmp(fec_mode, "no"))
strncmp is safer but would lead to 'no123' being valid.
> + mode = FEC_MODE_NO;
> + else if (!strcmp(fec_mode, "kr"))
> + mode = FEC_MODE_KR;
> + else if (!strcmp(fec_mode, "rs"))
> + mode = FEC_MODE_RS;
> + else
> + return -EINVAL;
> +
> + /* set the same FEC mode for all links */
> + val |= FIELD_PREP(REQ_FEC_MODE_A0, mode) |
> + FIELD_PREP(REQ_FEC_MODE_A1, mode) |
> + FIELD_PREP(REQ_FEC_MODE_A2, mode) |
> + FIELD_PREP(REQ_FEC_MODE_A3, mode) |
> + FIELD_PREP(REQ_FEC_MODE_B0, mode) |
> + FIELD_PREP(REQ_FEC_MODE_B1, mode) |
> + FIELD_PREP(REQ_FEC_MODE_B2, mode) |
> + FIELD_PREP(REQ_FEC_MODE_B3, mode);
> +
> + ret = n3000_nios_writel(ns, 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 ERR
> + * detected.
> + * We won't error out here even if error detected. Nios will release
> + * spi controller when INIT_DONE is set, so driver could continue to
> + * initialize spi controller device.
> + */
> + if (init_error_detected(ns)) {
> + dev_warn(dev, "NIOS_INIT_DONE OK, but err found during init\n");
> + goto dump_sts;
> + }
> + return 0;
> +
> +dump_sts:
> + dump_error_stat(ns);
> +
> + return ret;
> +}
> +
> +struct spi_board_info m10_n3000_info = {
> + .modalias = "m10-n3000",
> + .max_speed_hz = 12500000,
> + .bus_num = 0,
> + .chip_select = 0,
> +};
> +
> +static int create_altr_spi_controller(struct dfl_n3000_nios *ns)
Also here, altr -> altera
> +{
> + 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(CLK_POLARITY, v))
> + pdata.mode_bits |= SPI_CPOL;
> + if (FIELD_GET(CLK_PHASE, v))
> + pdata.mode_bits |= SPI_CPHA;
> +
> + pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> + pdata.bits_per_word_mask =
> + SPI_BPW_RANGE_MASK(1, FIELD_GET(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->altr_spi = platform_device_register_full(&pdevinfo);
> + return PTR_ERR_OR_ZERO(ns->altr_spi);
> +}
> +
> +static void destroy_altr_spi_controller(struct dfl_n3000_nios *ns)
> +{
> + platform_device_unregister(ns->altr_spi);
> +}
> +
> +static int ns_bus_poll_stat_timeout(void __iomem *base, u64 *v)
> +{
> + int loops = NS_REGBUS_WAIT_TIMEOUT;
Would not this be better as time based timeout ?
Looping over int is only good if you know the clock freq etc.
Could this polling be switch to interrupt ?
> +
> + /*
> + * Usually the state changes in few loops, so we try to be simple here
> + * for performance.
> + */
> + do {
> + *v = readq(base + NIOS_SPI_STAT);
> + if (*v & STAT_RW_VAL)
> + break;
> + cpu_relax();
> + } while (--loops);
> +
> + return loops ? 0 : -ETIMEDOUT;
> +}
> +
> +static int ns_bus_reg_write(void *context, unsigned int reg, unsigned int val)
what is 'ns_' ?
Maybe this should be 'nios_spi' or just drop the prefix.
> +{
> + void __iomem *base = context;
> + u64 v = 0;
> +
> + v |= FIELD_PREP(CTRL_CMD, CMD_WR);
> + v |= FIELD_PREP(CTRL_ADDR, reg);
> + v |= FIELD_PREP(CTRL_WR_DATA, val);
> + writeq(v, base + NIOS_SPI_CTRL);
> +
> + return ns_bus_poll_stat_timeout(base, &v);
> +}
> +
> +static int ns_bus_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + void __iomem *base = context;
> + u64 v = 0;
> + int ret;
> +
> + v |= FIELD_PREP(CTRL_CMD, CMD_RD);
> + v |= FIELD_PREP(CTRL_ADDR, reg);
> + writeq(v, base + NIOS_SPI_CTRL);
> +
> + ret = ns_bus_poll_stat_timeout(base, &v);
> + if (!ret)
> + *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_bus_reg_write,
> + .reg_read = ns_bus_reg_read,
> +};
> +
> +static int dfl_n3000_nios_probe(struct dfl_device *dfl_dev)
> +{
> + struct device *dev = &dfl_dev->dev;
> + struct dfl_n3000_nios *ns;
> + int ret;
> +
> + ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
> + if (!ns)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&dfl_dev->dev, ns);
> +
> + ns->dev = dev;
> +
> + ns->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
> + if (IS_ERR(ns->base)) {
> + dev_err(dev, "get mem resource fail!\n");
> + return PTR_ERR(ns->base);
> + }
> +
> + ns->regmap = devm_regmap_init(dev, NULL, ns->base, &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_altr_spi_controller(ns);
> + if (ret)
> + dev_err(dev, "altr spi controller create failed: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static void dfl_n3000_nios_remove(struct dfl_device *dfl_dev)
> +{
> + struct dfl_n3000_nios *ns = dev_get_drvdata(&dfl_dev->dev);
> +
> + destroy_altr_spi_controller(ns);
> +}
> +
> +#define FME_FEATURE_ID_N3000_NIOS 0xd
To minimize collisions, maybe collect these to a new common header like 'dfl-features-ids.h' or just add to some other visible existing header.
> +
> +static const struct dfl_device_id dfl_n3000_nios_ids[] = {
> + { FME_ID, FME_FEATURE_ID_N3000_NIOS },
> + { }
> +};
> +
> +static struct dfl_driver dfl_n3000_nios_driver = {
> + .drv = {
> + .name = "dfl-n3000-nios",
> + .dev_groups = n3000_nios_groups,
> + },
> + .id_table = dfl_n3000_nios_ids,
> + .probe = dfl_n3000_nios_probe,
> + .remove = dfl_n3000_nios_remove,
> +};
> +
> +module_dfl_driver(dfl_n3000_nios_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, dfl_n3000_nios_ids);
> +MODULE_DESCRIPTION("DFL N3000 NIOS driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature
2020-07-25 14:53 ` Tom Rix
@ 2020-07-31 8:56 ` Xu Yilun
2020-08-01 15:35 ` Tom Rix
0 siblings, 1 reply; 15+ messages in thread
From: Xu Yilun @ 2020-07-31 8:56 UTC (permalink / raw)
To: Tom Rix
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
On Sat, Jul 25, 2020 at 07:53:59AM -0700, Tom Rix wrote:
> Mostly little stuff.
>
> The polling on the int in ns_bus_poll_stat_timeout should be refactored.
>
> Tom
>
> On 7/23/20 7:09 PM, Xu Yilun wrote:
> > This patch adds support for the nios handshake private feature on Intel
> > N3000 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. When nios finished the
> > configuration, host takes over the ownership of the SPI master to control
> > an Intel MAX10 BMC Chip on the SPI bus.
> >
> > For NIOS firmware handshake part, this driver requests the retimer
> > configuration for NIOS with parameters from module param, and adds some
> > sysfs nodes for user to query NIOS state.
> >
> > 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 following the SPI work.
> >
> > 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>
> > ---
> > .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 16 +
> > drivers/fpga/Kconfig | 12 +
> > drivers/fpga/Makefile | 2 +
> > drivers/fpga/dfl-n3000-nios.c | 483 +++++++++++++++++++++
> > 4 files changed, 513 insertions(+)
> > 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..8346b74
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
> > @@ -0,0 +1,16 @@
> > +What: /sys/bus/dfl/devices/dfl-fme.X.X/fec_mode
> > +Date: July 2020
> > +KernelVersion: 5.9
> > +Contact: Xu Yilun <yilun.xu@intel.com>
> > +Description: Read-only. It returns the FEC mode of the ethernet retimer
> > + configured by NIOS firmware. "rs" for RS FEC mode, "kr" for
> > + KR FEC mode, "no" FOR NO FEC mode.
> > + Format: string
> > +
> > +What: /sys/bus/dfl/devices/dfl-fme.X.X/nios_fw_version
> > +Date: July 2020
> > +KernelVersion: 5.9
> > +Contact: Xu Yilun <yilun.xu@intel.com>
> > +Description: Read-only. It returns the NIOS firmware version in FPGA. Its
> > + format is "major.minor.patch".
> > + Format: %x.%x.%x
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index b2408a7..fa5f3ab 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -191,6 +191,18 @@ 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_N3000_NIOS
> > + tristate "FPGA DFL N3000 NIOS Driver"
> > + depends on FPGA_DFL
> > + select REGMAP
> depends on SPI_MASTER ?
I think we don't have to add this. The driver doesn't call any API
provided by SPI core, just fills the spi_board_info structure.
> > + help
> > + This is the driver for the nios handshake private feature on Intel
> > + N3000 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. When nios finished
> > + the configuration, host takes over the ownership of the SPI master to
> > + control an Intel MAX10 BMC Chip on the SPI bus.
> > +
> > 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..27f20f2 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_N3000_NIOS) += 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..d098a10
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-n3000-nios.c
> > @@ -0,0 +1,483 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for DFL N3000 NIOS private feature
> > + *
> > + * 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/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>
> > +
> > +#include "dfl.h"
> > +
> > +static char *fec_mode = "rs";
> > +module_param(fec_mode, charp, 0444);
> > +MODULE_PARM_DESC(child, "FEC mode");
>
> Should document what the valid values are and what they mean.
>
> ex/ what is 'rs' ?
>
> Maybe append something in the sysfs doc about fec_mode being settable.
Yes, I'll add the documents.
>
> > +
> > +/* N3000 NIOS private feature registers */
> > +#define NIOS_SPI_PARAM 0x8
> > +#define SHIFT_MODE BIT_ULL(1)
> > +#define SHIFT_MODE_MSB 0
> > +#define SHIFT_MODE_LSB 1
> > +#define DATA_WIDTH GENMASK_ULL(7, 2)
> > +#define NUM_CHIPSELECT GENMASK_ULL(13, 8)
> > +#define CLK_POLARITY BIT_ULL(14)
> > +#define CLK_PHASE BIT_ULL(15)
> > +#define 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 GENMASK_ULL(63, 62)
> > +#define CMD_NOP 0
> > +#define CMD_RD 1
> > +#define CMD_WR 2
> These are fairly generic #define names, consider adding a prefix.
Yes.
> > +
> > +#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 PKVL A, link 0, the same below */
> > +#define REQ_FEC_MODE_A0 GENMASK(9, 8)
> > +#define REQ_FEC_MODE_A1 GENMASK(11, 10)
> > +#define REQ_FEC_MODE_A2 GENMASK(13, 12)
> > +#define REQ_FEC_MODE_A3 GENMASK(15, 14)
> > +#define REQ_FEC_MODE_B0 GENMASK(17, 16)
> > +#define REQ_FEC_MODE_B1 GENMASK(19, 18)
> > +#define REQ_FEC_MODE_B2 GENMASK(21, 20)
> > +#define REQ_FEC_MODE_B3 GENMASK(23, 22)
> > +#define FEC_MODE_NO 0x0
> > +#define FEC_MODE_KR 0x1
> > +#define 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)
> > +
> > +#define PKVL_A_MODE_STS 0x1020
> > +#define PKVL_B_MODE_STS 0x1024
> > +
> > +#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */
> > +#define NIOS_INIT_TIMEOUT 10000000 /* usec */
> > +#define NIOS_INIT_TIME_INTV 100000 /* usec */
> > +
> > +struct dfl_n3000_nios {
> > + void __iomem *base;
> > + struct regmap *regmap;
> > + struct device *dev;
> > + struct platform_device *altr_spi;
> Make this element more meaningful, expand to 'altera_spi'
Yes.
> > +};
> > +
> > +static int n3000_nios_writel(struct dfl_n3000_nios *ns, unsigned int reg,
> > + unsigned int val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(ns->regmap, reg, val);
> > + if (ret)
> > + dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
> > + reg, val, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int n3000_nios_readl(struct dfl_n3000_nios *ns, unsigned int reg,
> > + unsigned int *val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_read(ns->regmap, reg, val);
> > + if (ret)
> > + dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t nios_fw_version_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
> > + unsigned int val;
> > + int ret;
>
> This and similar could be
>
> ssize_t ret.
I think it may not be necessary. It is not reasonable n3000_nios_readl()
has the return type ssize_t. So no matter how you define the ret, the
cast is always needed.
I see the general handling of this case in drivers is "int ret".
>
> > +
> > + ret = n3000_nios_readl(ns, 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);
> > +
> > +static ssize_t fec_mode_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
> > + unsigned int val, mode;
> > + int ret;
> > +
> > + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * FEC mode should always be the same for all links, as we set them
> > + * in this way.
> > + */
> > + mode = FIELD_GET(REQ_FEC_MODE_A0, val);
> > + if (mode != FIELD_GET(REQ_FEC_MODE_A1, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_A2, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_A3, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_B0, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_B1, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_B2, val) ||
> > + mode != FIELD_GET(REQ_FEC_MODE_B3, val))
> > + return -EFAULT;
> > +
> > + switch (mode) {
> > + case FEC_MODE_NO:
> > + return sprintf(buf, "no\n");
> > + case FEC_MODE_KR:
> > + return sprintf(buf, "kr\n");
> > + case 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_fec_mode.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(n3000_nios);
> > +
> > +static int init_error_detected(struct dfl_n3000_nios *ns)
> returning true/false, the return type should be boolean
Yes.
> > +{
> > + unsigned int val;
> > +
> > + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
> > + return true;
> > +
> > + if (val >= 0x100)
>
> A magic number like 0x100 should be #define-ed.
>
> similar below
Yes, I could improve this.
>
> > + return true;
> > +
> > + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
> > + return true;
> > +
> > + if (val >= 0x100)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static void dump_error_stat(struct dfl_n3000_nios *ns)
> > +{
> > + unsigned int val;
> > +
> > + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
> > + return;
>
> Not being able to read the register is itself and error.
>
> Should add something like 'Could not read register PKVL_A_MODE_STS'
>
> similar below
Yes, I could add the error log.
>
> > +
> > + dev_info(ns->dev, "PKVL_A_MODE_STS %x\n", val);
> > +
> > + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
> > + return;
> > +
> > + dev_info(ns->dev, "PKVL_B_MODE_STS %x\n", val);
> > +}
> > +
> > +static int n3000_nios_init_done_check(struct dfl_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 3.x.x version, INIT_START is introduced, so
> > + * driver needs to trigger START firstly and then check INIT_DONE.
> > + */
> > +
> > + ret = n3000_nios_readl(ns, 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. This is an
> > + * issue of FPGA image. We didn't error out because we need SPI master
> > + * to reprogram a new image.
> > + */
> > + if (val == 0) {
> > + dev_warn(dev, "NIOS version reg = 0x%x, skip INIT_DONE check, but PKVL may be uninitialized\n",
> > + val);
> > + return 0;
> > + }
> > +
> > + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
> > + /* read NIOS_INIT to check if PKVL INIT done or not */
> > + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
> > + if (ret)
> > + return ret;
> > +
> > + /* check if PKVLs 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;
> > +
> > + /* FEC mode will be ignored by hardware in 10G mode */
> > + if (!strcmp(fec_mode, "no"))
> strncmp is safer but would lead to 'no123' being valid.
We do use strcmp here, so are we OK here?
> > + mode = FEC_MODE_NO;
> > + else if (!strcmp(fec_mode, "kr"))
> > + mode = FEC_MODE_KR;
> > + else if (!strcmp(fec_mode, "rs"))
> > + mode = FEC_MODE_RS;
> > + else
> > + return -EINVAL;
> > +
> > + /* set the same FEC mode for all links */
> > + val |= FIELD_PREP(REQ_FEC_MODE_A0, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_A1, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_A2, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_A3, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_B0, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_B1, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_B2, mode) |
> > + FIELD_PREP(REQ_FEC_MODE_B3, mode);
> > +
> > + ret = n3000_nios_writel(ns, 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 ERR
> > + * detected.
> > + * We won't error out here even if error detected. Nios will release
> > + * spi controller when INIT_DONE is set, so driver could continue to
> > + * initialize spi controller device.
> > + */
> > + if (init_error_detected(ns)) {
> > + dev_warn(dev, "NIOS_INIT_DONE OK, but err found during init\n");
> > + goto dump_sts;
> > + }
> > + return 0;
> > +
> > +dump_sts:
> > + dump_error_stat(ns);
> > +
> > + return ret;
> > +}
> > +
> > +struct spi_board_info m10_n3000_info = {
> > + .modalias = "m10-n3000",
> > + .max_speed_hz = 12500000,
> > + .bus_num = 0,
> > + .chip_select = 0,
> > +};
> > +
> > +static int create_altr_spi_controller(struct dfl_n3000_nios *ns)
> Also here, altr -> altera
Yes
> > +{
> > + 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(CLK_POLARITY, v))
> > + pdata.mode_bits |= SPI_CPOL;
> > + if (FIELD_GET(CLK_PHASE, v))
> > + pdata.mode_bits |= SPI_CPHA;
> > +
> > + pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > + pdata.bits_per_word_mask =
> > + SPI_BPW_RANGE_MASK(1, FIELD_GET(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->altr_spi = platform_device_register_full(&pdevinfo);
> > + return PTR_ERR_OR_ZERO(ns->altr_spi);
> > +}
> > +
> > +static void destroy_altr_spi_controller(struct dfl_n3000_nios *ns)
> > +{
> > + platform_device_unregister(ns->altr_spi);
> > +}
> > +
> > +static int ns_bus_poll_stat_timeout(void __iomem *base, u64 *v)
> > +{
> > + int loops = NS_REGBUS_WAIT_TIMEOUT;
>
> Would not this be better as time based timeout ?
>
> Looping over int is only good if you know the clock freq etc.
Time based timeout is more reasonable, but it will drop the performance:
1. The indirect bus is accessed so frequently. It is on the critical
path of FPGA image flashing.
2. The state change time is very short, usually in 1 or 2 loops.
We've profiled that if we add time check here then, FPGA image flash
time would be several minutes longer.
Anyway, 10000 times loop is large enough.
>
> Could this polling be switch to interrupt ?
We don't have interrupt source here.
>
> > +
> > + /*
> > + * Usually the state changes in few loops, so we try to be simple here
> > + * for performance.
> > + */
> > + do {
> > + *v = readq(base + NIOS_SPI_STAT);
> > + if (*v & STAT_RW_VAL)
> > + break;
> > + cpu_relax();
> > + } while (--loops);
> > +
> > + return loops ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int ns_bus_reg_write(void *context, unsigned int reg, unsigned int val)
>
> what is 'ns_' ?
It is the abbreviation of nios_spi, is that OK?
I don't want a long prefix for everything, but still want a namespace.
>
> Maybe this should be 'nios_spi' or just drop the prefix.
>
> > +{
> > + void __iomem *base = context;
> > + u64 v = 0;
> > +
> > + v |= FIELD_PREP(CTRL_CMD, CMD_WR);
> > + v |= FIELD_PREP(CTRL_ADDR, reg);
> > + v |= FIELD_PREP(CTRL_WR_DATA, val);
> > + writeq(v, base + NIOS_SPI_CTRL);
> > +
> > + return ns_bus_poll_stat_timeout(base, &v);
> > +}
> > +
> > +static int ns_bus_reg_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > + void __iomem *base = context;
> > + u64 v = 0;
> > + int ret;
> > +
> > + v |= FIELD_PREP(CTRL_CMD, CMD_RD);
> > + v |= FIELD_PREP(CTRL_ADDR, reg);
> > + writeq(v, base + NIOS_SPI_CTRL);
> > +
> > + ret = ns_bus_poll_stat_timeout(base, &v);
> > + if (!ret)
> > + *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_bus_reg_write,
> > + .reg_read = ns_bus_reg_read,
> > +};
> > +
> > +static int dfl_n3000_nios_probe(struct dfl_device *dfl_dev)
> > +{
> > + struct device *dev = &dfl_dev->dev;
> > + struct dfl_n3000_nios *ns;
> > + int ret;
> > +
> > + ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
> > + if (!ns)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(&dfl_dev->dev, ns);
> > +
> > + ns->dev = dev;
> > +
> > + ns->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
> > + if (IS_ERR(ns->base)) {
> > + dev_err(dev, "get mem resource fail!\n");
> > + return PTR_ERR(ns->base);
> > + }
> > +
> > + ns->regmap = devm_regmap_init(dev, NULL, ns->base, &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_altr_spi_controller(ns);
> > + if (ret)
> > + dev_err(dev, "altr spi controller create failed: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void dfl_n3000_nios_remove(struct dfl_device *dfl_dev)
> > +{
> > + struct dfl_n3000_nios *ns = dev_get_drvdata(&dfl_dev->dev);
> > +
> > + destroy_altr_spi_controller(ns);
> > +}
> > +
> > +#define FME_FEATURE_ID_N3000_NIOS 0xd
>
> To minimize collisions, maybe collect these to a new common header like 'dfl-features-ids.h' or just add to some other visible existing header.
I'm going to abstract the dfl-bus related stuff in a new head file,
maybe we could change it when more dfl devices are added.
Thanks,
Yilun.
>
> > +
> > +static const struct dfl_device_id dfl_n3000_nios_ids[] = {
> > + { FME_ID, FME_FEATURE_ID_N3000_NIOS },
> > + { }
> > +};
> > +
> > +static struct dfl_driver dfl_n3000_nios_driver = {
> > + .drv = {
> > + .name = "dfl-n3000-nios",
> > + .dev_groups = n3000_nios_groups,
> > + },
> > + .id_table = dfl_n3000_nios_ids,
> > + .probe = dfl_n3000_nios_probe,
> > + .remove = dfl_n3000_nios_remove,
> > +};
> > +
> > +module_dfl_driver(dfl_n3000_nios_driver);
> > +
> > +MODULE_DEVICE_TABLE(dfl, dfl_n3000_nios_ids);
> > +MODULE_DESCRIPTION("DFL N3000 NIOS driver");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature
2020-07-31 8:56 ` Xu Yilun
@ 2020-08-01 15:35 ` Tom Rix
0 siblings, 0 replies; 15+ messages in thread
From: Tom Rix @ 2020-08-01 15:35 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
See below.
On 7/31/20 1:56 AM, Xu Yilun wrote:
> On Sat, Jul 25, 2020 at 07:53:59AM -0700, Tom Rix wrote:
>> Mostly little stuff.
>>
>> The polling on the int in ns_bus_poll_stat_timeout should be refactored.
>>
>> Tom
>>
>> On 7/23/20 7:09 PM, Xu Yilun wrote:
>>> This patch adds support for the nios handshake private feature on Intel
>>> N3000 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. When nios finished the
>>> configuration, host takes over the ownership of the SPI master to control
>>> an Intel MAX10 BMC Chip on the SPI bus.
>>>
>>> For NIOS firmware handshake part, this driver requests the retimer
>>> configuration for NIOS with parameters from module param, and adds some
>>> sysfs nodes for user to query NIOS state.
>>>
>>> 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 following the SPI work.
>>>
>>> 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>
>>> ---
>>> .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 16 +
>>> drivers/fpga/Kconfig | 12 +
>>> drivers/fpga/Makefile | 2 +
>>> drivers/fpga/dfl-n3000-nios.c | 483 +++++++++++++++++++++
>>> 4 files changed, 513 insertions(+)
>>> 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..8346b74
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios
>>> @@ -0,0 +1,16 @@
>>> +What: /sys/bus/dfl/devices/dfl-fme.X.X/fec_mode
>>> +Date: July 2020
>>> +KernelVersion: 5.9
>>> +Contact: Xu Yilun <yilun.xu@intel.com>
>>> +Description: Read-only. It returns the FEC mode of the ethernet retimer
>>> + configured by NIOS firmware. "rs" for RS FEC mode, "kr" for
>>> + KR FEC mode, "no" FOR NO FEC mode.
>>> + Format: string
>>> +
>>> +What: /sys/bus/dfl/devices/dfl-fme.X.X/nios_fw_version
>>> +Date: July 2020
>>> +KernelVersion: 5.9
>>> +Contact: Xu Yilun <yilun.xu@intel.com>
>>> +Description: Read-only. It returns the NIOS firmware version in FPGA. Its
>>> + format is "major.minor.patch".
>>> + Format: %x.%x.%x
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index b2408a7..fa5f3ab 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -191,6 +191,18 @@ 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_N3000_NIOS
>>> + tristate "FPGA DFL N3000 NIOS Driver"
>>> + depends on FPGA_DFL
>>> + select REGMAP
>> depends on SPI_MASTER ?
> I think we don't have to add this. The driver doesn't call any API
> provided by SPI core, just fills the spi_board_info structure.
ok
>
>>> + help
>>> + This is the driver for the nios handshake private feature on Intel
>>> + N3000 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. When nios finished
>>> + the configuration, host takes over the ownership of the SPI master to
>>> + control an Intel MAX10 BMC Chip on the SPI bus.
>>> +
>>> 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..27f20f2 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_N3000_NIOS) += 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..d098a10
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-n3000-nios.c
>>> @@ -0,0 +1,483 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Driver for DFL N3000 NIOS private feature
>>> + *
>>> + * 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/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>
>>> +
>>> +#include "dfl.h"
>>> +
>>> +static char *fec_mode = "rs";
>>> +module_param(fec_mode, charp, 0444);
>>> +MODULE_PARM_DESC(child, "FEC mode");
>> Should document what the valid values are and what they mean.
>>
>> ex/ what is 'rs' ?
>>
>> Maybe append something in the sysfs doc about fec_mode being settable.
> Yes, I'll add the documents.
>
>>> +
>>> +/* N3000 NIOS private feature registers */
>>> +#define NIOS_SPI_PARAM 0x8
>>> +#define SHIFT_MODE BIT_ULL(1)
>>> +#define SHIFT_MODE_MSB 0
>>> +#define SHIFT_MODE_LSB 1
>>> +#define DATA_WIDTH GENMASK_ULL(7, 2)
>>> +#define NUM_CHIPSELECT GENMASK_ULL(13, 8)
>>> +#define CLK_POLARITY BIT_ULL(14)
>>> +#define CLK_PHASE BIT_ULL(15)
>>> +#define 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 GENMASK_ULL(63, 62)
>>> +#define CMD_NOP 0
>>> +#define CMD_RD 1
>>> +#define CMD_WR 2
>> These are fairly generic #define names, consider adding a prefix.
> Yes.
>
>>> +
>>> +#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 PKVL A, link 0, the same below */
>>> +#define REQ_FEC_MODE_A0 GENMASK(9, 8)
>>> +#define REQ_FEC_MODE_A1 GENMASK(11, 10)
>>> +#define REQ_FEC_MODE_A2 GENMASK(13, 12)
>>> +#define REQ_FEC_MODE_A3 GENMASK(15, 14)
>>> +#define REQ_FEC_MODE_B0 GENMASK(17, 16)
>>> +#define REQ_FEC_MODE_B1 GENMASK(19, 18)
>>> +#define REQ_FEC_MODE_B2 GENMASK(21, 20)
>>> +#define REQ_FEC_MODE_B3 GENMASK(23, 22)
>>> +#define FEC_MODE_NO 0x0
>>> +#define FEC_MODE_KR 0x1
>>> +#define 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)
>>> +
>>> +#define PKVL_A_MODE_STS 0x1020
>>> +#define PKVL_B_MODE_STS 0x1024
>>> +
>>> +#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */
>>> +#define NIOS_INIT_TIMEOUT 10000000 /* usec */
>>> +#define NIOS_INIT_TIME_INTV 100000 /* usec */
>>> +
>>> +struct dfl_n3000_nios {
>>> + void __iomem *base;
>>> + struct regmap *regmap;
>>> + struct device *dev;
>>> + struct platform_device *altr_spi;
>> Make this element more meaningful, expand to 'altera_spi'
> Yes.
>
>>> +};
>>> +
>>> +static int n3000_nios_writel(struct dfl_n3000_nios *ns, unsigned int reg,
>>> + unsigned int val)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regmap_write(ns->regmap, reg, val);
>>> + if (ret)
>>> + dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n",
>>> + reg, val, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int n3000_nios_readl(struct dfl_n3000_nios *ns, unsigned int reg,
>>> + unsigned int *val)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regmap_read(ns->regmap, reg, val);
>>> + if (ret)
>>> + dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static ssize_t nios_fw_version_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
>>> + unsigned int val;
>>> + int ret;
>> This and similar could be
>>
>> ssize_t ret.
> I think it may not be necessary. It is not reasonable n3000_nios_readl()
> has the return type ssize_t. So no matter how you define the ret, the
> cast is always needed.
>
> I see the general handling of this case in drivers is "int ret".
Ok.
>
>>> +
>>> + ret = n3000_nios_readl(ns, 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);
>>> +
>>> +static ssize_t fec_mode_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev);
>>> + unsigned int val, mode;
>>> + int ret;
>>> +
>>> + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /*
>>> + * FEC mode should always be the same for all links, as we set them
>>> + * in this way.
>>> + */
>>> + mode = FIELD_GET(REQ_FEC_MODE_A0, val);
>>> + if (mode != FIELD_GET(REQ_FEC_MODE_A1, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_A2, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_A3, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_B0, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_B1, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_B2, val) ||
>>> + mode != FIELD_GET(REQ_FEC_MODE_B3, val))
>>> + return -EFAULT;
>>> +
>>> + switch (mode) {
>>> + case FEC_MODE_NO:
>>> + return sprintf(buf, "no\n");
>>> + case FEC_MODE_KR:
>>> + return sprintf(buf, "kr\n");
>>> + case 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_fec_mode.attr,
>>> + NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(n3000_nios);
>>> +
>>> +static int init_error_detected(struct dfl_n3000_nios *ns)
>> returning true/false, the return type should be boolean
> Yes.
>
>>> +{
>>> + unsigned int val;
>>> +
>>> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
>>> + return true;
>>> +
>>> + if (val >= 0x100)
>> A magic number like 0x100 should be #define-ed.
>>
>> similar below
> Yes, I could improve this.
>
>>> + return true;
>>> +
>>> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
>>> + return true;
>>> +
>>> + if (val >= 0x100)
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static void dump_error_stat(struct dfl_n3000_nios *ns)
>>> +{
>>> + unsigned int val;
>>> +
>>> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val))
>>> + return;
>> Not being able to read the register is itself and error.
>>
>> Should add something like 'Could not read register PKVL_A_MODE_STS'
>>
>> similar below
> Yes, I could add the error log.
>
>>> +
>>> + dev_info(ns->dev, "PKVL_A_MODE_STS %x\n", val);
>>> +
>>> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val))
>>> + return;
>>> +
>>> + dev_info(ns->dev, "PKVL_B_MODE_STS %x\n", val);
>>> +}
>>> +
>>> +static int n3000_nios_init_done_check(struct dfl_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 3.x.x version, INIT_START is introduced, so
>>> + * driver needs to trigger START firstly and then check INIT_DONE.
>>> + */
>>> +
>>> + ret = n3000_nios_readl(ns, 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. This is an
>>> + * issue of FPGA image. We didn't error out because we need SPI master
>>> + * to reprogram a new image.
>>> + */
>>> + if (val == 0) {
>>> + dev_warn(dev, "NIOS version reg = 0x%x, skip INIT_DONE check, but PKVL may be uninitialized\n",
>>> + val);
>>> + return 0;
>>> + }
>>> +
>>> + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) {
>>> + /* read NIOS_INIT to check if PKVL INIT done or not */
>>> + ret = n3000_nios_readl(ns, NIOS_INIT, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* check if PKVLs 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;
>>> +
>>> + /* FEC mode will be ignored by hardware in 10G mode */
>>> + if (!strcmp(fec_mode, "no"))
>> strncmp is safer but would lead to 'no123' being valid.
> We do use strcmp here, so are we OK here?
Yes.
>>> + mode = FEC_MODE_NO;
>>> + else if (!strcmp(fec_mode, "kr"))
>>> + mode = FEC_MODE_KR;
>>> + else if (!strcmp(fec_mode, "rs"))
>>> + mode = FEC_MODE_RS;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + /* set the same FEC mode for all links */
>>> + val |= FIELD_PREP(REQ_FEC_MODE_A0, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A1, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A2, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_A3, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B0, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B1, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B2, mode) |
>>> + FIELD_PREP(REQ_FEC_MODE_B3, mode);
>>> +
>>> + ret = n3000_nios_writel(ns, 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 ERR
>>> + * detected.
>>> + * We won't error out here even if error detected. Nios will release
>>> + * spi controller when INIT_DONE is set, so driver could continue to
>>> + * initialize spi controller device.
>>> + */
>>> + if (init_error_detected(ns)) {
>>> + dev_warn(dev, "NIOS_INIT_DONE OK, but err found during init\n");
>>> + goto dump_sts;
>>> + }
>>> + return 0;
>>> +
>>> +dump_sts:
>>> + dump_error_stat(ns);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +struct spi_board_info m10_n3000_info = {
>>> + .modalias = "m10-n3000",
>>> + .max_speed_hz = 12500000,
>>> + .bus_num = 0,
>>> + .chip_select = 0,
>>> +};
>>> +
>>> +static int create_altr_spi_controller(struct dfl_n3000_nios *ns)
>> Also here, altr -> altera
> Yes
>
>>> +{
>>> + 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(CLK_POLARITY, v))
>>> + pdata.mode_bits |= SPI_CPOL;
>>> + if (FIELD_GET(CLK_PHASE, v))
>>> + pdata.mode_bits |= SPI_CPHA;
>>> +
>>> + pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
>>> + pdata.bits_per_word_mask =
>>> + SPI_BPW_RANGE_MASK(1, FIELD_GET(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->altr_spi = platform_device_register_full(&pdevinfo);
>>> + return PTR_ERR_OR_ZERO(ns->altr_spi);
>>> +}
>>> +
>>> +static void destroy_altr_spi_controller(struct dfl_n3000_nios *ns)
>>> +{
>>> + platform_device_unregister(ns->altr_spi);
>>> +}
>>> +
>>> +static int ns_bus_poll_stat_timeout(void __iomem *base, u64 *v)
>>> +{
>>> + int loops = NS_REGBUS_WAIT_TIMEOUT;
>> Would not this be better as time based timeout ?
>>
>> Looping over int is only good if you know the clock freq etc.
> Time based timeout is more reasonable, but it will drop the performance:
> 1. The indirect bus is accessed so frequently. It is on the critical
> path of FPGA image flashing.
> 2. The state change time is very short, usually in 1 or 2 loops.
>
> We've profiled that if we add time check here then, FPGA image flash
> time would be several minutes longer.
>
> Anyway, 10000 times loop is large enough.
As this is unusual, please add a comment with this explanation.
>> Could this polling be switch to interrupt ?
> We don't have interrupt source here.
>
>>> +
>>> + /*
>>> + * Usually the state changes in few loops, so we try to be simple here
>>> + * for performance.
>>> + */
>>> + do {
>>> + *v = readq(base + NIOS_SPI_STAT);
>>> + if (*v & STAT_RW_VAL)
>>> + break;
>>> + cpu_relax();
>>> + } while (--loops);
>>> +
>>> + return loops ? 0 : -ETIMEDOUT;
>>> +}
>>> +
>>> +static int ns_bus_reg_write(void *context, unsigned int reg, unsigned int val)
>> what is 'ns_' ?
> It is the abbreviation of nios_spi, is that OK?
> I don't want a long prefix for everything, but still want a namespace.
Ok, then add a comment 'ns = nios_spi'
>> Maybe this should be 'nios_spi' or just drop the prefix.
>>
>>> +{
>>> + void __iomem *base = context;
>>> + u64 v = 0;
>>> +
>>> + v |= FIELD_PREP(CTRL_CMD, CMD_WR);
>>> + v |= FIELD_PREP(CTRL_ADDR, reg);
>>> + v |= FIELD_PREP(CTRL_WR_DATA, val);
>>> + writeq(v, base + NIOS_SPI_CTRL);
>>> +
>>> + return ns_bus_poll_stat_timeout(base, &v);
>>> +}
>>> +
>>> +static int ns_bus_reg_read(void *context, unsigned int reg, unsigned int *val)
>>> +{
>>> + void __iomem *base = context;
>>> + u64 v = 0;
>>> + int ret;
>>> +
>>> + v |= FIELD_PREP(CTRL_CMD, CMD_RD);
>>> + v |= FIELD_PREP(CTRL_ADDR, reg);
>>> + writeq(v, base + NIOS_SPI_CTRL);
>>> +
>>> + ret = ns_bus_poll_stat_timeout(base, &v);
>>> + if (!ret)
>>> + *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_bus_reg_write,
>>> + .reg_read = ns_bus_reg_read,
>>> +};
>>> +
>>> +static int dfl_n3000_nios_probe(struct dfl_device *dfl_dev)
>>> +{
>>> + struct device *dev = &dfl_dev->dev;
>>> + struct dfl_n3000_nios *ns;
>>> + int ret;
>>> +
>>> + ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL);
>>> + if (!ns)
>>> + return -ENOMEM;
>>> +
>>> + dev_set_drvdata(&dfl_dev->dev, ns);
>>> +
>>> + ns->dev = dev;
>>> +
>>> + ns->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
>>> + if (IS_ERR(ns->base)) {
>>> + dev_err(dev, "get mem resource fail!\n");
>>> + return PTR_ERR(ns->base);
>>> + }
>>> +
>>> + ns->regmap = devm_regmap_init(dev, NULL, ns->base, &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_altr_spi_controller(ns);
>>> + if (ret)
>>> + dev_err(dev, "altr spi controller create failed: %d\n", ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void dfl_n3000_nios_remove(struct dfl_device *dfl_dev)
>>> +{
>>> + struct dfl_n3000_nios *ns = dev_get_drvdata(&dfl_dev->dev);
>>> +
>>> + destroy_altr_spi_controller(ns);
>>> +}
>>> +
>>> +#define FME_FEATURE_ID_N3000_NIOS 0xd
>> To minimize collisions, maybe collect these to a new common header like 'dfl-features-ids.h' or just add to some other visible existing header.
> I'm going to abstract the dfl-bus related stuff in a new head file,
> maybe we could change it when more dfl devices are added.
Yes. that is fine.
Reviewed-by: Tom Rix <trix@redhat.com>
>
> Thanks,
> Yilun.
>
>>> +
>>> +static const struct dfl_device_id dfl_n3000_nios_ids[] = {
>>> + { FME_ID, FME_FEATURE_ID_N3000_NIOS },
>>> + { }
>>> +};
>>> +
>>> +static struct dfl_driver dfl_n3000_nios_driver = {
>>> + .drv = {
>>> + .name = "dfl-n3000-nios",
>>> + .dev_groups = n3000_nios_groups,
>>> + },
>>> + .id_table = dfl_n3000_nios_ids,
>>> + .probe = dfl_n3000_nios_probe,
>>> + .remove = dfl_n3000_nios_remove,
>>> +};
>>> +
>>> +module_dfl_driver(dfl_n3000_nios_driver);
>>> +
>>> +MODULE_DEVICE_TABLE(dfl, dfl_n3000_nios_ids);
>>> +MODULE_DESCRIPTION("DFL N3000 NIOS driver");
>>> +MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/4] Modularization of DFL private feature drivers
2020-07-24 2:09 [PATCH v2 0/4] Modularization of DFL private feature drivers Xu Yilun
` (3 preceding siblings ...)
2020-07-24 2:09 ` [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature Xu Yilun
@ 2020-07-24 3:03 ` Randy Dunlap
2020-07-24 3:30 ` Xu Yilun
4 siblings, 1 reply; 15+ messages in thread
From: Randy Dunlap @ 2020-07-24 3:03 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel; +Cc: trix, lgoncalv
On 7/23/20 7:09 PM, Xu Yilun wrote:
> This patchset makes it possible to develop independent driver modules
> for DFL private features. It also helps to leverage existing kernel
> drivers to enable some IP blocks in DFL.
>
> Patch #1: An improvement of feature id definition. The feature id will be used
> as the key field for dfl device/driver matching.
> Patch #2: Release the dfl mmio regions after enumeration, so that private
> feature drivers could request mmio region in their own drivers.
> Patch #3: Introduce the dfl bus, then dfl devices could be supported by
> independent dfl drivers.
> Patch #4: An example of the dfl driver for N3000 nios private feature.
>
Hi,
What is "nios"? Is that explained or described anywhere?
>
> Main changes from v1:
> - Add the new Patch #1, to improve the feature id definition.
> - Change the dfl bus uevent format.
> - Change the dfl device's sysfs name format.
> - refactor dfl_dev_add()
> - Add the Patch #4 as an example of the dfl driver.
> - A lot of minor fixes for comments from Hao and Tom.
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/4] Modularization of DFL private feature drivers
2020-07-24 3:03 ` [PATCH v2 0/4] Modularization of DFL private feature drivers Randy Dunlap
@ 2020-07-24 3:30 ` Xu Yilun
0 siblings, 0 replies; 15+ messages in thread
From: Xu Yilun @ 2020-07-24 3:30 UTC (permalink / raw)
To: Randy Dunlap; +Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv
On Thu, Jul 23, 2020 at 08:03:52PM -0700, Randy Dunlap wrote:
> On 7/23/20 7:09 PM, Xu Yilun wrote:
> > This patchset makes it possible to develop independent driver modules
> > for DFL private features. It also helps to leverage existing kernel
> > drivers to enable some IP blocks in DFL.
> >
> > Patch #1: An improvement of feature id definition. The feature id will be used
> > as the key field for dfl device/driver matching.
> > Patch #2: Release the dfl mmio regions after enumeration, so that private
> > feature drivers could request mmio region in their own drivers.
> > Patch #3: Introduce the dfl bus, then dfl devices could be supported by
> > independent dfl drivers.
> > Patch #4: An example of the dfl driver for N3000 nios private feature.
> >
>
> Hi,
> What is "nios"? Is that explained or described anywhere?
It is the NIOS2 soft processor mostly used in the FPGA. I see the there
is an arch/nios2 folder in kernel.
On Intel PAC N3000 card, the embedded NIOS2 core in FPGA does some
Board initialization work (Mostly the configuration of ethernet retimer)
on reboot. And the dfl-n3000-nios private feature in DFL is actually the
handshake interfaces for host to communicate with the NIOS2 core, about
what parameters to use, when the configuration is finished ...
Thanks,
Yilun
>
> >
> > Main changes from v1:
> > - Add the new Patch #1, to improve the feature id definition.
> > - Change the dfl bus uevent format.
> > - Change the dfl device's sysfs name format.
> > - refactor dfl_dev_add()
> > - Add the Patch #4 as an example of the dfl driver.
> > - A lot of minor fixes for comments from Hao and Tom.
>
> thanks.
> --
> ~Randy
^ permalink raw reply [flat|nested] 15+ messages in thread