Message-ID: <20201104123729.GA13371@kozik-lap>
Date:   Wed, 4 Nov 2020 13:37:29 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Sylwester Nawrocki <s.nawrocki@...sung.com>
Cc:     georgi.djakov@...aro.org, cw00.choi@...sung.com,
        devicetree@...r.kernel.org, robh+dt@...nel.org,
        a.swigon@...sung.com, myungjoo.ham@...sung.com,
        inki.dae@...sung.com, sw0312.kim@...sung.com,
        b.zolnierkie@...sung.com, m.szyprowski@...sung.com,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 2/7] interconnect: Add generic interconnect driver for
 Exynos SoCs

On Wed, Nov 04, 2020 at 11:36:52AM +0100, 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 described by specifying in the 'interconnects' property
> the interconnect consumer path for each interconnect provider DT node.
> 
> 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 hard coded 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 samsung,data-clk-ratio DT property is used to specify the ratio
> of the interconect bandwidth to the minimum data clock frequency
> for each bus.
> 
> Due to unspecified relative probing order, -EPROBE_DEFER may be
> propagated to ensure that the parent is probed before its children.
> 
> Signed-off-by: Artur Świgoń <a.swigon@...sung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@...sung.com>
> ---
> Changes for v8:
>  - renamed drivers/interconnect/exynos to drivers/interconnect/samsung,
>  - added missing driver sync_state callback assignment.
> 
> Changes for v7:
>  - adjusted to the DT property changes: "interconnects" instead
>    of "samsung,interconnect-parent", "samsung,data-clk-ratio"
>    instead of "bus-width",
>  - adaptation to of_icc_get_from_provider() function changes
>    in v5.10-rc1.
> 
> Changes for v6:
>  - corrected of_node dereferencing in exynos_icc_get_parent()
>    function,
>  - corrected initialization of icc_node->name so as to avoid
>    direct of_node->name dereferencing,
>  - added parsing of bus-width DT property.
> 
> Changes for v5:
>  - adjust to renamed exynos,interconnect-parent-node property,
>  - use automatically generated platform device id as the interconect
>    node id instead of a now unavailable devfreq->id field,
>  - add icc_ prefix to some variables to make the code more self-commenting,
>  - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>  - adjust to exynos,interconnect-parent-node property rename to
>    samsung,interconnect-parent,
>  - converted to a separate platform driver in drivers/interconnect.
> ---
>  drivers/interconnect/Kconfig          |   1 +
>  drivers/interconnect/Makefile         |   1 +
>  drivers/interconnect/samsung/Kconfig  |  13 +++
>  drivers/interconnect/samsung/Makefile |   4 +
>  drivers/interconnect/samsung/exynos.c | 199 ++++++++++++++++++++++++++++++++++
>  5 files changed, 218 insertions(+)
>  create mode 100644 drivers/interconnect/samsung/Kconfig
>  create mode 100644 drivers/interconnect/samsung/Makefile
>  create mode 100644 drivers/interconnect/samsung/exynos.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..d637a89 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -13,5 +13,6 @@ if INTERCONNECT
>  
>  source "drivers/interconnect/imx/Kconfig"
>  source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/samsung/Kconfig"
>  
>  endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..c2f9e9d 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -6,3 +6,4 @@ icc-core-objs				:= core.o bulk.o
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>  obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> +obj-$(CONFIG_INTERCONNECT_SAMSUNG)	+= samsung/
> \ No newline at end of file

This needs a fix.

> diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig
> new file mode 100644
> index 0000000..508ed64
> --- /dev/null
> +++ b/drivers/interconnect/samsung/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_SAMSUNG
> +	bool "Samsung interconnect drivers"

"Samsung SoC interconnect drivers"

> +	depends on ARCH_EXYNOS || COMPILE_TEST

Don't the depend on INTERCONNECT?

> +	help
> +	  Interconnect drivers for Samsung SoCs.
> +
> +

One line break

> +config INTERCONNECT_EXYNOS
> +	tristate "Exynos generic interconnect driver"
> +	depends on INTERCONNECT_SAMSUNG

How about:
default y if ARCH_EXYNOS

> +	help
> +	  Generic interconnect driver for Exynos SoCs.
> diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile
> new file mode 100644
> index 0000000..e19d1df
> --- /dev/null
> +++ b/drivers/interconnect/samsung/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +exynos-interconnect-objs		:= exynos.o

What is this line for?

Best regards,
Krzysztof


> +
> +obj-$(CONFIG_INTERCONNECT_EXYNOS)	+= exynos-interconnect.o
> diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
> new file mode 100644
> index 0000000..6559d8c

Powered by blists - more mailing lists