Message-ID: <115bcef9-c35c-6a80-e0a6-c862e6b6f011@oss.qualcomm.com>
Date: Sat, 19 Apr 2025 08:54:26 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        chaitanya chundru <quic_krichai@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        cros-qcom-dts-watchers@...omium.org, Jingoo Han <jingoohan1@...il.com>,
        Bartosz Golaszewski <brgl@...ev.pl>, quic_vbadigan@...cnic.com,
        amitk@...nel.org, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, jorge.ramirez@....qualcomm.com,
        Dmitry Baryshkov <lumag@...nel.org>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v5 8/9] PCI: pwrctrl: Add power control driver for tc9563



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

Powered by blists - more mailing lists