Re: [PATCH v4 2/3] cxl/core/regs: Add rcd_regs initialization at __rcrb_to_component()

[Dave Jiang <dave.jiang@intel.com>]

On 4/9/24 12:35 AM, Kobayashi,Daisuke wrote:
> Add rcd_regs initialization at __rcrb_to_component() to cache the cxl1.1
> device link status information. Reduce access to the memory map area
> where the RCRB is located by caching the cxl1.1 device link status information.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/core/regs.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 372786f80955..308eb951613e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
>  	u32 bar0, bar1;
>  	u16 cmd;
>  	u32 id;
> +	u16 offset;
> +	u32 cap_hdr;
>  
>  	if (which == CXL_RCRB_UPSTREAM)
>  		rcrb += SZ_4K;
> @@ -537,6 +539,22 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
>  	cmd = readw(addr + PCI_COMMAND);
>  	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>  	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
> +	offset = readw(addr + PCI_CAPABILITY_LIST);
> +	offset &= 0x00ff;

GENMASK(7,0) is preferred to 0x00ff. Although a properly defined mask would be nice.
Also please consider using FIELD_GET().

> +	cap_hdr = readl(addr + offset);
> +	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {

Same comment as above


> +		offset = (cap_hdr >> 8) & 0x000000ff;

Also here

> +		if (offset == 0) // End of capability list

Please use /* */ instead of // for Linux kernel code

> +			break;
> +		cap_hdr = readl(addr + offset);
> +	}
> +	if (offset) {
> +		ri->rcd_lnkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> +		ri->rcd_lnkctrl = readl(addr + offset + PCI_EXP_LNKCTL);
> +		ri->rcd_lnkstatus = readl(addr + offset + PCI_EXP_LNKSTA);
> +	}
> +
>  	iounmap(addr);
>  	release_mem_region(rcrb, SZ_4K);
>