| Message ID | 20240306073016.2163-2-quic_okukatla@quicinc.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add support for QoS configuration | expand |
On 06/03/2024 08:30, Odelu Kukatla wrote: > It adds QoS support for QNOC device and includes support for > configuring priority, priority forward disable, urgency forwarding. > This helps in priortizing the traffic originating from different > interconnect masters at NoC(Network On Chip). > > Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com> > --- > drivers/interconnect/qcom/icc-rpmh.c | 105 +++++++++++++++++++++++++++ > drivers/interconnect/qcom/icc-rpmh.h | 32 ++++++++ > 2 files changed, 137 insertions(+) > > + if (desc->config) { > + struct resource *res; > + void __iomem *base; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_warn(dev, "Skipping QoS, failed to get resource\n"); Why is this a warning? If the MMIO is not required, your driver should not spawn warnings. > + goto skip_qos_config; > + } > + > + base = devm_ioremap(dev, res->start, resource_size(res)); Combine these two (there's a helper for that). > + if (IS_ERR(base)) { > + dev_warn(dev, "Skipping QoS, ioremap failed: %ld\n", PTR_ERR(base)); > + goto skip_qos_config; > + }; > + > + qp->regmap = devm_regmap_init_mmio(dev, base, desc->config); > + Drop blank line. > + if (IS_ERR(qp->regmap)) { > + dev_warn(dev, "Skipping QoS, regmap failed; %ld\n", PTR_ERR(qp->regmap)); > + goto skip_qos_config; > + } > + > + qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks); > + if (qp->num_clks < 0) { > + dev_warn(dev, "Skipping QoS, failed to get clk: %d\n", qp->num_clks); dev_info(), because current binding sais clocks are not required and you are not allowed to affect ABI. While ABI here does not look broken, spawning new warnings to proper users is not welcomed. Best regards, Krzysztof
On 3/6/24 08:30, Odelu Kukatla wrote: > It adds QoS support for QNOC device and includes support for > configuring priority, priority forward disable, urgency forwarding. > This helps in priortizing the traffic originating from different > interconnect masters at NoC(Network On Chip). > > Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com> > --- > drivers/interconnect/qcom/icc-rpmh.c | 105 +++++++++++++++++++++++++++ > drivers/interconnect/qcom/icc-rpmh.h | 32 ++++++++ > 2 files changed, 137 insertions(+) > > diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c > index c1aa265c1f4e..b4681849df80 100644 > --- a/drivers/interconnect/qcom/icc-rpmh.c > +++ b/drivers/interconnect/qcom/icc-rpmh.c > @@ -1,19 +1,57 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > */ > > +#include <linux/clk.h> > #include <linux/interconnect.h> > #include <linux/interconnect-provider.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/slab.h> > +#include <linux/bitfield.h> Please keep the alphabetical order > > #include "bcm-voter.h" > #include "icc-common.h" > #include "icc-rpmh.h" > > +/* QNOC QoS */ > +#define QOSGEN_MAINCTL_LO(p, qp) (0x8 + (p->port_offsets[qp])) > +#define QOS_SLV_URG_MSG_EN_MASK BIT_MASK(3) Mixing BIT_MASK and GENMASK is very confusing.. > +#define QOS_DFLT_PRIO_MASK GENMASK(6, 4) > +#define QOS_DISABLE_MASK BIT_MASK(24) > + > +/** > + * qcom_icc_set_qos - initialize static QoS configurations > + * @qp: qcom icc provider to which @node belongs > + * @node: qcom icc node to operate on > + */ > +static void qcom_icc_set_qos(struct qcom_icc_provider *qp, > + struct qcom_icc_node *node) > +{ > + const struct qcom_icc_qosbox *qos = node->qosbox; > + int port; > + > + if (!qp->regmap || !qos) > + return; This is not possible if you follow the code flow, I think.. [...] > + * @prio: priority value assigned to requests on the node > + * @urg_fwd: whether to forward the urgency promotion issued by master(endpoint), or discard space before the opening brace, please also wrap to 80 lines > + * @prio_fwd_disable: whether to forward the priority driven by mster, or override by @prio typo: mster, please also wrap it Konrad
On 3/6/2024 2:01 PM, Krzysztof Kozlowski wrote: > On 06/03/2024 08:30, Odelu Kukatla wrote: >> It adds QoS support for QNOC device and includes support for >> configuring priority, priority forward disable, urgency forwarding. >> This helps in priortizing the traffic originating from different >> interconnect masters at NoC(Network On Chip). >> >> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com> >> --- >> drivers/interconnect/qcom/icc-rpmh.c | 105 +++++++++++++++++++++++++++ >> drivers/interconnect/qcom/icc-rpmh.h | 32 ++++++++ >> 2 files changed, 137 insertions(+) > >> >> + if (desc->config) { >> + struct resource *res; >> + void __iomem *base; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_warn(dev, "Skipping QoS, failed to get resource\n"); > > Why is this a warning? If the MMIO is not required, your driver should > not spawn warnings. > Right, it should not be a warning, will clean it up. > >> + goto skip_qos_config; >> + } >> + >> + base = devm_ioremap(dev, res->start, resource_size(res)); > > Combine these two (there's a helper for that). > I will address this in v4. >> + if (IS_ERR(base)) { >> + dev_warn(dev, "Skipping QoS, ioremap failed: %ld\n", PTR_ERR(base)); >> + goto skip_qos_config; >> + }; >> + >> + qp->regmap = devm_regmap_init_mmio(dev, base, desc->config); >> + > > Drop blank line. > >> + if (IS_ERR(qp->regmap)) { >> + dev_warn(dev, "Skipping QoS, regmap failed; %ld\n", PTR_ERR(qp->regmap)); >> + goto skip_qos_config; >> + } >> + >> + qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks); >> + if (qp->num_clks < 0) { >> + dev_warn(dev, "Skipping QoS, failed to get clk: %d\n", qp->num_clks); > > dev_info(), because current binding sais clocks are not required and you > are not allowed to affect ABI. While ABI here does not look broken, > spawning new warnings to proper users is not welcomed. > I will convert it to dev_info() in next version. But qp->num_clks = 0 if clock property is not added, so it does not spawn warning. will move all other dev_warn() to dev_info(). > Best regards, > Krzysztof > Thanks, Odelu
On 3/6/2024 9:48 PM, Konrad Dybcio wrote: > > > On 3/6/24 08:30, Odelu Kukatla wrote: >> It adds QoS support for QNOC device and includes support for >> configuring priority, priority forward disable, urgency forwarding. >> This helps in priortizing the traffic originating from different >> interconnect masters at NoC(Network On Chip). >> >> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com> >> --- >> drivers/interconnect/qcom/icc-rpmh.c | 105 +++++++++++++++++++++++++++ >> drivers/interconnect/qcom/icc-rpmh.h | 32 ++++++++ >> 2 files changed, 137 insertions(+) >> >> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c >> index c1aa265c1f4e..b4681849df80 100644 >> --- a/drivers/interconnect/qcom/icc-rpmh.c >> +++ b/drivers/interconnect/qcom/icc-rpmh.c >> @@ -1,19 +1,57 @@ >> // SPDX-License-Identifier: GPL-2.0 >> /* >> * Copyright (c) 2020, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. >> */ >> >> +#include <linux/clk.h> >> #include <linux/interconnect.h> >> #include <linux/interconnect-provider.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_platform.h> >> #include <linux/slab.h> >> +#include <linux/bitfield.h> > > Please keep the alphabetical order > I will address this in next version. >> >> #include "bcm-voter.h" >> #include "icc-common.h" >> #include "icc-rpmh.h" >> >> +/* QNOC QoS */ >> +#define QOSGEN_MAINCTL_LO(p, qp) (0x8 + (p->port_offsets[qp])) >> +#define QOS_SLV_URG_MSG_EN_MASK BIT_MASK(3) > > Mixing BIT_MASK and GENMASK is very confusing.. > I will fix this in v4. >> +#define QOS_DFLT_PRIO_MASK GENMASK(6, 4) >> +#define QOS_DISABLE_MASK BIT_MASK(24) >> + >> +/** >> + * qcom_icc_set_qos - initialize static QoS configurations >> + * @qp: qcom icc provider to which @node belongs >> + * @node: qcom icc node to operate on >> + */ >> +static void qcom_icc_set_qos(struct qcom_icc_provider *qp, >> + struct qcom_icc_node *node) >> +{ >> + const struct qcom_icc_qosbox *qos = node->qosbox; >> + int port; >> + >> + if (!qp->regmap || !qos) >> + return; > > This is not possible if you follow the code flow, I think.. > > [...] > Right, it is not needed. >> + * @prio: priority value assigned to requests on the node >> + * @urg_fwd: whether to forward the urgency promotion issued by master(endpoint), or discard > > space before the opening brace, please also wrap to 80 lines > I will fix this in v4. >> + * @prio_fwd_disable: whether to forward the priority driven by mster, or override by @prio > > typo: mster, please also wrap it > > Konrad Thanks, Odelu
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c index c1aa265c1f4e..b4681849df80 100644 --- a/drivers/interconnect/qcom/icc-rpmh.c +++ b/drivers/interconnect/qcom/icc-rpmh.c @@ -1,19 +1,57 @@ // SPDX-License-Identifier: GPL-2.0 /* * Copyright (c) 2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */ +#include <linux/clk.h> #include <linux/interconnect.h> #include <linux/interconnect-provider.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_platform.h> #include <linux/slab.h> +#include <linux/bitfield.h> #include "bcm-voter.h" #include "icc-common.h" #include "icc-rpmh.h" +/* QNOC QoS */ +#define QOSGEN_MAINCTL_LO(p, qp) (0x8 + (p->port_offsets[qp])) +#define QOS_SLV_URG_MSG_EN_MASK BIT_MASK(3) +#define QOS_DFLT_PRIO_MASK GENMASK(6, 4) +#define QOS_DISABLE_MASK BIT_MASK(24) + +/** + * qcom_icc_set_qos - initialize static QoS configurations + * @qp: qcom icc provider to which @node belongs + * @node: qcom icc node to operate on + */ +static void qcom_icc_set_qos(struct qcom_icc_provider *qp, + struct qcom_icc_node *node) +{ + const struct qcom_icc_qosbox *qos = node->qosbox; + int port; + + if (!qp->regmap || !qos) + return; + + for (port = 0; port < qos->num_ports; port++) { + regmap_update_bits(qp->regmap, QOSGEN_MAINCTL_LO(qos, port), + QOS_DISABLE_MASK, + FIELD_PREP(QOS_DISABLE_MASK, qos->prio_fwd_disable)); + + regmap_update_bits(qp->regmap, QOSGEN_MAINCTL_LO(qos, port), + QOS_DFLT_PRIO_MASK, + FIELD_PREP(QOS_DFLT_PRIO_MASK, qos->prio)); + + regmap_update_bits(qp->regmap, QOSGEN_MAINCTL_LO(qos, port), + QOS_SLV_URG_MSG_EN_MASK, + FIELD_PREP(QOS_SLV_URG_MSG_EN_MASK, qos->urg_fwd)); + } +} + /** * qcom_icc_pre_aggregate - cleans up stale values from prior icc_set * @node: icc node to operate on @@ -159,6 +197,36 @@ int qcom_icc_bcm_init(struct qcom_icc_bcm *bcm, struct device *dev) } EXPORT_SYMBOL_GPL(qcom_icc_bcm_init); +/** + * qcom_icc_rpmh_configure_qos - configure QoS parameters + * @qp: qcom icc provider associated with QoS endpoint nodes + * + * Return: 0 on success, or an error code otherwise + */ +static int qcom_icc_rpmh_configure_qos(struct qcom_icc_provider *qp) +{ + struct qcom_icc_node *qnode; + size_t i; + int ret; + + ret = clk_bulk_prepare_enable(qp->num_clks, qp->clks); + if (ret) + return ret; + + for (i = 0; i < qp->num_nodes; i++) { + qnode = qp->nodes[i]; + if (!qnode) + continue; + + if (qnode->qosbox) + qcom_icc_set_qos(qp, qnode); + } + + clk_bulk_disable_unprepare(qp->num_clks, qp->clks); + + return ret; +} + int qcom_icc_rpmh_probe(struct platform_device *pdev) { const struct qcom_icc_desc *desc; @@ -199,7 +267,9 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) qp->dev = dev; qp->bcms = desc->bcms; + qp->nodes = desc->nodes; qp->num_bcms = desc->num_bcms; + qp->num_nodes = desc->num_nodes; qp->voter = of_bcm_voter_get(qp->dev, NULL); if (IS_ERR(qp->voter)) @@ -229,6 +299,41 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev) data->nodes[i] = node; } + if (desc->config) { + struct resource *res; + void __iomem *base; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_warn(dev, "Skipping QoS, failed to get resource\n"); + goto skip_qos_config; + } + + base = devm_ioremap(dev, res->start, resource_size(res)); + if (IS_ERR(base)) { + dev_warn(dev, "Skipping QoS, ioremap failed: %ld\n", PTR_ERR(base)); + goto skip_qos_config; + }; + + qp->regmap = devm_regmap_init_mmio(dev, base, desc->config); + + if (IS_ERR(qp->regmap)) { + dev_warn(dev, "Skipping QoS, regmap failed; %ld\n", PTR_ERR(qp->regmap)); + goto skip_qos_config; + } + + qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks); + if (qp->num_clks < 0) { + dev_warn(dev, "Skipping QoS, failed to get clk: %d\n", qp->num_clks); + goto skip_qos_config; + } + + ret = qcom_icc_rpmh_configure_qos(qp); + if (ret) + dev_warn(dev, "Failed to program QoS: %d\n", ret); + } + +skip_qos_config: ret = icc_provider_register(provider); if (ret) goto err_remove_nodes; diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h index 2de29460e808..a0533d8aeffe 100644 --- a/drivers/interconnect/qcom/icc-rpmh.h +++ b/drivers/interconnect/qcom/icc-rpmh.h @@ -1,12 +1,14 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* * Copyright (c) 2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */ #ifndef __DRIVERS_INTERCONNECT_QCOM_ICC_RPMH_H__ #define __DRIVERS_INTERCONNECT_QCOM_ICC_RPMH_H__ #include <dt-bindings/interconnect/qcom,icc.h> +#include <linux/regmap.h> #define to_qcom_provider(_provider) \ container_of(_provider, struct qcom_icc_provider, provider) @@ -18,6 +20,11 @@ * @bcms: list of bcms that maps to the provider * @num_bcms: number of @bcms * @voter: bcm voter targeted by this provider + * @nodes: list of icc nodes that maps to the provider + * @num_nodes: number of @nodes + * @regmap: used for QoS, register access + * @clks : clks required for register access + * @num_clks: number of @clks */ struct qcom_icc_provider { struct icc_provider provider; @@ -25,6 +32,11 @@ struct qcom_icc_provider { struct qcom_icc_bcm * const *bcms; size_t num_bcms; struct bcm_voter *voter; + struct qcom_icc_node * const *nodes; + size_t num_nodes; + struct regmap *regmap; + struct clk_bulk_data *clks; + int num_clks; }; /** @@ -41,6 +53,23 @@ struct bcm_db { u8 reserved; }; +/** + * struct qcom_icc_qosbox - Qualcomm specific QoS config + * @prio: priority value assigned to requests on the node + * @urg_fwd: whether to forward the urgency promotion issued by master(endpoint), or discard + * @prio_fwd_disable: whether to forward the priority driven by mster, or override by @prio + * @num_ports: number of @ports + * @port_offsets: qos register offsets + */ + +struct qcom_icc_qosbox { + const u32 prio; + const bool urg_fwd; + const bool prio_fwd_disable; + const u32 num_ports; + const u32 port_offsets[] __counted_by(num_ports); +}; + #define MAX_LINKS 128 #define MAX_BCMS 64 #define MAX_BCM_PER_NODE 3 @@ -58,6 +87,7 @@ struct bcm_db { * @max_peak: current max aggregate value of all peak bw requests * @bcms: list of bcms associated with this logical node * @num_bcms: num of @bcms + * @qosbox: qos config data associated with node */ struct qcom_icc_node { const char *name; @@ -70,6 +100,7 @@ struct qcom_icc_node { u64 max_peak[QCOM_ICC_NUM_BUCKETS]; struct qcom_icc_bcm *bcms[MAX_BCM_PER_NODE]; size_t num_bcms; + const struct qcom_icc_qosbox *qosbox; }; /** @@ -114,6 +145,7 @@ struct qcom_icc_fabric { }; struct qcom_icc_desc { + const struct regmap_config *config; struct qcom_icc_node * const *nodes; size_t num_nodes; struct qcom_icc_bcm * const *bcms;
It adds QoS support for QNOC device and includes support for configuring priority, priority forward disable, urgency forwarding. This helps in priortizing the traffic originating from different interconnect masters at NoC(Network On Chip). Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com> --- drivers/interconnect/qcom/icc-rpmh.c | 105 +++++++++++++++++++++++++++ drivers/interconnect/qcom/icc-rpmh.h | 32 ++++++++ 2 files changed, 137 insertions(+)