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
