Re: [PATCH 2/3] PCI: cadence: Use DT bindings to set PHY latencies

[Bjorn Helgaas]

On Thu, Oct 13, 2022 at 08:26:48AM +0200, Dominic Rath wrote:
> From: Alexander Bahle <bahle@xxxxxxxxxxxxxxx>
> 
> Use optional "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps"
> DeviceTree bindings to set the CDNS_PCIE_LM_PTM_LAT_PARAM(_IDX)
> register(s) during PCIe host and ep setup.
> The properties expect a list of uint32 PHY latencies in picoseconds for
> every supported speed starting at PCIe Gen1, e.g.:

s/ep/endpoint/
s/properties expect a list/properties are lists/

Rewrap into a single paragraph or add a blank line between paragraphs.

>   max-link-speed = <2>;
>   tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
>   rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
> 
> There should be a value for every supported speed but it is not enforced or
> necessary. A warning is emitted to let users know that the PTM timestamps
> from this PCIe device may not be precise enough for some applications.

Not sure what "it is not enforced or necessary" means.  Maybe it just
means that if a value is missing, we don't program LAT_PARAM and we
emit a warning?

> +	param_count = of_property_count_u32_elems(np, key);
> +	if (param_count < 0 || param_count < max_link_speed) {
> +		dev_warn(dev,
> +			"no %s set for one or more speeds: %d\n",
> +			key, param_count);
> +	}
> +
> +	/* Don't set param for unsupported speed */
> +	if (param_count > max_link_speed)
> +		param_count = max_link_speed;
> +
> +	for (i = 0; i < param_count; i++) {
> +		if (of_property_read_u32_index(np, key, i,
> +					       &latency) < 0) {
> +			dev_err(dev, "failed to set latency for speed %d. %s\n",
> +				i, key);

Seems like these messages should contain "PTM" somewhere.

If they're truly optional properties, should these be dev_info()
instead of dev_warn/dev_err?

Bjorn