[PATCH] cxl: Replace HDM decoder granularity magic numbers

[PATCH] cxl: Replace HDM decoder granularity magic numbers

[Adam Manzanares]

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(-)

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
-- 
2.35.1

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Davidlohr Bueso]

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

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Jonathan Cameron]

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

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Davidlohr Bueso]

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

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Dan Williams]

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.

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Adam Manzanares]

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.

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Adam Manzanares]

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

RE: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Dan Williams]

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'.

Re: [PATCH] cxl: Replace HDM decoder granularity magic numbers

[Adam Manzanares]

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.