On Mon, Sep 28, 2015 at 6:10 PM, Stephen Boyd sboyd@codeaurora.org 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@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?
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