On Mon, Sep 28, 2015 at 6:10 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: > 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. yeah, should be ocmem_hdl I would kind of prefer to keep the check for null in the caller, just to simplify backports to 3.10 kernel (since otherwise the API matches downstream).. Although I guess downstream checks for null and spits out error msg and returns -EINVAL, so maybe that is enough.. >> 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@xxxxxxxxx> >> + * >> + * 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? needed for qcom_scm.h, although I guess I could just add the missing #includes in qcom_scm.h instead.. >> +#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? not currently.. downstream was saving it off in pdata but on closer look it doesn't seem to use it after probe either.. >> + unsigned num_macros; >> + bool interleaved; > > Is this used after probe? again, cargo culted from downstream, but it looks like we can drop.. >> + >> + 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? hmm, msm_writel() doesn't have any more barrier than writel().. so I kept the mb() from downstream.. >> + } >> + >> + 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? client would be used if we supported more than just gfx, so I left this param in case some day someone wants to support more than just gfx.. >> +{ >> + /* 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. yes, but I prefer them ;-) >> + 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. this was mostly just planning ahead for other users and/or moving ocmem out of drm/msm (ie. if it was a loadable module) >> + 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. sure, I could change that.. I was a bit lazy about including bindings doc, although right now it is purely generic binding, ie: qcom,msm-ocmem-8974@fdd00000 { compatible = "qcom,ocmem-msm8974"; reg-names = "ocmem_ctrl_physical", "ocmem_physical"; reg = <0xfdd00000 0x2000>, <0xfec00000 0x180000>; clock-names = "core_clk", "iface_clk"; clocks = <&rpmcc RPM_OCMEMGX_A_CLK>, <&mmcc OCMEMCX_OCMEMNOC_CLK>; }; >> + {} >> +}; >> + >> +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. saves me adding debug prints later when debugging probe-defer fun ;-) >> + 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. hmm, true, it is coming from a different clk driver compared to core_clk.. >> + 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? currently, no, since it is not split out into it's own .ko.. but it would eventually when split out (but iirc I'd end up w/ duplicate symbols issue if I added MODULE_DEVICE_TABLE() currently) >> + >> +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. currently, I don't think it would be.. but would be if split out of drm/msm.. BR, -R > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
