lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Aug 2021 07:46:14 -0500 From: Alex Elder <elder@...aro.org> To: Odelu Kukatla <okukatla@...eaurora.org>, georgi.djakov@...aro.org, bjorn.andersson@...aro.org, evgreen@...gle.com, Andy Gross <agross@...nel.org>, Georgi Djakov <djakov@...nel.org>, linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org Cc: sboyd@...nel.org, mdtipton@...eaurora.org, sibis@...eaurora.org, saravanak@...gle.com, seansw@....qualcomm.com, linux-arm-msm-owner@...r.kernel.org Subject: Re: [v6 2/3] interconnect: qcom: Add EPSS L3 support on SC7280 On 8/10/21 1:46 AM, Odelu Kukatla wrote: > Add Epoch Subsystem (EPSS) L3 interconnect provider support on > SC7280 SoCs. > > Signed-off-by: Odelu Kukatla <okukatla@...eaurora.org> I don't have much to say about what this is doing but I have a few suggestions. -Alex > --- > drivers/interconnect/qcom/osm-l3.c | 136 +++++++++++++++++++++++++++++++------ > drivers/interconnect/qcom/sc7280.h | 10 +++ > 2 files changed, 125 insertions(+), 21 deletions(-) > > diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c > index c7af143..3b16e73 100644 > --- a/drivers/interconnect/qcom/osm-l3.c > +++ b/drivers/interconnect/qcom/osm-l3.c > @@ -9,12 +9,14 @@ > #include <linux/io.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of_address.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > > #include <dt-bindings/interconnect/qcom,osm-l3.h> > > #include "sc7180.h" > +#include "sc7280.h" > #include "sc8180x.h" > #include "sdm845.h" > #include "sm8150.h" > @@ -33,17 +35,33 @@ > > /* EPSS Register offsets */ > #define EPSS_LUT_ROW_SIZE 4 > +#define EPSS_REG_L3_VOTE 0x90 > #define EPSS_REG_FREQ_LUT 0x100 > #define EPSS_REG_PERF_STATE 0x320 > +#define EPSS_CORE_OFFSET 0x4 > +#define EPSS_L3_VOTE_REG(base, cpu)\ > + (((base) + EPSS_REG_L3_VOTE) +\ > + ((cpu) * EPSS_CORE_OFFSET)) > > -#define OSM_L3_MAX_LINKS 1 > +#define L3_DOMAIN_CNT 4 > +#define L3_MAX_LINKS 9 It may not matter much, but if you specified the qcom_osm_l3_node->links[] field as the last field in the structure, I think it could be a flexible array and you wouldn't have to specify the maximum number of links. (You are already using the actual array size to set ->num_links in __DEFINE_QNODE().) > #define to_osm_l3_provider(_provider) \ > container_of(_provider, struct qcom_osm_l3_icc_provider, provider) > > +/** > + * @domain_base: an array of base address for each clock domain > + * @max_state: max supported frequency level > + * @per_core_dcvs: flag used to indicate whether the frequency scaling > + * for each core is enabled > + * @reg_perf_state: requested frequency level > + * @lut_tables: an array of supported frequency levels > + * @provider: interconnect provider of this node > + */ Run this to check your kernel doc validity: scripts/kernel-doc -none <file> [<file>...] > struct qcom_osm_l3_icc_provider { > - void __iomem *base; > + void __iomem *domain_base[L3_DOMAIN_CNT]; > unsigned int max_state; > + bool per_core_dcvs; > unsigned int reg_perf_state; > unsigned long lut_tables[LUT_MAX_ENTRIES]; > struct icc_provider provider; . . . > @@ -235,12 +322,17 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > if (!qp) > return -ENOMEM; > > - qp->base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(qp->base)) > - return PTR_ERR(qp->base); > + while (of_get_address(pdev->dev.of_node, i++, NULL, NULL)) > + nr_domain_bases++; Maybe you could combine these two loops by counting as you go. I.e.: i = 0; while (true) { void __iomem *base; if (of_get_address(pdev->dev.of_node, i, NULL, NULL)) break; base = devm_platform_ioremap_resource(pdev, i); if (IS_ERR(base)) return PTR_ERR(base); qp->domain_base[i++] = base } nr_domain_bases = i; > + > + for (i = 0; i < nr_domain_bases ; i++) { > + qp->domain_base[i] = devm_platform_ioremap_resource(pdev, i); > + if (IS_ERR(qp->domain_base[i])) > + return PTR_ERR(qp->domain_base[i]); > + } > > /* HW should be in enabled state to proceed */ > - if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) { > + if (!(readl_relaxed(qp->domain_base[0] + REG_ENABLE) & 0x1)) { > dev_err(&pdev->dev, "error hardware not enabled\n"); > return -ENODEV; > } > @@ -252,7 +344,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > qp->reg_perf_state = desc->reg_perf_state; > > for (i = 0; i < LUT_MAX_ENTRIES; i++) { > - info = readl_relaxed(qp->base + desc->reg_freq_lut + > + info = readl_relaxed(qp->domain_base[0] + desc->reg_freq_lut + > i * desc->lut_row_size); Maybe you could define a macro to encapsulate computing this register offset, along the lines of EPSS_L3_VOTE_REG(). (Here and elsewhere.) > src = FIELD_GET(LUT_SRC, info); > lval = FIELD_GET(LUT_L_VAL, info); > @@ -271,6 +363,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > prev_freq = freq; > } > qp->max_state = i; > + qp->per_core_dcvs = desc->per_core_dcvs; > > qnodes = desc->nodes; > num_nodes = desc->num_nodes; > @@ -326,6 +419,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > > static const struct of_device_id osm_l3_of_match[] = { > { .compatible = "qcom,sc7180-osm-l3", .data = &sc7180_icc_osm_l3 }, > + { .compatible = "qcom,sc7280-epss-l3", .data = &sc7280_icc_epss_l3 }, > { .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_icc_osm_l3 }, > { .compatible = "qcom,sm8150-osm-l3", .data = &sm8150_icc_osm_l3 }, > { .compatible = "qcom,sc8180x-osm-l3", .data = &sc8180x_icc_osm_l3 }, > diff --git a/drivers/interconnect/qcom/sc7280.h b/drivers/interconnect/qcom/sc7280.h > index 175e400..5df7600 100644 > --- a/drivers/interconnect/qcom/sc7280.h > +++ b/drivers/interconnect/qcom/sc7280.h > @@ -150,5 +150,15 @@ > #define SC7280_SLAVE_PCIE_1 139 > #define SC7280_SLAVE_QDSS_STM 140 > #define SC7280_SLAVE_TCU 141 > +#define SC7280_MASTER_EPSS_L3_APPS 142 > +#define SC7280_SLAVE_EPSS_L3_SHARED 143 > +#define SC7280_SLAVE_EPSS_L3_CPU0 144 > +#define SC7280_SLAVE_EPSS_L3_CPU1 145 > +#define SC7280_SLAVE_EPSS_L3_CPU2 146 > +#define SC7280_SLAVE_EPSS_L3_CPU3 147 > +#define SC7280_SLAVE_EPSS_L3_CPU4 148 > +#define SC7280_SLAVE_EPSS_L3_CPU5 149 > +#define SC7280_SLAVE_EPSS_L3_CPU6 150 > +#define SC7280_SLAVE_EPSS_L3_CPU7 151 > > #endif >
Powered by blists - more mailing lists