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@xxxxxxxxxxx>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> +++ 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
