[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d6c659d8-2e5c-cb60-d950-685c4ba319e2@foss.st.com>
Date: Tue, 7 Feb 2023 15:12:23 +0100
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: <Oleksii_Moisieiev@...m.com>, <gregkh@...uxfoundation.org>,
<herbert@...dor.apana.org.au>, <davem@...emloft.net>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<alexandre.torgue@...s.st.com>, <vkoul@...nel.org>,
<olivier.moysan@...s.st.com>, <arnaud.pouliquen@...s.st.com>,
<mchehab@...nel.org>, <fabrice.gasnier@...s.st.com>,
<ulf.hansson@...aro.org>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <linux-crypto@...r.kernel.org>,
<devicetree@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <dmaengine@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<alsa-devel@...a-project.org>, <linux-media@...r.kernel.org>,
<linux-mmc@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-phy@...ts.infradead.org>, <linux-serial@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <linux-usb@...r.kernel.org>,
Loic PALLARDY <loic.pallardy@...com>
Subject: Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and
STM32MP13 system bus
Hi Jonathan,
On 1/28/23 17:12, Jonathan Cameron wrote:
> On Fri, 27 Jan 2023 17:40:38 +0100
> Gatien Chevallier <gatien.chevallier@...s.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@...s.st.com>
>> Signed-off-by: Loic PALLARDY <loic.pallardy@...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
Powered by blists - more mailing lists