Re: [PATCH 5/5] PCI: rzg3s-host: Add support for RZ/V2H(P) SoC

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Prabhakar,

On 3/18/26 14:44, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support for the RZ/V2H(P) SoC PCIe controller to the rzg3s-host
> driver.
> 
> The RZ/V2H(P) SoC features two independent PCIe channels that share
> physical lanes. The hardware supports two configuration modes: single
> x4 mode where one controller uses all four lanes, or dual x2 mode
> where both controllers use two lanes each.
> 
> Introduce configure_lanes() function pointer to configure the PCIe
> lanes based on the number of channels enabled. Implement
> rzv2h_pcie_configure_lanes() to detect the active PCIe channels at
> boot time and program the lane mode via the system controller using
> the new RZG3S_SYSC_FUNC_ID_LINK_MASTER function ID.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>   drivers/pci/controller/pcie-rzg3s-host.c | 142 +++++++++++++++++++++++
>   1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index a629e861bbd0..d1bf1e750d9b 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -179,6 +179,16 @@
>   /* Timeouts experimentally determined */
>   #define RZG3S_REQ_ISSUE_TIMEOUT_US		2500
>   
> +/**
> + * enum rzg3s_sysc_link_mode - PCIe link configuration modes
> + * @RZG3S_SYSC_LINK_MODE_SINGLE_X4: Single port with x4 lanes
> + * @RZG3S_SYSC_LINK_MODE_DUAL_X2: Dual ports with x2 lanes each
> + */
> +enum rzg3s_sysc_link_mode {
> +	RZG3S_SYSC_LINK_MODE_SINGLE_X4 = 1,
> +	RZG3S_SYSC_LINK_MODE_DUAL_X2 = 3,
> +};
> +
>   /**
>    * struct rzg3s_sysc_function - System Controller function descriptor
>    * @offset: Register offset from the System Controller base address
> @@ -194,12 +204,14 @@ struct rzg3s_sysc_function {
>    * @RZG3S_SYSC_FUNC_ID_RST_RSM_B: RST_RSM_B SYSC function ID
>    * @RZG3S_SYSC_FUNC_ID_L1_ALLOW: L1 allow SYSC function ID
>    * @RZG3S_SYSC_FUNC_ID_MODE: Mode SYSC function ID
> + * @RZG3S_SYSC_FUNC_ID_LINK_MASTER: Link master SYSC function ID
>    * @RZG3S_SYSC_FUNC_ID_MAX: Max SYSC function ID
>    */
>   enum rzg3s_sysc_func_id {
>   	RZG3S_SYSC_FUNC_ID_RST_RSM_B,
>   	RZG3S_SYSC_FUNC_ID_L1_ALLOW,
>   	RZG3S_SYSC_FUNC_ID_MODE,
> +	RZG3S_SYSC_FUNC_ID_LINK_MASTER,
>   	RZG3S_SYSC_FUNC_ID_MAX,
>   };
>   
> @@ -261,6 +273,7 @@ struct rzg3s_pcie_host;
>    * @config_pre_init: Optional callback for SoC-specific pre-configuration
>    * @config_post_init: Callback for SoC-specific post-configuration
>    * @config_deinit: Callback for SoC-specific de-initialization
> + * @setup_lanes: Callback for setting up the number of lanes
>    * @power_resets: array with the resets that need to be de-asserted after
>    *                power-on
>    * @cfg_resets: array with the resets that need to be de-asserted after
> @@ -268,17 +281,20 @@ struct rzg3s_pcie_host;
>    * @sysc_info: System Controller info for each PCIe channel
>    * @num_power_resets: number of power resets
>    * @num_cfg_resets: number of configuration resets
> + * @num_channels: number of PCIe channels
>    */
>   struct rzg3s_pcie_soc_data {
>   	int (*init_phy)(struct rzg3s_pcie_host *host);
>   	void (*config_pre_init)(struct rzg3s_pcie_host *host);
>   	int (*config_post_init)(struct rzg3s_pcie_host *host);
>   	int (*config_deinit)(struct rzg3s_pcie_host *host);
> +	int (*setup_lanes)(struct rzg3s_pcie_host *host);
>   	const char * const *power_resets;
>   	const char * const *cfg_resets;
>   	struct rzg3s_sysc_info sysc_info[RZG3S_PCIE_CHANNEL_ID_MAX];
>   	u8 num_power_resets;
>   	u8 num_cfg_resets;
> +	u8 num_channels;
>   };
>   
>   /**
> @@ -309,6 +325,7 @@ struct rzg3s_pcie_port {
>    * @intx_irqs: INTx interrupts
>    * @max_link_speed: maximum supported link speed
>    * @channel_id: PCIe channel identifier, used for System Controller access
> + * @num_lanes: The number of lanes
>    */
>   struct rzg3s_pcie_host {
>   	void __iomem *axi;
> @@ -325,6 +342,7 @@ struct rzg3s_pcie_host {
>   	int intx_irqs[PCI_NUM_INTX];
>   	int max_link_speed;
>   	enum rzg3s_pcie_channel_id channel_id;
> +	u8 num_lanes;
>   };
>   
>   #define rzg3s_msi_to_host(_msi)	container_of(_msi, struct rzg3s_pcie_host, msi)
> @@ -1155,6 +1173,13 @@ static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
>   	rzg3s_pcie_update_bits(host->pcie, PCI_CLASS_REVISION, mask,
>   			       field_prep(mask, PCI_CLASS_BRIDGE_PCI_NORMAL));
>   
> +	if (host->num_lanes) {
> +		rzg3s_pcie_update_bits(host->pcie + RZG3S_PCI_CFG_PCIEC,
> +				       PCI_EXP_LNKCAP, PCI_EXP_LNKCAP_MLW,
> +				       FIELD_PREP(PCI_EXP_LNKCAP_MLW,
> +						  host->num_lanes));
> +	}
> +
>   	/* Disable access control to the CFGU */
>   	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
>   
> @@ -1687,6 +1712,63 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>   	return ret;
>   }
>   
> +static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
> +{
> +	struct device_node *np = host->dev->of_node;
> +	u32 domain;
> +	int ret;
> +
> +	if (host->data->num_channels == 1)
> +		return 0;
> +
> +	ret = of_property_read_u32(np, "linux,pci-domain", &domain);

This introduces some limits in the systems with RZ/V2H(P) SoCs with regards to 
the usage of linux,pci-domain. I would like the PCIe maintainers take on this.

As this is necessary to index in the system controller driver specific data (as 
there are different SYSC offsets for different PCIe controllers) I see the 
following alternatives, if any:

1/ add a dedicated DT property for this, e.g. renesas,pcie-controller-id
2/ Add dedicated DT bindings for RZ/V2H(P) SoC that would be used to specify the
    system controller register offset and mask for different functionalities.

    E.g.:
    renesas,sysc-l1-allow = <&sysc 0x1020 0x1>;
    renesas,sysc-mode = <&sysc 0x1024 0x1>;
    renesas,sysc-link-master = <&sysc 0x1060 0x300>;

    And use them in each controller DT node. E.g.:

    pcie0: pcie@add1 {
        // ...

        renesas,sysc-l1-allow = <&sysc 0x1020 0x1>;
        renesas,sysc-mode = <&sysc 0x1024 0x1>;
        renesas,sysc-link-master = <&sysc 0x1060 0x300>;

        // ...
    };

    pcie0: pcie@add1 {
        // ...

        renesas,sysc-l1-allow = <&sysc 0x1050 0x1>;
        renesas,sysc-mode = <&sysc 0x1054 0x1>;
        renesas,sysc-link-master = <&sysc 0x1060 0x300>;

        // ...
    };

3/ as sashiko.dev mentions [1], using aliases for the PCIe nodes should also be
    what you need here.

[1] 
https://sashiko.dev/#/patchset/20260318124450.163471-1-prabhakar.mahadev-lad.rj%40bp.renesas.com

> +	if (ret)
> +		return ret;
> +
> +	if (domain >= host->data->num_channels)
> +		return -EINVAL;
> +
> +	host->channel_id = domain;
> +
> +	return 0;
> +}
> +
> +static int rzv2h_pcie_setup_lanes(struct rzg3s_pcie_host *host)
> +{
> +	struct device_node *np = host->dev->of_node;
> +	static u8 rzv2h_num_total_lanes;
> +	u32 num_lanes;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * RZ/V2H(P) supports up to 4 lanes, but only in single x4 mode.
> +	 * Dual x2 mode is only supported with 2 total lanes. Validate
> +	 * the configuration to avoid conflicts with other host, if any.
> +	 */
> +	if (num_lanes != 4 && num_lanes != 2)
> +		return -EINVAL;
> +
> +	if (rzv2h_num_total_lanes == 2 && num_lanes != 2)
> +		return -EINVAL;
> +
> +	if (rzv2h_num_total_lanes == 4)
> +		return -EINVAL;
> +
> +	rzv2h_num_total_lanes += num_lanes;

There is a a valid concern raised by sashiko.dev [1] with regards to 
incrementing this if later the probe fails:

from [1]:
"For example, if rzg3s_pcie_resets_prepare_and_get() returns -EPROBE_DEFER,
the static variable is never decremented. On subsequent probe retries,
the variable will be artificially inflated, eventually causing the bounds
check to fail and returning a permanent -EINVAL. This would also prevent
driver unbind and rebind from working correctly."

also:

"Additionally, since the driver sets .probe_type = PROBE_PREFER_ASYNCHRONOUS,
could multiple PCIe controllers probing concurrently cause a data race when
reading and modifying this static variable without locking?"

> +
> +	host->num_lanes = num_lanes;
> +
> +	return rzg3s_sysc_config_func(host->sysc,
> +				      RZG3S_SYSC_FUNC_ID_LINK_MASTER,
> +				      num_lanes == 2 ?
> +				      RZG3S_SYSC_LINK_MODE_DUAL_X2 :
> +				      RZG3S_SYSC_LINK_MODE_SINGLE_X4);

I think this one should also be configured on resume (to have the same 
configuration sequence as in probe) even though RZ/V2H(P) don't currently 
support s2ram. E.g. so something like:

if (host->num_lanes) {
	ret = rzg3s_sysc_config_func(host->sysc,
				     RZG3S_SYSC_FUNC_ID_LINK_MASTER,
				     host->num_lanes == 2  ?
				     RZG3S_SYSC_LINK_MODE_DUAL_X2 :
				     RZG3S_SYSC_LINK_MODE_SINGLE_X4);
	if (ret)
		goto assert_rst_rsm_b;
}

after ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);

Thank you,
Claudiu