| Message ID | 20220822170510.125082-1-a.manzanares@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Replace HDM decoder granularity magic numbers | expand |
On Mon, 22 Aug 2022, Adam Manzanares wrote: >When reviewing the CFMWS parsing code that deals with the HDM decoders, >I noticed a couple of magic numbers. This commit replaces these magic numbers >with constants defined by the CXL 2.0 specification. Please use 3.0 spec :) Actually the whole drivers/cxl/* could use updating the comments for 3.0. >Signed-off-by: Adam Manzanares <a.manzanares@samsung.com> >--- > drivers/cxl/cxl.h | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > >diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >index f680450f0b16..ba3a66b5b9cd 100644 >--- a/drivers/cxl/cxl.h >+++ b/drivers/cxl/cxl.h >@@ -61,6 +61,10 @@ > #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) > #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) > >+/* HDM decoder control register constants CXL 2.0 8.2.5.12.7 */ This now becomes 8.2.4.19.7. >+#define CXL_DECODER_MIN_GRANULARITY 256 >+#define CXL_DECODER_MAX_GRANULARITY_ORDER 6 Considering there is a single user, I don't think we need to add the CXL_DECODER_MAX_GRANULARITY_ORDER. And if a new one is added it would be better to keep symmetry (bytes) and just do CXL_DECODER_MAX_GRANULARITY, but that's probably the reason why it doesn't exist in the first place. >+ > static inline int cxl_hdm_decoder_count(u32 cap_hdr) > { > int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); >@@ -71,9 +75,9 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr) > /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ > static inline int cxl_to_granularity(u16 ig, unsigned int *val) > { >- if (ig > 6) >+ if (ig > CXL_DECODER_MAX_GRANULARITY_ORDER) > return -EINVAL; >- *val = 256 << ig; >+ *val = CXL_DECODER_MIN_GRANULARITY << ig; > return 0; > } > >@@ -96,7 +100,7 @@ static inline int cxl_to_ways(u8 eniw, unsigned int *val) > > static inline int granularity_to_cxl(int g, u16 *ig) > { >- if (g > SZ_16K || g < 256 || !is_power_of_2(g)) >+ if (g > SZ_16K || g < CXL_DECODER_MIN_GRANULARITY || !is_power_of_2(g)) Looks good. Thanks, Davidlohr
Adam Manzanares wrote: > When reviewing the CFMWS parsing code that deals with the HDM decoders, > I noticed a couple of magic numbers. This commit replaces these magic numbers > with constants defined by the CXL 2.0 specification. I have a small quibble with CXL_DECODER_MAX_GRANULARITY_ORDER. How about CXL_DECODER_MAX_ENCODED_IG? Because the value is not the 'power of 2 order' it's the 'power of 2 order - 8'.
On Mon, 22 Aug 2022 10:17:03 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote: > On Mon, 22 Aug 2022, Adam Manzanares wrote: > > >When reviewing the CFMWS parsing code that deals with the HDM decoders, > >I noticed a couple of magic numbers. This commit replaces these magic numbers > >with constants defined by the CXL 2.0 specification. > > Please use 3.0 spec :) > > Actually the whole drivers/cxl/* could use updating the comments for 3.0. Interesting point. What do we want to do on this? Most similar cases I've been involved on rely on referring to 'oldest' compatible spec. (this is true for ACPI stuff for example). I don't care either way, but a policy on this will save us some time by ensuring we meet that policy before sending for review. Jonathan
On Wed, 24 Aug 2022, Jonathan Cameron wrote: >On Mon, 22 Aug 2022 10:17:03 -0700 >Davidlohr Bueso <dave@stgolabs.net> wrote: >> Actually the whole drivers/cxl/* could use updating the comments for 3.0. > >Interesting point. What do we want to do on this? Most similar >cases I've been involved on rely on referring to 'oldest' compatible spec. >(this is true for ACPI stuff for example). I find it incredibly annoying to reference table or section numbers from old specs mention in the code. Unless dealing with specific version things, why use old specs at all? The main drawback to this is obviously the constant need to update everything, so... Thanks, Davidlohr
Davidlohr Bueso wrote: > On Wed, 24 Aug 2022, Jonathan Cameron wrote: > > >On Mon, 22 Aug 2022 10:17:03 -0700 > >Davidlohr Bueso <dave@stgolabs.net> wrote: > > >> Actually the whole drivers/cxl/* could use updating the comments for 3.0. > > > >Interesting point. What do we want to do on this? Most similar > >cases I've been involved on rely on referring to 'oldest' compatible spec. > >(this is true for ACPI stuff for example). > > I find it incredibly annoying to reference table or section numbers from old > specs mention in the code. Unless dealing with specific version things, why > use old specs at all? > > The main drawback to this is obviously the constant need to update everything, > so... I think a happy medium is all new references being 3.0 and touching any old references and refresh them up to 3.0. That said if someone wants to do the work to refresh all of them, and someone else wants to review those updates I would take the patch.
On Wed, Aug 24, 2022 at 09:17:44AM -0700, Dan Williams wrote: > Davidlohr Bueso wrote: > > On Wed, 24 Aug 2022, Jonathan Cameron wrote: > > > > >On Mon, 22 Aug 2022 10:17:03 -0700 > > >Davidlohr Bueso <dave@stgolabs.net> wrote: > > > > >> Actually the whole drivers/cxl/* could use updating the comments for 3.0. > > > > > >Interesting point. What do we want to do on this? Most similar > > >cases I've been involved on rely on referring to 'oldest' compatible spec. > > >(this is true for ACPI stuff for example). > > > > I find it incredibly annoying to reference table or section numbers from old > > specs mention in the code. Unless dealing with specific version things, why > > use old specs at all? > > > > The main drawback to this is obviously the constant need to update everything, > > so... > > I think a happy medium is all new references being 3.0 and touching any > old references and refresh them up to 3.0. I'll shoot for the happy medium in a v2. > > That said if someone wants to do the work to refresh all of them, and > someone else wants to review those updates I would take the patch.
On Mon, Aug 22, 2022 at 10:34:46AM -0700, Dan Williams wrote: > Adam Manzanares wrote: > > When reviewing the CFMWS parsing code that deals with the HDM decoders, > > I noticed a couple of magic numbers. This commit replaces these magic numbers > > with constants defined by the CXL 2.0 specification. > > I have a small quibble with CXL_DECODER_MAX_GRANULARITY_ORDER. How about > CXL_DECODER_MAX_ENCODED_IG? Because the value is not the 'power of 2 > order' it's the 'power of 2 order - 8'. Sounds reasonable to me. I will also make this change in v2.
On Mon, Aug 22, 2022 at 10:17:03AM -0700, Davidlohr Bueso wrote: > On Mon, 22 Aug 2022, Adam Manzanares wrote: > > > When reviewing the CFMWS parsing code that deals with the HDM decoders, > > I noticed a couple of magic numbers. This commit replaces these magic numbers > > with constants defined by the CXL 2.0 specification. > > Please use 3.0 spec :) > > Actually the whole drivers/cxl/* could use updating the comments for 3.0. > > > Signed-off-by: Adam Manzanares <a.manzanares@samsung.com> > > --- > > drivers/cxl/cxl.h | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index f680450f0b16..ba3a66b5b9cd 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -61,6 +61,10 @@ > > #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) > > #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) > > > > +/* HDM decoder control register constants CXL 2.0 8.2.5.12.7 */ > > This now becomes 8.2.4.19.7. > > > +#define CXL_DECODER_MIN_GRANULARITY 256 > > +#define CXL_DECODER_MAX_GRANULARITY_ORDER 6 > > Considering there is a single user, I don't think we need to add > the CXL_DECODER_MAX_GRANULARITY_ORDER. And if a new one is added > it would be better to keep symmetry (bytes) and just do > CXL_DECODER_MAX_GRANULARITY, but that's probably the reason why > it doesn't exist in the first place. I wasn't so worried about the single user. I was hoping to give some additional context for the constants used in the granularity calculations. Given Dans suggestion for the name change are you more comfortable with the proposed change. If not, feel free to suggest an alternative to eliminate the magic number. > > > + > > static inline int cxl_hdm_decoder_count(u32 cap_hdr) > > { > > int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); > > @@ -71,9 +75,9 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr) > > /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ > > static inline int cxl_to_granularity(u16 ig, unsigned int *val) > > { > > - if (ig > 6) > > + if (ig > CXL_DECODER_MAX_GRANULARITY_ORDER) > > return -EINVAL; > > - *val = 256 << ig; > > + *val = CXL_DECODER_MIN_GRANULARITY << ig; > > return 0; > > } > > > > @@ -96,7 +100,7 @@ static inline int cxl_to_ways(u8 eniw, unsigned int *val) > > > > static inline int granularity_to_cxl(int g, u16 *ig) > > { > > - if (g > SZ_16K || g < 256 || !is_power_of_2(g)) > > + if (g > SZ_16K || g < CXL_DECODER_MIN_GRANULARITY || !is_power_of_2(g)) > > Looks good. > > Thanks, > Davidlohr
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index f680450f0b16..ba3a66b5b9cd 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -61,6 +61,10 @@ #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i) #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i) +/* HDM decoder control register constants CXL 2.0 8.2.5.12.7 */ +#define CXL_DECODER_MIN_GRANULARITY 256 +#define CXL_DECODER_MAX_GRANULARITY_ORDER 6 + static inline int cxl_hdm_decoder_count(u32 cap_hdr) { int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr); @@ -71,9 +75,9 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr) /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */ static inline int cxl_to_granularity(u16 ig, unsigned int *val) { - if (ig > 6) + if (ig > CXL_DECODER_MAX_GRANULARITY_ORDER) return -EINVAL; - *val = 256 << ig; + *val = CXL_DECODER_MIN_GRANULARITY << ig; return 0; } @@ -96,7 +100,7 @@ static inline int cxl_to_ways(u8 eniw, unsigned int *val) static inline int granularity_to_cxl(int g, u16 *ig) { - if (g > SZ_16K || g < 256 || !is_power_of_2(g)) + if (g > SZ_16K || g < CXL_DECODER_MIN_GRANULARITY || !is_power_of_2(g)) return -EINVAL; *ig = ilog2(g) - 8; return 0; @@ -248,7 +252,6 @@ enum cxl_decoder_type { */ #define CXL_DECODER_MAX_INTERLEAVE 16 -#define CXL_DECODER_MIN_GRANULARITY 256 /** * struct cxl_decoder - Common CXL HDM Decoder Attributes
When reviewing the CFMWS parsing code that deals with the HDM decoders, I noticed a couple of magic numbers. This commit replaces these magic numbers with constants defined by the CXL 2.0 specification. Signed-off-by: Adam Manzanares <a.manzanares@samsung.com> --- drivers/cxl/cxl.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)