Re: [PATCH] soc/qcom: add OCMEM driver

[Rob Clark <robdclark@gmail.com>]

On Tue, Oct 6, 2015 at 4:35 AM, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
> Hi Rob,
>
> <snip>
>
>> +
>> +struct ocmem_config {
>> +     uint8_t  num_regions;
>
> u8
>
>> +     uint32_t macro_size;
>
> u32

tbh, only place where stdint types are potentially an issue is in
uapi.  I changed the qcom-scm patches to use uN types instead since
the rest of the file was already using those (and when in Rome..)

but otherwise I prefer stdint types, since they are, well, standard..

>
>> +static const struct ocmem_config ocmem_8974_config = {
>> +     .num_regions = 3, .macro_size = SZ_128K,
>> +};
>> +
>> +static const struct of_device_id ocmem_dt_match[] = {
>> +     { .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },
>
> Just a suggestion. As we have REG_OCMEM_HW_PROFILE and
> REG_OCMEM_HW_VERSION register why not distinguish by version and hw
> profile instead of SoC?

I take what I can from HW_PROFILE reg, but afaict macro_size and
num_regions is missing..

>> +     {}
>> +};
>> +
>> +static int ocmem_dev_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     const struct ocmem_config *config = NULL;
>> +     const struct of_device_id *match;
>> +     struct resource *res;
>> +     uint32_t reg, num_banks, region_size;
>> +     int i, j, ret;
>> +
>> +     /* we need scm to be available: */
>> +     if (!qcom_scm_is_available())
>> +             return -EPROBE_DEFER;
>> +
>> +     match = of_match_device(ocmem_dt_match, dev);
>> +     if (match)
>> +             config = match->data;
>> +
>> +     if (!config) {
>> +             dev_err(dev, "unknown config: %s\n", dev->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");
>
> this dev_err will flood console in EPROBE_DEFER case, IMO should be removed.

It is kind of nice to have some hints when debugging probe-defer or
fail to load cases.. I guess I could change that to dev_info() for the
probe-defer case..

>> +             return PTR_ERR(ocmem->core_clk);
>> +     }
>> +
>> +     ocmem->iface_clk = devm_clk_get(dev, "iface_clk");
>> +     if (IS_ERR_OR_NULL(ocmem->iface_clk)) {
>> +             ret = PTR_ERR(ocmem->iface_clk);
>> +             ocmem->iface_clk = NULL;
>> +             /* in probe-defer case, propagate error up and try again later: */
>> +             if (ret == -EPROBE_DEFER)
>> +                     goto fail;
>
> Why error path of the iface clock is different comparing with core clk.

I guess there are some cases where there is no iface_clk?  This is
just based on what downstream driver was doing, which is the only
documentation I have ;-)

If someone knows better, I'd be happy to change it..

> Also I failed to found a case when common clk framework will return NULL?
>
> if (IS_ERR(ocmem->iface_clk)
>         return PTR_ERR(ocmem->iface_clk);
>
> this should be enough.

ok

>> +     }
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +                     "ocmem_ctrl_physical");
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "failed to get memory resource\n");
>> +             ret = -EINVAL;
>> +             goto fail;
>> +     }
>> +
>> +     ocmem->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
>> +                     resource_size(res));
>
> I think you could use following to simplify ioremap:
>
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>                         "ocmem_ctrl_physical");
>         ocmem->mmio = devm_ioremap_resource(dev, res);
>         if (IS_ERR(ocmem->mmio))
>                 return PTR_ERR(ocmem->mmio);

sure

BR,
-R


>> +     if (IS_ERR(ocmem->mmio)) {
>> +             dev_err(&pdev->dev, "failed to ioremap memory resource\n");
>> +             ret = -EINVAL;
>> +             goto fail;
>> +     }
>> +
>> +     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;
>> +     }
>> +
>
> <snip>
>
> --
> regards,
> Stan