lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMRc=MdFj+jmhae1=SzeSXJzYJ+13PFtyn14=EotCcY_0EEu_w@mail.gmail.com>
Date: Wed, 26 Jun 2024 17:25:12 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Krishna chaitanya chundru <quic_krichai@...cinc.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Jingoo Han <jingoohan1@...il.com>, quic_vbadigan@...cinc.com, quic_skananth@...cinc.com, 
	quic_nitegupt@...cinc.com, linux-arm-msm@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615

On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru
<quic_krichai@...cinc.com> 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@...cinc.com>
> ---
>  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.

2024?

> + */
> +
> +#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,

Break the line after "*ctx" unless you have a good reason for long lines?

> +                                  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);
> +       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);
> +               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");

Use dev_err_probe() here and wherever applicable.

> +               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);
> +               __free(adapter_node);

How did this even compile??! I think you meant:

struct device_node *adapter_node __free(device_node) = ... ?

Bart

> +               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);
> +}
> +
> +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@...cinc.com>");
> +MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ