lkml.org 
[lkml]   [2023]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus
    From
    Hi Jonathan,

    On 1/28/23 17:12, Jonathan Cameron wrote:
    > On Fri, 27 Jan 2023 17:40:38 +0100
    > Gatien Chevallier <gatien.chevallier@foss.st.com> wrote:
    >
    >> This driver is checking the access rights of the different
    >> peripherals connected to the system bus. If access is denied,
    >> the associated device tree node is skipped so the platform bus
    >> does not probe it.
    >>
    >> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
    >> Signed-off-by: Loic PALLARDY <loic.pallardy@st.com>
    >
    > Hi Gatien,
    >
    > A few comments inline,
    >
    > Thanks,
    >
    > Jonathan
    >
    >> diff --git a/drivers/bus/stm32_sys_bus.c b/drivers/bus/stm32_sys_bus.c
    >> new file mode 100644
    >> index 000000000000..c12926466bae
    >> --- /dev/null
    >> +++ b/drivers/bus/stm32_sys_bus.c
    >> @@ -0,0 +1,168 @@
    >> +// SPDX-License-Identifier: GPL-2.0
    >> +/*
    >> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
    >> + */
    >> +
    >> +#include <linux/bitfield.h>
    >> +#include <linux/bits.h>
    >> +#include <linux/device.h>
    >> +#include <linux/err.h>
    >> +#include <linux/io.h>
    >> +#include <linux/init.h>
    >> +#include <linux/kernel.h>
    >> +#include <linux/module.h>
    >> +#include <linux/of.h>
    >> +#include <linux/of_platform.h>
    >> +#include <linux/platform_device.h>
    >> +
    >> +/* ETZPC peripheral as firewall bus */
    >> +/* ETZPC registers */
    >> +#define ETZPC_DECPROT 0x10
    >> +
    >> +/* ETZPC miscellaneous */
    >> +#define ETZPC_PROT_MASK GENMASK(1, 0)
    >> +#define ETZPC_PROT_A7NS 0x3
    >> +#define ETZPC_DECPROT_SHIFT 1
    >
    > This define makes the code harder to read. What we care about is
    > the number of bits in the register divided by number of entries.
    > (which is 2) hence the shift by 1. See below for more on this.
    >
    >
    >> +
    >> +#define IDS_PER_DECPROT_REGS 16
    >
    >> +#define STM32MP15_ETZPC_ENTRIES 96
    >> +#define STM32MP13_ETZPC_ENTRIES 64
    >
    > These defines just make the code harder to check.
    > They aren't magic numbers, but rather just telling us how many
    > entries there are, so I would just put them in the structures directly.
    > Their use make it clear what they are without needing to give them a name.
    >

    Honestly, I'd rather read the hardware configuration registers to get
    this information instead of differentiating MP13/15. Would you agree on
    that?

    >
    >> +struct stm32_sys_bus_match_data {
    >
    > Comment on naming of this below.
    >
    >> + unsigned int max_entries;
    >> +};
    >> +
    >
    > +static int stm32_etzpc_get_access(struct sys_bus_data *pdata, struct device_node *np)
    > +{
    > + int err;
    > + u32 offset, reg_offset, sec_val, id;
    > +
    > + err = stm32_sys_bus_get_periph_id(pdata, np, &id);
    > + if (err)
    > + return err;
    > +
    > + /* Check access configuration, 16 peripherals per register */
    > + reg_offset = ETZPC_DECPROT + 0x4 * (id / IDS_PER_DECPROT_REGS);
    > + offset = (id % IDS_PER_DECPROT_REGS) << ETZPC_DECPROT_SHIFT;
    >
    > Use of defines in here is actively unhelpful when it comes to review. I would suggest letting
    > the maths be self explanatory (even if it's more code).
    >
    > offset = (id % IDS_PER_DECPROT_REGS) * (sizeof(u32) * BITS_PER_BYTE / IDS_PER_DECPROT_REGS);
    >
    > Or if you prefer have a define of
    >
    > #define DECPROT_BITS_PER_ID (sizeof(u32) * BITS_PER_BYTE / IDS_PER_DECPROT_REGS)
    >
    > and
    > offset = (id % IDS_PER_DECPROT_REGS) * DECPROT_BITS_PER_ID;
    >

    Ok I'll rework this for better understanding. Your suggestion seems fine

    > +
    > + /* Verify peripheral is non-secure and attributed to cortex A7 */
    > + sec_val = (readl(pdata->sys_bus_base + reg_offset) >> offset) & ETZPC_PROT_MASK;
    > + if (sec_val != ETZPC_PROT_A7NS) {
    > + dev_dbg(pdata->dev, "Invalid bus configuration: reg_offset %#x, value %d\n",
    > + reg_offset, sec_val);
    > + return -EACCES;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > ...
    >
    >> +static int stm32_sys_bus_probe(struct platform_device *pdev)
    >> +{
    >> + struct sys_bus_data *pdata;
    >> + void __iomem *mmio;
    >> + struct device_node *np = pdev->dev.of_node;
    >
    > I'd be consistent. You use dev_of_node() accessor elsewhere, so should
    > use it here as well >> +
    >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
    >> + if (!pdata)
    >> + return -ENOMEM;
    >> +
    >> + mmio = devm_platform_ioremap_resource(pdev, 0);
    >> + if (IS_ERR(mmio))
    >> + return PTR_ERR(mmio);
    >> +
    >> + pdata->sys_bus_base = mmio;
    >> + pdata->pconf = of_device_get_match_data(&pdev->dev);
    >> + pdata->dev = &pdev->dev;
    >> +
    >> + platform_set_drvdata(pdev, pdata);
    >
    > Does this get used? I can't immediately spot where but maybe I just
    > missed it.
    >

    Not for now :)

    >> +
    >> + stm32_sys_bus_populate(pdata);
    >> +
    >> + /* Populate all available nodes */
    >> + return of_platform_populate(np, NULL, NULL, &pdev->dev);
    >
    > As np only used here, I'd not bother with the local variable in this function.
    >

    Agreed

    >> +}
    >> +
    >> +static const struct stm32_sys_bus_match_data stm32mp15_sys_bus_data = {
    >
    > Naming a structure after where it comes from is a little unusual and
    > confusion when a given call gets it from somewhere else.
    >
    > I'd expect it to be named after what sort of thing it contains.
    > stm32_sys_bus_info or something like that.
    >

    Then, this shall be removed thanks to the read to hardware registers.

    >> + .max_entries = STM32MP15_ETZPC_ENTRIES,
    >> +};
    >> +
    >> +static const struct stm32_sys_bus_match_data stm32mp13_sys_bus_data = {
    >> + .max_entries = STM32MP13_ETZPC_ENTRIES,
    >> +};
    >> +
    >> +static const struct of_device_id stm32_sys_bus_of_match[] = {
    >> + { .compatible = "st,stm32mp15-sys-bus", .data = &stm32mp15_sys_bus_data },
    >> + { .compatible = "st,stm32mp13-sys-bus", .data = &stm32mp13_sys_bus_data },
    >
    > Alphabetical order usually preferred when there isn't a strong reason for
    > another choice.
    >

    I second that

    >> + {}
    >> +};
    >> +MODULE_DEVICE_TABLE(of, stm32_sys_bus_of_match);
    >> +
    >> +static struct platform_driver stm32_sys_bus_driver = {
    >> + .probe = stm32_sys_bus_probe,
    >> + .driver = {
    >> + .name = "stm32-sys-bus",
    >> + .of_match_table = stm32_sys_bus_of_match,
    >> + },
    >> +};
    >> +
    >> +static int __init stm32_sys_bus_init(void)
    >> +{
    >> + return platform_driver_register(&stm32_sys_bus_driver);
    >> +}
    >> +arch_initcall(stm32_sys_bus_init);
    >> +
    >
    > Unwanted trailing blank line.
    >

    Good spot, thanks

    >

    Best regards,
    Gatien

    \
     
     \ /
      Last update: 2023-03-27 00:12    [W:8.596 / U:0.976 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site