Re: [PATCH net-next v3 03/14] net: ethernet: qualcomm: Add PPE driver for IPQ9574 SoC

[Jie Gan <jie.gan@oss.qualcomm.com>]

On 2/11/2025 9:58 PM, Jie Luo wrote:
> 
> 
> 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.)
> 
Got your point here. So there may have multiple definitions like 
ppe_icc_data here, right? But the num_icc here is hardcoded.
Maybe it would be better defined within the ppe_icc_data, if possible?
Then just directly use ppe_icc_data->num_icc?

Never mind, that's just my thought on the flexibility.

Jie
>>
>>> +
>>> +    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
>