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 at 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
