On 4/19/2025 1:46 AM, Bjorn Helgaas wrote:
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.
The switch has a DSP to which embedded ethernet was connected, I will
update the text in next patch.
+#define TC9563_GPIO_CONFIG 0x801208
+#define TC9563_RESET_GPIO 0x801210
I guess these are i2c register addresses?
yes
+#define TC9563_BUS_CONTROL 0x801014
Unused.
I will remove
+#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?
ack
+#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.
The I2C in the switch expects big endian format for address and this
requirement is specific to i2c on this switch only. Not every i2c
devices may have this requirement.
+ /* 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.
ack it is a miss from my side, I will fix in next patch.
Thanks for catching this.
+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?
This is from The tc9653 databook. I will add spec citation in next
patch.
+ 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?
These are entry delays, not the exit latencies from the Spec.
+ 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?
ack
+ 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.
The switch has inbuilt integrated ethernet and we described in the DT
for the same.
- Krishna Chaitanya.
+ for_each_child_of_node_scoped(child, child1) {
+ ret = tc9563_pwrctrl_parse_device_dt(ctx, child1, port++);
+ if (ret)
+ break;
+ }
+ }
Bjorn
