Re: [PATCH 4/4] drm/msm: add OCMEM driver

[Stephen Boyd <sboyd@codeaurora.org>]

On 09/28, Rob Clark wrote:
> @@ -322,10 +319,8 @@ static void a3xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
>  	if (a3xx_gpu->ocmem_base)

Is this supposed to be ocmem_base or ocmem_hdl? Perhaps this
check could be put inside the ocmem_free() itself so that the
caller doesn't have to care.

>  		ocmem_free(OCMEM_GRAPHICS, a3xx_gpu->ocmem_hdl);
> -#endif
>  
>  	kfree(a3xx_gpu);
>  }
> @@ -289,10 +288,8 @@ static void a4xx_destroy(struct msm_gpu *gpu)
>  
>  	adreno_gpu_cleanup(adreno_gpu);
>  
> -#ifdef CONFIG_MSM_OCMEM
> -	if (a4xx_gpu->ocmem_base)
> +	if (a4xx_gpu->ocmem_hdl)

This one changed, so a3xx above seems highly suspicious.

>  		ocmem_free(OCMEM_GRAPHICS, a4xx_gpu->ocmem_hdl);
> -#endif
>  
>  	kfree(a4xx_gpu);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 2bbe85a..f042ba8 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -172,4 +172,7 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev);
>  void __init adreno_register(void);
>  void __exit adreno_unregister(void);
>  
> +void __init ocmem_register(void);
> +void __exit ocmem_unregister(void);

__init and __exit in header files is useless

> +
>  #endif /* __MSM_GPU_H__ */
> diff --git a/drivers/gpu/drm/msm/ocmem/ocmem.c b/drivers/gpu/drm/msm/ocmem/ocmem.c
> new file mode 100644
> index 0000000..d3cdd64
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/ocmem/ocmem.c
> @@ -0,0 +1,396 @@
> +/*
> + * 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/kernel.h>
> +#include <linux/cpuset.h>

What is this include for?

> +#include <linux/qcom_scm.h>
> +
> +#include "msm_drv.h"
> +#include "ocmem.h"
> +#include "ocmem.xml.h"
> +
> +enum region_mode {
> +	WIDE_MODE = 0x0,
> +	THIN_MODE,
> +	MODE_DEFAULT = WIDE_MODE,
> +};
> +
> +enum ocmem_tz_client {
> +	TZ_UNUSED = 0x0,
> +	TZ_GRAPHICS,
> +	TZ_VIDEO,
> +	TZ_LP_AUDIO,
> +	TZ_SENSORS,
> +	TZ_OTHER_OS,
> +	TZ_DEBUG,
> +};
> +
> +struct ocmem_region {
> +	unsigned psgsc_ctrl;
> +	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;
> +};
> +
> +struct ocmem_config {
> +	uint8_t  num_regions;
> +	uint32_t macro_size;
> +};
> +
> +struct ocmem {
> +	struct device *dev;
> +	const struct ocmem_config *config;
> +	struct resource *ocmem_mem;
> +	struct clk *core_clk;
> +	struct clk *iface_clk;
> +	void __iomem *mmio;
> +
> +	unsigned num_ports;

Is this used after probe?

> +	unsigned num_macros;
> +	bool interleaved;

Is this used after probe?

> +
> +	struct ocmem_region *regions;
> +};
> +
> +struct ocmem *ocmem;

static?

> +
> +static bool ocmem_exists(void);
> +
> +static inline void ocmem_write(struct ocmem *ocmem, u32 reg, u32 data)
> +{
> +	msm_writel(data, ocmem->mmio + reg);
> +}
> +
> +static inline u32 ocmem_read(struct ocmem *ocmem, u32 reg)
> +{
> +	return msm_readl(ocmem->mmio + reg);
> +}
> +
> +static int ocmem_clk_enable(struct ocmem *ocmem)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(ocmem->core_clk);
> +	if (ret)
> +		return ret;
> +
> +	if (ocmem->iface_clk) {
> +		ret = clk_prepare_enable(ocmem->iface_clk);

clk_prepare_enable() on NULL does nothing so it should be safe to
drop the if.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void update_ocmem(struct ocmem *ocmem)
> +{
> +	uint32_t region_mode_ctrl = 0x0;
> +	unsigned pos = 0;
> +	unsigned i = 0;
> +
> +	if (!qcom_scm_ocmem_lock_available()) {
> +		for (i = 0; i < ocmem->config->num_regions; i++) {
> +			struct ocmem_region *region = &ocmem->regions[i];
> +			pos = i << 2;
> +			if (region->mode == THIN_MODE)
> +				region_mode_ctrl |= BIT(pos);
> +		}
> +		dev_dbg(ocmem->dev, "ocmem_region_mode_control %x\n", region_mode_ctrl);
> +		ocmem_write(ocmem, REG_OCMEM_REGION_MODE_CTL, region_mode_ctrl);
> +		/* Barrier to commit the region mode */
> +		mb();

msm_writel() already has a barrier, so now we have a double
barrier?

> +	}
> +
> +	for (i = 0; i < ocmem->config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +
> +		ocmem_write(ocmem, REG_OCMEM_PSGSC_CTL(i),
> +				OCMEM_PSGSC_CTL_MACRO0_MODE(region->macro_state[0]) |
> +				OCMEM_PSGSC_CTL_MACRO1_MODE(region->macro_state[1]) |
> +				OCMEM_PSGSC_CTL_MACRO2_MODE(region->macro_state[2]) |
> +				OCMEM_PSGSC_CTL_MACRO3_MODE(region->macro_state[3]));
> +	}
> +}
> +
> +inline unsigned long phys_to_offset(unsigned long addr)

static? drop inline?

> +{
> +	if ((addr < ocmem->ocmem_mem->start) ||
> +		(addr >= ocmem->ocmem_mem->end))
> +		return 0;
> +	return addr - ocmem->ocmem_mem->start;
> +}
> +
> +static unsigned long device_address(enum ocmem_client client, unsigned long addr)

client is not used, so remove it?

> +{
> +	/* TODO, gpu uses phys_to_offset, but others do not.. */
> +	return phys_to_offset(addr);
> +}
> +
> +static void update_range(struct ocmem *ocmem, struct ocmem_buf *buf,
> +		enum ocmem_macro_state mstate, enum region_mode rmode)
> +{
> +	unsigned long offset = 0;
> +	int i, j;
> +
> +	/*
> +	 * TODO probably should assert somewhere that range is aligned
> +	 * to macro boundaries..
> +	 */
> +
> +	for (i = 0; i < ocmem->config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +		if ((buf->offset <= offset) && (offset < (buf->offset + buf->len)))

useless parentheses here.

> +			region->mode = rmode;
> +		for (j = 0; j < region->num_macros; j++) {
> +			if ((buf->offset <= offset) && (offset < (buf->offset + buf->len)))
> +				region->macro_state[j] = mstate;
> +			offset += region->macro_size;
> +		}
> +	}
> +
> +	update_ocmem(ocmem);
> +}
> +
> +struct ocmem_buf *ocmem_allocate(enum ocmem_client client, unsigned long size)
> +{
> +	struct ocmem_buf *buf;
> +
> +	if (!ocmem) {
> +		if (ocmem_exists())

Does this ever trigger? From what I can tell the only place
ocmem_allocate() is called from is the open path of the gpu
device node, and that shouldn't happen until ocmem and gpu
drivers have both probed, in which case ocmem is already
non-NULLL or it never will exist so we should return ENXIO
without searching.

> +			return ERR_PTR(-EPROBE_DEFER);
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);

if (!buf)?

> +
> +	/*
> +	 * TODO less hard-coded allocation that works for more than
> +	 * one user:
> +	 */
> +
> +	buf->offset = 0;
> +	buf->addr = device_address(client, buf->offset);
> +	buf->len = size;
> +
> +	update_range(ocmem, buf, CORE_ON, WIDE_MODE);
> +
> +	if (qcom_scm_ocmem_lock_available()) {
> +		int ret;
> +		ret = qcom_scm_ocmem_lock(TZ_GRAPHICS, buf->offset, buf->len,
> +				WIDE_MODE);
> +		if (ret)
> +			dev_err(ocmem->dev, "could not lock: %d\n", ret);
> +	} else {
> +		if (client == OCMEM_GRAPHICS) {
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START, buf->offset);
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END, buf->offset + buf->len);
> +		}
> +	}
> +
> +	return buf;
> +}
> +
> +void ocmem_free(enum ocmem_client client, struct ocmem_buf *buf)
> +{
> +	update_range(ocmem, buf, CLK_OFF, MODE_DEFAULT);
> +
> +	if (qcom_scm_ocmem_lock_available()) {
> +		int ret;
> +		ret = qcom_scm_ocmem_unlock(TZ_GRAPHICS, buf->offset, buf->len);
> +		if (ret)
> +			dev_err(ocmem->dev, "could not lock: %d\n", ret);

could not unlock?

> +	} else {
> +		if (client == OCMEM_GRAPHICS) {
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_START, 0x0);
> +			ocmem_write(ocmem, REG_OCMEM_GFX_MPU_END, 0x0);
> +		}
> +	}
> +
> +	kfree(buf);
> +}
> +
> +static const struct ocmem_config ocmem_8974_config = {
> +	.num_regions = 3, .macro_size = SZ_128K,
> +};
> +
> +static const struct of_device_id dt_match[] = {

Perhaps ocmem_dt_match? There's probably quite a few dt_match
arrays out there.

> +	{ .compatible = "qcom,msm-ocmem-8974", .data = &ocmem_8974_config },

Is there a binding for this somewhere? I'd prefer we move msm
int the name to the numbers part and call it qcom,ocmem-msm8974.

> +	{}
> +};
> +
> +static int ocmem_dev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct ocmem_config *config = NULL;
> +	uint32_t reg, num_banks, region_size;
> +	int i, j, ret;
> +
> +	struct device_node *of_node = dev->of_node;
> +	const struct of_device_id *match;
> +
> +	match = of_match_node(dt_match, dev->of_node);
> +	if (match)
> +		config = match->data;
> +
> +	if (!config) {

Just use of_match_device() instead.

> +		dev_err(dev, "unknown config: %s\n", 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");
> +	if (IS_ERR(ocmem->core_clk)) {
> +		dev_err(dev, "Unable to get the core clock\n");

Maybe we should be silent, in case this returns a probe defer
error.

> +		return PTR_ERR(ocmem->core_clk);
> +	}
> +
> +	ocmem->iface_clk = devm_clk_get(dev, "iface_clk");
> +	if (IS_ERR_OR_NULL(ocmem->iface_clk))

This should make sure it isn't a probe defer error.

> +		ocmem->iface_clk = NULL;
> +
> +	/* The core clock is synchronous with graphics */
> +	WARN_ON(clk_set_rate(ocmem->core_clk, 1000) < 0);
> +
> +	ocmem->mmio = msm_ioremap(pdev, "ocmem_ctrl_physical", "OCMEM");
> +	if (IS_ERR(ocmem->mmio))
> +		return PTR_ERR(ocmem->mmio);
> +
> +	ocmem->ocmem_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +			"ocmem_physical");
> +	if (!ocmem->ocmem_mem) {
> +		dev_err(dev, "could not get OCMEM region\n");
> +		return -ENXIO;
> +	}
> +
> +	ret = ocmem_clk_enable(ocmem);
> +	if (ret)
> +		goto fail;
> +
> +	if (qcom_scm_is_available() && qcom_scm_ocmem_secure_available()) {
> +		dev_info(dev, "configuring scm\n");

debug noise?

> +		ret = qcom_scm_ocmem_secure_cfg(0x5);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	reg = ocmem_read(ocmem, REG_OCMEM_HW_PROFILE);
> +	ocmem->num_ports = FIELD(reg, OCMEM_HW_PROFILE_NUM_PORTS);
> +	ocmem->num_macros = FIELD(reg, OCMEM_HW_PROFILE_NUM_MACROS);
> +	ocmem->interleaved = !!(reg & OCMEM_HW_PROFILE_INTERLEAVING);
> +
> +	num_banks = ocmem->num_ports / 2;
> +	region_size = config->macro_size * num_banks;
> +
> +	dev_info(dev, "%u ports, %u regions, %u macros, %sinterleaved\n",
> +			ocmem->num_ports, config->num_regions, ocmem->num_macros,
> +			ocmem->interleaved ? "" : "not ");
> +
> +	ocmem->regions = devm_kzalloc(dev, sizeof(struct ocmem_region) *
> +			config->num_regions, GFP_KERNEL);

devm_kcalloc()?

> +	if (!ocmem->regions) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	for (i = 0; i < config->num_regions; i++) {
> +		struct ocmem_region *region = &ocmem->regions[i];
> +
> +		if (WARN_ON(num_banks > ARRAY_SIZE(region->macro_state))) {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
> +		region->mode = MODE_DEFAULT;
> +		region->num_macros = num_banks;
> +
> +		if ((i == (config->num_regions - 1)) &&

One too many parentheses here.

> +				(reg & OCMEM_HW_PROFILE_LAST_REGN_HALFSIZE)) {
> +			region->macro_size = config->macro_size / 2;
> +			region->region_size = region_size / 2;
> +		} else {
> +			region->macro_size = config->macro_size;
> +			region->region_size = region_size;
> +		}
> +
> +		for (j = 0; j < ARRAY_SIZE(region->macro_state); j++)
> +			region->macro_state[j] = CLK_OFF;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	dev_err(dev, "probe failed\n");

debug noise?

> +	ocmem_dev_remove(pdev);
> +	return ret;
> +}
> +
> +static struct platform_driver ocmem_driver = {
> +	.probe = ocmem_dev_probe,
> +	.remove = ocmem_dev_remove,
> +	.driver = {
> +		.name = "ocmem",
> +		.of_match_table = dt_match,
> +	},
> +};

Does this need a module table?

> +
> +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;
> +}

I hope this function isn't necessary.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project