Message-ID: <63f1d25c-087a-46dd-9053-60334a0095d5@quicinc.com>
Date: Tue, 11 Feb 2025 21:58:19 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Jie Gan <jie.gan@....qualcomm.com>, Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, "Rob
 Herring" <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        "Conor
 Dooley" <conor+dt@...nel.org>,
        Lei Wei <quic_leiwei@...cinc.com>,
        "Suruchi
 Agarwal" <quic_suruchia@...cinc.com>,
        Pavithra R <quic_pavir@...cinc.com>, Simon Horman <horms@...nel.org>,
        Jonathan Corbet <corbet@....net>, Kees Cook
	<kees@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        "Philipp
 Zabel" <p.zabel@...gutronix.de>
CC: <linux-arm-msm@...r.kernel.org>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-hardening@...r.kernel.org>,
        <quic_kkumarcs@...cinc.com>, <quic_linchen@...cinc.com>,
        <srinivas.kandagatla@...aro.org>, <bartosz.golaszewski@...aro.org>,
        <john@...ozen.org>
Subject: Re: [PATCH net-next v3 03/14] net: ethernet: qualcomm: Add PPE driver
 for IPQ9574 SoC



On 2/10/2025 10:12 AM, Jie Gan wrote:
>> +static int ppe_clock_init_and_reset(struct ppe_device *ppe_dev)
>> +{
>> +    unsigned long ppe_rate = ppe_dev->clk_rate;
>> +    struct device *dev = ppe_dev->dev;
>> +    struct reset_control *rstc;
>> +    struct clk_bulk_data *clks;
>> +    struct clk *clk;
>> +    int ret, i;
>> +
>> +    for (i = 0; i < ppe_dev->num_icc_paths; i++) {
>> +        ppe_dev->icc_paths[i].name = ppe_icc_data[i].name;
>> +        ppe_dev->icc_paths[i].avg_bw = ppe_icc_data[i].avg_bw ? :
>> +                           Bps_to_icc(ppe_rate);
> it's ppe_dev->icc_paths[i].avg_bw = ppe_icc_data[i].avg_bw ? 
> ppe_icc_data[i].avg_bw : Bps_to_icc(ppe_rate);  ?

I feel the above used notation is also fine for readability, and is
shorter and simpler.

> 
> 
>> +        ppe_dev->icc_paths[i].peak_bw = ppe_icc_data[i].peak_bw ? :
>> +                        Bps_to_icc(ppe_rate);
> Same with previous one?

Same response as for the previous comment is applicable here as well.

> 
>> +    }
>> +
>> +    ret = devm_of_icc_bulk_get(dev, ppe_dev->num_icc_paths,
>> +                   ppe_dev->icc_paths);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = icc_bulk_set_bw(ppe_dev->num_icc_paths, ppe_dev->icc_paths);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* The PPE clocks have a common parent clock. Setting the clock
> Should be:
> /*
>   * The PPE clocks have a common parent clock. Setting the clock
>   * rate of "ppe" ensures the clock rate of all PPE clocks is
>   * configured to the same rate.
>   */
> 

I think for drivers/net, the above format follows the recommended
commenting style. Pls see: https://www.kernel.org/doc/html/v6.10/
process/coding-style.html

For files in net/ and drivers/net/ the preferred style for long
(multi-line) comments is a little different.

> BTW, it's better wrapped with ~75 characters per line.

Yes, the comments should be wrapped to ~75 characters.

> 
>> +     * rate of "ppe" ensures the clock rate of all PPE clocks is
>> +     * configured to the same rate.
>> +     */
>> +    clk = devm_clk_get(dev, "ppe");
>> +    if (IS_ERR(clk))
>> +        return PTR_ERR(clk);
>> +
>> +    ret = clk_set_rate(clk, ppe_rate);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = devm_clk_bulk_get_all_enabled(dev, &clks);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    /* Reset the PPE. */
>> +    rstc = devm_reset_control_get_exclusive(dev, NULL);
>> +    if (IS_ERR(rstc))
>> +        return PTR_ERR(rstc);
>> +
>> +    ret = reset_control_assert(rstc);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* The delay 10 ms of assert is necessary for resetting PPE. */
>> +    usleep_range(10000, 11000);
>> +
>> +    return reset_control_deassert(rstc);
>> +}
>> +
>> +static int qcom_ppe_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct ppe_device *ppe_dev;
>> +    void __iomem *base;
>> +    int ret, num_icc;
> I think it's better with:
>      int num_icc = ARRAY_SIZE(ppe_icc_data);

This will impact the “reverse xmas tree” rule for local variable
definitions. Also, the num_icc will vary as per the different SoC,
so we will need to initialize the num_icc in a separate statement.

(Note: This driver will be extended to support different SoC in
the future.)

> 
>> +
>> +    num_icc = ARRAY_SIZE(ppe_icc_data);
>> +    ppe_dev = devm_kzalloc(dev, struct_size(ppe_dev, icc_paths, 
>> num_icc),
>> +                   GFP_KERNEL);
>> +    if (!ppe_dev)
>> +        return -ENOMEM;
>> +
>> +    base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(base))
>> +        return dev_err_probe(dev, PTR_ERR(base), "PPE ioremap 
>> failed\n");
>> +
>> +    ppe_dev->regmap = devm_regmap_init_mmio(dev, base, 
>> &regmap_config_ipq9574);
>> +    if (IS_ERR(ppe_dev->regmap))
>> +        return dev_err_probe(dev, PTR_ERR(ppe_dev->regmap),
>> +                     "PPE initialize regmap failed\n");
>> +    ppe_dev->dev = dev;
>> +    ppe_dev->clk_rate = PPE_CLK_RATE;
>> +    ppe_dev->num_ports = PPE_PORT_MAX;
>> +    ppe_dev->num_icc_paths = num_icc;
>> +
>> +    ret = ppe_clock_init_and_reset(ppe_dev);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "PPE clock config failed\n");
>> +
>> +    platform_set_drvdata(pdev, ppe_dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_ppe_of_match[] = {
>> +    { .compatible = "qcom,ipq9574-ppe" },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_ppe_of_match);
>> +
>> +static struct platform_driver qcom_ppe_driver = {
>> +    .driver = {
>> +        .name = "qcom_ppe",
>> +        .of_match_table = qcom_ppe_of_match,
>> +    },
>> +    .probe    = qcom_ppe_probe,
>> +};
>> +module_platform_driver(qcom_ppe_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. IPQ PPE driver");
>> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe.h b/drivers/net/ 
>> ethernet/qualcomm/ppe/ppe.h
>> new file mode 100644
>> index 000000000000..cc6767b7c2b8
>> --- /dev/null
>> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only
>> + *
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#ifndef __PPE_H__
>> +#define __PPE_H__
>> +
>> +#include <linux/compiler.h>
>> +#include <linux/interconnect.h>
>> +
>> +struct device;
> #include <linux/device.h> ?
> 
>> +struct regmap;
> Same with previous one, include it's header file?

The driver only need to reference these structures but don't
need their full definitions. So it should be fine to declare
the existence of these structures here.

> 
>> +
>> +/**
>> + * struct ppe_device - PPE device private data.
>> + * @dev: PPE device structure.
>> + * @regmap: PPE register map.
>> + * @clk_rate: PPE clock rate.
>> + * @num_ports: Number of PPE ports.
>> + * @num_icc_paths: Number of interconnect paths.
>> + * @icc_paths: Interconnect path array.
>> + *
>> + * PPE device is the instance of PPE hardware, which is used to
>> + * configure PPE packet process modules such as BM (buffer management),
>> + * QM (queue management), and scheduler.
>> + */
>> +struct ppe_device {
>> +    struct device *dev;
>> +    struct regmap *regmap;
>> +    unsigned long clk_rate;
>> +    unsigned int num_ports;
>> +    unsigned int num_icc_paths;
>> +    struct icc_bulk_data icc_paths[] __counted_by(num_icc_paths);
>> +};
>> +#endif
>>
> 
> Thanks,
> Jie

Powered by blists - more mailing lists