| Message ID | 20230815152712.1760046-2-amitsinght@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: MPAM: add support for priority partitioning control | expand |
On Tue, 15 Aug 2023 20:57:01 +0530 Amit Singh Tomar <amitsinght@marvell.com> wrote: Hi Amit, My comments are likely to be fairly superficial, at least partly because we haven't yet had a good opportunity for a review of James' MPAM series on top of which this sits. (side note that I'm keen that James posts that so we can have at it properly) Note James has a new version of the tree out so applying this was a little messy. > At the moment, configuring multiple resource instances (mapped to same > control) under a resource class is not supported. For instance, on MARVELL > SoC MPAMF_IDR_NS[RIS_MAX] (under LLC MSC) is 0x2, and there are three > different resource at index 0,1,2. These are enumerated in > TAD_CMN_MPAM_RIS_E: > > 0: MSC > 1: LTG > 2: DTG Sometimes I think the MPAM spec is just too flexible. We are going to see an awful lot of corner cases. > > LLC MSC resource at index 1, and 2 have cache portion partitioning > feature, i.e., If MPAMCFG_PART_SEL_NS[RIS] is set to 1 (LTG) or to 2 (DTG), > then MPAMF_IDR_NS[HAS_CPOR_PART] is set to 1. LTG resource has 16 > portion bitmap, and DTG has 18 portion bitmap (mapped to same > CPOR control), and only one can be configured. This patch description seems to first describe a clash between LTG and DTG before going on to what the patch actually changes. Do we need that initial description or can it be simplified to just talk about... > > LLC MSC resource at index 0 has the Priority partitioning features. > If MPAMCFG_PART_SEL_NS[RIS] is set to 0 (MSC), then MPAMF_IDR_NS[HAS_PRI_PART] > is set to 1, leaving HAS_CPOR_PART bit to 0. CPOR and PRI_PART are > mutually exclusive resources as far configuration is concerned. > > With this change, multiple resource instances that maps to different > control, say LTG for CPOR, and MSC for PRI_PART is handled properly. this second part? > > Signed-off-by: Amit Singh Tomar <amitsinght@marvell.com> > --- > drivers/platform/mpam/mpam_devices.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/mpam/mpam_devices.c b/drivers/platform/mpam/mpam_devices.c > index 589ff1ef2b6b..137cbff925ba 100644 > --- a/drivers/platform/mpam/mpam_devices.c > +++ b/drivers/platform/mpam/mpam_devices.c > @@ -1829,6 +1829,19 @@ static void mpam_enable_init_class_features(struct mpam_class *class) > class->props = ris->props; > } > > +/* Club different resource properties under a class that resctrl uses, > + * for instance, L3 cache that supports both CPOR, and DSPRI need to have > + * knowledge of both cpbm_wd and dspri_wd. Perhaps highlight that this case only matters if the two properties are under different RIS. I'm fairly sure that a valid implementation would export both CPOR and DSPRI on the same RIS. > + */ > +static void mpam_enable_club_class_features(struct mpam_class *class, > + struct mpam_msc_ris *ris) > +{ > + class->props.features |= ris->props.features; > + class->props.cpbm_wd |= ris->props.cpbm_wd; > + class->props.dspri_wd |= ris->props.dspri_wd; > + class->props.num_csu_mon |= ris->props.num_csu_mon; > +} > + > /* Merge all the common resource features into class. */ > static void mpam_enable_merge_features(void) > { > @@ -1843,7 +1856,16 @@ static void mpam_enable_merge_features(void) > > list_for_each_entry(comp, &class->components, class_list) { > list_for_each_entry(ris, &comp->ris, comp_list) { > - __resource_props_mismatch(ris, class); > + /* There can be multiple resources under a class which is > + * mapped to different controls, for instance L3 cache > + * can have both CPOR and DSPRI implemented, and following implemented with different RIS > + * would avoid property mismatch later on when different > + * resources are present. > + */ > + if (class->props.features != ris->props.features) > + mpam_enable_club_class_features(class, ris); I think the __resource_props_mismatch still needs calling. Consider a pair or RIS A and B A supports CPOR only with 12 partitions. B supports CPOR with 10 partitions and DSPRI This will expand the set to include both CPOR and DSPRI but fail to catch the mismatch in partitions. I'll hack some tests on top of my QEMU code to see if this works as I think it does. Jonathn > + else > + __resource_props_mismatch(ris, class); > > class->nrdy_usec = max(class->nrdy_usec, > ris->msc->nrdy_usec);
diff --git a/drivers/platform/mpam/mpam_devices.c b/drivers/platform/mpam/mpam_devices.c index 589ff1ef2b6b..137cbff925ba 100644 --- a/drivers/platform/mpam/mpam_devices.c +++ b/drivers/platform/mpam/mpam_devices.c @@ -1829,6 +1829,19 @@ static void mpam_enable_init_class_features(struct mpam_class *class) class->props = ris->props; } +/* Club different resource properties under a class that resctrl uses, + * for instance, L3 cache that supports both CPOR, and DSPRI need to have + * knowledge of both cpbm_wd and dspri_wd. + */ +static void mpam_enable_club_class_features(struct mpam_class *class, + struct mpam_msc_ris *ris) +{ + class->props.features |= ris->props.features; + class->props.cpbm_wd |= ris->props.cpbm_wd; + class->props.dspri_wd |= ris->props.dspri_wd; + class->props.num_csu_mon |= ris->props.num_csu_mon; +} + /* Merge all the common resource features into class. */ static void mpam_enable_merge_features(void) { @@ -1843,7 +1856,16 @@ static void mpam_enable_merge_features(void) list_for_each_entry(comp, &class->components, class_list) { list_for_each_entry(ris, &comp->ris, comp_list) { - __resource_props_mismatch(ris, class); + /* There can be multiple resources under a class which is + * mapped to different controls, for instance L3 cache + * can have both CPOR and DSPRI implemented, and following + * would avoid property mismatch later on when different + * resources are present. + */ + if (class->props.features != ris->props.features) + mpam_enable_club_class_features(class, ris); + else + __resource_props_mismatch(ris, class); class->nrdy_usec = max(class->nrdy_usec, ris->msc->nrdy_usec);
At the moment, configuring multiple resource instances (mapped to same control) under a resource class is not supported. For instance, on MARVELL SoC MPAMF_IDR_NS[RIS_MAX] (under LLC MSC) is 0x2, and there are three different resource at index 0,1,2. These are enumerated in TAD_CMN_MPAM_RIS_E: 0: MSC 1: LTG 2: DTG LLC MSC resource at index 1, and 2 have cache portion partitioning feature, i.e., If MPAMCFG_PART_SEL_NS[RIS] is set to 1 (LTG) or to 2 (DTG), then MPAMF_IDR_NS[HAS_CPOR_PART] is set to 1. LTG resource has 16 portion bitmap, and DTG has 18 portion bitmap (mapped to same CPOR control), and only one can be configured. LLC MSC resource at index 0 has the Priority partitioning features. If MPAMCFG_PART_SEL_NS[RIS] is set to 0 (MSC), then MPAMF_IDR_NS[HAS_PRI_PART] is set to 1, leaving HAS_CPOR_PART bit to 0. CPOR and PRI_PART are mutually exclusive resources as far configuration is concerned. With this change, multiple resource instances that maps to different control, say LTG for CPOR, and MSC for PRI_PART is handled properly. Signed-off-by: Amit Singh Tomar <amitsinght@marvell.com> --- drivers/platform/mpam/mpam_devices.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)