| Message ID | 166077130837.1743055.16772443540776610507.stgit@djiang5-desk4.jf.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add sanity check for interleave setup | expand |
On Wed, 17 Aug 2022 14:21:48 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register. rev3.0 - though I don't really care that much... Dropping the v works too. CXL 3.0 is fine by me. > CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave > is capable. Bit 12 indicates that 16 way interleave is capable. > > Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in > cxl_interleave_verify() call to make sure those CAP bits matches the passed > in interleave value. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> One comment on naming inline, but other than that bikeshedding. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/hdm.c | 6 ++++++ > drivers/cxl/core/region.c | 3 +++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/cxlmem.h | 5 +++++ > 4 files changed, 16 insertions(+) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index d1d2caea5c62..2f91ff9b0227 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -80,6 +80,12 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > cxlhdm->interleave_mask |= GENMASK(11, 8); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > cxlhdm->interleave_mask |= GENMASK(14, 12); > + > + cxlhdm->interleave_cap = CXL_HDM_INTERLEAVE_CAP_DEFAULT; DEFAULT is somewhat odd naming for a capability value. BASELINE maybe? > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) > + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_3_6_12; > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) > + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_16; > } > > static void __iomem *map_hdm_decoder_regs(struct cxl_port *port, > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 28272b0196e6..9851ab2782f2 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -960,6 +960,9 @@ static int cxl_interleave_capable(struct cxl_port *port, struct device *dev, > if (eiw == 0) > return 0; > > + if (!test_bit(ways, &cxlhdm->interleave_cap)) > + return -EINVAL; > + > if (is_power_of_2(eiw)) > addr_mask = GENMASK(eig + 8 + eiw - 1, eig + 8); > else > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f680450f0b16..11f2a14f42eb 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -42,6 +42,8 @@ > #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) > #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) > +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) > +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) > #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 > #define CXL_HDM_DECODER_ENABLE BIT(1) > #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 88e3a8e54b6a..4e65c9cc1d30 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -393,11 +393,16 @@ static inline void cxl_mem_active_dec(void) > } > #endif > > +#define CXL_HDM_INTERLEAVE_CAP_DEFAULT BIT(1) | BIT(2) | BIT(4) | BIT(8) > +#define CXL_HDM_INTERLEAVE_CAP_3_6_12 BIT(3) | BIT(6) | BIT(12) > +#define CXL_HDM_INTERLEAVE_CAP_16 BIT(16) > + > struct cxl_hdm { > struct cxl_component_regs regs; > unsigned int decoder_count; > unsigned int target_count; > unsigned int interleave_mask; > + unsigned long interleave_cap; > struct cxl_port *port; > }; > > >
On Wed, 17 Aug 2022 14:21:48 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register. > CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave > is capable. Bit 12 indicates that 16 way interleave is capable. > > Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in > cxl_interleave_verify() call to make sure those CAP bits matches the passed Whilst looking at mocking patch, I noticed there isn't a cxl_interleave_verify() renamed? Looks like that was in v2 from your change log. > in interleave value. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/hdm.c | 6 ++++++ > drivers/cxl/core/region.c | 3 +++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/cxlmem.h | 5 +++++ > 4 files changed, 16 insertions(+) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index d1d2caea5c62..2f91ff9b0227 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -80,6 +80,12 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > cxlhdm->interleave_mask |= GENMASK(11, 8); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) > cxlhdm->interleave_mask |= GENMASK(14, 12); > + > + cxlhdm->interleave_cap = CXL_HDM_INTERLEAVE_CAP_DEFAULT; > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) > + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_3_6_12; > + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) > + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_16; > } > > static void __iomem *map_hdm_decoder_regs(struct cxl_port *port, > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 28272b0196e6..9851ab2782f2 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -960,6 +960,9 @@ static int cxl_interleave_capable(struct cxl_port *port, struct device *dev, > if (eiw == 0) > return 0; > > + if (!test_bit(ways, &cxlhdm->interleave_cap)) > + return -EINVAL; > + > if (is_power_of_2(eiw)) > addr_mask = GENMASK(eig + 8 + eiw - 1, eig + 8); > else > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index f680450f0b16..11f2a14f42eb 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -42,6 +42,8 @@ > #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) > #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) > #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) > +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) > +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) > #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 > #define CXL_HDM_DECODER_ENABLE BIT(1) > #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 88e3a8e54b6a..4e65c9cc1d30 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -393,11 +393,16 @@ static inline void cxl_mem_active_dec(void) > } > #endif > > +#define CXL_HDM_INTERLEAVE_CAP_DEFAULT BIT(1) | BIT(2) | BIT(4) | BIT(8) > +#define CXL_HDM_INTERLEAVE_CAP_3_6_12 BIT(3) | BIT(6) | BIT(12) > +#define CXL_HDM_INTERLEAVE_CAP_16 BIT(16) > + > struct cxl_hdm { > struct cxl_component_regs regs; > unsigned int decoder_count; > unsigned int target_count; > unsigned int interleave_mask; > + unsigned long interleave_cap; > struct cxl_port *port; > }; > > >
On 8/24/2022 7:46 AM, Jonathan Cameron wrote: > On Wed, 17 Aug 2022 14:21:48 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register. > rev3.0 - though I don't really care that much... Dropping the v works too. > CXL 3.0 is fine by me. I'll make it rev3.0 throughout. >> CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave >> is capable. Bit 12 indicates that 16 way interleave is capable. >> >> Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in >> cxl_interleave_verify() call to make sure those CAP bits matches the passed >> in interleave value. >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > One comment on naming inline, but other than that bikeshedding. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> --- >> drivers/cxl/core/hdm.c | 6 ++++++ >> drivers/cxl/core/region.c | 3 +++ >> drivers/cxl/cxl.h | 2 ++ >> drivers/cxl/cxlmem.h | 5 +++++ >> 4 files changed, 16 insertions(+) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index d1d2caea5c62..2f91ff9b0227 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -80,6 +80,12 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) >> cxlhdm->interleave_mask |= GENMASK(11, 8); >> if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) >> cxlhdm->interleave_mask |= GENMASK(14, 12); >> + >> + cxlhdm->interleave_cap = CXL_HDM_INTERLEAVE_CAP_DEFAULT; > DEFAULT is somewhat odd naming for a capability value. > BASELINE maybe? Ok will change to BASELINE. > >> + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) >> + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_3_6_12; >> + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) >> + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_16; >> } >> >> static void __iomem *map_hdm_decoder_regs(struct cxl_port *port, >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 28272b0196e6..9851ab2782f2 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -960,6 +960,9 @@ static int cxl_interleave_capable(struct cxl_port *port, struct device *dev, >> if (eiw == 0) >> return 0; >> >> + if (!test_bit(ways, &cxlhdm->interleave_cap)) >> + return -EINVAL; >> + >> if (is_power_of_2(eiw)) >> addr_mask = GENMASK(eig + 8 + eiw - 1, eig + 8); >> else >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index f680450f0b16..11f2a14f42eb 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -42,6 +42,8 @@ >> #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) >> #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) >> #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) >> +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) >> +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) >> #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 >> #define CXL_HDM_DECODER_ENABLE BIT(1) >> #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h >> index 88e3a8e54b6a..4e65c9cc1d30 100644 >> --- a/drivers/cxl/cxlmem.h >> +++ b/drivers/cxl/cxlmem.h >> @@ -393,11 +393,16 @@ static inline void cxl_mem_active_dec(void) >> } >> #endif >> >> +#define CXL_HDM_INTERLEAVE_CAP_DEFAULT BIT(1) | BIT(2) | BIT(4) | BIT(8) >> +#define CXL_HDM_INTERLEAVE_CAP_3_6_12 BIT(3) | BIT(6) | BIT(12) >> +#define CXL_HDM_INTERLEAVE_CAP_16 BIT(16) >> + >> struct cxl_hdm { >> struct cxl_component_regs regs; >> unsigned int decoder_count; >> unsigned int target_count; >> unsigned int interleave_mask; >> + unsigned long interleave_cap; >> struct cxl_port *port; >> }; >> >> >>
On 8/24/2022 7:56 AM, Jonathan Cameron wrote: > On Wed, 17 Aug 2022 14:21:48 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register. >> CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave >> is capable. Bit 12 indicates that 16 way interleave is capable. >> >> Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in >> cxl_interleave_verify() call to make sure those CAP bits matches the passed > Whilst looking at mocking patch, I noticed there isn't a cxl_interleave_verify() > renamed? Looks like that was in v2 from your change log. I need to fixup the commit headers all to cxl_interleave_capable(). Forgot about that. > >> in interleave value. >> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/cxl/core/hdm.c | 6 ++++++ >> drivers/cxl/core/region.c | 3 +++ >> drivers/cxl/cxl.h | 2 ++ >> drivers/cxl/cxlmem.h | 5 +++++ >> 4 files changed, 16 insertions(+) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index d1d2caea5c62..2f91ff9b0227 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -80,6 +80,12 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) >> cxlhdm->interleave_mask |= GENMASK(11, 8); >> if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) >> cxlhdm->interleave_mask |= GENMASK(14, 12); >> + >> + cxlhdm->interleave_cap = CXL_HDM_INTERLEAVE_CAP_DEFAULT; >> + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) >> + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_3_6_12; >> + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) >> + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_16; >> } >> >> static void __iomem *map_hdm_decoder_regs(struct cxl_port *port, >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 28272b0196e6..9851ab2782f2 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -960,6 +960,9 @@ static int cxl_interleave_capable(struct cxl_port *port, struct device *dev, >> if (eiw == 0) >> return 0; >> >> + if (!test_bit(ways, &cxlhdm->interleave_cap)) >> + return -EINVAL; >> + >> if (is_power_of_2(eiw)) >> addr_mask = GENMASK(eig + 8 + eiw - 1, eig + 8); >> else >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index f680450f0b16..11f2a14f42eb 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -42,6 +42,8 @@ >> #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) >> #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) >> #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) >> +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) >> +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) >> #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 >> #define CXL_HDM_DECODER_ENABLE BIT(1) >> #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h >> index 88e3a8e54b6a..4e65c9cc1d30 100644 >> --- a/drivers/cxl/cxlmem.h >> +++ b/drivers/cxl/cxlmem.h >> @@ -393,11 +393,16 @@ static inline void cxl_mem_active_dec(void) >> } >> #endif >> >> +#define CXL_HDM_INTERLEAVE_CAP_DEFAULT BIT(1) | BIT(2) | BIT(4) | BIT(8) >> +#define CXL_HDM_INTERLEAVE_CAP_3_6_12 BIT(3) | BIT(6) | BIT(12) >> +#define CXL_HDM_INTERLEAVE_CAP_16 BIT(16) >> + >> struct cxl_hdm { >> struct cxl_component_regs regs; >> unsigned int decoder_count; >> unsigned int target_count; >> unsigned int interleave_mask; >> + unsigned long interleave_cap; >> struct cxl_port *port; >> }; >> >> >>
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index d1d2caea5c62..2f91ff9b0227 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -80,6 +80,12 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) cxlhdm->interleave_mask |= GENMASK(11, 8); if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap)) cxlhdm->interleave_mask |= GENMASK(14, 12); + + cxlhdm->interleave_cap = CXL_HDM_INTERLEAVE_CAP_DEFAULT; + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap)) + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_3_6_12; + if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap)) + cxlhdm->interleave_cap |= CXL_HDM_INTERLEAVE_CAP_16; } static void __iomem *map_hdm_decoder_regs(struct cxl_port *port, diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 28272b0196e6..9851ab2782f2 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -960,6 +960,9 @@ static int cxl_interleave_capable(struct cxl_port *port, struct device *dev, if (eiw == 0) return 0; + if (!test_bit(ways, &cxlhdm->interleave_cap)) + return -EINVAL; + if (is_power_of_2(eiw)) addr_mask = GENMASK(eig + 8 + eiw - 1, eig + 8); else diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f680450f0b16..11f2a14f42eb 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -42,6 +42,8 @@ #define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4) #define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8) #define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9) +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11) +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12) #define CXL_HDM_DECODER_CTRL_OFFSET 0x4 #define CXL_HDM_DECODER_ENABLE BIT(1) #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10) diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 88e3a8e54b6a..4e65c9cc1d30 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -393,11 +393,16 @@ static inline void cxl_mem_active_dec(void) } #endif +#define CXL_HDM_INTERLEAVE_CAP_DEFAULT BIT(1) | BIT(2) | BIT(4) | BIT(8) +#define CXL_HDM_INTERLEAVE_CAP_3_6_12 BIT(3) | BIT(6) | BIT(12) +#define CXL_HDM_INTERLEAVE_CAP_16 BIT(16) + struct cxl_hdm { struct cxl_component_regs regs; unsigned int decoder_count; unsigned int target_count; unsigned int interleave_mask; + unsigned long interleave_cap; struct cxl_port *port; };