LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: rishabhb@codeaurora.org
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	linux-arm-msm@vger.kernel.org,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm@lists.infradead.org, tsoni@codeaurora.org,
	ckadabi@codeaurora.org, evgreen@chromium.org,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver
Date: Tue, 22 May 2018 11:33:43 -0700	[thread overview]
Message-ID: <f1970a64be17267e0f044242dc0764d6@codeaurora.org> (raw)
In-Reply-To: <CAHp75VdO7+u-PQ+RBH71sxHHG0WkAyEVxv0BNPaycbcRnrZang@mail.gmail.com>

On 2018-05-18 14:01, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
> <rishabhb@codeaurora.org> wrote:
>> LLCC (Last Level Cache Controller) provides additional cache memory
>> in the system. LLCC is partitioned into multiple slices and each
>> slice gets its own priority, size, ID and other config parameters.
>> LLCC driver programs these parameters for each slice. Clients that
>> are assigned to use LLCC need to get information such size & ID of the
>> slice they get and activate or deactivate the slice as needed. LLCC 
>> driver
>> provides API for the clients to perform these operations.
> 
>> +static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
>> +       { .compatible = "qcom,sdm845-llcc", },
> 
>> +       { },
> 
> Slightly better w/o comma
Changed
> 
>> +};
> 
>> +static struct platform_driver sdm845_qcom_llcc_driver = {
>> +       .driver = {
>> +               .name = "sdm845-llcc",
> 
>> +               .owner = THIS_MODULE,
> 
> No need. See below.
> 
>> +               .of_match_table = sdm845_qcom_llcc_of_match,
>> +       },
>> +       .probe = sdm845_qcom_llcc_probe,
>> +};
> 
>> +
>> +static int __init sdm845_init_qcom_llcc_init(void)
>> +{
>> +       return platform_driver_register(&sdm845_qcom_llcc_driver);
>> +}
>> +module_init(sdm845_init_qcom_llcc_init);
>> +
>> +static void __exit sdm845_exit_qcom_llcc_exit(void)
>> +{
>> +       platform_driver_unregister(&sdm845_qcom_llcc_driver);
>> +}
>> +module_exit(sdm845_exit_qcom_llcc_exit);
> 
> Why not to use module_platform_driver() macro?
Done
> 
>> +#define ACTIVATE                      0x1
>> +#define DEACTIVATE                    0x2
>> +#define ACT_CTRL_OPCODE_ACTIVATE      0x1
>> +#define ACT_CTRL_OPCODE_DEACTIVATE    0x2
>> +#define ACT_CTRL_ACT_TRIG             0x1
> 
> Are these bits? Perhaps BIT() ?
> 
isn't it just better to use fixed size as u suggest in the next comment?

>> +#define ACT_CTRL_OPCODE_SHIFT         0x1
>> +#define ATTR1_PROBE_TARGET_WAYS_SHIFT 0x2
>> +#define ATTR1_FIXED_SIZE_SHIFT        0x3
>> +#define ATTR1_PRIORITY_SHIFT          0x4
>> +#define ATTR1_MAX_CAP_SHIFT           0x10
> 
> Better to use fixed size pattern, i.e. 0x01, 0x02, 0x03, 0x04, 0x10.
> 
>> +#define ATTR0_RES_WAYS_MASK           0x00000fff
>> +#define ATTR0_BONUS_WAYS_MASK         0x0fff0000
> 
> GENMASK()
Done
> 
>> +#define LLCC_LB_CNT_MASK               0xf0000000
> 
> Ditto.
> 
>> +#define MAX_CAP_TO_BYTES(n) (n * 1024)
> 
> (n * SZ_1K) ?
Done
> 
>> +#define LLCC_TRP_ACT_CTRLn(n) (n * 0x1000)
> 
> SZ_4K ?
> 
>> +#define LLCC_TRP_STATUSn(n)   (4 + n * 0x1000)
> 
> Ditto.
> 
>> +struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>> +{
>> +       const struct llcc_slice_config *cfg;
>> +       struct llcc_slice_desc *desc;
>> +       u32 sz, count = 0;
>> +
>> +       cfg = drv_data->cfg;
>> +       sz = drv_data->cfg_size;
>> +
> 
>> +       while (cfg && count < sz) {
>> +               if (cfg->usecase_id == uid)
>> +                       break;
>> +               cfg++;
>> +               count++;
>> +       }
>> +       if (cfg == NULL || count == sz)
>> +               return ERR_PTR(-ENODEV);
> 
> if (!cfg)
>           return ERR_PTR(-ENODEV);
> 
> while (cfg->... != uid) {
>   cfg++;
>   count++;
> }
> 
> if (count == sz)
>  return ...
> 
> Though I would rather put it to for () loop.
> 
In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.

>> +static int llcc_update_act_ctrl(u32 sid,
>> +                               u32 act_ctrl_reg_val, u32 status)
>> +{
>> +       u32 act_ctrl_reg;
>> +       u32 status_reg;
>> +       u32 slice_status;
> 
>> +       int ret = 0;
> 
> Useless assignment. Check entire patch series for a such.
Checked and  removed these.
> 
>> +       ret = regmap_read_poll_timeout(drv_data->regmap, status_reg,
>> +       slice_status, !(slice_status & status), 0, 
>> LLCC_STATUS_READ_DELAY);
> 
> Wrong indentation.
Corrected
> 
>> +       return ret;
>> +}
> 
>> +       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
>> +                                 DEACTIVATE);
> 
> Perhaps one line (~83 characters here is OK) ?
The checkpatch script complains about such lines.
> 
>> +       ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
>> +                                 ACTIVATE);
> 
> Ditto.
> 
>> +               attr1_cfg = bcast_off +
>> +                               
>> LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
>> +               attr0_cfg = bcast_off +
>> +                               
>> LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);
> 
> Ditto.
> 
>> +               attr1_val |= llcc_table[i].probe_target_ways <<
>> +                               ATTR1_PROBE_TARGET_WAYS_SHIFT;
>> +               attr1_val |= llcc_table[i].fixed_size <<
>> +                               ATTR1_FIXED_SIZE_SHIFT;
>> +               attr1_val |= llcc_table[i].priority << 
>> ATTR1_PRIORITY_SHIFT;
> 
> foo |=
>   bar << SHIFT;
> 
> would look slightly better.
> 
>> +int qcom_llcc_probe(struct platform_device *pdev,
>> +                     const struct llcc_slice_config *llcc_cfg, u32 
>> sz)
>> +{
> 
>> +       drv_data->offsets = devm_kzalloc(dev, num_banks * sizeof(u32),
>> +                                                       GFP_KERNEL);
>> +       if (!drv_data->offsets)
>> +               return -ENOMEM;
> 
> devm_kcalloc() ?
changed
>> +
>> +       for (i = 0; i < num_banks; i++)
>> +               drv_data->offsets[i] = (i * BANK_OFFSET_STRIDE);
> 
> Pointless parens.
Removed
> 
>> +       drv_data->bitmap = devm_kcalloc(dev,
>> +       BITS_TO_LONGS(drv_data->max_slices), sizeof(unsigned long),
>> +                                               GFP_KERNEL);
>> +       if (!drv_data->bitmap)
>> +               return -ENOMEM;
> 
> Perhaps at some point someone will add
> bitmap_alloc()
> devm_bitmap_alloc()
> 
>> +       bitmap_zero(drv_data->bitmap, drv_data->max_slices);
> 
> Pointless
Removed

  reply	other threads:[~2018-05-22 18:33 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-16 17:43 [PATCH v7 0/2] SDM845 System Cache Driver Rishabh Bhatnagar
2018-05-16 17:43 ` [PATCH v7 1/2] dt-bindings: Documentation for qcom, llcc Rishabh Bhatnagar
2018-05-18 14:33   ` Rob Herring
2018-05-16 17:43 ` [PATCH v7 2/2] drivers: soc: Add LLCC driver Rishabh Bhatnagar
2018-05-17 22:30   ` Evan Green
2018-05-18 21:01   ` Andy Shevchenko
2018-05-22 18:33     ` rishabhb [this message]
2018-05-22 19:38       ` Andy Shevchenko
2018-05-22 20:40         ` rishabhb
2018-05-22 20:46           ` Andy Shevchenko
2018-05-23 17:59         ` rishabhb

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f1970a64be17267e0f044242dc0764d6@codeaurora.org \
    --to=rishabhb@codeaurora.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=ckadabi@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=evgreen@chromium.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-arm@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=tsoni@codeaurora.org \
    --subject='Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).