lkml.org 
[lkml]   [2019]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V12 01/12] PCI: Add #defines for some of PCIe spec r4.0 features
From
Date
On 7/9/2019 7:44 PM, Bjorn Helgaas wrote:
> On Mon, Jul 01, 2019 at 06:09:59PM +0530, Vidya Sagar wrote:
>> Add #defines only for the Data Link Feature and Physical Layer 16.0 GT/s
>> features.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
> Please include spec references in the commit log, e.g., PCIe r5.0, sec
> 7.7.4, for Data Link Feature and sec 7.7.5 for Physical Layer 16 GT/s.
Done.

>
>> include/uapi/linux/pci_regs.h | 22 +++++++++++++++++++++-
>> 1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index f28e562d7ca8..1c79f6a097d2 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -713,7 +713,9 @@
>> #define PCI_EXT_CAP_ID_DPC 0x1D /* Downstream Port Containment */
>> #define PCI_EXT_CAP_ID_L1SS 0x1E /* L1 PM Substates */
>> #define PCI_EXT_CAP_ID_PTM 0x1F /* Precision Time Measurement */
>> -#define PCI_EXT_CAP_ID_MAX PCI_EXT_CAP_ID_PTM
>> +#define PCI_EXT_CAP_ID_DLF 0x25 /* Data Link Feature */
>> +#define PCI_EXT_CAP_ID_PL 0x26 /* Physical Layer 16.0 GT/s */
>
> Maybe PCI_EXT_CAP_ID_PL_16GT so there's a little more hint of what
> this is for?
Ok. I'll add 16GT.

>
>> +#define PCI_EXT_CAP_ID_MAX PCI_EXT_CAP_ID_PL
>>
>> #define PCI_EXT_CAP_DSN_SIZEOF 12
>> #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40
>> @@ -1053,4 +1055,22 @@
>> #define PCI_L1SS_CTL1_LTR_L12_TH_SCALE 0xe0000000 /* LTR_L1.2_THRESHOLD_Scale */
>> #define PCI_L1SS_CTL2 0x0c /* Control 2 Register */
>>
>> +/* Data Link Feature */
>> +#define PCI_DLF_CAP 0x04 /* Capabilities Register */
>> +#define PCI_DLF_LOCAL_DLF_SUP_MASK 0x007fffff /* Local Data Link Feature Supported */
>> +#define PCI_DLF_EXCHANGE_ENABLE 0x80000000 /* Data Link Feature Exchange Enable */
>> +#define PCI_DLF_STS 0x08 /* Status Register */
>> +#define PCI_DLF_REMOTE_DLF_SUP_MASK 0x007fffff /* Remote Data Link Feature Supported */
>> +#define PCI_DLF_REMOTE_DLF_SUP_VALID 0x80000000 /* Remote Data Link Feature Support Valid */
>
> I'm a little bit ambivalent about adding #defines that aren't used. I
> personally would probably just add the things we use, so the header
> file gives a clue about what's currently implemented. But I guess
> either way is fine.
Fine. I'll remove all unused defines in the next patch.

>
>> +/* Physical Layer 16.0 GT/s */
>> +#define PCI_PL_16GT_CAP 0x04 /* Capabilities Register */
>> +#define PCI_PL_16GT_CTRL 0x08 /* Control Register */
>> +#define PCI_PL_16GT_STS 0x0c /* Status Register */
>> +#define PCI_PL_16GT_LDPM_STS 0x10 /* Local Data Parity Mismatch Status Register */
>> +#define PCI_PL_16GT_FRDPM_STS 0x14 /* First Retimer Data Parity Mismatch Status Register */
>> +#define PCI_PL_16GT_SRDPM_STS 0x18 /* Second Retimer Data Parity Mismatch Status Register */
>> +#define PCI_PL_16GT_RSVD 0x1C /* Reserved */
>
> Use lower-case hex consistently here. There's no global consistency
> in this file, but we can at least be consistent in each section. But
> I'm even more hesitant about included unused #defines for "reserved"
> fields, so if you drop this it would take care of both :)
Done.

>
>> +#define PCI_PL_16GT_LE_CTRL 0x20 /* Lane Equalization Control Register */
>
> This is the only register you actually use. You defined a local
> PL16G_CAP_OFF_DSP_16G_TX_PRESET_MASK for this register. Shouldn't
> that be defined here instead of in
> drivers/pci/controller/dwc/pcie-tegra194.c?
I'll take care of it in the next patch.

>
>> #endif /* LINUX_PCI_REGS_H */
>> --
>> 2.17.1
>>

\
 
 \ /
  Last update: 2019-07-10 07:19    [W:0.110 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site