Re: [PATCH v5 8/9] PCI: pwrctrl: Add power control driver for tc9563

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Apr 12, 2025 at 07:19:57AM +0530, Krishna Chaitanya Chundru wrote:
> TC9563 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 TC9563 by upstream port. TC9563 switch
> needs to be configured after powering on and before PCIe link was up.

This is described as a generic driver for TC9563, but the ethernet MAC
stuff built into doesn't sound generic.  Maybe this could be clarified
here and in the Kconfig help text.

> +#define TC9563_GPIO_CONFIG		0x801208
> +#define TC9563_RESET_GPIO		0x801210

I guess these are i2c register addresses?

> +#define TC9563_BUS_CONTROL		0x801014

Unused.

> +#define TC9563_PORT_L0S_DELAY		0x82496c
> +#define TC9563_PORT_L1_DELAY		0x824970

I guess these correspond to "L0s Exit Latency" and "L1 Exit Latency"
in the PCIe spec?  Can we name them to suggest that?  Where do the
values come from?

> +#define TC9563_EMBEDDED_ETH_DELAY	0x8200d8
> +#define TC9563_ETH_L1_DELAY_MASK	GENMASK(27, 18)
> +#define TC9563_ETH_L1_DELAY_VALUE(x)	FIELD_PREP(TC9563_ETH_L1_DELAY_MASK, x)
> +#define TC9563_ETH_L0S_DELAY_MASK	GENMASK(17, 13)
> +#define TC9563_ETH_L0S_DELAY_VALUE(x)	FIELD_PREP(TC9563_ETH_L0S_DELAY_MASK, x)

> +#define TC9563_PWRCTL_MAX_SUPPLY	6
> +
> +struct tc9563_pwrctrl_ctx {
> +	struct regulator_bulk_data supplies[TC9563_PWRCTL_MAX_SUPPLY];
> +	struct tc9563_pwrctrl_cfg cfg[TC9563_MAX];
> +	struct gpio_desc *reset_gpio;
> +	struct i2c_adapter *adapter;
> +	struct i2c_client *client;
> +	struct pci_pwrctrl pwrctrl;
> +};

> +static int tc9563_pwrctrl_i2c_write(struct i2c_client *client,
> +				    u32 reg_addr, u32 reg_val)
> +{
> +	struct i2c_msg msg;
> +	u8 msg_buf[7];
> +	int ret;
> +
> +	msg.addr = client->addr;
> +	msg.len = 7;
> +	msg.flags = 0;
> +
> +	/* Big Endian for reg addr */
> +	put_unaligned_be24(reg_addr, &msg_buf[0]);

Of the 1000+ calls to i2c_transfer(), I only see about 25 uses of
put_unaligned_be*() beforehand.  Are most of the other 975 calls
broken?  Or maybe they are only used on platforms of known endianness
so they don't need it?  Just a question; I have no idea how i2c works.

> +	/* Little Endian for reg val */
> +	put_unaligned_le32(reg_val, &msg_buf[3]);
> +
> +	msg.buf = msg_buf;
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	return ret == 1 ? 0 : ret;
> +}

> +	ret = of_property_read_u8_array(node, "nfts", cfg->nfts, 2);
> +	if (ret && ret != -EINVAL)
> +		return ret;

Asked elsewhere whether "nfts" is supposed to match the DT "n-fts"
property.

> +static int tc9563_pwrctrl_bring_up(struct tc9563_pwrctrl_ctx *ctx)
> +{
> +	struct tc9563_pwrctrl_cfg *cfg;
> +	int ret, i;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> +	if (ret < 0)
> +		return dev_err_probe(ctx->pwrctrl.dev, ret, "cannot enable regulators\n");
> +
> +	gpiod_set_value(ctx->reset_gpio, 0);
> +
> +	 /* wait for the internal osc frequency to stablise */

s/stablise/stabilize/ (or "stabilise" if you prefer)

> +	usleep_range(10000, 10500);

Where do these values come from?  Can we add a spec citation?

> +		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(ctx, i, false, cfg->l0s_delay);
> +		if (ret) {
> +			dev_err(ctx->pwrctrl.dev, "Setting L0s entry delay failed\n");

Since these are *entry* delays, maybe they're not related to the "Exit
Latencies" from the PCIe spec.  But if they *are* related, can we use
the same terms here?

> +		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(ctx, i, true, cfg->l1_delay);
> +		if (ret) {
> +			dev_err(ctx->pwrctrl.dev, "Setting L1 entry delay failed\n");

> +		ret = tc9563_pwrctrl_set_tx_amplitude(ctx, i, cfg->tx_amp);
> +		if (ret) {
> +			dev_err(ctx->pwrctrl.dev, "Setting Tx amplitube failed\n");

s/amplitube/amplitude/

> +			goto power_off;
> +		}
> +
> +		ret = tc9563_pwrctrl_set_nfts(ctx, i, cfg->nfts);
> +		if (ret) {
> +			dev_err(ctx->pwrctrl.dev, "Setting nfts failed\n");

s/nfts/N_FTS/ to match spec usage.

> +static int tc9563_pwrctrl_probe(struct platform_device *pdev)
> +{

> ...
> +	ctx->supplies[0].supply = "vddc";
> +	ctx->supplies[1].supply = "vdd18";
> +	ctx->supplies[2].supply = "vdd09";
> +	ctx->supplies[3].supply = "vddio1";
> +	ctx->supplies[4].supply = "vddio2";
> +	ctx->supplies[5].supply = "vddio18";

Seems like this could be made into a const static array, maybe next to
TC9563_PWRCTL_MAX_SUPPLY?

> +	for_each_child_of_node_scoped(pdev->dev.of_node, child) {
> +		ret = tc9563_pwrctrl_parse_device_dt(ctx, child, port++);
> +		if (ret)
> +			break;
> +		/* Embedded ethernet device are under DSP3 */
> +		if (port == TC9563_DSP3)

Is this ethernet thing integrated into the TC9563?  Seems like the
sort of topology thing that would normally be described via DT.

> +			for_each_child_of_node_scoped(child, child1) {
> +				ret = tc9563_pwrctrl_parse_device_dt(ctx, child1, port++);
> +				if (ret)
> +					break;
> +			}
> +	}

Bjorn