Hi Hoan,
Apologies for the delay in getting to this.
On Mon, Apr 03, 2017 at 09:47:57AM -0700, Hoan Tran wrote:
> This patch adds support for SoC-wide (AKA uncore) Performance Monitoring
> Unit version 3.
>
> It can support up to
> - 2 IOB PMU instances
> - 8 L3C PMU instances
> - 2 MCB PMU instances
> - 8 MCU PMU instances
Please elaborate on the differences compared with prior versions.
I see that counters are 64-bit. Are there any other important details to
be aware of?
[...]
> +static struct attribute *l3c_pmu_v3_events_attrs[] = {
> + XGENE_PMU_EVENT_ATTR(read-hit, 0x01),
> + XGENE_PMU_EVENT_ATTR(read-miss, 0x02),
> + XGENE_PMU_EVENT_ATTR(reads, 0x08),
> + XGENE_PMU_EVENT_ATTR(writes, 0x09),
> +};
Some of these are singular (e.g. "read-hit", "read-miss"), while others
are plural (e.g. "reads", "writes").
The existing driver use the singular form. Please remain consistent with
that. e.g. turn "reads" into "read".
> + XGENE_PMU_EVENT_ATTR(PA-req-buf-alloc-all, 0x01),
Likewise, please consistently use lower case.
> + XGENE_PMU_EVENT_ATTR(pmu-act-sent, 0x01),
Surely we don't need "pmu" in the event names?
[...]
> static inline u64 xgene_pmu_read_counter32(struct xgene_pmu_dev *pmu_dev,
> int idx)
> {
> return (u64)readl(pmu_dev->inf->csr + PMU_PMEVCNTR0 + (4 * idx));
> }
I don't think the cast is necessary. Please remove it.
> static inline void
> +xgene_pmu_write_counter64(struct xgene_pmu_dev *pmu_dev, int idx, u64 val)
> +{
> + u32 cnt_lo, cnt_hi;
> +
> + cnt_lo = val & 0xFFFFFFFF;
> + cnt_hi = val >> 32;
cnt_hi = upper_32_bits(val);
cnt_lo = lower_32_bits(val);
(both are in <linux/kernel.h>)
> +
> + /* v3 has 64-bit counter registers composed by 2 32-bit registers */
> + xgene_pmu_write_counter32(pmu_dev, 2 * idx, val);
> + xgene_pmu_write_counter32(pmu_dev, 2 * idx + 1, val >> 32);
Please use cnt_hi and cnt_lo for the writes.
[...]
> @@ -621,7 +989,7 @@ static void xgene_perf_event_set_period(struct perf_event *event)
> struct xgene_pmu *xgene_pmu = pmu_dev->parent;
> struct hw_perf_event *hw = &event->hw;
> /*
> - * The X-Gene PMU counters have a period of 2^32. To account for the
> + * The X-Gene PMU counters have a period of 2^32 or more. To account for the
> * possiblity of extreme interrupt latency we program for a period of
> * half that. Hopefully we can handle the interrupt before another 2^31
> * events occur and the counter overtakes its previous value.
This comment is now a little out-of-date, as we don't start 64-bit
counters at half their period.
I guess we don't expect a 64-bit counter to overflow, so can we state
that in the comment?
[...]
> -static int acpi_pmu_probe_active_mcb_mcu(struct xgene_pmu *xgene_pmu,
> +static int acpi_pmu_probe_active_mcb_mcu_l3c(struct xgene_pmu *xgene_pmu,
> struct platform_device *pdev)
> {
> void __iomem *csw_csr, *mcba_csr, *mcbb_csr;
> struct resource *res;
> unsigned int reg;
> + u32 mcb0routing;
> + u32 mcb1routing;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> csw_csr = devm_ioremap_resource(&pdev->dev, res);
> @@ -899,41 +1309,72 @@ static int acpi_pmu_probe_active_mcb_mcu(struct xgene_pmu *xgene_pmu,
> return PTR_ERR(csw_csr);
> }
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> - mcba_csr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(mcba_csr)) {
> - dev_err(&pdev->dev, "ioremap failed for MCBA CSR resource\n");
> - return PTR_ERR(mcba_csr);
> - }
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> - mcbb_csr = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(mcbb_csr)) {
> - dev_err(&pdev->dev, "ioremap failed for MCBB CSR resource\n");
> - return PTR_ERR(mcbb_csr);
> - }
> -
> reg = readl(csw_csr + CSW_CSWCR);
> - if (reg & CSW_CSWCR_DUALMCB_MASK) {
> - /* Dual MCB active */
> - xgene_pmu->mcb_active_mask = 0x3;
> - /* Probe all active MC(s) */
> - reg = readl(mcbb_csr + CSW_CSWCR);
> - xgene_pmu->mc_active_mask =
> - (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0xF : 0x5;
> + if (xgene_pmu->version == PCP_PMU_V3) {
> + mcb0routing = CSW_CSWCR_MCB0_ROUTING(reg);
> + mcb1routing = CSW_CSWCR_MCB0_ROUTING(reg);
> + if (reg & CSW_CSWCR_DUALMCB_MASK) {
> + /* Dual MCB active */
> + xgene_pmu->mcb_active_mask = 0x3;
> + /* Probe all active L3C(s), maximum is 8 */
> + xgene_pmu->l3c_active_mask = 0xFF;
> + /* Probe all active MC(s), maximum is 8 */
> + if ((mcb0routing == 0x2) && (mcb1routing == 0x2))
> + xgene_pmu->mc_active_mask = 0xFF;
> + else if ((mcb0routing == 0x1) && (mcb1routing == 0x1))
> + xgene_pmu->mc_active_mask = 0x33;
> + else
> + xgene_pmu->mc_active_mask = 0x11;
> +
> + } else {
> + /* Single MCB active */
> + xgene_pmu->mcb_active_mask = 0x1;
> + /* Probe all active L3C(s), maximum is 4 */
> + xgene_pmu->l3c_active_mask = 0x0F;
> + /* Probe all active MC(s), maximum is 4 */
> + if ((mcb0routing == 0x2) && (mcb1routing == 0x0))
> + xgene_pmu->mc_active_mask = 0x0F;
> + else if ((mcb0routing == 0x1) && (mcb1routing == 0x0))
> + xgene_pmu->mc_active_mask = 0x03;
> + else
> + xgene_pmu->mc_active_mask = 0x01;
> + }
> } else {
> - /* Single MCB active */
> - xgene_pmu->mcb_active_mask = 0x1;
> - /* Probe all active MC(s) */
> - reg = readl(mcba_csr + CSW_CSWCR);
> - xgene_pmu->mc_active_mask =
> - (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0x3 : 0x1;
> - }
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + mcba_csr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(mcba_csr)) {
> + dev_err(&pdev->dev, "ioremap failed for MCBA CSR resource\n");
> + return PTR_ERR(mcba_csr);
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> + mcbb_csr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(mcbb_csr)) {
> + dev_err(&pdev->dev, "ioremap failed for MCBB CSR resource\n");
> + return PTR_ERR(mcbb_csr);
> + }
>
> + xgene_pmu->l3c_active_mask = 0x1;
> + if (reg & CSW_CSWCR_DUALMCB_MASK) {
> + /* Dual MCB active */
> + xgene_pmu->mcb_active_mask = 0x3;
> + /* Probe all active MC(s) */
> + reg = readl(mcbb_csr + CSW_CSWCR);
> + xgene_pmu->mc_active_mask =
> + (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0xF : 0x5;
> + } else {
> + /* Single MCB active */
> + xgene_pmu->mcb_active_mask = 0x1;
> + /* Probe all active MC(s) */
> + reg = readl(mcba_csr + CSW_CSWCR);
> + xgene_pmu->mc_active_mask =
> + (reg & MCBADDRMR_DUALMCU_MODE_MASK) ? 0x3 : 0x1;
> + }
> + }
> return 0;
> }
Please split these into separate functions. e.g. have
acpi_pmu_probe_active_mcb_mcu_v1_2() and
acpi_pmu_probe_active_mcb_mcu_v3().
> static char *xgene_pmu_dev_name(struct device *dev, u32 type, int id)
> @@ -997,6 +1439,8 @@ static char *xgene_pmu_dev_name(struct device *dev, u32 type, int id)
> return devm_kasprintf(dev, GFP_KERNEL, "l3c%d", id);
> case PMU_TYPE_IOB:
> return devm_kasprintf(dev, GFP_KERNEL, "iob%d", id);
> + case PMU_TYPE_IOB_SLOW:
> + return devm_kasprintf(dev, GFP_KERNEL, "iob-slow%d", id);
What is IOB slow? How does it relate to the existing IOB?
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
