Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 26, 2024 at 06:07:55PM GMT, Krishna chaitanya chundru wrote:
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
> 
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
> 
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>
> ---
>  drivers/pci/pwrctl/Kconfig             |   7 +
>  drivers/pci/pwrctl/Makefile            |   1 +
>  drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
> 
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index f1b824955d4b..a419b250006d 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -14,4 +14,11 @@ config PCI_PWRCTL_PWRSEQ
>  	  Enable support for the PCI power control driver for device
>  	  drivers using the Power Sequencing subsystem.
>  
> +config PCI_PWRCTL_QPS615
> +	tristate "PCI Power Control driver for QPS615"
> +	select PCI_PWRCTL
> +	help
> +	  Enable support for the PCI power control driver for QPS615 and
> +	  configures it.
> +
>  endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index d308aae4800c..ac563a70c023 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_PWRCTL)		+= pci-pwrctl-core.o
>  pci-pwrctl-core-y			:= core.o
>  
>  obj-$(CONFIG_PCI_PWRCTL_PWRSEQ)		+= pci-pwrctl-pwrseq.o
> +obj-$(CONFIG_PCI_PWRCTL_QPS615)		+= pci-pwrctl-qps615.o
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-qps615.c b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> new file mode 100644
> index 000000000000..1f2caf5d7da2
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include "../pci.h"
> +
> +struct qcom_qps615_pwrctl_i2c_setting {
> +	u32 slv_addr;
> +	u32 reg_addr;
> +	u32 val;
> +};
> +
> +struct qcom_qps615_pwrctl_ctx {
> +	struct i2c_adapter *adapter;
> +	struct pci_pwrctl pwrctl;
> +	struct device_link *link;
> +	struct regulator *vdd;
> +};
> +
> +/* write 32-bit value to 24 bit register */
> +static int qps615_switch_i2c_write(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> +				   u32 reg_val)
> +{
> +	struct i2c_msg msg;
> +	u8 msg_buf[7];
> +	int ret;
> +
> +	msg.addr = slv_addr;
> +	msg.len = 7;
> +	msg.flags = 0;
> +
> +	/* Big Endian for reg addr */
> +	msg_buf[0] = (u8)(reg_addr >> 16);
> +	msg_buf[1] = (u8)(reg_addr >> 8);
> +	msg_buf[2] = (u8)reg_addr;
> +
> +	/* Little Endian for reg val */
> +	msg_buf[3] = (u8)(reg_val);
> +	msg_buf[4] = (u8)(reg_val >> 8);
> +	msg_buf[5] = (u8)(reg_val >> 16);
> +	msg_buf[6] = (u8)(reg_val >> 24);
> +
> +	msg.buf = msg_buf;
> +	ret = i2c_transfer(ctx->adapter, &msg, 1);
> +	return ret == 1 ? 0 : ret;
> +}
> +
> +/* read 32 bit value from 24 bit reg addr */
> +static int qps615_switch_i2c_read(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> +				  u32 *reg_val)
> +{
> +	u8 wr_data[3], rd_data[4] = {0};
> +	struct i2c_msg msg[2];
> +	int ret;
> +
> +	msg[0].addr = slv_addr;
> +	msg[0].len = 3;
> +	msg[0].flags = 0;
> +
> +	/* Big Endian for reg addr */
> +	wr_data[0] = (u8)(reg_addr >> 16);
> +	wr_data[1] = (u8)(reg_addr >> 8);
> +	wr_data[2] = (u8)reg_addr;
> +
> +	msg[0].buf = wr_data;
> +
> +	msg[1].addr = slv_addr;
> +	msg[1].len = 4;
> +	msg[1].flags = I2C_M_RD;
> +
> +	msg[1].buf = rd_data;
> +
> +	ret = i2c_transfer(ctx->adapter, &msg[0], 2);
> +	if (ret != 2)
> +		return ret;
> +
> +	*reg_val = (rd_data[3] << 24) | (rd_data[2] << 16) | (rd_data[1] << 8) | rd_data[0];
> +
> +	return 0;
> +}
> +
> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> +	struct device *dev = ctx->pwrctl.dev;
> +	struct qcom_qps615_pwrctl_i2c_setting *set;
> +	const struct firmware *fw;
> +	const u8 *pos, *eof;
> +	int ret;
> +	u32 val;
> +
> +	ret = request_firmware(&fw, "qcom/qps615.bin", dev);

Oh, what if any other board uses the same bridge? Do you expect that the
rootfs is device-specific?

> +	if (ret < 0) {
> +		dev_err(dev, "firmware loading failed with ret %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!fw) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	pos = fw->data;
> +	eof = fw->data + fw->size;
> +
> +	while (pos < (fw->data + fw->size)) {
> +		set = (struct qcom_qps615_pwrctl_i2c_setting *)pos;
> +
> +		ret = qps615_switch_i2c_write(ctx, set->slv_addr, set->reg_addr, set->val);

No! This makes no way for anybody to understand what is going on. Please
write a proper driver, that defines registers and writes sensible data
to those registers. Having a driver that writes data from the
configuration file to the random registers is a no-go.

> +		if (ret) {
> +			dev_err(dev,
> +				"I2c write failed for slv addr:%x at addr%x with val %x ret %d\n",
> +				set->slv_addr, set->reg_addr, set->val, ret);
> +			goto err;
> +		}
> +
> +		ret = qps615_switch_i2c_read(ctx,  set->slv_addr, set->reg_addr, &val);
> +		if (ret) {
> +			dev_err(dev, "I2c read failed for slv addr:%x at addr%x ret %d\n",
> +				set->slv_addr, set->reg_addr, ret);
> +			goto err;
> +		}
> +
> +		if (set->val != val) {
> +			dev_err(dev,
> +				"I2c read's mismatch for slv:%x at addr%x exp%d got%d\n",
> +				set->slv_addr, set->reg_addr, set->val, val);
> +			goto err;
> +		}
> +		pos += sizeof(struct qcom_qps615_pwrctl_i2c_setting);
> +	}
> +
> +err:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static int qcom_qps615_power_on(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(ctx->vdd);
> +	if (ret) {
> +		dev_err(ctx->pwrctl.dev, "cannot enable vdda regulator\n");
> +		return ret;
> +	}
> +
> +	ret = qcom_qps615_pwrctl_init(ctx);
> +	if (ret)
> +		regulator_disable(ctx->vdd);
> +
> +	return ret;
> +}
> +
> +static int qcom_qps615_power_off(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> +	return regulator_disable(ctx->vdd);
> +}
> +
> +static int qcom_qps615_pwrctl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct qcom_qps615_pwrctl_ctx *ctx;
> +	struct device_node *adapter_node;
> +	struct pci_host_bridge *bridge;
> +	struct i2c_adapter *adapter;
> +	struct pci_bus *bus;
> +
> +	bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> +	if (!bus)
> +		return -ENODEV;
> +
> +	bridge = pci_find_host_bridge(bus);
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	adapter_node = of_parse_phandle(node, "switch-i2c-cntrl", 0);
> +	if (adapter_node) {
> +		adapter = of_get_i2c_adapter_by_node(adapter_node);

Somebody didn't read the comment before of_get_i2c_adapter_by_node().

> +		__free(adapter_node);
> +		if (!adapter)
> +			return dev_err_probe(dev, -EPROBE_DEFER,
> +					     "failed to parse switch-i2c-cntrl\n");
> +	}
> +
> +	ctx->pwrctl.dev = dev;
> +	ctx->adapter = adapter;
> +	ctx->vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(ctx->vdd))
> +		return dev_err_probe(dev, PTR_ERR(ctx->vdd),
> +				     "failed to get vdd regulator\n");
> +
> +	ctx->link = device_link_add(&bridge->dev, dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> +	platform_set_drvdata(pdev, ctx);
> +
> +	bridge->ops->stop_link(bus);
> +	qcom_qps615_power_on(ctx);
> +	bridge->ops->start_link(bus);
> +
> +	return devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl);
> +}

I'd suggest turning this into a driver which uses components to bind the
PCIe power control part and the i2c clinet device. Then you can write
the i2c client addresses as ussual.

> +
> +static const struct of_device_id qcom_qps615_pwrctl_of_match[] = {
> +	{
> +		.compatible = "pci1179,0623",
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qps615_pwrctl_of_match);
> +
> +static int pci_pwrctl_suspend_noirq(struct device *dev)
> +{
> +	struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> +	struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> +	bridge->ops->stop_link(bus);
> +	qcom_qps615_power_off(ctx);
> +
> +	return 0;
> +}
> +
> +static int pci_pwrctl_resume_noirq(struct device *dev)
> +{
> +	struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> +	struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> +	qcom_qps615_power_on(ctx);
> +	bridge->ops->start_link(bus);
> +
> +	return 0;
> +}
> +
> +static void qcom_qps615_pwrctl_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> +	device_link_del(ctx->link);
> +	pci_pwrctl_suspend_noirq(dev);
> +}
> +
> +static const struct dev_pm_ops pci_pwrctl_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(pci_pwrctl_suspend_noirq, pci_pwrctl_resume_noirq)
> +};
> +
> +static struct platform_driver qcom_qps615_pwrctl_driver = {
> +	.driver = {
> +		.name = "pwrctl-qps615",
> +		.of_match_table = qcom_qps615_pwrctl_of_match,
> +		.pm = &pci_pwrctl_pm_ops,
> +	},
> +	.probe = qcom_qps615_pwrctl_probe,
> +	.remove_new = qcom_qps615_pwrctl_remove,
> +};
> +module_platform_driver(qcom_qps615_pwrctl_driver);
> +
> +MODULE_AUTHOR("Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.42.0
> 

-- 
With best wishes
Dmitry




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux