diff mbox series

[v7,07/26] PM / devfreq: rockchip-dfi: introduce channel mask

Message ID 20230704093242.583575-8-s.hauer@pengutronix.de (mailing list archive)
State Changes Requested, archived
Delegated to: Chanwoo Choi
Headers show
Series Add perf support to the rockchip-dfi driver | expand

Commit Message

Sascha Hauer July 4, 2023, 9:32 a.m. UTC
Different Rockchip SoC variants have a different number of channels.
Introduce a channel mask to make the number of channels configurable
from SoC initialization code.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

Chanwoo Choi Oct. 6, 2023, 5:21 p.m. UTC | #1
Hi,

On 23. 7. 4. 18:32, Sascha Hauer wrote:
> Different Rockchip SoC variants have a different number of channels.
> Introduce a channel mask to make the number of channels configurable
> from SoC initialization code.
> 
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 126bb744645b6..82de24a027579 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -18,10 +18,11 @@
>  #include <linux/list.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/bits.h>
>  
>  #include <soc/rockchip/rk3399_grf.h>
>  
> -#define RK3399_DMC_NUM_CH	2
> +#define DMC_MAX_CHANNELS	2
>  
>  /* DDRMON_CTRL */
>  #define DDRMON_CTRL	0x04
> @@ -44,7 +45,7 @@ struct dmc_count_channel {
>  };
>  
>  struct dmc_count {
> -	struct dmc_count_channel c[RK3399_DMC_NUM_CH];
> +	struct dmc_count_channel c[DMC_MAX_CHANNELS];
>  };
>  
>  /*
> @@ -61,6 +62,7 @@ struct rockchip_dfi {
>  	struct regmap *regmap_pmu;
>  	struct clk *clk;
>  	u32 ddr_type;
> +	unsigned int channel_mask;
>  };
>  
>  static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> @@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
>  	u32 i;
>  	void __iomem *dfi_regs = dfi->regs;
>  
> -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> +		if (!(dfi->channel_mask & BIT(i)))
> +			continue;
>  		count->c[i].access = readl_relaxed(dfi_regs +
>  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>  		count->c[i].total = readl_relaxed(dfi_regs +
> @@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
>  	rockchip_dfi_read_counters(edev, &count);
>  
>  	/* We can only report one channel, so find the busiest one */
> -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> -		u32 a = count.c[i].access - last->c[i].access;
> -		u32 t = count.c[i].total - last->c[i].total;
> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {

Instead of DMC_MAX_CHANNELS defintion,
you can initialize the max channel in each rkXXXX_dfi_init() like 'dfi->channel_count'.
It reduces the unnecessary loop by initializing the proper max channel.

> +		u32 a, t;
> +
> +		if (!(dfi->channel_mask & BIT(i)))
> +			continue;
> +
> +		a = count.c[i].access - last->c[i].access;
> +		t = count.c[i].total - last->c[i].total;
>  
>  		if (a > access) {
>  			access = a;
> @@ -185,6 +194,8 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
>  	dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
>  			RK3399_PMUGRF_DDRTYPE_MASK;
>  
> +	dfi->channel_mask = GENMASK(1, 0);
> +
>  	return 0;
>  };
>
Sascha Hauer Oct. 16, 2023, 11:22 a.m. UTC | #2
On Sat, Oct 07, 2023 at 02:21:10AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 23. 7. 4. 18:32, Sascha Hauer wrote:
> > Different Rockchip SoC variants have a different number of channels.
> > Introduce a channel mask to make the number of channels configurable
> > from SoC initialization code.
> > 
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 126bb744645b6..82de24a027579 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -18,10 +18,11 @@
> >  #include <linux/list.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/bits.h>
> >  
> >  #include <soc/rockchip/rk3399_grf.h>
> >  
> > -#define RK3399_DMC_NUM_CH	2
> > +#define DMC_MAX_CHANNELS	2
> >  
> >  /* DDRMON_CTRL */
> >  #define DDRMON_CTRL	0x04
> > @@ -44,7 +45,7 @@ struct dmc_count_channel {
> >  };
> >  
> >  struct dmc_count {
> > -	struct dmc_count_channel c[RK3399_DMC_NUM_CH];
> > +	struct dmc_count_channel c[DMC_MAX_CHANNELS];
> >  };
> >  
> >  /*
> > @@ -61,6 +62,7 @@ struct rockchip_dfi {
> >  	struct regmap *regmap_pmu;
> >  	struct clk *clk;
> >  	u32 ddr_type;
> > +	unsigned int channel_mask;
> >  };
> >  
> >  static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> > @@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
> >  	u32 i;
> >  	void __iomem *dfi_regs = dfi->regs;
> >  
> > -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > +		if (!(dfi->channel_mask & BIT(i)))
> > +			continue;
> >  		count->c[i].access = readl_relaxed(dfi_regs +
> >  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> >  		count->c[i].total = readl_relaxed(dfi_regs +
> > @@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> >  	rockchip_dfi_read_counters(edev, &count);
> >  
> >  	/* We can only report one channel, so find the busiest one */
> > -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> > -		u32 a = count.c[i].access - last->c[i].access;
> > -		u32 t = count.c[i].total - last->c[i].total;
> > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> 
> Instead of DMC_MAX_CHANNELS defintion,
> you can initialize the max channel in each rkXXXX_dfi_init() like 'dfi->channel_count'.
> It reduces the unnecessary loop by initializing the proper max channel.

That is not easily possible. Some SoCs, eg the RK3588 have four
channels, but not all channels are necessarily enabled it also
might not be the first channels that are enabled. On a RK3588
the channel mask might for example be 0b0101.

Sascha
Sascha Hauer Oct. 16, 2023, 12:45 p.m. UTC | #3
On Mon, Oct 16, 2023 at 01:22:16PM +0200, Sascha Hauer wrote:
> On Sat, Oct 07, 2023 at 02:21:10AM +0900, Chanwoo Choi wrote:
> > Hi,
> > 
> > On 23. 7. 4. 18:32, Sascha Hauer wrote:
> > > Different Rockchip SoC variants have a different number of channels.
> > > Introduce a channel mask to make the number of channels configurable
> > > from SoC initialization code.
> > > 
> > > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > > index 126bb744645b6..82de24a027579 100644
> > > --- a/drivers/devfreq/event/rockchip-dfi.c
> > > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > > @@ -18,10 +18,11 @@
> > >  #include <linux/list.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/bits.h>
> > >  
> > >  #include <soc/rockchip/rk3399_grf.h>
> > >  
> > > -#define RK3399_DMC_NUM_CH	2
> > > +#define DMC_MAX_CHANNELS	2
> > >  
> > >  /* DDRMON_CTRL */
> > >  #define DDRMON_CTRL	0x04
> > > @@ -44,7 +45,7 @@ struct dmc_count_channel {
> > >  };
> > >  
> > >  struct dmc_count {
> > > -	struct dmc_count_channel c[RK3399_DMC_NUM_CH];
> > > +	struct dmc_count_channel c[DMC_MAX_CHANNELS];
> > >  };
> > >  
> > >  /*
> > > @@ -61,6 +62,7 @@ struct rockchip_dfi {
> > >  	struct regmap *regmap_pmu;
> > >  	struct clk *clk;
> > >  	u32 ddr_type;
> > > +	unsigned int channel_mask;
> > >  };
> > >  
> > >  static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> > > @@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
> > >  	u32 i;
> > >  	void __iomem *dfi_regs = dfi->regs;
> > >  
> > > -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> > > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > > +		if (!(dfi->channel_mask & BIT(i)))
> > > +			continue;
> > >  		count->c[i].access = readl_relaxed(dfi_regs +
> > >  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> > >  		count->c[i].total = readl_relaxed(dfi_regs +
> > > @@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> > >  	rockchip_dfi_read_counters(edev, &count);
> > >  
> > >  	/* We can only report one channel, so find the busiest one */
> > > -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> > > -		u32 a = count.c[i].access - last->c[i].access;
> > > -		u32 t = count.c[i].total - last->c[i].total;
> > > +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > 
> > Instead of DMC_MAX_CHANNELS defintion,
> > you can initialize the max channel in each rkXXXX_dfi_init() like 'dfi->channel_count'.
> > It reduces the unnecessary loop by initializing the proper max channel.
> 
> That is not easily possible. Some SoCs, eg the RK3588 have four
> channels, but not all channels are necessarily enabled it also
> might not be the first channels that are enabled. On a RK3588
> the channel mask might for example be 0b0101.

Nah, forget this comment. Of course I can initialize a variable with a
maximum value of channels that could be available on this SoC and only
iterate over these. Will do.

Sascha
Chanwoo Choi Oct. 17, 2023, 8:28 a.m. UTC | #4
On 23. 10. 16. 21:45, Sascha Hauer wrote:
> On Mon, Oct 16, 2023 at 01:22:16PM +0200, Sascha Hauer wrote:
>> On Sat, Oct 07, 2023 at 02:21:10AM +0900, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 23. 7. 4. 18:32, Sascha Hauer wrote:
>>>> Different Rockchip SoC variants have a different number of channels.
>>>> Introduce a channel mask to make the number of channels configurable
>>>> from SoC initialization code.
>>>>
>>>> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>> ---
>>>>  drivers/devfreq/event/rockchip-dfi.c | 23 +++++++++++++++++------
>>>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
>>>> index 126bb744645b6..82de24a027579 100644
>>>> --- a/drivers/devfreq/event/rockchip-dfi.c
>>>> +++ b/drivers/devfreq/event/rockchip-dfi.c
>>>> @@ -18,10 +18,11 @@
>>>>  #include <linux/list.h>
>>>>  #include <linux/of.h>
>>>>  #include <linux/of_device.h>
>>>> +#include <linux/bits.h>
>>>>  
>>>>  #include <soc/rockchip/rk3399_grf.h>
>>>>  
>>>> -#define RK3399_DMC_NUM_CH	2
>>>> +#define DMC_MAX_CHANNELS	2
>>>>  
>>>>  /* DDRMON_CTRL */
>>>>  #define DDRMON_CTRL	0x04
>>>> @@ -44,7 +45,7 @@ struct dmc_count_channel {
>>>>  };
>>>>  
>>>>  struct dmc_count {
>>>> -	struct dmc_count_channel c[RK3399_DMC_NUM_CH];
>>>> +	struct dmc_count_channel c[DMC_MAX_CHANNELS];
>>>>  };
>>>>  
>>>>  /*
>>>> @@ -61,6 +62,7 @@ struct rockchip_dfi {
>>>>  	struct regmap *regmap_pmu;
>>>>  	struct clk *clk;
>>>>  	u32 ddr_type;
>>>> +	unsigned int channel_mask;
>>>>  };
>>>>  
>>>>  static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>>>> @@ -95,7 +97,9 @@ static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
>>>>  	u32 i;
>>>>  	void __iomem *dfi_regs = dfi->regs;
>>>>  
>>>> -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
>>>> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
>>>> +		if (!(dfi->channel_mask & BIT(i)))
>>>> +			continue;
>>>>  		count->c[i].access = readl_relaxed(dfi_regs +
>>>>  				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>>>>  		count->c[i].total = readl_relaxed(dfi_regs +
>>>> @@ -145,9 +149,14 @@ static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
>>>>  	rockchip_dfi_read_counters(edev, &count);
>>>>  
>>>>  	/* We can only report one channel, so find the busiest one */
>>>> -	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
>>>> -		u32 a = count.c[i].access - last->c[i].access;
>>>> -		u32 t = count.c[i].total - last->c[i].total;
>>>> +	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
>>>
>>> Instead of DMC_MAX_CHANNELS defintion,
>>> you can initialize the max channel in each rkXXXX_dfi_init() like 'dfi->channel_count'.
>>> It reduces the unnecessary loop by initializing the proper max channel.
>>
>> That is not easily possible. Some SoCs, eg the RK3588 have four
>> channels, but not all channels are necessarily enabled it also
>> might not be the first channels that are enabled. On a RK3588
>> the channel mask might for example be 0b0101.
> 
> Nah, forget this comment. Of course I can initialize a variable with a
> maximum value of channels that could be available on this SoC and only
> iterate over these. Will do.
> 

Thanks.
diff mbox series

Patch

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 126bb744645b6..82de24a027579 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -18,10 +18,11 @@ 
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/bits.h>
 
 #include <soc/rockchip/rk3399_grf.h>
 
-#define RK3399_DMC_NUM_CH	2
+#define DMC_MAX_CHANNELS	2
 
 /* DDRMON_CTRL */
 #define DDRMON_CTRL	0x04
@@ -44,7 +45,7 @@  struct dmc_count_channel {
 };
 
 struct dmc_count {
-	struct dmc_count_channel c[RK3399_DMC_NUM_CH];
+	struct dmc_count_channel c[DMC_MAX_CHANNELS];
 };
 
 /*
@@ -61,6 +62,7 @@  struct rockchip_dfi {
 	struct regmap *regmap_pmu;
 	struct clk *clk;
 	u32 ddr_type;
+	unsigned int channel_mask;
 };
 
 static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
@@ -95,7 +97,9 @@  static void rockchip_dfi_read_counters(struct devfreq_event_dev *edev, struct dm
 	u32 i;
 	void __iomem *dfi_regs = dfi->regs;
 
-	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
+	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+		if (!(dfi->channel_mask & BIT(i)))
+			continue;
 		count->c[i].access = readl_relaxed(dfi_regs +
 				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
 		count->c[i].total = readl_relaxed(dfi_regs +
@@ -145,9 +149,14 @@  static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
 	rockchip_dfi_read_counters(edev, &count);
 
 	/* We can only report one channel, so find the busiest one */
-	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
-		u32 a = count.c[i].access - last->c[i].access;
-		u32 t = count.c[i].total - last->c[i].total;
+	for (i = 0; i < DMC_MAX_CHANNELS; i++) {
+		u32 a, t;
+
+		if (!(dfi->channel_mask & BIT(i)))
+			continue;
+
+		a = count.c[i].access - last->c[i].access;
+		t = count.c[i].total - last->c[i].total;
 
 		if (a > access) {
 			access = a;
@@ -185,6 +194,8 @@  static int rk3399_dfi_init(struct rockchip_dfi *dfi)
 	dfi->ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
 			RK3399_PMUGRF_DDRTYPE_MASK;
 
+	dfi->channel_mask = GENMASK(1, 0);
+
 	return 0;
 };