Re: [PATCH] soc/qcom: add OCMEM driver

From: Stanimir Varbanov <svarbanov@mm-sol.com>
To: Rob Clark <robdclark@gmail.com>, linux-arm-msm@vger.kernel.org
Cc: Stephen Boyd <sboyd@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@sonymobile.com>
Subject: Re: [PATCH] soc/qcom: add OCMEM driver
Date: Fri, 23 Oct 2015 15:11:49 +0300	[thread overview]
Message-ID: <562A2405.7070103@mm-sol.com> (raw)
In-Reply-To: <1444686342-19344-1-git-send-email-robdclark@gmail.com>

Hi Rob,

On 10/13/2015 12:45 AM, Rob Clark wrote:
> The OCMEM driver handles allocation and configuration of the On Chip
> MEMory that is present on some snapdragon devices.
> 
> Devices which have OCMEM do not have GMEM inside the gpu core, so the
> gpu must instead use OCMEM to be functional.  Since currently the gpu
> is the only OCMEM user with an upstream driver, this is just a minimal
> implementation sufficient for statically allocating to the gpu it's
> chunk of OCMEM.
> 
> v2: minor updates from review comments (use devm_ioremap_resource(),
> devm_clk_get() doesn't return NULL, etc..)
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/soc/qcom/Kconfig     |   6 +
>  drivers/soc/qcom/Makefile    |   1 +
>  drivers/soc/qcom/ocmem.c     | 401 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/soc/qcom/ocmem.xml.h | 106 ++++++++++++
>  include/soc/qcom/ocmem.h     |  40 +++++
>  5 files changed, 554 insertions(+)
>  create mode 100644 drivers/soc/qcom/ocmem.c
>  create mode 100644 drivers/soc/qcom/ocmem.xml.h
>  create mode 100644 include/soc/qcom/ocmem.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 266060b..b485043 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -74,3 +74,9 @@ config QCOM_WCNSS_CTRL
>  	help
>  	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
>  	  firmware to a newly booted WCNSS chip.
> +
> +config QCOM_OCMEM
> +	tristate "Qualcomm OCMEM driver"
> +	depends on QCOM_SMD

Why this driver depends on SMD, I cannot see where smd is used here.

Maybe depends on ARCH_QCOM should be sufficient?

> +	help
> +	  Allocator for OCMEM (On Chip MEMory).
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index fdd664e..940bb35 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>  obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
>  obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> +obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
> new file mode 100644
> index 0000000..506a072
> --- /dev/null
> +++ b/drivers/soc/qcom/ocmem.c
> @@ -0,0 +1,401 @@
> +/*
> + * Copyright (C) 2015 Red Hat
> + * Author: Rob Clark <robdclark@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom_scm.h>

Do we need to update the Kconfig to enable/depend on scm driver?

> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <soc/qcom/ocmem.h>
> +#include "ocmem.xml.h"
> +
> +enum region_mode {
> +	WIDE_MODE = 0x0,
> +	THIN_MODE,
> +	MODE_DEFAULT = WIDE_MODE,
> +};
> +
> +struct ocmem_region {
> +	unsigned psgsc_ctrl;

this is not used

> +	bool interleaved;
> +	enum region_mode mode;
> +	unsigned int num_macros;
> +	enum ocmem_macro_state macro_state[4];
> +	unsigned long macro_size;
> +	unsigned long region_size;
> +};
> +

<snip>

> +
> +static const struct of_device_id ocmem_dt_match[] = {
> +	{ .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },
> +	{}
> +};
> +
> +static int ocmem_dev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct ocmem_config *config = NULL;
> +	const struct of_device_id *match;
> +	struct resource *res;
> +	uint32_t reg, num_banks, region_size;
> +	int i, j, ret;
> +
> +	/* we need scm to be available: */
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	match = of_match_device(ocmem_dt_match, dev);
> +	if (match)
> +		config = match->data;
> +
> +	if (!config) {
> +		dev_err(dev, "unknown config: %s\n", dev->of_node->name);
> +		return -ENXIO;
> +	}
> +
> +	ocmem = devm_kzalloc(dev, sizeof(*ocmem), GFP_KERNEL);
> +	if (!ocmem)
> +		return -ENOMEM;
> +
> +	ocmem->dev = dev;
> +	ocmem->config = config;
> +
> +	ocmem->core_clk = devm_clk_get(dev, "core_clk");

We have a convention to avoid _clk suffix for qcom clocks in dt nodes.

> +	if (IS_ERR(ocmem->core_clk)) {
> +		dev_info(dev, "Unable to get the core clock\n");
> +		return PTR_ERR(ocmem->core_clk);
> +	}
> +
> +	ocmem->iface_clk = devm_clk_get(dev, "iface_clk");
> +	if (IS_ERR(ocmem->iface_clk)) {
> +		ret = PTR_ERR(ocmem->iface_clk);
> +		ocmem->iface_clk = NULL;
> +		/* in probe-defer case, propagate error up and try again later: */
> +		if (ret == -EPROBE_DEFER)
> +			goto fail;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +			"ocmem_ctrl_physical");
> +	ocmem->mmio = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ocmem->mmio)) {
> +		dev_err(&pdev->dev, "failed to ioremap memory resource\n");
> +		ret = -EINVAL;

ret = PTR_ERR(ocmem->mmio); ?

> +		goto fail;
> +	}
> +

<snip>

> +
> +static bool ocmem_exists(void)
> +{
> +	struct device_driver *drv = &ocmem_driver.driver;
> +	struct device *d;
> +
> +	d = bus_find_device(&platform_bus_type, NULL, drv,
> +			(void *)platform_bus_type.match);
> +	if (d) {
> +		put_device(d);
> +		return true;
> +	}
> +
> +	return false;
> +}

do you have a dt binding document? I think that ocmem dt client nodes
should have a phandle to ocmem dt node and this could eliminate the need
of ocmem_exist(), no ?

<snip>

-- 
regards,
Stan