Re: [PATCH v7 2/6] interconnect: Add generic interconnect driver for Exynos SoCs

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 03.11.2020 10:37, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
> 
> samsung,interconnect-parent -> interconnects?

Yes, I will rephrase the whole commit message as it's a bit outdated now.

I've changed the sentence to:
"The SoC topology is a graph (or more specifically, a tree) and its
edges are described by specifying in the 'interconnects' property
the interconnect consumer path for each interconnect provider DT node."

>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> The bus-width DT property is to determine the interconnect data
>> width and traslate requested bandwidth to clock frequency for each
>> bus.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

>> +++ b/drivers/interconnect/exynos/exynos.c

>> +struct exynos_icc_priv {
>> +	struct device *dev;
>> +
>> +	/* One interconnect node per provider */
>> +	struct icc_provider provider;
>> +	struct icc_node *node;
>> +
>> +	struct dev_pm_qos_request qos_req;
>> +	u32 bus_clk_ratio;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +	struct of_phandle_args args;
>> +	struct icc_node_data *icc_node_data;
>> +	struct icc_node *icc_node;
>> +	int num, ret;
>> +
>> +	num = of_count_phandle_with_args(np, "interconnects",
>> +					 "#interconnect-cells");
>> +	if (num < 1)
>> +		return NULL; /* parent nodes are optional */
>> +
>> +	/* Get the interconnect target node */
>> +	ret = of_parse_phandle_with_args(np, "interconnects",
>> +					"#interconnect-cells", 0, &args);
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +
>> +	icc_node_data = of_icc_get_from_provider(&args);
>> +	of_node_put(args.np);
>> +
>> +	if (IS_ERR(icc_node_data))
>> +		return ERR_CAST(icc_node_data);
>> +
>> +	icc_node = icc_node_data->node;
>> +	kfree(icc_node_data);
>> +
>> +	return icc_node;
>> +}
> 
> I have a question about exynos_icc_get_parent().
> As I checked, this function returns the only one icc_node
> as parent node. But, bus_display dt node in the exynos4412.dtsi
> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
> 
> When I checked the return value of exynos_icc_get_parent()
> during probing for bus_display device, exynos_icc_get_parent() function
> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> of icc node?

Yes, as we use the interconnect consumer bindings we need to specify a path,
i.e. a <initiator, target> pair. When the provider node initializes it will
link itself to that path. Currently the provider driver uses just the first 
phandle.

> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -472,7 +472,7 @@
>                         clocks = <&clock CLK_ACLK160>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_display_opp_table>;
>                         interconnects = <&bus_leftbus &bus_dmc>;
>                         #interconnect-cells = <0>;
>                         status = "disabled";
>                 };

-- 
Regards,
Sylwester