| 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
| ||
|
Message-ID: <pyxerd3lirbh2p43m74ohwocjjb7uh56xxmaxbrkay3svossik@ksd3yojw5wgr>
Date: Sat, 5 Aug 2023 22:11:52 -0700
From: Bjorn Andersson <andersson@...nel.org>
To: Krishna Kurapati <quic_kriskura@...cinc.com>
Cc: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Andy Gross <agross@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
Johan Hovold <johan@...nel.org>,
Mathias Nyman <mathias.nyman@...el.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
quic_pkondeti@...cinc.com, quic_ppratap@...cinc.com,
quic_jackp@...cinc.com, ahalaney@...hat.com,
quic_shazhuss@...cinc.com
Subject: Re: [PATCH v10 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM
Glue driver
On Fri, Jul 28, 2023 at 04:03:02AM +0530, Krishna Kurapati wrote:
> Refactor setup_irq call to facilitate reading multiport IRQ's along
> with non mulitport ones. For SA8295, there are 4-DP/4-DM and 2-SS
> IRQ's. Check whether device is multiport capable or not and read all
> interrupts for DP/DM/SS on each port accordingly.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 277 ++++++++++++++++++++++++-----------
> 1 file changed, 190 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 3de43df6bbe8..ad89ded116d3 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -64,33 +64,61 @@ struct dwc3_acpi_pdata {
> bool is_urs;
> };
>
> +struct dwc3_qcom_of_match_data {
> + u8 num_ports;
> +};
> +
> struct dwc3_qcom {
> - struct device *dev;
> - void __iomem *qscratch_base;
> - struct platform_device *dwc3;
> - struct platform_device *urs_usb;
> - struct clk **clks;
> - int num_clocks;
> - struct reset_control *resets;
> -
> - int hs_phy_irq;
> - int dp_hs_phy_irq;
> - int dm_hs_phy_irq;
> - int ss_phy_irq;
> - enum usb_device_speed usb2_speed;
> -
> - struct extcon_dev *edev;
> - struct extcon_dev *host_edev;
> - struct notifier_block vbus_nb;
> - struct notifier_block host_nb;
> + struct device *dev;
> + void __iomem *qscratch_base;
> + struct platform_device *dwc3;
> + struct platform_device *urs_usb;
> + struct clk **clks;
> + int num_clocks;
> + struct reset_control *resets;
> +
> + int hs_phy_irq;
> + int dp_hs_phy_irq[DWC3_MAX_PORTS];
> + int dm_hs_phy_irq[DWC3_MAX_PORTS];
> + int ss_phy_irq[DWC3_MAX_PORTS];
> + enum usb_device_speed usb2_speed;
> +
> + struct extcon_dev *edev;
> + struct extcon_dev *host_edev;
> + struct notifier_block vbus_nb;
> + struct notifier_block host_nb;
> +
> + const struct dwc3_acpi_pdata *acpi_pdata;
> +
> + enum usb_dr_mode mode;
> + bool is_suspended;
> + bool pm_suspended;
> + struct icc_path *icc_path_ddr;
> + struct icc_path *icc_path_apps;
> + const struct dwc3_qcom_of_match_data *data;
Please don't adjust indentation of unrelated code, it makes it hard to
see what actually changed.
> +};
> +
> +static const struct dwc3_qcom_of_match_data qcom_dwc3 = {
> + .num_ports = 1,
> +};
>
> - const struct dwc3_acpi_pdata *acpi_pdata;
> +static const struct dwc3_qcom_of_match_data sx8280xp_qcom_dwc3 = {
> + .num_ports = 4,
> +};
>
> - enum usb_dr_mode mode;
> - bool is_suspended;
> - bool pm_suspended;
> - struct icc_path *icc_path_ddr;
> - struct icc_path *icc_path_apps;
> +/*
> + * Driver needs to read HS/DP_HS/DM_HS/SS IRQ's. Currently, for
> + * SA8295 which supports mutliport, thre are 4 DP/ 4 DM/ 2 SS IRQ's
> + * and 1 HS IRQ present. So avoid trying to read HS_PHY_IRQ for 4
> + * ports of SA8295.
> + */
The last part here is relevant information, but it doesn't seem to
relate to this define.
Also, does all platforms have this configuration of interrupts?
> +#define MAX_PHY_IRQ 4
> +
> +enum dwc3_qcom_phy_irq_identifier {
> + HS_PHY_IRQ = 0,
> + DP_HS_PHY_IRQ,
> + DM_HS_PHY_IRQ,
> + SS_PHY_IRQ,
> };
This enum is unused.
>
[..]
> +static int dwc3_get_acpi_index(const struct dwc3_acpi_pdata *pdata, int irq_index)
> +{
> + int acpi_index = -1;
> +
> + if (!pdata)
> + return -1;
> +
> + if (irq_index == DP_HS_PHY_IRQ)
> + acpi_index = pdata->dp_hs_phy_irq_index;
> + else if (irq_index == DM_HS_PHY_IRQ)
> + acpi_index = pdata->dm_hs_phy_irq_index;
> + else if (irq_index == SS_PHY_IRQ)
> + acpi_index = pdata->ss_phy_irq_index;
It looks favourable to put these in an array, instead of having to pull
them out of 4 different variables conditionally.
> +
> + return acpi_index;
> +}
> +
> +static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index)
> +{
> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> + bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false;
> + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
> + char *disp_name;
> + int acpi_index;
> + char *dt_name;
> + int ret;
> + int irq;
> + int i;
> +
> + /*
> + * We need to read only DP/DM/SS IRQ's here.
> + * So loop over from 1->3 and accordingly modify respective phy_irq[].
> + */
> + for (i = 1; i < MAX_PHY_IRQ; i++) {
> +
> + if (!is_mp_supported && (port_index == 0)) {
> + if (i == DP_HS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "dp_hs_phy_irq");
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 DP_HS");
> + } else if (i == DM_HS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "dm_hs_phy_irq");
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 DM_HS");
> + } else if (i == SS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "ss_phy_irq");
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 SS");
> + }
> + } else {
> + if (i == DP_HS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "dp_hs_phy_%d", port_index + 1);
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 DP_HS%d", port_index + 1);
> + } else if (i == DM_HS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "dm_hs_phy_%d", port_index + 1);
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 DM_HS%d", port_index + 1);
> + } else if (i == SS_PHY_IRQ) {
> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "ss_phy_%d", port_index + 1);
> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "qcom_dwc3 SS%d", port_index + 1);
> + }
There is too much repetition in this for my liking.
> }
> - qcom->hs_phy_irq = irq;
> - }
>
> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
> - pdata ? pdata->dp_hs_phy_irq_index : -1);
> - if (irq > 0) {
> - irq_set_status_flags(irq, IRQ_NOAUTOEN);
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> - "qcom_dwc3 DP_HS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
> - return ret;
> + if (!dt_name || !disp_name)
> + return -ENOMEM;
> +
> + acpi_index = !is_mp_supported ? dwc3_get_acpi_index(pdata, i) : -1;
> +
> + irq = dwc3_qcom_get_irq(pdev, dt_name, acpi_index);
> + if (irq > 0) {
> + ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq);
> + if (ret)
> + return ret;
> +
> + if (i == DP_HS_PHY_IRQ)
> + qcom->dp_hs_phy_irq[port_index] = irq;
> + else if (i == DM_HS_PHY_IRQ)
> + qcom->dm_hs_phy_irq[port_index] = irq;
> + else if (i == SS_PHY_IRQ)
> + qcom->ss_phy_irq[port_index] = irq;
> }
> - qcom->dp_hs_phy_irq = irq;
> }
>
> - irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
> - pdata ? pdata->dm_hs_phy_irq_index : -1);
> + return 0;
> +}
> +
> +static int dwc3_qcom_setup_irq(struct platform_device *pdev)
> +{
> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
> + int irq;
> + int ret;
> + int i;
> +
> + irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
> + pdata ? pdata->hs_phy_irq_index : -1);
> if (irq > 0) {
> - irq_set_status_flags(irq, IRQ_NOAUTOEN);
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> - "qcom_dwc3 DM_HS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
> + ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS",irq);
> + if (ret)
It would be nice to have this refactored out in a separate commit.
> return ret;
> - }
> - qcom->dm_hs_phy_irq = irq;
> + qcom->hs_phy_irq = irq;
> }
>
> - irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
> - pdata ? pdata->ss_phy_irq_index : -1);
> - if (irq > 0) {
> - irq_set_status_flags(irq, IRQ_NOAUTOEN);
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> - "qcom_dwc3 SS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);
> + for (i = 0; i < qcom->data->num_ports; i++) {
> + ret = dwc3_get_port_irq(pdev, i);
> + if (ret)
> return ret;
> - }
> - qcom->ss_phy_irq = irq;
> }
>
> return 0;
> @@ -811,6 +905,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, qcom);
> qcom->dev = &pdev->dev;
>
> + qcom->data = of_device_get_match_data(qcom->dev);
> +
> if (has_acpi_companion(dev)) {
> qcom->acpi_pdata = acpi_device_get_match_data(dev);
> if (!qcom->acpi_pdata) {
> @@ -1023,8 +1119,15 @@ static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
> };
>
> static const struct of_device_id dwc3_qcom_of_match[] = {
> - { .compatible = "qcom,dwc3" },
> - { }
> + {
> + .compatible = "qcom,dwc3",
> + .data = &qcom_dwc3,
> + },
> + {
> + .compatible = "qcom,sc8280xp-dwc3-mp",
> + .data = &sx8280xp_qcom_dwc3,
> + },
I would prefer that we don't add a separate compatible, but rather just
try to parse the interrupts for multiport and fall back to single port.
If/when we figure out how to peak into the dwc3 core, we could
potentially introduce a check to aid the developer.
Regards,
Bjorn
> + { },
> };
> MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
>
> --
> 2.40.0
>
Powered by blists - more mailing lists