[PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Heiko Stuebner]

With the T-HEAD C9XX cores being designed before or during the ratification
to the SSCOFPMF extension, they implement functionality very similar but
not equal to it. So add some adaptions to allow the C9XX to still handle
its PMU through the regular SBI PMU interface instead of defining new
interfaces or drivers.

To work properly, this requires a matching change in SBI, though the actual
interface between kernel and SBI does not change.

The main differences are a the overflow CSR and irq number.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 6f6681bbfd36..4589166e0de4 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
 	NULL,
 };
 
+#define THEAD_C9XX_RV_IRQ_PMU			17
+#define THEAD_C9XX_CSR_SCOUNTEROF		0x5c5
+
 /*
  * RISC-V doesn't have hetergenous harts yet. This need to be part of
  * per_cpu in case of harts with different pmu counters
  */
 static union sbi_pmu_ctr_info *pmu_ctr_list;
+static unsigned int riscv_pmu_irq_num;
 static unsigned int riscv_pmu_irq;
+static bool is_thead_c9xx;
 
 struct sbi_pmu_event_data {
 	union {
@@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
 	fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
 	event = cpu_hw_evt->events[fidx];
 	if (!event) {
-		csr_clear(CSR_SIP, SIP_LCOFIP);
+		csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
 		return IRQ_NONE;
 	}
 
@@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
 	pmu_sbi_stop_hw_ctrs(pmu);
 
 	/* Overflow status register should only be read after counter are stopped */
-	overflow = csr_read(CSR_SSCOUNTOVF);
+	overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
+				  : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
 
 	/*
 	 * Overflow interrupt pending bit should only be cleared after stopping
 	 * all the counters to avoid any race condition.
 	 */
-	csr_clear(CSR_SIP, SIP_LCOFIP);
+	csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
 
 	/* No overflow bit is set */
 	if (!overflow)
@@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
 
 	if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
 		cpu_hw_evt->irq = riscv_pmu_irq;
-		csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
-		csr_set(CSR_IE, BIT(RV_IRQ_PMU));
+		csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
+		csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
 		enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
 	}
 
@@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
 {
 	if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
 		disable_percpu_irq(riscv_pmu_irq);
-		csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
+		csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
 	}
 
 	/* Disable all counters access for user mode now */
@@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 	struct device_node *cpu, *child;
 	struct irq_domain *domain = NULL;
 
-	if (!riscv_isa_extension_available(NULL, SSCOFPMF))
+	is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
+			 sbi_get_marchid() == 0 &&
+			 sbi_get_mimpid() == 0);
+
+	if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
 		return -EOPNOTSUPP;
 
 	for_each_of_cpu_node(cpu) {
@@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 		return -ENODEV;
 	}
 
-	riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
+	riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
+	riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
 	if (!riscv_pmu_irq) {
 		pr_err("Failed to map PMU interrupt for node\n");
 		return -ENODEV;
-- 
2.35.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Heiko Stuebner]

With the T-HEAD C9XX cores being designed before or during the ratification
to the SSCOFPMF extension, they implement functionality very similar but
not equal to it. So add some adaptions to allow the C9XX to still handle
its PMU through the regular SBI PMU interface instead of defining new
interfaces or drivers.

To work properly, this requires a matching change in SBI, though the actual
interface between kernel and SBI does not change.

The main differences are a the overflow CSR and irq number.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 6f6681bbfd36..4589166e0de4 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
 	NULL,
 };
 
+#define THEAD_C9XX_RV_IRQ_PMU			17
+#define THEAD_C9XX_CSR_SCOUNTEROF		0x5c5
+
 /*
  * RISC-V doesn't have hetergenous harts yet. This need to be part of
  * per_cpu in case of harts with different pmu counters
  */
 static union sbi_pmu_ctr_info *pmu_ctr_list;
+static unsigned int riscv_pmu_irq_num;
 static unsigned int riscv_pmu_irq;
+static bool is_thead_c9xx;
 
 struct sbi_pmu_event_data {
 	union {
@@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
 	fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
 	event = cpu_hw_evt->events[fidx];
 	if (!event) {
-		csr_clear(CSR_SIP, SIP_LCOFIP);
+		csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
 		return IRQ_NONE;
 	}
 
@@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
 	pmu_sbi_stop_hw_ctrs(pmu);
 
 	/* Overflow status register should only be read after counter are stopped */
-	overflow = csr_read(CSR_SSCOUNTOVF);
+	overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
+				  : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
 
 	/*
 	 * Overflow interrupt pending bit should only be cleared after stopping
 	 * all the counters to avoid any race condition.
 	 */
-	csr_clear(CSR_SIP, SIP_LCOFIP);
+	csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
 
 	/* No overflow bit is set */
 	if (!overflow)
@@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
 
 	if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
 		cpu_hw_evt->irq = riscv_pmu_irq;
-		csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
-		csr_set(CSR_IE, BIT(RV_IRQ_PMU));
+		csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
+		csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
 		enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
 	}
 
@@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
 {
 	if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
 		disable_percpu_irq(riscv_pmu_irq);
-		csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
+		csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
 	}
 
 	/* Disable all counters access for user mode now */
@@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 	struct device_node *cpu, *child;
 	struct irq_domain *domain = NULL;
 
-	if (!riscv_isa_extension_available(NULL, SSCOFPMF))
+	is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
+			 sbi_get_marchid() == 0 &&
+			 sbi_get_mimpid() == 0);
+
+	if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
 		return -EOPNOTSUPP;
 
 	for_each_of_cpu_node(cpu) {
@@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 		return -ENODEV;
 	}
 
-	riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
+	riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
+	riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
 	if (!riscv_pmu_irq) {
 		pr_err("Failed to map PMU interrupt for node\n");
 		return -ENODEV;
-- 
2.35.1

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Atish Patra]

On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> With the T-HEAD C9XX cores being designed before or during the ratification
> to the SSCOFPMF extension, they implement functionality very similar but
> not equal to it. So add some adaptions to allow the C9XX to still handle
> its PMU through the regular SBI PMU interface instead of defining new
> interfaces or drivers.
>

IMO, vendor specific workarounds in the generic implementation is not
a good idea.
If we have to support it, I think we should just put the IRQ number in
the DT and parse from the DT.
The initial sscofpmf series was based on the DT. It was removed later
as there was no need for it at that time.
We can always revive it.

Regarding the CSR number difference and static key enablement, can we
use code patching techniques here as well ?
At least all the T-HEAD C9XX core erratas are in one place.

The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
with erratas. I don't prefer this approach
but it keeps the vendor specific checks out of the generic code.

> To work properly, this requires a matching change in SBI, though the actual
> interface between kernel and SBI does not change.
>

Do you have a working OpenSBI implementation for this ?

> The main differences are a the overflow CSR and irq number.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 6f6681bbfd36..4589166e0de4 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
>         NULL,
>  };
>
> +#define THEAD_C9XX_RV_IRQ_PMU                  17
> +#define THEAD_C9XX_CSR_SCOUNTEROF              0x5c5
> +
>  /*
>   * RISC-V doesn't have hetergenous harts yet. This need to be part of
>   * per_cpu in case of harts with different pmu counters
>   */
>  static union sbi_pmu_ctr_info *pmu_ctr_list;
> +static unsigned int riscv_pmu_irq_num;
>  static unsigned int riscv_pmu_irq;
> +static bool is_thead_c9xx;
>
>  struct sbi_pmu_event_data {
>         union {
> @@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>         fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
>         event = cpu_hw_evt->events[fidx];
>         if (!event) {
> -               csr_clear(CSR_SIP, SIP_LCOFIP);
> +               csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
>                 return IRQ_NONE;
>         }
>
> @@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>         pmu_sbi_stop_hw_ctrs(pmu);
>
>         /* Overflow status register should only be read after counter are stopped */
> -       overflow = csr_read(CSR_SSCOUNTOVF);
> +       overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
> +                                 : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
>
>         /*
>          * Overflow interrupt pending bit should only be cleared after stopping
>          * all the counters to avoid any race condition.
>          */
> -       csr_clear(CSR_SIP, SIP_LCOFIP);
> +       csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
>
>         /* No overflow bit is set */
>         if (!overflow)
> @@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
>
>         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
>                 cpu_hw_evt->irq = riscv_pmu_irq;
> -               csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
> -               csr_set(CSR_IE, BIT(RV_IRQ_PMU));
> +               csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
> +               csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
>                 enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
>         }
>
> @@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
>  {
>         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
>                 disable_percpu_irq(riscv_pmu_irq);
> -               csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
> +               csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
>         }
>
>         /* Disable all counters access for user mode now */
> @@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>         struct device_node *cpu, *child;
>         struct irq_domain *domain = NULL;
>
> -       if (!riscv_isa_extension_available(NULL, SSCOFPMF))
> +       is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
> +                        sbi_get_marchid() == 0 &&
> +                        sbi_get_mimpid() == 0);
> +
> +       if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
>                 return -EOPNOTSUPP;
>
>         for_each_of_cpu_node(cpu) {
> @@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>                 return -ENODEV;
>         }
>
> -       riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
> +       riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
> +       riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
>         if (!riscv_pmu_irq) {
>                 pr_err("Failed to map PMU interrupt for node\n");
>                 return -ENODEV;
> --
> 2.35.1
>


-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Atish Patra]

On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> With the T-HEAD C9XX cores being designed before or during the ratification
> to the SSCOFPMF extension, they implement functionality very similar but
> not equal to it. So add some adaptions to allow the C9XX to still handle
> its PMU through the regular SBI PMU interface instead of defining new
> interfaces or drivers.
>

IMO, vendor specific workarounds in the generic implementation is not
a good idea.
If we have to support it, I think we should just put the IRQ number in
the DT and parse from the DT.
The initial sscofpmf series was based on the DT. It was removed later
as there was no need for it at that time.
We can always revive it.

Regarding the CSR number difference and static key enablement, can we
use code patching techniques here as well ?
At least all the T-HEAD C9XX core erratas are in one place.

The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
with erratas. I don't prefer this approach
but it keeps the vendor specific checks out of the generic code.

> To work properly, this requires a matching change in SBI, though the actual
> interface between kernel and SBI does not change.
>

Do you have a working OpenSBI implementation for this ?

> The main differences are a the overflow CSR and irq number.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 6f6681bbfd36..4589166e0de4 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
>         NULL,
>  };
>
> +#define THEAD_C9XX_RV_IRQ_PMU                  17
> +#define THEAD_C9XX_CSR_SCOUNTEROF              0x5c5
> +
>  /*
>   * RISC-V doesn't have hetergenous harts yet. This need to be part of
>   * per_cpu in case of harts with different pmu counters
>   */
>  static union sbi_pmu_ctr_info *pmu_ctr_list;
> +static unsigned int riscv_pmu_irq_num;
>  static unsigned int riscv_pmu_irq;
> +static bool is_thead_c9xx;
>
>  struct sbi_pmu_event_data {
>         union {
> @@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>         fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
>         event = cpu_hw_evt->events[fidx];
>         if (!event) {
> -               csr_clear(CSR_SIP, SIP_LCOFIP);
> +               csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
>                 return IRQ_NONE;
>         }
>
> @@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>         pmu_sbi_stop_hw_ctrs(pmu);
>
>         /* Overflow status register should only be read after counter are stopped */
> -       overflow = csr_read(CSR_SSCOUNTOVF);
> +       overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
> +                                 : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
>
>         /*
>          * Overflow interrupt pending bit should only be cleared after stopping
>          * all the counters to avoid any race condition.
>          */
> -       csr_clear(CSR_SIP, SIP_LCOFIP);
> +       csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
>
>         /* No overflow bit is set */
>         if (!overflow)
> @@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
>
>         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
>                 cpu_hw_evt->irq = riscv_pmu_irq;
> -               csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
> -               csr_set(CSR_IE, BIT(RV_IRQ_PMU));
> +               csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
> +               csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
>                 enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
>         }
>
> @@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
>  {
>         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
>                 disable_percpu_irq(riscv_pmu_irq);
> -               csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
> +               csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
>         }
>
>         /* Disable all counters access for user mode now */
> @@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>         struct device_node *cpu, *child;
>         struct irq_domain *domain = NULL;
>
> -       if (!riscv_isa_extension_available(NULL, SSCOFPMF))
> +       is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
> +                        sbi_get_marchid() == 0 &&
> +                        sbi_get_mimpid() == 0);
> +
> +       if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
>                 return -EOPNOTSUPP;
>
>         for_each_of_cpu_node(cpu) {
> @@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
>                 return -ENODEV;
>         }
>
> -       riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
> +       riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
> +       riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
>         if (!riscv_pmu_irq) {
>                 pr_err("Failed to map PMU interrupt for node\n");
>                 return -ENODEV;
> --
> 2.35.1
>


-- 
Regards,
Atish

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Anup Patel]

On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > With the T-HEAD C9XX cores being designed before or during the ratification
> > to the SSCOFPMF extension, they implement functionality very similar but
> > not equal to it. So add some adaptions to allow the C9XX to still handle
> > its PMU through the regular SBI PMU interface instead of defining new
> > interfaces or drivers.
> >
>
> IMO, vendor specific workarounds in the generic implementation is not
> a good idea.
> If we have to support it, I think we should just put the IRQ number in
> the DT and parse from the DT.
> The initial sscofpmf series was based on the DT. It was removed later
> as there was no need for it at that time.
> We can always revive it.
>
> Regarding the CSR number difference and static key enablement, can we
> use code patching techniques here as well ?
> At least all the T-HEAD C9XX core erratas are in one place.
>
> The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> with erratas. I don't prefer this approach
> but it keeps the vendor specific checks out of the generic code.

Whether to have a DT node (or not) was already discussed and concluded
in the past.

We don't need a DT node just to get the IRQ number. The T-HEAD custom
IRQ number can be derived based on mvendorid.

Also, all these T-HEAD specific changes in SBI PMU driver should be
implemented as erratas using ALTERNATIVE() macros.

Regards,
Anup



>
> > To work properly, this requires a matching change in SBI, though the actual
> > interface between kernel and SBI does not change.
> >
>
> Do you have a working OpenSBI implementation for this ?
>
> > The main differences are a the overflow CSR and irq number.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 6f6681bbfd36..4589166e0de4 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
> >         NULL,
> >  };
> >
> > +#define THEAD_C9XX_RV_IRQ_PMU                  17
> > +#define THEAD_C9XX_CSR_SCOUNTEROF              0x5c5
> > +
> >  /*
> >   * RISC-V doesn't have hetergenous harts yet. This need to be part of
> >   * per_cpu in case of harts with different pmu counters
> >   */
> >  static union sbi_pmu_ctr_info *pmu_ctr_list;
> > +static unsigned int riscv_pmu_irq_num;
> >  static unsigned int riscv_pmu_irq;
> > +static bool is_thead_c9xx;
> >
> >  struct sbi_pmu_event_data {
> >         union {
> > @@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
> >         fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
> >         event = cpu_hw_evt->events[fidx];
> >         if (!event) {
> > -               csr_clear(CSR_SIP, SIP_LCOFIP);
> > +               csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
> >                 return IRQ_NONE;
> >         }
> >
> > @@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
> >         pmu_sbi_stop_hw_ctrs(pmu);
> >
> >         /* Overflow status register should only be read after counter are stopped */
> > -       overflow = csr_read(CSR_SSCOUNTOVF);
> > +       overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
> > +                                 : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
> >
> >         /*
> >          * Overflow interrupt pending bit should only be cleared after stopping
> >          * all the counters to avoid any race condition.
> >          */
> > -       csr_clear(CSR_SIP, SIP_LCOFIP);
> > +       csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
> >
> >         /* No overflow bit is set */
> >         if (!overflow)
> > @@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
> >
> >         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
> >                 cpu_hw_evt->irq = riscv_pmu_irq;
> > -               csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
> > -               csr_set(CSR_IE, BIT(RV_IRQ_PMU));
> > +               csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
> > +               csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
> >                 enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
> >         }
> >
> > @@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
> >  {
> >         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
> >                 disable_percpu_irq(riscv_pmu_irq);
> > -               csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
> > +               csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
> >         }
> >
> >         /* Disable all counters access for user mode now */
> > @@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
> >         struct device_node *cpu, *child;
> >         struct irq_domain *domain = NULL;
> >
> > -       if (!riscv_isa_extension_available(NULL, SSCOFPMF))
> > +       is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
> > +                        sbi_get_marchid() == 0 &&
> > +                        sbi_get_mimpid() == 0);
> > +
> > +       if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
> >                 return -EOPNOTSUPP;
> >
> >         for_each_of_cpu_node(cpu) {
> > @@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
> >                 return -ENODEV;
> >         }
> >
> > -       riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
> > +       riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
> > +       riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
> >         if (!riscv_pmu_irq) {
> >                 pr_err("Failed to map PMU interrupt for node\n");
> >                 return -ENODEV;
> > --
> > 2.35.1
> >
>
>
> --
> Regards,
> Atish
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Anup Patel]

On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > With the T-HEAD C9XX cores being designed before or during the ratification
> > to the SSCOFPMF extension, they implement functionality very similar but
> > not equal to it. So add some adaptions to allow the C9XX to still handle
> > its PMU through the regular SBI PMU interface instead of defining new
> > interfaces or drivers.
> >
>
> IMO, vendor specific workarounds in the generic implementation is not
> a good idea.
> If we have to support it, I think we should just put the IRQ number in
> the DT and parse from the DT.
> The initial sscofpmf series was based on the DT. It was removed later
> as there was no need for it at that time.
> We can always revive it.
>
> Regarding the CSR number difference and static key enablement, can we
> use code patching techniques here as well ?
> At least all the T-HEAD C9XX core erratas are in one place.
>
> The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> with erratas. I don't prefer this approach
> but it keeps the vendor specific checks out of the generic code.

Whether to have a DT node (or not) was already discussed and concluded
in the past.

We don't need a DT node just to get the IRQ number. The T-HEAD custom
IRQ number can be derived based on mvendorid.

Also, all these T-HEAD specific changes in SBI PMU driver should be
implemented as erratas using ALTERNATIVE() macros.

Regards,
Anup



>
> > To work properly, this requires a matching change in SBI, though the actual
> > interface between kernel and SBI does not change.
> >
>
> Do you have a working OpenSBI implementation for this ?
>
> > The main differences are a the overflow CSR and irq number.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  drivers/perf/riscv_pmu_sbi.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 6f6681bbfd36..4589166e0de4 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -41,12 +41,17 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
> >         NULL,
> >  };
> >
> > +#define THEAD_C9XX_RV_IRQ_PMU                  17
> > +#define THEAD_C9XX_CSR_SCOUNTEROF              0x5c5
> > +
> >  /*
> >   * RISC-V doesn't have hetergenous harts yet. This need to be part of
> >   * per_cpu in case of harts with different pmu counters
> >   */
> >  static union sbi_pmu_ctr_info *pmu_ctr_list;
> > +static unsigned int riscv_pmu_irq_num;
> >  static unsigned int riscv_pmu_irq;
> > +static bool is_thead_c9xx;
> >
> >  struct sbi_pmu_event_data {
> >         union {
> > @@ -575,7 +580,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
> >         fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
> >         event = cpu_hw_evt->events[fidx];
> >         if (!event) {
> > -               csr_clear(CSR_SIP, SIP_LCOFIP);
> > +               csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
> >                 return IRQ_NONE;
> >         }
> >
> > @@ -583,13 +588,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
> >         pmu_sbi_stop_hw_ctrs(pmu);
> >
> >         /* Overflow status register should only be read after counter are stopped */
> > -       overflow = csr_read(CSR_SSCOUNTOVF);
> > +       overflow = !is_thead_c9xx ? csr_read(CSR_SSCOUNTOVF)
> > +                                 : csr_read(THEAD_C9XX_CSR_SCOUNTEROF);
> >
> >         /*
> >          * Overflow interrupt pending bit should only be cleared after stopping
> >          * all the counters to avoid any race condition.
> >          */
> > -       csr_clear(CSR_SIP, SIP_LCOFIP);
> > +       csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
> >
> >         /* No overflow bit is set */
> >         if (!overflow)
> > @@ -653,8 +659,8 @@ static int pmu_sbi_starting_cpu(unsigned int cpu, struct hlist_node *node)
> >
> >         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
> >                 cpu_hw_evt->irq = riscv_pmu_irq;
> > -               csr_clear(CSR_IP, BIT(RV_IRQ_PMU));
> > -               csr_set(CSR_IE, BIT(RV_IRQ_PMU));
> > +               csr_clear(CSR_IP, BIT(riscv_pmu_irq_num));
> > +               csr_set(CSR_IE, BIT(riscv_pmu_irq_num));
> >                 enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
> >         }
> >
> > @@ -665,7 +671,7 @@ static int pmu_sbi_dying_cpu(unsigned int cpu, struct hlist_node *node)
> >  {
> >         if (riscv_isa_extension_available(NULL, SSCOFPMF)) {
> >                 disable_percpu_irq(riscv_pmu_irq);
> > -               csr_clear(CSR_IE, BIT(RV_IRQ_PMU));
> > +               csr_clear(CSR_IE, BIT(riscv_pmu_irq_num));
> >         }
> >
> >         /* Disable all counters access for user mode now */
> > @@ -681,7 +687,11 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
> >         struct device_node *cpu, *child;
> >         struct irq_domain *domain = NULL;
> >
> > -       if (!riscv_isa_extension_available(NULL, SSCOFPMF))
> > +       is_thead_c9xx = (sbi_get_mvendorid() == THEAD_VENDOR_ID &&
> > +                        sbi_get_marchid() == 0 &&
> > +                        sbi_get_mimpid() == 0);
> > +
> > +       if (!riscv_isa_extension_available(NULL, SSCOFPMF) && !is_thead_c9xx)
> >                 return -EOPNOTSUPP;
> >
> >         for_each_of_cpu_node(cpu) {
> > @@ -703,7 +713,8 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
> >                 return -ENODEV;
> >         }
> >
> > -       riscv_pmu_irq = irq_create_mapping(domain, RV_IRQ_PMU);
> > +       riscv_pmu_irq_num = !is_thead_c9xx ? RV_IRQ_PMU : THEAD_C9XX_RV_IRQ_PMU;
> > +       riscv_pmu_irq = irq_create_mapping(domain, riscv_pmu_irq_num);
> >         if (!riscv_pmu_irq) {
> >                 pr_err("Failed to map PMU interrupt for node\n");
> >                 return -ENODEV;
> > --
> > 2.35.1
> >
>
>
> --
> Regards,
> Atish
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Heiko Stuebner]

Am Donnerstag, 18. August 2022, 10:24:33 CEST schrieb Anup Patel:
> On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> > >
> > > With the T-HEAD C9XX cores being designed before or during the ratification
> > > to the SSCOFPMF extension, they implement functionality very similar but
> > > not equal to it. So add some adaptions to allow the C9XX to still handle
> > > its PMU through the regular SBI PMU interface instead of defining new
> > > interfaces or drivers.
> > >
> >
> > IMO, vendor specific workarounds in the generic implementation is not
> > a good idea.
> > If we have to support it, I think we should just put the IRQ number in
> > the DT and parse from the DT.
> > The initial sscofpmf series was based on the DT. It was removed later
> > as there was no need for it at that time.
> > We can always revive it.
> >
> > Regarding the CSR number difference and static key enablement, can we
> > use code patching techniques here as well ?
> > At least all the T-HEAD C9XX core erratas are in one place.
> >
> > The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> > with erratas. I don't prefer this approach
> > but it keeps the vendor specific checks out of the generic code.
> 
> Whether to have a DT node (or not) was already discussed and concluded
> in the past.
> 
> We don't need a DT node just to get the IRQ number. The T-HEAD custom
> IRQ number can be derived based on mvendorid.

Yeah, I remember reading that discussion and thus went with the mvendorid
way in this patch.

> Also, all these T-HEAD specific changes in SBI PMU driver should be
> implemented as erratas using ALTERNATIVE() macros.

(1) "All these T-HEAD specific changes ..."
Actually the only T-HEAD-specific change is reading that different CSR
for the overflow information, the rest only makes the irq-number variable

(2) ALTERNATIVE macros are working on assembler instructions, so are you
sugesting to replace the generic csr_read() for the overflow-csr with a
custom copy like

#define sbi_pmu_read_overflow(void)						\
({								\
	register unsigned long __v;				\
	ALT_THEAD_PMU_OVERFLOW(__v); \
	__v;							\
})

and then in errata_list.h

#define ALT_THEAD_PMU_OVERFLOW(__ovl) \
__asm__ __volatile__ (alternative(
	"csrr %0, " __ASM_STR(CSR_SSCOUNTOVF),	\
	"csrr %0, " __ASM_STR(THEAD_C9XX_CSR_SCOUNTEROF), THEAD_VENDOR_ID, \
		ERRATA_THEAD_PMU, CONFIG_ERRATA_THEAD_PMU) \
			      : "=r" (__ovl) :			\
			      : "memory");

I'm not yet seeing what you're gaining by going with this approach:
- that the overflow-csr is the same bitwise but only at a different
  address is specific to the c9xx, other deviants might implement things
  completely different
- you're not getting rid of the thead mention
- and we're now duplicating the generic csr-read code

Is this the preferred way or what am I overlooking?

Thanks
Heiko



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Heiko Stuebner]

Am Donnerstag, 18. August 2022, 10:24:33 CEST schrieb Anup Patel:
> On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> > >
> > > With the T-HEAD C9XX cores being designed before or during the ratification
> > > to the SSCOFPMF extension, they implement functionality very similar but
> > > not equal to it. So add some adaptions to allow the C9XX to still handle
> > > its PMU through the regular SBI PMU interface instead of defining new
> > > interfaces or drivers.
> > >
> >
> > IMO, vendor specific workarounds in the generic implementation is not
> > a good idea.
> > If we have to support it, I think we should just put the IRQ number in
> > the DT and parse from the DT.
> > The initial sscofpmf series was based on the DT. It was removed later
> > as there was no need for it at that time.
> > We can always revive it.
> >
> > Regarding the CSR number difference and static key enablement, can we
> > use code patching techniques here as well ?
> > At least all the T-HEAD C9XX core erratas are in one place.
> >
> > The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> > with erratas. I don't prefer this approach
> > but it keeps the vendor specific checks out of the generic code.
> 
> Whether to have a DT node (or not) was already discussed and concluded
> in the past.
> 
> We don't need a DT node just to get the IRQ number. The T-HEAD custom
> IRQ number can be derived based on mvendorid.

Yeah, I remember reading that discussion and thus went with the mvendorid
way in this patch.

> Also, all these T-HEAD specific changes in SBI PMU driver should be
> implemented as erratas using ALTERNATIVE() macros.

(1) "All these T-HEAD specific changes ..."
Actually the only T-HEAD-specific change is reading that different CSR
for the overflow information, the rest only makes the irq-number variable

(2) ALTERNATIVE macros are working on assembler instructions, so are you
sugesting to replace the generic csr_read() for the overflow-csr with a
custom copy like

#define sbi_pmu_read_overflow(void)						\
({								\
	register unsigned long __v;				\
	ALT_THEAD_PMU_OVERFLOW(__v); \
	__v;							\
})

and then in errata_list.h

#define ALT_THEAD_PMU_OVERFLOW(__ovl) \
__asm__ __volatile__ (alternative(
	"csrr %0, " __ASM_STR(CSR_SSCOUNTOVF),	\
	"csrr %0, " __ASM_STR(THEAD_C9XX_CSR_SCOUNTEROF), THEAD_VENDOR_ID, \
		ERRATA_THEAD_PMU, CONFIG_ERRATA_THEAD_PMU) \
			      : "=r" (__ovl) :			\
			      : "memory");

I'm not yet seeing what you're gaining by going with this approach:
- that the overflow-csr is the same bitwise but only at a different
  address is specific to the c9xx, other deviants might implement things
  completely different
- you're not getting rid of the thead mention
- and we're now duplicating the generic csr-read code

Is this the preferred way or what am I overlooking?

Thanks
Heiko

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Anup Patel]

On Thu, Aug 25, 2022 at 7:34 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Am Donnerstag, 18. August 2022, 10:24:33 CEST schrieb Anup Patel:
> > On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> > > >
> > > > With the T-HEAD C9XX cores being designed before or during the ratification
> > > > to the SSCOFPMF extension, they implement functionality very similar but
> > > > not equal to it. So add some adaptions to allow the C9XX to still handle
> > > > its PMU through the regular SBI PMU interface instead of defining new
> > > > interfaces or drivers.
> > > >
> > >
> > > IMO, vendor specific workarounds in the generic implementation is not
> > > a good idea.
> > > If we have to support it, I think we should just put the IRQ number in
> > > the DT and parse from the DT.
> > > The initial sscofpmf series was based on the DT. It was removed later
> > > as there was no need for it at that time.
> > > We can always revive it.
> > >
> > > Regarding the CSR number difference and static key enablement, can we
> > > use code patching techniques here as well ?
> > > At least all the T-HEAD C9XX core erratas are in one place.
> > >
> > > The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> > > with erratas. I don't prefer this approach
> > > but it keeps the vendor specific checks out of the generic code.
> >
> > Whether to have a DT node (or not) was already discussed and concluded
> > in the past.
> >
> > We don't need a DT node just to get the IRQ number. The T-HEAD custom
> > IRQ number can be derived based on mvendorid.
>
> Yeah, I remember reading that discussion and thus went with the mvendorid
> way in this patch.
>
> > Also, all these T-HEAD specific changes in SBI PMU driver should be
> > implemented as erratas using ALTERNATIVE() macros.
>
> (1) "All these T-HEAD specific changes ..."
> Actually the only T-HEAD-specific change is reading that different CSR
> for the overflow information, the rest only makes the irq-number variable

If it is just overflow CSR then it is simpler to do instruction patching
in drivers/perf/riscv_pmu_sbi.c itself.

>
> (2) ALTERNATIVE macros are working on assembler instructions, so are you
> sugesting to replace the generic csr_read() for the overflow-csr with a
> custom copy like
>
> #define sbi_pmu_read_overflow(void)                                             \
> ({                                                              \
>         register unsigned long __v;                             \
>         ALT_THEAD_PMU_OVERFLOW(__v); \
>         __v;                                                    \
> })
>
> and then in errata_list.h
>
> #define ALT_THEAD_PMU_OVERFLOW(__ovl) \
> __asm__ __volatile__ (alternative(
>         "csrr %0, " __ASM_STR(CSR_SSCOUNTOVF),  \
>         "csrr %0, " __ASM_STR(THEAD_C9XX_CSR_SCOUNTEROF), THEAD_VENDOR_ID, \
>                 ERRATA_THEAD_PMU, CONFIG_ERRATA_THEAD_PMU) \
>                               : "=r" (__ovl) :                  \
>                               : "memory");
>
> I'm not yet seeing what you're gaining by going with this approach:
> - that the overflow-csr is the same bitwise but only at a different
>   address is specific to the c9xx, other deviants might implement things
>   completely different
> - you're not getting rid of the thead mention
> - and we're now duplicating the generic csr-read code
>
> Is this the preferred way or what am I overlooking?

Yes, better to have special sbi_pmu_read_overflow() in
drivers/perf/riscv_pmu_sbi.c itself which is based on ALTERNATIVES.

I am suggesting ALTERNATIVEs only because overflow CSR is
accessed in the interrupt handler which is in hot-path when we run
"perf record".

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

[Anup Patel]

On Thu, Aug 25, 2022 at 7:34 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Am Donnerstag, 18. August 2022, 10:24:33 CEST schrieb Anup Patel:
> > On Thu, Aug 18, 2022 at 1:03 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Wed, Aug 17, 2022 at 4:13 AM Heiko Stuebner <heiko@sntech.de> wrote:
> > > >
> > > > With the T-HEAD C9XX cores being designed before or during the ratification
> > > > to the SSCOFPMF extension, they implement functionality very similar but
> > > > not equal to it. So add some adaptions to allow the C9XX to still handle
> > > > its PMU through the regular SBI PMU interface instead of defining new
> > > > interfaces or drivers.
> > > >
> > >
> > > IMO, vendor specific workarounds in the generic implementation is not
> > > a good idea.
> > > If we have to support it, I think we should just put the IRQ number in
> > > the DT and parse from the DT.
> > > The initial sscofpmf series was based on the DT. It was removed later
> > > as there was no need for it at that time.
> > > We can always revive it.
> > >
> > > Regarding the CSR number difference and static key enablement, can we
> > > use code patching techniques here as well ?
> > > At least all the T-HEAD C9XX core erratas are in one place.
> > >
> > > The alternate would be just to say T-HEAD C9XX support SSCOFPMF but
> > > with erratas. I don't prefer this approach
> > > but it keeps the vendor specific checks out of the generic code.
> >
> > Whether to have a DT node (or not) was already discussed and concluded
> > in the past.
> >
> > We don't need a DT node just to get the IRQ number. The T-HEAD custom
> > IRQ number can be derived based on mvendorid.
>
> Yeah, I remember reading that discussion and thus went with the mvendorid
> way in this patch.
>
> > Also, all these T-HEAD specific changes in SBI PMU driver should be
> > implemented as erratas using ALTERNATIVE() macros.
>
> (1) "All these T-HEAD specific changes ..."
> Actually the only T-HEAD-specific change is reading that different CSR
> for the overflow information, the rest only makes the irq-number variable

If it is just overflow CSR then it is simpler to do instruction patching
in drivers/perf/riscv_pmu_sbi.c itself.

>
> (2) ALTERNATIVE macros are working on assembler instructions, so are you
> sugesting to replace the generic csr_read() for the overflow-csr with a
> custom copy like
>
> #define sbi_pmu_read_overflow(void)                                             \
> ({                                                              \
>         register unsigned long __v;                             \
>         ALT_THEAD_PMU_OVERFLOW(__v); \
>         __v;                                                    \
> })
>
> and then in errata_list.h
>
> #define ALT_THEAD_PMU_OVERFLOW(__ovl) \
> __asm__ __volatile__ (alternative(
>         "csrr %0, " __ASM_STR(CSR_SSCOUNTOVF),  \
>         "csrr %0, " __ASM_STR(THEAD_C9XX_CSR_SCOUNTEROF), THEAD_VENDOR_ID, \
>                 ERRATA_THEAD_PMU, CONFIG_ERRATA_THEAD_PMU) \
>                               : "=r" (__ovl) :                  \
>                               : "memory");
>
> I'm not yet seeing what you're gaining by going with this approach:
> - that the overflow-csr is the same bitwise but only at a different
>   address is specific to the c9xx, other deviants might implement things
>   completely different
> - you're not getting rid of the thead mention
> - and we're now duplicating the generic csr-read code
>
> Is this the preferred way or what am I overlooking?

Yes, better to have special sbi_pmu_read_overflow() in
drivers/perf/riscv_pmu_sbi.c itself which is based on ALTERNATIVES.

I am suggesting ALTERNATIVEs only because overflow CSR is
accessed in the interrupt handler which is in hot-path when we run
"perf record".

Regards,
Anup