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

[Krishna Chaitanya Chundru]

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@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx>
> > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
>
>
> > +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