| Message ID | 20230704093242.583575-8-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add perf support to the rockchip-dfi driver | expand |
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; > }; >
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
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
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 --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; };