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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux