Re: [PATCH v4 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver

[Krzysztof Kozlowski]

On 05/08/2022 11:55, Jiucheng Xu wrote:
>>> +static int __init g12_ddr_pmu_probe(struct platform_device *pdev)
>>> +{
>>> +	struct ddr_pmu *pmu;
>>> +
>>> +	if (of_device_is_compatible(pdev->dev.of_node,
>>> +				    "amlogic,g12a-ddr-pmu")) {
>>> +		format_attr_nna.attr.mode = 0;
>>> +		format_attr_gdc.attr.mode = 0;
>>> +		format_attr_arm1.attr.mode = 0;
>>> +		format_attr_mipi_isp.attr.mode = 0;
>> No. That's not correct patter. You must use variant specific driver data.
> 
> Do you mean use of_device_id.data? Could your please give me an
> 
> example code in kernel source?

90% of Linux kernel drivers?

> 
>>
>>> +	} else if (of_device_is_compatible(pdev->dev.of_node,
>>> +					   "amlogic,sm1-ddr-pmu")) {
>>> +		format_attr_gdc.attr.mode = 0;
>>> +		format_attr_arm1.attr.mode = 0;
>>> +		format_attr_mipi_isp.attr.mode = 0;
>> No. That's not correct patter. You must use variant specific driver data.
>>
>>> +	}
>>> +
>>> +	pmu = devm_kzalloc(&pdev->dev, sizeof(struct ddr_pmu), GFP_KERNEL);
>>> +	if (!pmu)
>>> +		return -ENOMEM;
>>> +
>>> +	/*
>>> +	 * G12 series Soc have single dmc controller and
>>> +	 * 4x ddr bandwidth monitor channels
>>> +	 */
>>> +	pmu->info.dmc_nr = 1;
>>> +	pmu->info.chann_nr = 4;
>>> +	pmu->info.ops = &g12_ops;
>>> +	pmu->info.fmt_attr = g12_pmu_format_attrs;
>>> +
>>> +	return meson_ddr_pmu_create(pdev, pmu);
>>> +}
>>> +
>>> +static int __exit g12_ddr_pmu_remove(struct platform_device *pdev)
>>> +{
>>> +	meson_ddr_pmu_remove(pdev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id meson_ddr_pmu_dt_match[] = {
>>> +	{
>>> +		.compatible = "amlogic,g12-ddr-pmu",
>> Undocumented compatible. Did you run checkpatch and fix all the errors?
> 
> Yes, I run "./scripts/checkpatch --strict *.patch", and no errors/warnings.
> 
> Any other options should be used to check strictly?
> 
> I think it could be removed.

Either you document it or drop it. It anyway looks a bit odd -
unspecific (neither for g12a, nor for g12b).

> 
>>
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,g12a-ddr-pmu",
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,g12b-ddr-pmu",
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,sm1-ddr-pmu",
>> Why four different entries for the same devices without driver data?
>> This is confusing.
> Do you mean use a common compatible and different driver data?

What I meant is that current version this is useless and confusing.
Different devices have different compatibles with different driver data.
Same devices have just the same compatible (so same driver data). You
mixed two different approaches.

>>
>>> +	},
>>> +	{}
>>> +};
>>> +
>>> +static struct platform_driver g12_ddr_pmu_driver = {
>>> +	.driver = {
>>> +		.name = "amlogic,ddr-pmu",
>>> +		.of_match_table = meson_ddr_pmu_dt_match,
>>> +	},
>>> +	.remove = __exit_p(g12_ddr_pmu_remove),
>> You made the driver non-hotpluggable - why?
>> In the same time it is still unbindable, whis is a bit confusing. If you
>> can unbind it, you should be able to hot-unplug it.
> Sorry, I couldn't know why the driver is non-hotpluggable. Could you 
> tell the detail?

You used module_platform_driver_probe, so the one with documentation:
 /* non-hotpluggable platform devices may use this so that probe() and

  * its support may live in __init sections, conserving runtime memory.

  */



>>> +};
>>> +
>>> +module_platform_driver_probe(g12_ddr_pmu_driver, g12_ddr_pmu_probe);
>>> +MODULE_AUTHOR("Jiucheng Xu");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("Amlogic G12 series SoC DDR PMU");
>>> diff --git a/include/soc/amlogic/meson_ddr_pmu.h b/include/soc/amlogic/meson_ddr_pmu.h
>>> new file mode 100644
>>> index 000000000000..882efe3c2f58
>>> --- /dev/null
>>> +++ b/include/soc/amlogic/meson_ddr_pmu.h
>>> @@ -0,0 +1,76 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2022 Amlogic, Inc. All rights reserved.
>>> + */
>>> +
>>> +#ifndef __MESON_DDR_PMU_H__
>>> +#define __MESON_DDR_PMU_H__
>>> +
>>> +#define MAX_CHANNEL_NUM		8
>>> +
>>> +enum {
>>> +	ALL_CHAN_COUNTER_ID,
>>> +	CHAN1_COUNTER_ID,
>>> +	CHAN2_COUNTER_ID,
>>> +	CHAN3_COUNTER_ID,
>>> +	CHAN4_COUNTER_ID,
>>> +	CHAN5_COUNTER_ID,
>>> +	CHAN6_COUNTER_ID,
>>> +	CHAN7_COUNTER_ID,
>>> +	CHAN8_COUNTER_ID,
>>> +	COUNTER_MAX_ID,
>>> +};
>>> +
>>> +struct dmc_hw_info;
>>> +
>>> +struct dmc_counter {
>>> +	u64 all_cnt;	/* The count of all requests come in/out ddr controller */
>>> +	union {
>>> +		u64 all_req;
>>> +		struct {
>>> +			u64 all_idle_cnt;
>>> +			u64 all_16bit_cnt;
>>> +		};
>>> +	};
>>> +	u64 channel_cnt[MAX_CHANNEL_NUM]; /* To save a DMC bandwidth-monitor channel counter */
>>> +};
>>> +
>>> +struct dmc_pmu_hw_ops {
>>> +	void (*enable)(struct dmc_hw_info *info);
>>> +	void (*disable)(struct dmc_hw_info *info);
>>> +	/* Bind an axi line to a bandwidth-monitor channel */
>>> +	void (*config_axi_id)(struct dmc_hw_info *info, int axi_id, int chann);
>>> +	int (*irq_handler)(struct dmc_hw_info *info,
>>> +			   struct dmc_counter *counter);
>>> +	void (*get_counters)(struct dmc_hw_info *info,
>>> +			     struct dmc_counter *counter);
>>> +};
>>> +
>>> +struct dmc_hw_info {
>>> +	struct dmc_pmu_hw_ops *ops;
>>> +	void __iomem *ddr_reg[4];
>>> +	unsigned long timer_value;	/* Timer value in TIMER register */
>>> +	void __iomem *pll_reg;
>>> +	int irq_num;			/* irq vector number */
>>> +	int dmc_nr;			/* The number of dmc controller */
>>> +	int chann_nr;			/* The number of dmc bandwidth monitor channels */
>>> +	int id;				/* The number of supported channels */
>>> +	struct attribute **fmt_attr;
>>> +};
>>> +
>>> +struct ddr_pmu {
>>> +	struct pmu pmu;
>>> +	struct dmc_hw_info info;
>>> +	struct dmc_counter counters;	/* save counters from hw */
>>> +	bool pmu_enabled;
>>> +	struct device *dev;
>>> +	char *name;
>>> +	struct hlist_node node;
>>> +	enum cpuhp_state cpuhp_state;
>>> +	int cpu;			/* for cpu hotplug */
>>> +};
>> Linux-wide headers should not include your private data structures.
>> Entier header looks unused - should be made private.
> Do you mean the header should be in driver dir, or the structures should 
> be within .c file?

This or that, up to you. Definitely not in include/linux/.


Best regards,
Krzysztof