Re: [PATCH] PCI: j721e: Extend j721e_pcie_ctrl_init() for non syscon nodes

[Andrew Davis]

On 1/29/24 4:49 AM, Siddharth Vadapalli wrote:
> The "ti,syscon-pcie-ctrl" device-tree property is used to obtain
> reference to the "pcie_ctrl" register within the System Controller Module
> in order to configure the link speed, number of lanes and the mode of
> operation of the PCIe controller. The existing implementation of the
> "j721e_pcie_ctrl_init()" function handles the case where the compatible for
> the System Controller Module node specified using the "ti,syscon-pcie-ctrl"
> property is "syscon". Since the System Controller Module can be modelled as
> a "simple-bus" as well, extend the implementation of the
> "j721e_pcie_ctrl_init()" function to handle the "simple-bus" case.
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> ---
>
> Hello,
>
> This patch is based on linux-next tagged next-20240129.
>
> The System Controller Module is modelled as a "simple-bus" in J784S4 SoC at
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi#L45
> The existing SoCs such as J721E and J7200 which currently model the node as
> a Syscon node will also be updated to model it as a "simple-bus".
> Therefore this patch aims to update the driver in order to handle the
> migration of the System Controller Module to the "simple-bus" compatible
> without breaking PCIe functionality on existing TI SoCs which make use
> of the pci-j721e.c driver.
>
> Regards,
> Siddharth.
>
>   drivers/pci/controller/cadence/pci-j721e.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 85718246016b..2ace7e78a880 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -224,12 +224,20 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>   {
>   	struct device *dev = pcie->cdns_pcie->dev;
>   	struct device_node *node = dev->of_node;
> +	struct device_node *scm_conf;
>   	struct of_phandle_args args;
>   	unsigned int offset = 0;
>   	struct regmap *syscon;
>   	int ret;
>   
> -	syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-ctrl");
> +	scm_conf = of_parse_phandle(node, "ti,syscon-pcie-ctrl", 0);
> +	if (!scm_conf) {
> +		dev_err(dev, "unable to get System Controller node\n");
> +		return -ENODEV;
> +	}
> +
> +	syscon = device_node_to_regmap(scm_conf);

Turning the entire "simple-bus" region into a regmap using this
function is just as broken as having it as a "syscon". The core
problem we are solving by getting rid of the blanket syscon nodes
is that it causes multiple mappings of the same register. This
can cause issues with regmap caching, read–modify–write, etc..

What you want to do is add a subnode to the simple-bus, have that
encapsulate just the registers used for PCIe, and have the PCIe
node point to that. Then this patch isn't needed.

For an example, see how it's done for DSS[0].

Andrew

[0] https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502

> +	of_node_put(scm_conf);
>   	if (IS_ERR(syscon)) {
>   		dev_err(dev, "Unable to get ti,syscon-pcie-ctrl regmap\n");
>   		return PTR_ERR(syscon);