| Message ID | 20220621123854.8686-1-tianfei.zhang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] memory: dfl-emif: Update the dfl emif driver support revision 1 | expand |
It does not seem like version 0 is being handled in all places, please review for backward compatibility. On 6/21/22 5:38 AM, Tianfei Zhang wrote: > From: Debarati Biswas <debaratix.biswas@intel.com> > > The next generation (revision 1) of the DFL EMIF feature device requires > support for more than 4 memory banks. It does not support the selective more than 4 or 8 ? below seems like 8. ver > 0 is not the same as ver == 1, I prefer the later > clearing of memory banks. A capability register replaces the previous > control register, and contains a bitmask to indicate the presence of each > memory bank. This bitmask aligns with the previous control register > bitmask that served the same purpose. The control and capability > registers are treated like a C Union structure in order to support both > the new and old revisions of the EMIF device. > > Signed-off-by: Debarati Biswas <debaratix.biswas@intel.com> > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com> > --- > drivers/memory/dfl-emif.c | 62 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 57 insertions(+), 5 deletions(-) > > diff --git a/drivers/memory/dfl-emif.c b/drivers/memory/dfl-emif.c > index 3f719816771d..da06cd30a016 100644 > --- a/drivers/memory/dfl-emif.c > +++ b/drivers/memory/dfl-emif.c > @@ -24,11 +24,24 @@ > #define EMIF_STAT_CLEAR_BUSY_SFT 16 > #define EMIF_CTRL 0x10 > #define EMIF_CTRL_CLEAR_EN_SFT 0 > -#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(3, 0) > +#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(7, 0) This would seem to be version specific and not a static value. ver 0 is (3,0) , ver > 0 (7,0) like what is done below. This #define is used in emif_clear_attr() but there is not version check in this macro. > > #define EMIF_POLL_INVL 10000 /* us */ > #define EMIF_POLL_TIMEOUT 5000000 /* us */ > > +/* > + * The Capability Register replaces the Control Register (at the same > + * offset) for EMIF feature revisions > 0. The bitmask that indicates > + * the presence of memory channels exists in both the Capability Register > + * and Control Register definitions. These can be thought of as a C union. > + * The Capability Register definitions are used to check for the existence > + * of a memory channel, and the Control Register definitions are used for > + * managing the memory-clear functionality in revision 0. > + */ > +#define EMIF_CAPABILITY_BASE 0x10 > +#define EMIF_CAPABILITY_CHN_MSK_V0 GENMASK_ULL(3, 0) > +#define EMIF_CAPABILITY_CHN_MSK GENMASK_ULL(7, 0) > + > struct dfl_emif { > struct device *dev; > void __iomem *base; > @@ -106,16 +119,30 @@ emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 0); > emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 1); > emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 2); > emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 3); > +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 4); > +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 5); > +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 6); > +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 7); > > emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 0); > emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 1); > emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 2); > emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 3); > +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 4); > +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 5); > +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 6); > +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 7); > + > > emif_clear_attr(0); > emif_clear_attr(1); > emif_clear_attr(2); > emif_clear_attr(3); > +emif_clear_attr(4); > +emif_clear_attr(5); > +emif_clear_attr(6); > +emif_clear_attr(7); > + > > static struct attribute *dfl_emif_attrs[] = { > &emif_attr_inf0_init_done.attr.attr, > @@ -134,6 +161,22 @@ static struct attribute *dfl_emif_attrs[] = { > &emif_attr_inf3_cal_fail.attr.attr, > &emif_attr_inf3_clear.attr.attr, > > + &emif_attr_inf4_init_done.attr.attr, > + &emif_attr_inf4_cal_fail.attr.attr, > + &emif_attr_inf4_clear.attr.attr, > + > + &emif_attr_inf5_init_done.attr.attr, > + &emif_attr_inf5_cal_fail.attr.attr, > + &emif_attr_inf5_clear.attr.attr, > + > + &emif_attr_inf6_init_done.attr.attr, > + &emif_attr_inf6_cal_fail.attr.attr, > + &emif_attr_inf6_clear.attr.attr, > + > + &emif_attr_inf7_init_done.attr.attr, > + &emif_attr_inf7_cal_fail.attr.attr, > + &emif_attr_inf7_clear.attr.attr, > + > NULL, > }; > > @@ -143,15 +186,24 @@ static umode_t dfl_emif_visible(struct kobject *kobj, > struct dfl_emif *de = dev_get_drvdata(kobj_to_dev(kobj)); > struct emif_attr *eattr = container_of(attr, struct emif_attr, > attr.attr); > + struct dfl_device *ddev = to_dfl_dev(de->dev); > u64 val; > > /* > - * This device supports upto 4 memory interfaces, but not all > + * This device supports up to 8 memory interfaces, but not all > * interfaces are used on different platforms. The read out value of > - * CLEAN_EN field (which is a bitmap) could tell how many interfaces > - * are available. > + * CAPABILITY_CHN_MSK field (which is a bitmap) indicates which > + * interfaces are available. > */ > - val = FIELD_GET(EMIF_CTRL_CLEAR_EN_MSK, readq(de->base + EMIF_CTRL)); > + if (ddev->revision > 0 && strstr(attr->name, "_clear")) This check does not match to comment, why is this needed ? Tom > + return 0; > + > + if (ddev->revision == 0) > + val = FIELD_GET(EMIF_CAPABILITY_CHN_MSK_V0, > + readq(de->base + EMIF_CAPABILITY_BASE)); > + else > + val = FIELD_GET(EMIF_CAPABILITY_CHN_MSK, > + readq(de->base + EMIF_CAPABILITY_BASE)); > > return (val & BIT_ULL(eattr->index)) ? attr->mode : 0; > }
diff --git a/drivers/memory/dfl-emif.c b/drivers/memory/dfl-emif.c index 3f719816771d..da06cd30a016 100644 --- a/drivers/memory/dfl-emif.c +++ b/drivers/memory/dfl-emif.c @@ -24,11 +24,24 @@ #define EMIF_STAT_CLEAR_BUSY_SFT 16 #define EMIF_CTRL 0x10 #define EMIF_CTRL_CLEAR_EN_SFT 0 -#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(3, 0) +#define EMIF_CTRL_CLEAR_EN_MSK GENMASK_ULL(7, 0) #define EMIF_POLL_INVL 10000 /* us */ #define EMIF_POLL_TIMEOUT 5000000 /* us */ +/* + * The Capability Register replaces the Control Register (at the same + * offset) for EMIF feature revisions > 0. The bitmask that indicates + * the presence of memory channels exists in both the Capability Register + * and Control Register definitions. These can be thought of as a C union. + * The Capability Register definitions are used to check for the existence + * of a memory channel, and the Control Register definitions are used for + * managing the memory-clear functionality in revision 0. + */ +#define EMIF_CAPABILITY_BASE 0x10 +#define EMIF_CAPABILITY_CHN_MSK_V0 GENMASK_ULL(3, 0) +#define EMIF_CAPABILITY_CHN_MSK GENMASK_ULL(7, 0) + struct dfl_emif { struct device *dev; void __iomem *base; @@ -106,16 +119,30 @@ emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 0); emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 1); emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 2); emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 3); +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 4); +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 5); +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 6); +emif_state_attr(init_done, EMIF_STAT_INIT_DONE_SFT, 7); emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 0); emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 1); emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 2); emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 3); +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 4); +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 5); +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 6); +emif_state_attr(cal_fail, EMIF_STAT_CALC_FAIL_SFT, 7); + emif_clear_attr(0); emif_clear_attr(1); emif_clear_attr(2); emif_clear_attr(3); +emif_clear_attr(4); +emif_clear_attr(5); +emif_clear_attr(6); +emif_clear_attr(7); + static struct attribute *dfl_emif_attrs[] = { &emif_attr_inf0_init_done.attr.attr, @@ -134,6 +161,22 @@ static struct attribute *dfl_emif_attrs[] = { &emif_attr_inf3_cal_fail.attr.attr, &emif_attr_inf3_clear.attr.attr, + &emif_attr_inf4_init_done.attr.attr, + &emif_attr_inf4_cal_fail.attr.attr, + &emif_attr_inf4_clear.attr.attr, + + &emif_attr_inf5_init_done.attr.attr, + &emif_attr_inf5_cal_fail.attr.attr, + &emif_attr_inf5_clear.attr.attr, + + &emif_attr_inf6_init_done.attr.attr, + &emif_attr_inf6_cal_fail.attr.attr, + &emif_attr_inf6_clear.attr.attr, + + &emif_attr_inf7_init_done.attr.attr, + &emif_attr_inf7_cal_fail.attr.attr, + &emif_attr_inf7_clear.attr.attr, + NULL, }; @@ -143,15 +186,24 @@ static umode_t dfl_emif_visible(struct kobject *kobj, struct dfl_emif *de = dev_get_drvdata(kobj_to_dev(kobj)); struct emif_attr *eattr = container_of(attr, struct emif_attr, attr.attr); + struct dfl_device *ddev = to_dfl_dev(de->dev); u64 val; /* - * This device supports upto 4 memory interfaces, but not all + * This device supports up to 8 memory interfaces, but not all * interfaces are used on different platforms. The read out value of - * CLEAN_EN field (which is a bitmap) could tell how many interfaces - * are available. + * CAPABILITY_CHN_MSK field (which is a bitmap) indicates which + * interfaces are available. */ - val = FIELD_GET(EMIF_CTRL_CLEAR_EN_MSK, readq(de->base + EMIF_CTRL)); + if (ddev->revision > 0 && strstr(attr->name, "_clear")) + return 0; + + if (ddev->revision == 0) + val = FIELD_GET(EMIF_CAPABILITY_CHN_MSK_V0, + readq(de->base + EMIF_CAPABILITY_BASE)); + else + val = FIELD_GET(EMIF_CAPABILITY_CHN_MSK, + readq(de->base + EMIF_CAPABILITY_BASE)); return (val & BIT_ULL(eattr->index)) ? attr->mode : 0; }