[PATCH] drm/amdgpu: modify a pair of functions for the pcie port wreg/rreg

[Huang Rui]

On Tue, Jan 18, 2022 at 09:36:56PM +0800, Lazar, Lijo wrote:
> 
> 
> On 1/18/2022 4:56 PM, Xiaojian Du wrote:
> > This patch will modify a pair of functions for pcie port wreg/rreg.
> > AMD GPU have had an independent NBIO block from SOC15 arch.
> > If the dirver wants to read/write the address space of the pcie devices,
> > it has to go through the NBIO block.
> > This patch will move the pcie port wreg/rreg functions to
> > "amdgpu_device.c", so that to make the functions can be used on the
> > future GPU ASICs.
> > 
> > Signed-off-by: Xiaojian Du <Xiaojian.Du at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 +++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 +++++++++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/nv.c            | 34 ++--------------------
> >   3 files changed, 39 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index b2da840f4718..691d7868d64d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1421,6 +1421,10 @@ void amdgpu_device_invalidate_hdp(struct amdgpu_device *adev,
> >   		struct amdgpu_ring *ring);
> >   
> >   void amdgpu_device_halt(struct amdgpu_device *adev);
> > +u32 amdgpu_device_pcie_port_rreg(struct amdgpu_device *adev,
> > +				u32 reg);
> > +void amdgpu_device_pcie_port_wreg(struct amdgpu_device *adev,
> > +				u32 reg, u32 v);
> >   
> >   /* atpx handler */
> >   #if defined(CONFIG_VGA_SWITCHEROO)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index ff4cf0e2a01f..10f2b7cbb49d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -6023,3 +6023,36 @@ void amdgpu_device_halt(struct amdgpu_device *adev)
> >   	pci_disable_device(pdev);
> >   	pci_wait_for_pending_transaction(pdev);
> >   }
> > +
> > +u32 amdgpu_device_pcie_port_rreg(struct amdgpu_device *adev,
> > +				u32 reg)
> > +{
> > +	unsigned long flags, address, data;
> > +	u32 r;
> > +
> > +	address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> > +	data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
> > +
> > +	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > +	WREG32(address, reg * 4);
> > +	(void)RREG32(address);
> > +	r = RREG32(data);
> > +	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> > +	return r;
> > +}
> > +
> > +void amdgpu_device_pcie_port_wreg(struct amdgpu_device *adev,
> > +				u32 reg, u32 v)
> > +{
> > +	unsigned long flags, address, data;
> > +
> > +	address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> > +	data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
> > +
> > +	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > +	WREG32(address, reg * 4);
> > +	(void)RREG32(address);
> > +	WREG32(data, v);
> > +	(void)RREG32(data);
> > +	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> > +}
> > diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> > index e52d1114501c..17480c1eeae8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> > @@ -256,21 +256,6 @@ static u64 nv_pcie_rreg64(struct amdgpu_device *adev, u32 reg)
> >   	return amdgpu_device_indirect_rreg64(adev, address, data, reg);
> >   }
> >   
> > -static u32 nv_pcie_port_rreg(struct amdgpu_device *adev, u32 reg)
> > -{
> > -	unsigned long flags, address, data;
> > -	u32 r;
> > -	address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> > -	data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
> > -
> > -	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > -	WREG32(address, reg * 4);
> > -	(void)RREG32(address);
> > -	r = RREG32(data);
> > -	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> > -	return r;
> > -}
> > -
> >   static void nv_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
> >   {
> >   	unsigned long address, data;
> > @@ -281,21 +266,6 @@ static void nv_pcie_wreg64(struct amdgpu_device *adev, u32 reg, u64 v)
> >   	amdgpu_device_indirect_wreg64(adev, address, data, reg, v);
> >   }
> >   
> > -static void nv_pcie_port_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
> > -{
> > -	unsigned long flags, address, data;
> > -
> > -	address = adev->nbio.funcs->get_pcie_port_index_offset(adev);
> > -	data = adev->nbio.funcs->get_pcie_port_data_offset(adev);
> > -
> > -	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
> > -	WREG32(address, reg * 4);
> > -	(void)RREG32(address);
> > -	WREG32(data, v);
> > -	(void)RREG32(data);
> > -	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> > -}
> > -
> >   static u32 nv_didt_rreg(struct amdgpu_device *adev, u32 reg)
> >   {
> >   	unsigned long flags, address, data;
> > @@ -709,8 +679,8 @@ static int nv_common_early_init(void *handle)
> >   	adev->pcie_wreg = &nv_pcie_wreg;
> >   	adev->pcie_rreg64 = &nv_pcie_rreg64;
> >   	adev->pcie_wreg64 = &nv_pcie_wreg64;
> 
> Looks good, would be better if the above ones also are changed to common 
> implementation.

Agree with Lijo. Xiaojian, let's make a patch to move this part to
amdgpu_device.c as well.

Thanks,
Ray

> 
> Thanks,
> Lijo
> 
> > -	adev->pciep_rreg = &nv_pcie_port_rreg;
> > -	adev->pciep_wreg = &nv_pcie_port_wreg;
> > +	adev->pciep_rreg = amdgpu_device_pcie_port_rreg;
> > +	adev->pciep_wreg = amdgpu_device_pcie_port_wreg;
> >   
> >   	/* TODO: will add them during VCN v2 implementation */
> >   	adev->uvd_ctx_rreg = NULL;
> >