On 2018-05-18 14:01, Andy Shevchenko wrote:
On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
<rishabhb@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
