diff mbox series

[v2,7/8] cxl: Add emulation when HDM decoders are not committed

Message ID 167330064247.975161.16867413974628215063.stgit@djiang5-mobl3.local
State Superseded
Headers show
Series cxl: Introduce HDM decoder emulation from DVSEC range registers | expand

Commit Message

Dave Jiang Jan. 9, 2023, 9:44 p.m. UTC 
For the case where DVSEC range register(s) are active and HDM decoders are
not committed, use RR to provide emulation. A first pass is done to note
whether any decoders are committed. If there are no committed endpoint
decoders, then DVSEC ranges will be used for emulation.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
 drivers/cxl/cxl.h      |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

Comments

Jonathan Cameron Jan. 13, 2023, 2:07 p.m. UTC | #1 
On Mon, 09 Jan 2023 14:44:03 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> For the case where DVSEC range register(s) are active and HDM decoders are
> not committed, use RR to provide emulation. A first pass is done to note
> whether any decoders are committed. If there are no committed endpoint
> decoders, then DVSEC ranges will be used for emulation.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hi Dave

One trivial suggestion inline. With that tidied up
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
>  drivers/cxl/cxl.h      |    1 +
>  2 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index ed5e9ef3aa9b..40844ff2fe52 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -729,6 +729,33 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
> +{
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	bool committed;
> +	u32 ctrl;
> +	int i;
> +
> +	if (!is_cxl_endpoint(cxlhdm->port))
> +		return false;
> +
> +	if (!hdm)
> +		return true;
> +
> +	/*
> +	 * If any decoders are committed already, there should not be any
> +	 * emulated DVSEC decoders.
> +	 */
> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> +		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);

FIELD_GET() is nicer than !! I think.

Also, local variable isn't useful so I'd get rid of it.

		if (FIELD_GET(ctrl, CXL_HDM_DECODER0_CTRL_COMMITTED))
			return false;


> +		if (committed)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +

>
Dave Jiang Jan. 17, 2023, 11:19 p.m. UTC | #2 
On 1/13/23 7:07 AM, Jonathan Cameron wrote:
> On Mon, 09 Jan 2023 14:44:03 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> For the case where DVSEC range register(s) are active and HDM decoders are
>> not committed, use RR to provide emulation. A first pass is done to note
>> whether any decoders are committed. If there are no committed endpoint
>> decoders, then DVSEC ranges will be used for emulation.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave
> 
> One trivial suggestion inline. With that tidied up
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>   drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
>>   drivers/cxl/cxl.h      |    1 +
>>   2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index ed5e9ef3aa9b..40844ff2fe52 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -729,6 +729,33 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>>   	return 0;
>>   }
>>   
>> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
>> +{
>> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>> +	bool committed;
>> +	u32 ctrl;
>> +	int i;
>> +
>> +	if (!is_cxl_endpoint(cxlhdm->port))
>> +		return false;
>> +
>> +	if (!hdm)
>> +		return true;
>> +
>> +	/*
>> +	 * If any decoders are committed already, there should not be any
>> +	 * emulated DVSEC decoders.
>> +	 */
>> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
>> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
>> +		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> 
> FIELD_GET() is nicer than !! I think.
> 
> Also, local variable isn't useful so I'd get rid of it.
> 
> 		if (FIELD_GET(ctrl, CXL_HDM_DECODER0_CTRL_COMMITTED))
> 			return false;

Compiler does not seem to like single bit mask. How about
		if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
			return false;
> 
> 
>> +		if (committed)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
> 
>>
>
Jonathan Cameron Jan. 18, 2023, 10:12 a.m. UTC | #3 
On Tue, 17 Jan 2023 16:19:39 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 1/13/23 7:07 AM, Jonathan Cameron wrote:
> > On Mon, 09 Jan 2023 14:44:03 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> For the case where DVSEC range register(s) are active and HDM decoders are
> >> not committed, use RR to provide emulation. A first pass is done to note
> >> whether any decoders are committed. If there are no committed endpoint
> >> decoders, then DVSEC ranges will be used for emulation.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> > Hi Dave
> > 
> > One trivial suggestion inline. With that tidied up
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >   
> >> ---
> >>   drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
> >>   drivers/cxl/cxl.h      |    1 +
> >>   2 files changed, 33 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >> index ed5e9ef3aa9b..40844ff2fe52 100644
> >> --- a/drivers/cxl/core/hdm.c
> >> +++ b/drivers/cxl/core/hdm.c
> >> @@ -729,6 +729,33 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> >>   	return 0;
> >>   }
> >>   
> >> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
> >> +{
> >> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> >> +	bool committed;
> >> +	u32 ctrl;
> >> +	int i;
> >> +
> >> +	if (!is_cxl_endpoint(cxlhdm->port))
> >> +		return false;
> >> +
> >> +	if (!hdm)
> >> +		return true;
> >> +
> >> +	/*
> >> +	 * If any decoders are committed already, there should not be any
> >> +	 * emulated DVSEC decoders.
> >> +	 */
> >> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
> >> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> >> +		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);  
> > 
> > FIELD_GET() is nicer than !! I think.
> > 
> > Also, local variable isn't useful so I'd get rid of it.
> > 
> > 		if (FIELD_GET(ctrl, CXL_HDM_DECODER0_CTRL_COMMITTED))
> > 			return false;  
> 
> Compiler does not seem to like single bit mask. How about

That's strange - number of bits shouldn't matter.
hough I got parameters backwards above which doesn't help.
FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl)

> 		if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
> 			return false;
> > 
> >   
> >> +		if (committed)
> >> +			return false;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +  
> >   
> >>  
> >
Dave Jiang Jan. 18, 2023, 3:22 p.m. UTC | #4 
On 1/18/23 3:12 AM, Jonathan Cameron wrote:
> On Tue, 17 Jan 2023 16:19:39 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> On 1/13/23 7:07 AM, Jonathan Cameron wrote:
>>> On Mon, 09 Jan 2023 14:44:03 -0700
>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>    
>>>> For the case where DVSEC range register(s) are active and HDM decoders are
>>>> not committed, use RR to provide emulation. A first pass is done to note
>>>> whether any decoders are committed. If there are no committed endpoint
>>>> decoders, then DVSEC ranges will be used for emulation.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> Hi Dave
>>>
>>> One trivial suggestion inline. With that tidied up
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>    
>>>> ---
>>>>    drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
>>>>    drivers/cxl/cxl.h      |    1 +
>>>>    2 files changed, 33 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>> index ed5e9ef3aa9b..40844ff2fe52 100644
>>>> --- a/drivers/cxl/core/hdm.c
>>>> +++ b/drivers/cxl/core/hdm.c
>>>> @@ -729,6 +729,33 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
>>>> +{
>>>> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>>>> +	bool committed;
>>>> +	u32 ctrl;
>>>> +	int i;
>>>> +
>>>> +	if (!is_cxl_endpoint(cxlhdm->port))
>>>> +		return false;
>>>> +
>>>> +	if (!hdm)
>>>> +		return true;
>>>> +
>>>> +	/*
>>>> +	 * If any decoders are committed already, there should not be any
>>>> +	 * emulated DVSEC decoders.
>>>> +	 */
>>>> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
>>>> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
>>>> +		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>>>
>>> FIELD_GET() is nicer than !! I think.
>>>
>>> Also, local variable isn't useful so I'd get rid of it.
>>>
>>> 		if (FIELD_GET(ctrl, CXL_HDM_DECODER0_CTRL_COMMITTED))
>>> 			return false;
>>
>> Compiler does not seem to like single bit mask. How about
> 
> That's strange - number of bits shouldn't matter.
> hough I got parameters backwards above which doesn't help.
> FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl)

ah yeah that was it. oops.

> 
>> 		if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
>> 			return false;
>>>
>>>    
>>>> +		if (committed)
>>>> +			return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>    
>>>>   
>>>    
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index ed5e9ef3aa9b..40844ff2fe52 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -729,6 +729,33 @@  static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
 	return 0;
 }
 
+static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	bool committed;
+	u32 ctrl;
+	int i;
+
+	if (!is_cxl_endpoint(cxlhdm->port))
+		return false;
+
+	if (!hdm)
+		return true;
+
+	/*
+	 * If any decoders are committed already, there should not be any
+	 * emulated DVSEC decoders.
+	 */
+	for (i = 0; i < cxlhdm->decoder_count; i++) {
+		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
+		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
+		if (committed)
+			return false;
+	}
+
+	return true;
+}
+
 static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
 			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
@@ -744,16 +771,12 @@  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		unsigned char target_id[8];
 	} target_list;
 
+	if (info->emulate_decoders)
+		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
+
 	if (is_endpoint_decoder(&cxld->dev))
 		cxled = to_cxl_endpoint_decoder(&cxld->dev);
 
-	if (!hdm) {
-		if (!cxled)
-			return -EINVAL;
-
-		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
-	}
-
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
@@ -889,6 +912,8 @@  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 
 	cxl_settle_decoders(cxlhdm);
 
+	info->emulate_decoders = should_emulate_decoders(cxlhdm);
+
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
 		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
 		int rc, target_count = cxlhdm->target_count;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0ec047cced90..f1aa57a95150 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -640,6 +640,7 @@  struct cxl_endpoint_dvsec_info {
 	bool mem_enabled;
 	int ranges;
 	struct range dvsec_range[2];
+	bool emulate_decoders;
 };
 
 struct cxl_hdm;