diff mbox series

[v3,07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

Message ID 20241007041218.157516-8-dlemoal@kernel.org (mailing list archive)
State New
Delegated to: Manivannan Sadhasivam
Headers show
Series [v3,01/12] PCI: rockchip-ep: Fix address translation unit programming | expand

Commit Message

Damien Le Moal Oct. 7, 2024, 4:12 a.m. UTC
Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
 1 file changed, 30 insertions(+), 24 deletions(-)

Comments

Manivannan Sadhasivam Oct. 10, 2024, 7:25 a.m. UTC | #1
On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Btw, can someone from Rockchip confirm if this hiding is necessary for all the
SoCs? It looks to me like an SoC quirk.

- Mani

> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 523e9cdfd241..7a1798fcc2ad 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>  	pci_epc_mem_exit(ep->epc);
>  }
>  
> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> +{
> +	u32 cfg_msi, cfg_msix_cp;
> +
> +	/*
> +	 * MSI-X is not supported but the controller still advertises the MSI-X
> +	 * capability by default, which can lead to the Root Complex side
> +	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
> +	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
> +	 * the next pointer from the MSI-X entry and set that in the MSI
> +	 * capability entry (which is the previous entry). This way the MSI-X
> +	 * entry is skipped (left out of the linked-list) and not advertised.
> +	 */
> +	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> +				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> +
> +	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
> +
> +	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> +					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
> +					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
> +
> +	cfg_msi |= cfg_msix_cp;
> +
> +	rockchip_pcie_write(rockchip, cfg_msi,
> +			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> +}
> +
>  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -588,7 +616,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	struct rockchip_pcie *rockchip;
>  	struct pci_epc *epc;
>  	int err;
> -	u32 cfg_msi, cfg_msix_cp;
>  
>  	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
>  	if (!ep)
> @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_disable_clocks;
>  
> +	rockchip_pcie_ep_hide_msix_cap(rockchip);
> +
>  	/* Establish the link automatically */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
> @@ -626,29 +655,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	/* Only enable function 0 by default */
>  	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
>  
> -	/*
> -	 * MSI-X is not supported but the controller still advertises the MSI-X
> -	 * capability by default, which can lead to the Root Complex side
> -	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
> -	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
> -	 * the next pointer from the MSI-X entry and set that in the MSI
> -	 * capability entry (which is the previous entry). This way the MSI-X
> -	 * entry is skipped (left out of the linked-list) and not advertised.
> -	 */
> -	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> -				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> -
> -	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
> -
> -	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> -					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
> -					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
> -
> -	cfg_msi |= cfg_msix_cp;
> -
> -	rockchip_pcie_write(rockchip, cfg_msi,
> -			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> -
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
>  
> -- 
> 2.46.2
>
Manivannan Sadhasivam Oct. 10, 2024, 8:09 a.m. UTC | #2
On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> > Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> > to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> > changes.
> > 
> > Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> SoCs? It looks to me like an SoC quirk.
> 
> - Mani
> 
> > ---
> >  drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> > index 523e9cdfd241..7a1798fcc2ad 100644
> > --- a/drivers/pci/controller/pcie-rockchip-ep.c
> > +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> > @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
> >  	pci_epc_mem_exit(ep->epc);
> >  }
> >  
> > +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)

Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
function essentially disables MSIx which is broken. Again, it'd be good to know
if this applies to all SoCs or just a few.

- Mani

> > +{
> > +	u32 cfg_msi, cfg_msix_cp;
> > +
> > +	/*
> > +	 * MSI-X is not supported but the controller still advertises the MSI-X
> > +	 * capability by default, which can lead to the Root Complex side
> > +	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
> > +	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
> > +	 * the next pointer from the MSI-X entry and set that in the MSI
> > +	 * capability entry (which is the previous entry). This way the MSI-X
> > +	 * entry is skipped (left out of the linked-list) and not advertised.
> > +	 */
> > +	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> > +				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> > +
> > +	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
> > +
> > +	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> > +					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
> > +					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
> > +
> > +	cfg_msi |= cfg_msix_cp;
> > +
> > +	rockchip_pcie_write(rockchip, cfg_msi,
> > +			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> > +}
> > +
> >  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -588,7 +616,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> >  	struct rockchip_pcie *rockchip;
> >  	struct pci_epc *epc;
> >  	int err;
> > -	u32 cfg_msi, cfg_msix_cp;
> >  
> >  	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> >  	if (!ep)
> > @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> >  	if (err)
> >  		goto err_disable_clocks;
> >  
> > +	rockchip_pcie_ep_hide_msix_cap(rockchip);
> > +
> >  	/* Establish the link automatically */
> >  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> >  			    PCIE_CLIENT_CONFIG);
> > @@ -626,29 +655,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> >  	/* Only enable function 0 by default */
> >  	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
> >  
> > -	/*
> > -	 * MSI-X is not supported but the controller still advertises the MSI-X
> > -	 * capability by default, which can lead to the Root Complex side
> > -	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
> > -	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
> > -	 * the next pointer from the MSI-X entry and set that in the MSI
> > -	 * capability entry (which is the previous entry). This way the MSI-X
> > -	 * entry is skipped (left out of the linked-list) and not advertised.
> > -	 */
> > -	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> > -				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> > -
> > -	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
> > -
> > -	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> > -					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
> > -					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
> > -
> > -	cfg_msi |= cfg_msix_cp;
> > -
> > -	rockchip_pcie_write(rockchip, cfg_msi,
> > -			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
> > -
> >  	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
> >  			    PCIE_CLIENT_CONFIG);
> >  
> > -- 
> > 2.46.2
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Damien Le Moal Oct. 10, 2024, 8:37 a.m. UTC | #3
On 2024/10/10 17:09, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>>> changes.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
>> SoCs? It looks to me like an SoC quirk.
>>
>> - Mani
>>
>>> ---
>>>  drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
>>>  1 file changed, 30 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
>>> index 523e9cdfd241..7a1798fcc2ad 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>>>  	pci_epc_mem_exit(ep->epc);
>>>  }
>>>  
>>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> 
> Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> function essentially disables MSIx which is broken. Again, it'd be good to know
> if this applies to all SoCs or just a few.

This is for the rk3399... I am not aware of multiple versions of that SoC.
The pcie_rockchip driver is for that SoC only as far as I know. This is unlike
the Designware IP block which is used in multiple SoCs.

> 
> - Mani
> 
>>> +{
>>> +	u32 cfg_msi, cfg_msix_cp;
>>> +
>>> +	/*
>>> +	 * MSI-X is not supported but the controller still advertises the MSI-X
>>> +	 * capability by default, which can lead to the Root Complex side
>>> +	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
>>> +	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
>>> +	 * the next pointer from the MSI-X entry and set that in the MSI
>>> +	 * capability entry (which is the previous entry). This way the MSI-X
>>> +	 * entry is skipped (left out of the linked-list) and not advertised.
>>> +	 */
>>> +	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
>>> +				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
>>> +
>>> +	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
>>> +
>>> +	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
>>> +					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
>>> +					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
>>> +
>>> +	cfg_msi |= cfg_msix_cp;
>>> +
>>> +	rockchip_pcie_write(rockchip, cfg_msi,
>>> +			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
>>> +}
>>> +
>>>  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> @@ -588,7 +616,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>>>  	struct rockchip_pcie *rockchip;
>>>  	struct pci_epc *epc;
>>>  	int err;
>>> -	u32 cfg_msi, cfg_msix_cp;
>>>  
>>>  	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
>>>  	if (!ep)
>>> @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>>>  	if (err)
>>>  		goto err_disable_clocks;
>>>  
>>> +	rockchip_pcie_ep_hide_msix_cap(rockchip);
>>> +
>>>  	/* Establish the link automatically */
>>>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>>>  			    PCIE_CLIENT_CONFIG);
>>> @@ -626,29 +655,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>>>  	/* Only enable function 0 by default */
>>>  	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
>>>  
>>> -	/*
>>> -	 * MSI-X is not supported but the controller still advertises the MSI-X
>>> -	 * capability by default, which can lead to the Root Complex side
>>> -	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
>>> -	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
>>> -	 * the next pointer from the MSI-X entry and set that in the MSI
>>> -	 * capability entry (which is the previous entry). This way the MSI-X
>>> -	 * entry is skipped (left out of the linked-list) and not advertised.
>>> -	 */
>>> -	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
>>> -				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
>>> -
>>> -	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
>>> -
>>> -	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
>>> -					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
>>> -					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
>>> -
>>> -	cfg_msi |= cfg_msix_cp;
>>> -
>>> -	rockchip_pcie_write(rockchip, cfg_msi,
>>> -			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
>>> -
>>>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
>>>  			    PCIE_CLIENT_CONFIG);
>>>  
>>> -- 
>>> 2.46.2
>>>
>>
>> -- 
>> மணிவண்ணன் சதாசிவம்
>
Damien Le Moal Oct. 11, 2024, 8:25 a.m. UTC | #4
On 10/10/24 16:25, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>> changes.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> SoCs? It looks to me like an SoC quirk.

All SoCs ? Are there several versions of the RK3399 ?
As far as I know, there is only one. This is unlike the designware IP block used
in the RK3588 which can also be found in other SoC and may have some variations
due to different synthesis parameters.
Damien Le Moal Oct. 11, 2024, 8:30 a.m. UTC | #5
On 10/10/24 17:09, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>>> changes.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
>> SoCs? It looks to me like an SoC quirk.
>>
>> - Mani
>>
>>> ---
>>>  drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
>>>  1 file changed, 30 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
>>> index 523e9cdfd241..7a1798fcc2ad 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>>>  	pci_epc_mem_exit(ep->epc);
>>>  }
>>>  
>>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> 
> Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> function essentially disables MSIx which is broken. Again, it'd be good to know
> if this applies to all SoCs or just a few.

The function does not disable anything but *really* simply hides the capability
in the capability list so that the host does not see it. So I think the better
name is:

rockchip_pcie_ep_hide_broken_msix_cap()
Manivannan Sadhasivam Oct. 12, 2024, 12:12 p.m. UTC | #6
On Fri, Oct 11, 2024 at 05:25:56PM +0900, Damien Le Moal wrote:
> On 10/10/24 16:25, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> >> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> >> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> >> changes.
> >>
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> > SoCs? It looks to me like an SoC quirk.
> 
> All SoCs ? Are there several versions of the RK3399 ?

There seems to be some:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=296602b8e5f7

> As far as I know, there is only one. This is unlike the designware IP block used
> in the RK3588 which can also be found in other SoC and may have some variations
> due to different synthesis parameters.
> 

But anyway, let's keep the quirk until we hear otherwise.

- Mani

> -- 
> Damien Le Moal
> Western Digital Research
Manivannan Sadhasivam Oct. 12, 2024, 12:14 p.m. UTC | #7
On Fri, Oct 11, 2024 at 05:30:00PM +0900, Damien Le Moal wrote:
> On 10/10/24 17:09, Manivannan Sadhasivam wrote:
> > On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
> >> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> >>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> >>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> >>> changes.
> >>>
> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>
> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>
> >> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> >> SoCs? It looks to me like an SoC quirk.
> >>
> >> - Mani
> >>
> >>> ---
> >>>  drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
> >>>  1 file changed, 30 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> >>> index 523e9cdfd241..7a1798fcc2ad 100644
> >>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> >>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> >>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
> >>>  	pci_epc_mem_exit(ep->epc);
> >>>  }
> >>>  
> >>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> > 
> > Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> > function essentially disables MSIx which is broken. Again, it'd be good to know
> > if this applies to all SoCs or just a few.
> 
> The function does not disable anything but *really* simply hides the capability
> in the capability list so that the host does not see it. So I think the better
> name is:
> 
> rockchip_pcie_ep_hide_broken_msix_cap()

Sounds good to me.

- Mani
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 523e9cdfd241..7a1798fcc2ad 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -581,6 +581,34 @@  static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
 	pci_epc_mem_exit(ep->epc);
 }
 
+static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
+{
+	u32 cfg_msi, cfg_msix_cp;
+
+	/*
+	 * MSI-X is not supported but the controller still advertises the MSI-X
+	 * capability by default, which can lead to the Root Complex side
+	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
+	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
+	 * the next pointer from the MSI-X entry and set that in the MSI
+	 * capability entry (which is the previous entry). This way the MSI-X
+	 * entry is skipped (left out of the linked-list) and not advertised.
+	 */
+	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
+				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
+
+	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
+
+	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
+					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
+					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
+
+	cfg_msi |= cfg_msix_cp;
+
+	rockchip_pcie_write(rockchip, cfg_msi,
+			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
+}
+
 static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -588,7 +616,6 @@  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	struct rockchip_pcie *rockchip;
 	struct pci_epc *epc;
 	int err;
-	u32 cfg_msi, cfg_msix_cp;
 
 	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
 	if (!ep)
@@ -619,6 +646,8 @@  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	if (err)
 		goto err_disable_clocks;
 
+	rockchip_pcie_ep_hide_msix_cap(rockchip);
+
 	/* Establish the link automatically */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
@@ -626,29 +655,6 @@  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	/* Only enable function 0 by default */
 	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
 
-	/*
-	 * MSI-X is not supported but the controller still advertises the MSI-X
-	 * capability by default, which can lead to the Root Complex side
-	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
-	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
-	 * the next pointer from the MSI-X entry and set that in the MSI
-	 * capability entry (which is the previous entry). This way the MSI-X
-	 * entry is skipped (left out of the linked-list) and not advertised.
-	 */
-	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
-				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
-
-	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
-
-	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
-					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
-					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
-
-	cfg_msi |= cfg_msix_cp;
-
-	rockchip_pcie_write(rockchip, cfg_msi,
-			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
-
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
 			    PCIE_CLIENT_CONFIG);