Re: [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver

[Georgi Djakov <georgi.djakov@linaro.org>]

Hi Amit,

On 25.05.18 г. 11:27, Amit Kucheria wrote:
> On Fri, Mar 9, 2018 at 11:09 PM, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> Add driver for the Qualcomm interconnect buses found in msm8916 based
>> platforms.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>  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