Received-SPF: pass (google.com: domain of linux-kernel+bounces-154769-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9baa0835-9cac-4bb8-b172-9e1351ffa6c9@theobroma-systems.com>
Date: Tue, 23 Apr 2024 11:10:35 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] arm64: dts: rockchip: add PCIe3 support on
 rk3588-jaguar
To: Jonas Karlman <jonas@kwiboo.se>, Heiko Stuebner <heiko@sntech.de>,
 Heiko Stuebner <heiko.stuebner@cherry.de>
Cc: linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 linux-kernel@vger.kernel.org
References: <20240422133502.2592975-1-heiko@sntech.de>
 <f33c0c0b-979b-4f50-89f7-976832968494@kwiboo.se>
Content-Language: en-US
From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
In-Reply-To: <f33c0c0b-979b-4f50-89f7-976832968494@kwiboo.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?dDI3V3Q1VkVPUGRlVWdhVGp3V1hYbkptTkg2WUUwd3E2NEVLZEd5VmhuV3dj?=
 =?utf-8?B?Q0QwNUxSTmlJRmpIbTQ0MmM3Qk5ReEgxNFdhMGRMVGMrYkVNWXFhOFFUN1hM?=
 =?utf-8?B?QW91UERvaVljQXJKQlhWckJ4Wm56Sll3eWIrSDczYmhDTzVyek5kWFk3SFl1?=
 =?utf-8?B?VFZqTmpPLy80YkgwKzJYZk1UM2xDbXJHZnc3clI4VjdpcGordFVsRlZXMk0y?=
 =?utf-8?B?YVRzT2h2ckhyeld1MUxGKzQrTDU2OTFhVTFrTEZnQit0NFFxUGNzWmEzVWgy?=
 =?utf-8?B?RzRBbWFkV0xvWXZDSE1OSU11L1RTNW4zVkRwSStVQTZTcjVadURFNFBocXlv?=
 =?utf-8?B?T2tpaGhGTTlrdlFvSDc2ZzY4NXR3ZE5Fek9DdE56QXJwMDczWWNHK0E2N1ly?=
 =?utf-8?B?eEU0VllLdkVwbmlYcEVnRlFidmJ5bjdHa2RRd09IY3EwbHpGUCt4Z3MrVVU2?=
 =?utf-8?B?NXA4M2gvMzRGdEhWMm4wUnFLVDI0S3prSDA2dEUwNlpMMWlQODFtaFhqVENl?=
 =?utf-8?B?YnlLRjB1SlR6Ym1pMHFLdnB2L0IyQmFkZkowTjkwSW5qLzRvdXgvejN5R1NK?=
 =?utf-8?B?RWlkMDJDNjB6TjN0UVIrU1oxaXlwQno3aW81aTN1VS8vQmFlTVRGUUhoM09n?=
 =?utf-8?B?Y3ViOUpOcml1ZTUrYVpmM3FYYjllejhvaXZWSmFwcWdtUWdEdTdxQldRUGJ2?=
 =?utf-8?B?YThtOWc5L281bmxZV3dzeGtZUFN5UWZjdmF4T1c5Q29kYk1vakF2VnBzQnZZ?=
 =?utf-8?B?T3UxdDZmQ0ZrZ0twYzlZYUZGTTE0T1JPd3hydjZadEZacm1TNWlSSHlvdzlG?=
 =?utf-8?B?TDRVc0pIc2dWSVg2eW1tTWxhYU5qcDVPTjd3cHJUenpvS2hCNjRFVjhmNVdZ?=
 =?utf-8?B?cnlnb3YxVlAzdUpuQmRQem03RGhVMFQwckRUejZnazQxTUwzRU1jaG9YRU03?=
 =?utf-8?B?elFLaktZWnZvV2pUeEp0SFdmSW91WVQvUEFTclBDeWM3dEhpMEExWGZhblAz?=
 =?utf-8?B?cmdxS0czcldJMEhKZXJweWJWdlU2NFJhQ0xuY2xGUXpMYUlVUk9rUHdhRG9k?=
 =?utf-8?B?QUo2Z3N4RDFoTnVqcitrclBwRWk4TWdiRWRlM0hiYVZkSHhxZ0h4Q0srTCs5?=
 =?utf-8?B?SmhtdnJGNitBdHNtOGtxMFBnYXVFRG5iVzVTUXBtTFBaQkVlRDR4UllNeU5j?=
 =?utf-8?B?K2xPNzFaZXBmYUhMWmZvOE4rRXR3WFNRY0JLMnN2WmJscjlDaW9VdFNOWW5R?=
 =?utf-8?B?dFp5QlpFTjQwVUZQMm1pd1VWSzJJdUNFT0lEQnE0ZXplRVo2aFlhcFRueElm?=
 =?utf-8?B?aWNSQy93WlVJczJuMVdITnNGQWMrME9rQ05LMmVtcEtId1FXL2NZSkx0cTVB?=
 =?utf-8?B?bDNzQTIxTzRqbUo5SUdpa29HbGZ4czhDVzQ3R0FYSXQzVlFWeDlOUkprTFkr?=
 =?utf-8?B?bnBKMENIbDRoWXBIczkzOTdmTjJQSTNsVGFac3J5bjJWYVNBU2dWamZkcTI3?=
 =?utf-8?B?ckt5Snl2MjFaTG01enpueUVURjV4azdUZDFWditNT2dVbEhVNEVKUExQcGIx?=
 =?utf-8?B?eC9yazB6bjhYcUJWRzNjcmt6ckFRSjdaQkk3M1hycEt1RFVFS2VIVFYrWEVG?=
 =?utf-8?B?V1VMb3BzQWM2RklwOWladTRUY1hsVTlycUxtcHYxaU1kekFxU2t4emE0RTBH?=
 =?utf-8?B?RWtwbWcwdmJyU2JYWkJPdlZ2RE42WW9raWlVbGdyZ1dGaHVLZXREb1VqTG9r?=
 =?utf-8?B?QUZyVVpoTWVDZ2ZKb3lFa2JTeXZHK0hFWVBUVU5YZ0ZER1J1M2VwT1E0K3Qy?=
 =?utf-8?B?dnVGd0JmY2pPdVpnMmFFRXJQTEdJS1pFUGRjTDBpaWJhVDFIdHBnNmJpdzkr?=
 =?utf-8?B?cUV0OUljL21KcHZxdjUyaTg0ZE1jdW1Wc2tTZUkyWEQwbS9qdG0za09meERB?=
 =?utf-8?B?ZGRsMzB3UnJpSnNhUCtiNmdiNmNFM05NN3dSbHdOSklNbjMvRkpRdVVrK0hT?=
 =?utf-8?B?QzErRUh4M0xuWFJFQ2lYRDFwRGJ6UktDNkdjME0rdkc1eGpGZmdQcjN1VnV4?=
 =?utf-8?B?bEhmNFJKZVhxZDRaSGh5QmI3OXlKb3E1Q25WelNkaUVwb2p1aTJLbW5GTmQx?=
 =?utf-8?B?TjU4WnNOVjIvZDZ1aTF1UU94WUJtY1RhMkhQT0VEVVFLZlM4TzRNaTVUaEJ4?=
 =?utf-8?Q?ZGjrh4+nqNDTYCmnmDrgYfk=3D?=
X-OriginatorOrg: theobroma-systems.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2af0c88d-2208-4393-2589-08dc63753e12
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB4833.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 09:10:38.5790
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5e0e1b52-21b5-4e7b-83bb-514ec460677e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vt4AnHtprl3lhnEX5WJAP5Kn6t9Y25+b/bDBPD/qXiJtgPB5c6kXpjmVhKysOclPwvhM+j/Az2CWihs/OOYXUNkv06XQA9nRiLz/zKpXXlbkxXgxcWQs0Bvi7oD7kGf1
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR04MB8082

Hi Jonas, Heiko,

On 4/22/24 20:36, Jonas Karlman wrote:
> Hi Heiko,
> 
> On 2024-04-22 15:35, Heiko Stuebner wrote:
>> From: Heiko Stuebner <heiko.stuebner@cherry.de>
>>
>> The Jaguar SBC provides an M.2 slot connected to the pcie3 controller.
>> In contrast to a number of other boards the pcie-refclk is gpio-controlled,
>> so the necessary clock and is added to the list of pcie3 clocks.
>>
>> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
>> Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>> changes in v3:
>> - drop unnecessary pinctrl comment (Quentin)
>> - add Quentin's review
>>
>> changes in v2:
>> - "an" M.2 slot (Dragan)
>> - pinctrl for refclk-en and reset pin (Quentin)
>> - don't repurpose the pcie30x4_pins pinctrl entry for only wake (Quentin)
>>
>>   .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 59 +++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
>> index 4076c92668ba..0ad53d7768a3 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
>> @@ -72,6 +72,27 @@ led-1 {
>>   		};
>>   	};
>>   
>> +	/*
>> +	 * 100MHz reference clock for PCIe peripherals from PI6C557-05BLE
>> +	 * clock generator.
>> +	 * The clock output is gated via the OE pin on the clock generator.
>> +	 * This is modeled as a fixed-clock plus a gpio-gate-clock.
>> +	 */
>> +	pcie_refclk_gen: pcie-refclk-gen-clock {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <1000000000>;
> 
> This should probably be 100000000 (100 MHz) and not 1 GHz?
> 

Good catch, we also need to fix this for Tiger where the same mistake 
was made it seems :/

Wondering if we couldn't have some kind of constant to make it more 
readable in DTSes? E.g.

#define 1K	1000
#define 1Ki	1024
#define 1M	1000000
#define 1Mi	1048576


And in the DT we could simply have <(100 * 1M)> wherever appropriate?

Thanks,
Quentin