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:   Thu, 13 Oct 2022 08:59:03 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Dominic Rath <dominic.rath@ibv-augsburg.de>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        tjoseph@cadence.com, bhelgaas@google.com, lpieralisi@kernel.org,
        nm@ti.com, vigneshr@ti.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
        Alexander Bahle <bahle@ibv-augsburg.de>,
        Dominic Rath <rath@ibv-augsburg.de>
Subject: Re: [PATCH 2/3] PCI: cadence: Use DT bindings to set PHY latencies
Message-ID: <20221013135903.GA3243887@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221013062649.303184-3-dominic.rath@ibv-augsburg.de>
Precedence: bulk

On Thu, Oct 13, 2022 at 08:26:48AM +0200, Dominic Rath wrote:
> From: Alexander Bahle <bahle@ibv-augsburg.de>
> 
> 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