diff mbox series

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

Message ID 20240409073528.13214-3-kobayashi.da-06@fujitsu.com (mailing list archive)
State Superseded
Headers show
Series cxl: Export cxl1.1 device link status to sysfs | expand

Commit Message

Daisuke Kobayashi (Fujitsu) April 9, 2024, 7:35 a.m. UTC
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(+)

Comments

Dave Jiang April 9, 2024, 7:15 p.m. UTC | #1
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);
>
Daisuke Kobayashi (Fujitsu) April 10, 2024, 7:14 a.m. UTC | #2
Dave Jiang wrote:
> 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().
> 
Thank you for your feedback.
I will update the patch to use those macros.

> > +	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
> 
Also I will fix it.

> > +			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);
> >
diff mbox series

Patch

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;
+	cap_hdr = readl(addr + offset);
+	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
+		offset = (cap_hdr >> 8) & 0x000000ff;
+		if (offset == 0) // End of capability list
+			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);