Re: [PATCH 33/50] wifi: ath12k: add mhi.c

Jeff Johnson
2022-08-18T22:26:08Z

On 8/12/2022 9:09 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
>
> (Patches split into one patch per file for easier review, but the final
> commit will be one big patch. See the cover letter for more info.)
>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/mhi.c | 615 ++++++++++++++++++++++++++++++++++
> 1 file changed, 615 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c
> new file mode 100644
> index 000000000000..f77634994d97
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath12k/mhi.c
> @@ -0,0 +1,615 @@
> +// SPDX-License-Identifier: BSD-3-Clause-Clear
> +/*
> + * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/msi.h>
> +#include <linux/pci.h>
> +
> +#include "core.h"
> +#include "debug.h"
> +#include "mhi.h"
> +#include "pci.h"
> +
> +#define MHI_TIMEOUT_DEFAULT_MS 90000
> +
> +static struct mhi_channel_config ath12k_mhi_channels_qcn9274[] = {

should this be const?
in struct mhi_controller_config:
const struct mhi_channel_config *ch_cfg;

> + {
> + .num = 0,
> + .name = "LOOPBACK",
> + .num_elements = 32,
> + .event_ring = 1,
> + .dir = DMA_TO_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 1,
> + .name = "LOOPBACK",
> + .num_elements = 32,
> + .event_ring = 1,
> + .dir = DMA_FROM_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 20,
> + .name = "IPCR",
> + .num_elements = 32,
> + .event_ring = 1,
> + .dir = DMA_TO_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 21,
> + .name = "IPCR",
> + .num_elements = 32,
> + .event_ring = 1,
> + .dir = DMA_FROM_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = true,
> + },
> +};
> +
> +static struct mhi_event_config ath12k_mhi_events_qcn9274[] = {

seems this should be const
but for some reason struct mhi_controller_config has:
struct mhi_event_config *event_cfg;

(not const) so this can't be const :(

perhaps someone can propose a MHI interface change?
especially since internally to MHI in parse_ev_cfg() we have:
const struct mhi_event_config *event_cfg;
[...]
for (i = 0; i < num; i++) {
event_cfg = &config->event_cfg[i];

so it is treated as const

> + {
> + .num_elements = 32,
> + .irq_moderation_ms = 0,
> + .irq = 1,
> + .data_type = MHI_ER_CTRL,
> + .mode = MHI_DB_BRST_DISABLE,
> + .hardware_event = false,
> + .client_managed = false,
> + .offload_channel = false,
> + },
> + {
> + .num_elements = 256,
> + .irq_moderation_ms = 1,
> + .irq = 2,
> + .mode = MHI_DB_BRST_DISABLE,
> + .priority = 1,
> + .hardware_event = false,
> + .client_managed = false,
> + .offload_channel = false,
> + },
> +};
> +
> +struct mhi_controller_config ath12k_mhi_config_qcn9274 = {
> + .max_channels = 30,
> + .timeout_ms = 10000,
> + .use_bounce_buf = false,
> + .buf_len = 0,
> + .num_channels = ARRAY_SIZE(ath12k_mhi_channels_qcn9274),
> + .ch_cfg = ath12k_mhi_channels_qcn9274,
> + .num_events = ARRAY_SIZE(ath12k_mhi_events_qcn9274),
> + .event_cfg = ath12k_mhi_events_qcn9274,
> +};
> +
> +static struct mhi_channel_config ath12k_mhi_channels_wcn7850[] = {

const

> + {
> + .num = 0,
> + .name = "LOOPBACK",
> + .num_elements = 32,
> + .event_ring = 0,
> + .dir = DMA_TO_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 1,
> + .name = "LOOPBACK",
> + .num_elements = 32,
> + .event_ring = 0,
> + .dir = DMA_FROM_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 20,
> + .name = "IPCR",
> + .num_elements = 64,
> + .event_ring = 1,
> + .dir = DMA_TO_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = false,
> + },
> + {
> + .num = 21,
> + .name = "IPCR",
> + .num_elements = 64,
> + .event_ring = 1,
> + .dir = DMA_FROM_DEVICE,
> + .ee_mask = 0x4,
> + .pollcfg = 0,
> + .doorbell = MHI_DB_BRST_DISABLE,
> + .lpm_notify = false,
> + .offload_channel = false,
> + .doorbell_mode_switch = false,
> + .auto_queue = true,
> + },
> +};
> +
> +static struct mhi_event_config ath12k_mhi_events_wcn7850[] = {

keep not const (for now) :(

> + {
> + .num_elements = 32,
> + .irq_moderation_ms = 0,
> + .irq = 1,
> + .mode = MHI_DB_BRST_DISABLE,
> + .data_type = MHI_ER_CTRL,
> + .hardware_event = false,
> + .client_managed = false,
> + .offload_channel = false,
> + },
> + {
> + .num_elements = 256,
> + .irq_moderation_ms = 1,
> + .irq = 2,
> + .mode = MHI_DB_BRST_DISABLE,
> + .priority = 1,
> + .hardware_event = false,
> + .client_managed = false,
> + .offload_channel = false,
> + },
> +};
> +
> +struct mhi_controller_config ath12k_mhi_config_wcn7850 = {

and this one should be const since it is registered via:

int mhi_register_controller(struct mhi_controller *mhi_cntrl,
const struct mhi_controller_config *config);

> + .max_channels = 128,
> + .timeout_ms = 2000,
> + .use_bounce_buf = false,
> + .buf_len = 0,
> + .num_channels = ARRAY_SIZE(ath12k_mhi_channels_wcn7850),
> + .ch_cfg = ath12k_mhi_channels_wcn7850,
> + .num_events = ARRAY_SIZE(ath12k_mhi_events_wcn7850),
> + .event_cfg = ath12k_mhi_events_wcn7850,
> +};
> +

snip

> +static int ath12k_mhi_get_msi(struct ath12k_pci *ab_pci)
> +{
> + struct ath12k_base *ab = ab_pci->ab;
> + u32 user_base_data, base_vector;
> + int ret, num_vectors, i;
> + int *irq;
> +
> + ret = ath12k_pci_get_user_msi_assignment(ab,
> + "MHI", &num_vectors,
> + &user_base_data, &base_vector);
> + if (ret)
> + return ret;
> +
> + ath12k_dbg(ab, ATH12K_DBG_PCI, "Number of assigned MSI for MHI is %d, base vector is %d\n",
> + num_vectors, base_vector);
> +
> + irq = kcalloc(num_vectors, sizeof(int), GFP_KERNEL);

prefer sizeof(*irq)?

> + if (!irq)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_vectors; i++)
> + irq[i] = ath12k_pci_get_msi_irq(ab->dev,
> + base_vector + i);
> +
> + ab_pci->mhi_ctrl->irq = irq;
> + ab_pci->mhi_ctrl->nr_irqs = num_vectors;
> +
> + return 0;
> +}
> +

snip

> +void ath12k_mhi_unregister(struct ath12k_pci *ab_pci)
> +{
> + struct mhi_controller *mhi_ctrl = ab_pci->mhi_ctrl;
> +
> + mhi_unregister_controller(mhi_ctrl);
> + kfree(mhi_ctrl->irq);
> + mhi_free_controller(mhi_ctrl);

consider setting ab_pci->mhi_ctrl = NULL to avoid dangling pointer?

> +}

rest snipped