Re: [PATCH u-boot-marvell 4/9] arm: mvebu: a38x: serdes: Don't overwrite read-only SAR PCIe registers

[Stefan Roese <sr@denx.de>]

On 24.09.21 22:59, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Device/Port Type bits of PCIe Root Port PCI Express Capabilities Register
> are read-only SAR registers and are initialized according to current mode
> configured by PCIe controller. Changing PCIe controller mode (from Root
> Complex mode to Endpoint mode or the other way) is possible via PCI
> Express Control Register (offset 0x41A00), bit 1 (ConfRoot Complex). This
> has to be done in PCIe controller driver (in our case pci_mvebu.c). Note
> that default mode is Root Complex.
> 
> Maximum Link Speed bits of PCIe Root Port Link Capabilities Register are
> platform specific and overwriting them does not make sense. They are set by
> PCIe controller according to current SerDes configuration. For A38x it is
> 5.0 GT/s if SerDes supports appropriate speed.
> 
> Maximum Link Width bits of PCIe Root Port Link Capabilities Register are
> read-only SAR registers, but unfortunately if this is not set correctly
> here, then access PCI config space of the endpoint card behind this Root
> Port does not work.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c    | 22 ----------
>   arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h    |  4 ++
>   .../serdes/a38x/high_speed_env_spec.c         | 40 +++++++------------
>   3 files changed, 19 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
> index 0eb31d589c..7c18df8113 100644
> --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
> +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.c
> @@ -28,28 +28,6 @@ int hws_pex_config(const struct serdes_map *serdes_map, u8 count)
>   
>   	DEBUG_INIT_FULL_S("\n### hws_pex_config ###\n");
>   
> -	for (idx = 0; idx < count; idx++) {
> -		serdes_type = serdes_map[idx].serdes_type;
> -		/* configuration for PEX only */
> -		if ((serdes_type != PEX0) && (serdes_type != PEX1) &&
> -		    (serdes_type != PEX2) && (serdes_type != PEX3))
> -			continue;
> -
> -		if ((serdes_type != PEX0) &&
> -		    ((serdes_map[idx].serdes_mode == PEX_ROOT_COMPLEX_X4) ||
> -		     (serdes_map[idx].serdes_mode == PEX_END_POINT_X4))) {
> -			/* for PEX by4 - relevant for the first port only */
> -			continue;
> -		}
> -
> -		/* Set Device/Port Type to RootComplex */
> -		pex_idx = serdes_type - PEX0;
> -		tmp = reg_read(PEX_CAPABILITIES_REG(pex_idx));
> -		tmp &= ~(0xf << 20);
> -		tmp |= (0x4 << 20);
> -		reg_write(PEX_CAPABILITIES_REG(pex_idx), tmp);
> -	}
> -
>   	tmp = reg_read(SOC_CONTROL_REG1);
>   	tmp &= ~0x03;
>   
> diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> index a882d24208..5d70166fc5 100644
> --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h
> @@ -6,8 +6,12 @@
>   #ifndef _CTRL_PEX_H
>   #define _CTRL_PEX_H
>   
> +#include <pci.h>
>   #include "high_speed_env_spec.h"
>   
> +/* Direct access to PEX0 Root Port's PCIe Capability structure */
> +#define PEX0_RP_PCIE_CFG_OFFSET		(0x00080000 + 0x60)
> +
>   /* Sample at Reset */
>   #define MPP_SAMPLE_AT_RESET(id)		(0xe4200 + (id * 4))
>   
> diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> index 09192acef2..a712fa8994 100644
> --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
> @@ -1714,7 +1714,7 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
>   				(serdes_mode == PEX_END_POINT_X1);
>   			pex_idx = serdes_type - PEX0;
>   
> -			if ((is_pex_by1 == 1) || (serdes_type == PEX0)) {
> +			if (serdes_type == PEX0) {
>   				/* For PEX by 4, init only the PEX 0 */
>   				reg_data = reg_read(SOC_CONTROL_REG1);
>   				if (is_pex_by1 == 1)
> @@ -1723,30 +1723,20 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up,
>   					reg_data &= ~0x4000;
>   				reg_write(SOC_CONTROL_REG1, reg_data);
>   
> -				/* Set Maximum Link Width to X1 or X4 */
> -				reg_data = reg_read(PEX_CFG_DIRECT_ACCESS(
> -						     pex_idx,
> -						     PEX_LINK_CAPABILITY_REG));
> -				reg_data &= ~0x3f0;
> -				if (is_pex_by1 == 1)
> -					reg_data |= 0x10;
> -				else
> -					reg_data |= 0x40;
> -				reg_write(PEX_CFG_DIRECT_ACCESS(
> -					   pex_idx,
> -					   PEX_LINK_CAPABILITY_REG),
> -					  reg_data);
> -
> -				/* Set Maximum Link Speed to 5 GT/s */
> -				reg_data = reg_read(PEX_CFG_DIRECT_ACCESS(
> -						     pex_idx,
> -						     PEX_LINK_CAPABILITY_REG));
> -				reg_data &= ~0xf;
> -				reg_data |= 0x2;
> -				reg_write(PEX_CFG_DIRECT_ACCESS(
> -					   pex_idx,
> -					   PEX_LINK_CAPABILITY_REG),
> -					  reg_data);
> +				/*
> +				 * Set Maximum Link Width to X1 or X4 in Root
> +				 * Port's PCIe Link Capability register.
> +				 * This register is read-only but if is not set
> +				 * correctly then access to PCI config space of
> +				 * endpoint card behind this Root Port does not
> +				 * work.
> +				 */
> +				reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET +
> +						    PCI_EXP_LNKCAP);
> +				reg_data &= ~PCI_EXP_LNKCAP_MLW;
> +				reg_data |= (is_pex_by1 ? 1 : 4) << 4;
> +				reg_write(PEX0_RP_PCIE_CFG_OFFSET +
> +					  PCI_EXP_LNKCAP, reg_data);
>   
>   				/*
>   				 * Set Common Clock Configuration to indicates
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de