From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50C7AC636CD for ; Tue, 7 Feb 2023 14:15:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231804AbjBGOPH (ORCPT ); Tue, 7 Feb 2023 09:15:07 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55912 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232343AbjBGOO5 (ORCPT ); Tue, 7 Feb 2023 09:14:57 -0500 Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0EFA96A51; Tue, 7 Feb 2023 06:14:55 -0800 (PST) Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 317ArBxI023356; Tue, 7 Feb 2023 15:12:28 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=bRPLjENYLm4EkeQxpcXBxnDMwaCRtrrOyJyCcEKFKwM=; b=v+r2Y+dW5qpe2evs4+p2Rph0HUeYJkhrIakAtRF4fJW7pf/iMoA2BrjUcXOgAS4O+JMl IXKV7Hp74w1KJP0PvEekg8P/i3SR3P0AwBgPlA+9ojsAnk0s7LDy3CWyXOWlKcwqqPfu wPE7CdTQ6u+DTonaxS8J7QYwU2uD3eROQ4b0UVJqA3TaS3QYgy35kLMkIT8d6Z2toWJ5 tMeMMxNPT8PXIpemJJgIQyZVYQ9Vv2xTMUAyQ8CNlfu2WPa6wKy9p9vIs1O/gAb/LlqC 1+7jKtxsEn7AYZrcWlego6inAn0ozOi+IQs8ea9RiHb5iRVStFIyQmQyADvaliPIL6nK lw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3nhfk72dkh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Feb 2023 15:12:28 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 763D6100034; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 48EEA21B516; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from [10.201.20.249] (10.201.20.249) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Tue, 7 Feb 2023 15:12:24 +0100 Message-ID: Date: Tue, 7 Feb 2023 15:12:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus Content-Language: en-US To: Jonathan Cameron CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Loic PALLARDY References: <20230127164040.1047583-1-gatien.chevallier@foss.st.com> <20230127164040.1047583-5-gatien.chevallier@foss.st.com> <20230128161217.0e79436e@jic23-huawei> From: Gatien CHEVALLIER In-Reply-To: <20230128161217.0e79436e@jic23-huawei> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.201.20.249] X-ClientProxiedBy: EQNCAS1NODE3.st.com (10.75.129.80) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-02-07_05,2023-02-06_03,2022-06-22_01 Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org Hi Jonathan, On 1/28/23 17:12, Jonathan Cameron wrote: > On Fri, 27 Jan 2023 17:40:38 +0100 > Gatien Chevallier 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 >> Signed-off-by: Loic PALLARDY > > 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CAE0CC636CC for ; Tue, 7 Feb 2023 14:14:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:CC:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Xm7bxTpdS61o4YNI80LHf0YtR+2VBX55hMsUKm1GDHk=; b=lsIkcVj/dMxBIR hTzxNfMy4TppUtL/V/rlktLUI5ElM4gzEHfIOX57LOzDtHQeROKCDgwYaVXRQeE99Mfe23mVTQTkZ LrmRwlHV96vcrwoKh6i1tHDwTKMPiVCjMe5stBOqihPP9qdaAcC0BTyiABf2xGOQoLGITYSmDcuD0 Og1LMmJetUYfV0fyN8Aplu1mRbWTprVxcknd5ENL/NHENeE3GotpgImdZXc1us5J+83wSZSMYN9ss 6GEpW1PgBQl8cM3nl834xaFR8hWVLDxtq0ybJIf03ZOjVjgemPZpGrQicoYbwN68tEtVWmsZ5LBAj 8PKMxA+P1vupgA0Mtliw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOkF-00CPax-8g; Tue, 07 Feb 2023 14:14:51 +0000 Received: from mx07-00178001.pphosted.com ([185.132.182.106]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOkC-00CPZq-Nj; Tue, 07 Feb 2023 14:14:50 +0000 Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 317ArBxI023356; Tue, 7 Feb 2023 15:12:28 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=bRPLjENYLm4EkeQxpcXBxnDMwaCRtrrOyJyCcEKFKwM=; b=v+r2Y+dW5qpe2evs4+p2Rph0HUeYJkhrIakAtRF4fJW7pf/iMoA2BrjUcXOgAS4O+JMl IXKV7Hp74w1KJP0PvEekg8P/i3SR3P0AwBgPlA+9ojsAnk0s7LDy3CWyXOWlKcwqqPfu wPE7CdTQ6u+DTonaxS8J7QYwU2uD3eROQ4b0UVJqA3TaS3QYgy35kLMkIT8d6Z2toWJ5 tMeMMxNPT8PXIpemJJgIQyZVYQ9Vv2xTMUAyQ8CNlfu2WPa6wKy9p9vIs1O/gAb/LlqC 1+7jKtxsEn7AYZrcWlego6inAn0ozOi+IQs8ea9RiHb5iRVStFIyQmQyADvaliPIL6nK lw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3nhfk72dkh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Feb 2023 15:12:28 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 763D6100034; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 48EEA21B516; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from [10.201.20.249] (10.201.20.249) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Tue, 7 Feb 2023 15:12:24 +0100 Message-ID: Date: Tue, 7 Feb 2023 15:12:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus Content-Language: en-US To: Jonathan Cameron CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Loic PALLARDY References: <20230127164040.1047583-1-gatien.chevallier@foss.st.com> <20230127164040.1047583-5-gatien.chevallier@foss.st.com> <20230128161217.0e79436e@jic23-huawei> From: Gatien CHEVALLIER In-Reply-To: <20230128161217.0e79436e@jic23-huawei> X-Originating-IP: [10.201.20.249] X-ClientProxiedBy: EQNCAS1NODE3.st.com (10.75.129.80) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-02-07_05,2023-02-06_03,2022-06-22_01 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230207_061449_078297_E26155D8 X-CRM114-Status: GOOD ( 42.65 ) X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org Hi Jonathan, On 1/28/23 17:12, Jonathan Cameron wrote: > On Fri, 27 Jan 2023 17:40:38 +0100 > Gatien Chevallier 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 >> Signed-off-by: Loic PALLARDY > > 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B9982C636CD for ; Tue, 7 Feb 2023 14:15:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:CC:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=rd56SbI7Op0ObTGr39DlFcnkxZmy/PZNOrE7SIYtxks=; b=TZDIflbFAlHvkL fWGXxXQ+dALp4tC+4uL38SH412SM3S0AGYx2dpb93uNmqDOOUtTrzyFZNE0mx/0Dux/ZDaDeI2Hp7 tWD7reGhj/UFhOfqzRFH8jwg/3dPeTqAxx0TZ+SfcskcNLgiqgN0VFdCM9Ar/IedwpcDN9U/LUwGW dY9mIJM8IKq866GCYCkb+UoZHVeMkSgU9/ZzZkvqber5loj/nuH9E174AhIOtZscVsMi52nkHLpqm Kya+CqrG0kVQGUMO4+hNsefcstzQIZEKBgU8dFE7XL9BIUkhDy3imTZH8sd4KU8zVeuEIzIDg6Coq R2y5eCXPKZ6iE3mM6fEw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOkG-00CPbI-O8; Tue, 07 Feb 2023 14:14:52 +0000 Received: from mx07-00178001.pphosted.com ([185.132.182.106]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pPOkC-00CPZq-Nj; Tue, 07 Feb 2023 14:14:50 +0000 Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 317ArBxI023356; Tue, 7 Feb 2023 15:12:28 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=bRPLjENYLm4EkeQxpcXBxnDMwaCRtrrOyJyCcEKFKwM=; b=v+r2Y+dW5qpe2evs4+p2Rph0HUeYJkhrIakAtRF4fJW7pf/iMoA2BrjUcXOgAS4O+JMl IXKV7Hp74w1KJP0PvEekg8P/i3SR3P0AwBgPlA+9ojsAnk0s7LDy3CWyXOWlKcwqqPfu wPE7CdTQ6u+DTonaxS8J7QYwU2uD3eROQ4b0UVJqA3TaS3QYgy35kLMkIT8d6Z2toWJ5 tMeMMxNPT8PXIpemJJgIQyZVYQ9Vv2xTMUAyQ8CNlfu2WPa6wKy9p9vIs1O/gAb/LlqC 1+7jKtxsEn7AYZrcWlego6inAn0ozOi+IQs8ea9RiHb5iRVStFIyQmQyADvaliPIL6nK lw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3nhfk72dkh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Feb 2023 15:12:28 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 763D6100034; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 48EEA21B516; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from [10.201.20.249] (10.201.20.249) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Tue, 7 Feb 2023 15:12:24 +0100 Message-ID: Date: Tue, 7 Feb 2023 15:12:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus Content-Language: en-US To: Jonathan Cameron CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Loic PALLARDY References: <20230127164040.1047583-1-gatien.chevallier@foss.st.com> <20230127164040.1047583-5-gatien.chevallier@foss.st.com> <20230128161217.0e79436e@jic23-huawei> From: Gatien CHEVALLIER In-Reply-To: <20230128161217.0e79436e@jic23-huawei> X-Originating-IP: [10.201.20.249] X-ClientProxiedBy: EQNCAS1NODE3.st.com (10.75.129.80) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-02-07_05,2023-02-06_03,2022-06-22_01 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230207_061449_078297_E26155D8 X-CRM114-Status: GOOD ( 42.65 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Jonathan, On 1/28/23 17:12, Jonathan Cameron wrote: > On Fri, 27 Jan 2023 17:40:38 +0100 > Gatien Chevallier 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 >> Signed-off-by: Loic PALLARDY > > 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DF186C636CC for ; Wed, 8 Feb 2023 06:33:11 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 0D786203; Wed, 8 Feb 2023 07:32:19 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 0D786203 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1675837989; bh=WGIk0RcZug+yiCumQBgNz7m9ygzBh+DTvs8frQyiJW4=; h=Date:Subject:To:References:From:In-Reply-To:CC:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=haLFRfoEHQlH8xmfvyaYBD7RFWfne9ZOCYGLOwEBujLrVzi+eD5d0QzeEm9y0quna vR1CJfjoWDZmY7UoEVJveBeW7iE1/UQ35Hx0et8UMM6EMHgTmonzTOJIhHgNgtqGJJ vyRjkFhWPf2PJJ63xrAmaMKfhNdEsYfj/AnC7T6c= Received: from mailman-core.alsa-project.org (mailman-core.alsa-project.org [10.254.200.10]) by alsa1.perex.cz (Postfix) with ESMTP id 00095F800E2; Wed, 8 Feb 2023 07:31:51 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 7DC39F80152; Wed, 8 Feb 2023 00:54:49 +0100 (CET) Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 9A05EF8010B for ; Wed, 8 Feb 2023 00:54:46 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 9A05EF8010B Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key, unprotected) header.d=foss.st.com header.i=@foss.st.com header.a=rsa-sha256 header.s=selector1 header.b=v+r2Y+dW Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 317ArBxI023356; Tue, 7 Feb 2023 15:12:28 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=bRPLjENYLm4EkeQxpcXBxnDMwaCRtrrOyJyCcEKFKwM=; b=v+r2Y+dW5qpe2evs4+p2Rph0HUeYJkhrIakAtRF4fJW7pf/iMoA2BrjUcXOgAS4O+JMl IXKV7Hp74w1KJP0PvEekg8P/i3SR3P0AwBgPlA+9ojsAnk0s7LDy3CWyXOWlKcwqqPfu wPE7CdTQ6u+DTonaxS8J7QYwU2uD3eROQ4b0UVJqA3TaS3QYgy35kLMkIT8d6Z2toWJ5 tMeMMxNPT8PXIpemJJgIQyZVYQ9Vv2xTMUAyQ8CNlfu2WPa6wKy9p9vIs1O/gAb/LlqC 1+7jKtxsEn7AYZrcWlego6inAn0ozOi+IQs8ea9RiHb5iRVStFIyQmQyADvaliPIL6nK lw== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3nhfk72dkh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 07 Feb 2023 15:12:28 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 763D6100034; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 48EEA21B516; Tue, 7 Feb 2023 15:12:27 +0100 (CET) Received: from [10.201.20.249] (10.201.20.249) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Tue, 7 Feb 2023 15:12:24 +0100 Message-ID: Date: Tue, 7 Feb 2023 15:12:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH v3 4/6] bus: stm32_sys_bus: add support for STM32MP15 and STM32MP13 system bus Content-Language: en-US To: Jonathan Cameron References: <20230127164040.1047583-1-gatien.chevallier@foss.st.com> <20230127164040.1047583-5-gatien.chevallier@foss.st.com> <20230128161217.0e79436e@jic23-huawei> From: Gatien CHEVALLIER In-Reply-To: <20230128161217.0e79436e@jic23-huawei> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.201.20.249] X-ClientProxiedBy: EQNCAS1NODE3.st.com (10.75.129.80) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1 definitions=2023-02-07_05,2023-02-06_03,2022-06-22_01 X-MailFrom: prvs=040240bbbf=gatien.chevallier@foss.st.com X-Mailman-Rule-Hits: max-recipients X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-alsa-devel.alsa-project.org-0; header-match-alsa-devel.alsa-project.org-1; nonmember-moderation; administrivia; implicit-dest; max-size; news-moderation; no-subject; digests; suspicious-header Message-ID-Hash: GKRZCYCWOZHNKF4JY77YP6357JHPPQ2S X-Message-ID-Hash: GKRZCYCWOZHNKF4JY77YP6357JHPPQ2S X-Mailman-Approved-At: Wed, 08 Feb 2023 06:31:47 +0000 CC: Oleksii_Moisieiev@epam.com, gregkh@linuxfoundation.org, herbert@gondor.apana.org.au, davem@davemloft.net, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, alexandre.torgue@foss.st.com, vkoul@kernel.org, olivier.moysan@foss.st.com, arnaud.pouliquen@foss.st.com, mchehab@kernel.org, fabrice.gasnier@foss.st.com, ulf.hansson@linaro.org, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, linux-crypto@vger.kernel.org, devicetree@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org, linux-i2c@vger.kernel.org, linux-iio@vger.kernel.org, alsa-devel@alsa-project.org, linux-media@vger.kernel.org, linux-mmc@vger.kernel.org, netdev@vger.kernel.org, linux-phy@lists.infradead.org, linux-serial@vger.kernel.org, linux-spi@vger.kernel.org, linux-usb@vger.kernel.org, Loic PALLARDY X-Mailman-Version: 3.3.8 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi Jonathan, On 1/28/23 17:12, Jonathan Cameron wrote: > On Fri, 27 Jan 2023 17:40:38 +0100 > Gatien Chevallier 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 >> Signed-off-by: Loic PALLARDY > > 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* 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