Re: [PATCH v4 08/10] PCI: pwrctrl: Add power control driver for tc956x

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 2/25/2025 5:39 PM, Konrad Dybcio wrote:
> On 25.02.2025 10:34 AM, Krishna Chaitanya Chundru wrote:
>> TC956x is a PCIe switch which has one upstream and three downstream
>> ports. To one of the downstream ports ethernet MAC is connected as endpoint
>> device. Other two downstream ports are supposed to connect to external
>> device. One Host can connect to TC956x by upstream port. TC956x switch
>> needs to be configured after powering on and before PCIe link was up.
>>
>> The PCIe controller driver already enables link training at the host side
>> even before this driver probe happens, due to this when driver enables
>> power to the switch it participates in the link training and PCIe link
>> may come up before configuring the switch through i2c. Once the link is
>> up the configuration done through i2c will not have any affect.To prevent
>> the host from participating in link training, disable link training on the
>> host side to ensure the link does not come up before the switch is
>> configured via I2C.
>>
>> Based up on dt property and type of the port, tc956x is configured
>> through i2c.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
>> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> ---
> 
> 
>> +struct tc956x_pwrctrl_cfg {
>> +	u32 l0s_delay;
>> +	u32 l1_delay;
>> +	u32 tx_amp;
>> +	u8 nfts[2]; /* GEN1 & GEN2*/
> 
> GEN2 */
> 
> [...]
> 
>> +static int tc956x_pwrctrl_set_l0s_l1_entry_delay(struct tc956x_pwrctrl_ctx *ctx,
>> +						 enum tc956x_pwrctrl_ports port, bool is_l1, u32 ns)
>> +{
>> +	u32 rd_val, units;
>> +	int ret;
>> +
>> +	if (!ns)
>> +		return 0;
>> +
>> +	/* convert to units of 256ns */
>> +	units = ns / 256;
> 
> Should we round up here, so that values in 1 <= x < 256 give a delay
> value of 1 unit? Or maybe such values are never expected?
> 
I will add a check above to return if ns < 256 as 0 is not expected
value.
> [...]
> 
>> +static int tc956x_pwrctrl_set_tx_amplitude(struct tc956x_pwrctrl_ctx *ctx,
>> +					   enum tc956x_pwrctrl_ports port, u32 amp)
>> +{
>> +	int port_access;
>> +
>> +	if (amp < TC956X_TX_MARGIN_MIN_VAL)
>> +		return 0;
>> +
>> +	/*  txmargin = (Amp(uV) - 400000) / 3125 */
> 
> double space
> 
>> +	amp = (amp - TC956X_TX_MARGIN_MIN_VAL) / 3125;
> 
> similarly here, is 0 an expected value for 1 <= x < 3125?
> 
Here 0 is expected value in this case.

- Krishna Chaitanya.
> Konrad