Hi Amit,
On 25.05.18 г. 11:27, Amit Kucheria wrote:
> On Fri, Mar 9, 2018 at 11:09 PM, Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>> Add driver for the Qualcomm interconnect buses found in msm8916 based
>> platforms.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
>> ---
>> drivers/interconnect/Kconfig | 5 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/qcom/Kconfig | 11 +
>> drivers/interconnect/qcom/Makefile | 2 +
>> drivers/interconnect/qcom/msm8916.c | 482 ++++++++++++++++++++++++++++++++++++
>> include/linux/interconnect/qcom.h | 350 ++++++++++++++++++++++++++
>> 6 files changed, 851 insertions(+)
>> create mode 100644 drivers/interconnect/qcom/Kconfig
>> create mode 100644 drivers/interconnect/qcom/msm8916.c
>> create mode 100644 include/linux/interconnect/qcom.h
[..]
>> +#define DEFINE_QNODE(_name, _id, _port, _buswidth, _ap_owned, \
>> + _mas_rpm_id, _slv_rpm_id, _qos_mode, \
>> + _numlinks, ...) \
>> + static struct qcom_icc_node _name = { \
>> + .id = _id, \
>> + .name = #_name, \
>> + .port = _port, \
>> + .buswidth = _buswidth, \
>> + .qos_mode = _qos_mode, \
>> + .ap_owned = _ap_owned, \
>> + .mas_rpm_id = _mas_rpm_id, \
>> + .slv_rpm_id = _slv_rpm_id, \
>> + .num_links = _numlinks, \
>> + .links = { __VA_ARGS__ }, \
>> + }
>
> Move this macro definition just above its use below.
>
Ok.
>> +enum qcom_qos_mode {
>> + QCOM_QOS_MODE_BYPASS = 0,
>> + QCOM_QOS_MODE_FIXED,
>> + QCOM_QOS_MODE_MAX,
>> +};
>> +
>> +struct qcom_icc_provider {
>> + struct icc_provider provider;
>> + void __iomem *base;
>> + struct clk *bus_clk;
>> + struct clk *bus_a_clk;
>> +};
>> +
>> +#define MSM8916_MAX_LINKS 8
>> +
>> +/**
>> + * struct qcom_icc_node - Qualcomm specific interconnect nodes
>> + * @name: the node name used in debugfs
>> + * @links: an array of nodes where we can go next while traversing
>> + * @id: a unique node identifier
>> + * @num_links: the total number of @links
>> + * @port: the offset index into the masters QoS register space
>> + * @buswidth: width of the interconnect between a node and the bus
>
> units?
Will add.
[..]
>> +static int qcom_icc_init(struct icc_node *node)
>> +{
>> + struct qcom_icc_provider *qp = to_qcom_provider(node->provider);
>> + /* TODO: init qos and priority */
>> +
>
> No need to set_rate here before enabling the clock?
Yes, i am planing to initially set the bus clocks at max rate. The rate
would be adjusted as soon as the consumers start placing requests.
>
>> + clk_prepare_enable(qp->bus_clk);
>> + clk_prepare_enable(qp->bus_a_clk);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst,
>> + u32 avg, u32 peak)
>> +{
>> + struct qcom_icc_provider *qp;
>> + struct qcom_icc_node *qn;
>> + struct icc_node *node;
>> + struct icc_provider *provider;
>> + u64 avg_bw;
>> + u64 peak_bw;
>> + u64 rate = 0;
>> + int ret = 0;
>> +
>> + if (!src)
>> + node = dst;
>> + else
>> + node = src;
>> +
>> + qn = node->data;
>> + provider = node->provider;
>> + qp = to_qcom_provider(node->provider);
>> +
>> + /* convert from kbps to bps */
>> + avg_bw = avg * 1000ULL;
>> + peak_bw = peak * 1000ULL;
>> +
>
> Since the core uses kbps and various SoC HW might use bps (or other
> units), perhaps consider providing a macro in the core header such as:
>
> #define icc_units_to_bps(bw) (bw * 1000ULL)
>
> and then move this conversion to the top of the function where you
> define the variable
>
> u64 avg_bw = icc_units_to_bps(avg);
> u64 peak_bw = icc_units_to_bps(peak);
>
> Since other drivers will end up copying this driver, it might help
> prevent silly errors in the drivers and has a side-effect of allowing
> the core to change internal units w/o any driver changes down the
> line, if needed.
Ok, i am fine with this.
Thanks,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
