Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Sep 2022 14:39:48 +0100
From:   Jonathan Cameron <Jonathan.Cameron@huawei.com>
To:     Shuai Xue <xueshuai@linux.alibaba.com>
CC:     Robin Murphy <robin.murphy@arm.com>,
        Bjorn Helgaas <helgaas@kernel.org>, <will@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <rdunlap@infradead.org>,
        <mark.rutland@arm.com>, <baolin.wang@linux.alibaba.com>,
        <zhuo.song@linux.alibaba.com>, <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver
Message-ID: <20220927143948.00004c43@huawei.com>
In-Reply-To: <2085a695-7fa2-b560-3164-c62cb17dd5f7@linux.alibaba.com>
References: <20220926171857.GA1609097@bhelgaas>
        <7502d496-9ec1-1ca4-c643-376ec2aa662e@linux.alibaba.com>
        <20220927110435.00005b4d@huawei.com>
        <5372edb4-5717-42a0-142e-91657a9b18c3@arm.com>
        <2085a695-7fa2-b560-3164-c62cb17dd5f7@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, 27 Sep 2022 20:49:26 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> + Jonathan
> 
> 在 2022/9/27 PM6:14, Robin Murphy 写道:
> > On 2022-09-27 11:04, Jonathan Cameron wrote:  
> >> On Tue, 27 Sep 2022 13:13:29 +0800
> >> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> >>  
> >>> 在 2022/9/27 AM1:18, Bjorn Helgaas 写道:  
> >>>> On Mon, Sep 26, 2022 at 09:31:34PM +0800, Shuai Xue wrote:  
> >>>>> 在 2022/9/23 PM11:54, Jonathan Cameron 写道:  
> >>>>>>> I found a similar definition in arch/ia64/pci/pci.c .
> >>>>>>>
> >>>>>>>     #define PCI_SAL_ADDRESS(seg, bus, devfn, reg)        \
> >>>>>>>     (((u64) seg << 24) | (bus << 16) | (devfn << 8) | (reg))
> >>>>>>>
> >>>>>>> Should we move it into a common header first?  
> >>>>>>
> >>>>>> Maybe. The bus, devfn, reg part is standard bdf, but I don't think
> >>>>>> the PCI 6.0 spec defined a version with the seg in the upper bits.
> >>>>>> I'm not sure if we want to adopt that in LInux.  
> >>>>>
> >>>>> I found lots of code use seg,bus,devfn,reg with format "%04x:%02x:%02x.%x",
> >>>>> I am not quite familiar with PCIe spec. What do you think about it, Bjorn?  
> >>>>
> >>>> The PCIe spec defines an address encoding for bus/device/function/reg
> >>>> for the purposes of ECAM (PCIe r6.0, sec 7.2.2), but as far as I know,
> >>>> it doesn't define anything similar that includes the segment.  The
> >>>> segment is really outside the scope of PCIe because each segment is a
> >>>> completely separate PCIe hierarchy.  
> >>>
> >>> Thank you for your explanation.
> >>>  
> >>>>
> >>>> So I probably wouldn't make this a generic definition.  But if/when
> >>>> you print things like this out, please do use the format spec you
> >>>> mentioned above so it matches the style used elsewhere.
> >>>>      
> >>>
> >>> Agree. The print format of bus/device/function/reg is "%04x:%02x:%02x.%x",
> >>> so I named the PMU as the same format. Then the usage flow would be:
> >>>
> >>> - lspci to get the device root port in format seg/bus/device/function/reg.
> >>>     10:00.0 PCI bridge: Device 1ded:8000 (rev 01)
> >>> - select its PMU name pcie_bdf_100000.
> >>> - monitor with perf:
> >>>     perf stat -a -e pcie_bdf_100000/Rx_PCIe_TLP_Data_Payload/  
> >>
> >> I think you probably want something in there to indicate it's an RP
> >> and the bdf part may be redundant...  
> > 
> > Indeed that seems horribly unclear; personally I reckon something like "dw_pcie_200" would be more appropriate. The address is just a disambiguator between multiple instances so doesn't need any further emphasis, but what is crucial to the user is exactly what kind of PMU it is (especially if there's potential for other unrelated PCIe functions to start exposing their own different PMUs).  
> 
> I see your point. The current prefix `pcie_bdf` is not appropriate,
> 
> - it does not indicate it is for a root point as Jonathan mentioned.
> - its prefix is not `dwc`
> 
> Is dwc_rootport_100000 more appropriate?
> 
> - `dwc` indicates the PMU is for Synopsys DesignWare Cores PCIe controller IP
> - `rootport` indicates the PMU is for a root port device
> - `100000` indicates the device address

Looks good to me.

J
> 
> 
> Thank you.
> 
> Best Regards,
> Shuai
> 
> 
> 
>