| Message ID | 1592458104-2961-3-git-send-email-akolli@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath11k: Add IPQ6018 support | expand |
Hi Anilkumar, On Thu, Jun 18, 2020 at 3:31 PM Anilkumar Kolli <akolli@codeaurora.org> wrote: > > No functional changes, added target ce service configurations > to hw_params. > > Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org> > --- > V3: > - added ce svc configs in hw_params > > drivers/net/wireless/ath/ath11k/ahb.c | 20 +++++++++++++++----- > drivers/net/wireless/ath/ath11k/core.c | 8 +------- > drivers/net/wireless/ath/ath11k/core.h | 1 + > drivers/net/wireless/ath/ath11k/hw.h | 2 ++ > 4 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c > index 7e9bfeaaf4d2..aa74d27e5871 100644 > --- a/drivers/net/wireless/ath/ath11k/ahb.c > +++ b/drivers/net/wireless/ath/ath11k/ahb.c > @@ -152,7 +152,7 @@ static const struct ce_pipe_config target_ce_config_wlan[] = { > * This table is derived from the CE_PCI TABLE, above. > * It is passed to the Target at startup for use by firmware. > */ > -static const struct service_to_pipe target_service_to_ce_map_wlan[] = { > +static const struct service_to_pipe target_service_to_ce_map_wlan_ipq8074[] = { > { > .service_id = __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO), > .pipedir = __cpu_to_le32(PIPEDIR_OUT), /* out = UL = host -> target */ > @@ -644,8 +644,8 @@ static void ath11k_ahb_init_qmi_ce_config(struct ath11k_base *ab) > > cfg->tgt_ce_len = ARRAY_SIZE(target_ce_config_wlan) - 1; > cfg->tgt_ce = target_ce_config_wlan; > - cfg->svc_to_ce_map_len = ARRAY_SIZE(target_service_to_ce_map_wlan); > - cfg->svc_to_ce_map = target_service_to_ce_map_wlan; > + cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len; > + cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map; > } > > static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab) > @@ -853,8 +853,8 @@ static int ath11k_ahb_map_service_to_pipe(struct ath11k_base *ab, u16 service_id > bool ul_set = false, dl_set = false; > int i; > > - for (i = 0; i < ARRAY_SIZE(target_service_to_ce_map_wlan); i++) { > - entry = &target_service_to_ce_map_wlan[i]; > + for (i = 0; i < ab->hw_params.svc_to_ce_map_len; i++) { > + entry = &ab->hw_params.svc_to_ce_map[i]; > > if (__le32_to_cpu(entry->service_id) != service_id) > continue; > @@ -950,6 +950,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev) > goto err_hal_srng_deinit; > } > > + ret = ath11k_init_hw_params(ab); > + if (ret) { > + ath11k_err(ab, "failed to get hw params %d\n", ret); > + return ret; > + } > + > + ab->hw_params.svc_to_ce_map_len = > + ARRAY_SIZE(target_service_to_ce_map_wlan_ipq8074); > + ab->hw_params.svc_to_ce_map = target_service_to_ce_map_wlan_ipq8074; I think you misunderstood my point about this, the point wasn't to copy the svc map to hw_params, but define it in hw_params: + { + .hw_rev = ATH11K_HW_IPQ6018, + .name = "ipq6018 hw1.0", + .fw = { + .dir = "IPQ6018/hw1.0", + .board_size = 256 * 1024, + .cal_size = 256 * 1024, + }, + .max_radios = 2, + .bdf_addr = 0x4ABC0000, + .hw_ops = &ipq6018_ops, + .svc_to_ce_map_len = ARRAY_SIZE(target_service_to_ce_map_wlan_ipq6018, + .svc_to_ce_map = target_service_to_ce_map_wlan_ipq6018, + }, That completely eliminates special case code based on the hardware ID in the driver. Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
On 2020-06-19 04:40, Julian Calaby wrote: > Hi Anilkumar, > > On Thu, Jun 18, 2020 at 3:31 PM Anilkumar Kolli <akolli@codeaurora.org> > wrote: >> >> No functional changes, added target ce service configurations >> to hw_params. >> >> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org> >> --- >> V3: >> - added ce svc configs in hw_params >> >> drivers/net/wireless/ath/ath11k/ahb.c | 20 +++++++++++++++----- >> drivers/net/wireless/ath/ath11k/core.c | 8 +------- >> drivers/net/wireless/ath/ath11k/core.h | 1 + >> drivers/net/wireless/ath/ath11k/hw.h | 2 ++ >> 4 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c >> b/drivers/net/wireless/ath/ath11k/ahb.c >> index 7e9bfeaaf4d2..aa74d27e5871 100644 >> --- a/drivers/net/wireless/ath/ath11k/ahb.c >> +++ b/drivers/net/wireless/ath/ath11k/ahb.c >> @@ -152,7 +152,7 @@ static const struct ce_pipe_config >> target_ce_config_wlan[] = { >> * This table is derived from the CE_PCI TABLE, above. >> * It is passed to the Target at startup for use by firmware. >> */ >> -static const struct service_to_pipe target_service_to_ce_map_wlan[] = >> { >> +static const struct service_to_pipe >> target_service_to_ce_map_wlan_ipq8074[] = { >> { >> .service_id = >> __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO), >> .pipedir = __cpu_to_le32(PIPEDIR_OUT), /* out = UL = >> host -> target */ >> @@ -644,8 +644,8 @@ static void ath11k_ahb_init_qmi_ce_config(struct >> ath11k_base *ab) >> >> cfg->tgt_ce_len = ARRAY_SIZE(target_ce_config_wlan) - 1; >> cfg->tgt_ce = target_ce_config_wlan; >> - cfg->svc_to_ce_map_len = >> ARRAY_SIZE(target_service_to_ce_map_wlan); >> - cfg->svc_to_ce_map = target_service_to_ce_map_wlan; >> + cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len; >> + cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map; >> } >> >> static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab) >> @@ -853,8 +853,8 @@ static int ath11k_ahb_map_service_to_pipe(struct >> ath11k_base *ab, u16 service_id >> bool ul_set = false, dl_set = false; >> int i; >> >> - for (i = 0; i < ARRAY_SIZE(target_service_to_ce_map_wlan); >> i++) { >> - entry = &target_service_to_ce_map_wlan[i]; >> + for (i = 0; i < ab->hw_params.svc_to_ce_map_len; i++) { >> + entry = &ab->hw_params.svc_to_ce_map[i]; >> >> if (__le32_to_cpu(entry->service_id) != service_id) >> continue; >> @@ -950,6 +950,16 @@ static int ath11k_ahb_probe(struct >> platform_device *pdev) >> goto err_hal_srng_deinit; >> } >> >> + ret = ath11k_init_hw_params(ab); >> + if (ret) { >> + ath11k_err(ab, "failed to get hw params %d\n", ret); >> + return ret; >> + } >> + >> + ab->hw_params.svc_to_ce_map_len = >> + >> ARRAY_SIZE(target_service_to_ce_map_wlan_ipq8074); >> + ab->hw_params.svc_to_ce_map = >> target_service_to_ce_map_wlan_ipq8074; > > I think you misunderstood my point about this, the point wasn't to > copy the svc map to hw_params, but define it in hw_params: > > + { > + .hw_rev = ATH11K_HW_IPQ6018, > + .name = "ipq6018 hw1.0", > + .fw = { > + .dir = "IPQ6018/hw1.0", > + .board_size = 256 * 1024, > + .cal_size = 256 * 1024, > + }, > + .max_radios = 2, > + .bdf_addr = 0x4ABC0000, > + .hw_ops = &ipq6018_ops, > + .svc_to_ce_map_len = > ARRAY_SIZE(target_service_to_ce_map_wlan_ipq6018, > + .svc_to_ce_map = target_service_to_ce_map_wlan_ipq6018, > + }, > > That completely eliminates special case code based on the hardware ID > in the driver. > The static array of structures target_service_to_ce_map_wlan_ipq6018[] is defined in ahb.c and hw_params are initialised in core.c, this will not work. no? Thanks Anil
Hi Anilkumar, On Sat, Jun 20, 2020 at 2:24 AM <akolli@codeaurora.org> wrote: > > On 2020-06-19 04:40, Julian Calaby wrote: > > Hi Anilkumar, > > > > On Thu, Jun 18, 2020 at 3:31 PM Anilkumar Kolli <akolli@codeaurora.org> > > wrote: > >> > >> No functional changes, added target ce service configurations > >> to hw_params. > >> > >> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org> > >> --- > >> V3: > >> - added ce svc configs in hw_params > >> > >> drivers/net/wireless/ath/ath11k/ahb.c | 20 +++++++++++++++----- > >> drivers/net/wireless/ath/ath11k/core.c | 8 +------- > >> drivers/net/wireless/ath/ath11k/core.h | 1 + > >> drivers/net/wireless/ath/ath11k/hw.h | 2 ++ > >> 4 files changed, 19 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c > >> b/drivers/net/wireless/ath/ath11k/ahb.c > >> index 7e9bfeaaf4d2..aa74d27e5871 100644 > >> --- a/drivers/net/wireless/ath/ath11k/ahb.c > >> +++ b/drivers/net/wireless/ath/ath11k/ahb.c > >> @@ -152,7 +152,7 @@ static const struct ce_pipe_config > >> target_ce_config_wlan[] = { > >> * This table is derived from the CE_PCI TABLE, above. > >> * It is passed to the Target at startup for use by firmware. > >> */ > >> -static const struct service_to_pipe target_service_to_ce_map_wlan[] = > >> { > >> +static const struct service_to_pipe > >> target_service_to_ce_map_wlan_ipq8074[] = { > >> { > >> .service_id = > >> __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO), > >> .pipedir = __cpu_to_le32(PIPEDIR_OUT), /* out = UL = > >> host -> target */ > >> @@ -644,8 +644,8 @@ static void ath11k_ahb_init_qmi_ce_config(struct > >> ath11k_base *ab) > >> > >> cfg->tgt_ce_len = ARRAY_SIZE(target_ce_config_wlan) - 1; > >> cfg->tgt_ce = target_ce_config_wlan; > >> - cfg->svc_to_ce_map_len = > >> ARRAY_SIZE(target_service_to_ce_map_wlan); > >> - cfg->svc_to_ce_map = target_service_to_ce_map_wlan; > >> + cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len; > >> + cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map; > >> } > >> > >> static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab) > >> @@ -853,8 +853,8 @@ static int ath11k_ahb_map_service_to_pipe(struct > >> ath11k_base *ab, u16 service_id > >> bool ul_set = false, dl_set = false; > >> int i; > >> > >> - for (i = 0; i < ARRAY_SIZE(target_service_to_ce_map_wlan); > >> i++) { > >> - entry = &target_service_to_ce_map_wlan[i]; > >> + for (i = 0; i < ab->hw_params.svc_to_ce_map_len; i++) { > >> + entry = &ab->hw_params.svc_to_ce_map[i]; > >> > >> if (__le32_to_cpu(entry->service_id) != service_id) > >> continue; > >> @@ -950,6 +950,16 @@ static int ath11k_ahb_probe(struct > >> platform_device *pdev) > >> goto err_hal_srng_deinit; > >> } > >> > >> + ret = ath11k_init_hw_params(ab); > >> + if (ret) { > >> + ath11k_err(ab, "failed to get hw params %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ab->hw_params.svc_to_ce_map_len = > >> + > >> ARRAY_SIZE(target_service_to_ce_map_wlan_ipq8074); > >> + ab->hw_params.svc_to_ce_map = > >> target_service_to_ce_map_wlan_ipq8074; > > > > I think you misunderstood my point about this, the point wasn't to > > copy the svc map to hw_params, but define it in hw_params: > > > > + { > > + .hw_rev = ATH11K_HW_IPQ6018, > > + .name = "ipq6018 hw1.0", > > + .fw = { > > + .dir = "IPQ6018/hw1.0", > > + .board_size = 256 * 1024, > > + .cal_size = 256 * 1024, > > + }, > > + .max_radios = 2, > > + .bdf_addr = 0x4ABC0000, > > + .hw_ops = &ipq6018_ops, > > + .svc_to_ce_map_len = > > ARRAY_SIZE(target_service_to_ce_map_wlan_ipq6018, > > + .svc_to_ce_map = target_service_to_ce_map_wlan_ipq6018, > > + }, > > > > That completely eliminates special case code based on the hardware ID > > in the driver. > > > The static array of structures target_service_to_ce_map_wlan_ipq6018[] > is defined in ahb.c and hw_params are initialised in core.c, this will > not work. no? My understanding, and please correct me if I'm wrong, is that the service map is a property of the hardware. So why not define all the hardware properties in one place? You could either move the hardware definitions and service maps to a new file (maybe ahb_conf.c), move those maps to ahb.c or just add them to some header file so ahb.c can reference them. Thanks,
akolli@codeaurora.org writes: > On 2020-06-19 04:40, Julian Calaby wrote: >> >> On Thu, Jun 18, 2020 at 3:31 PM Anilkumar Kolli >> <akolli@codeaurora.org> wrote: >>> >>> @@ -950,6 +950,16 @@ static int ath11k_ahb_probe(struct >>> platform_device *pdev) >>> goto err_hal_srng_deinit; >>> } >>> >>> + ret = ath11k_init_hw_params(ab); >>> + if (ret) { >>> + ath11k_err(ab, "failed to get hw params %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ab->hw_params.svc_to_ce_map_len = >>> + >>> ARRAY_SIZE(target_service_to_ce_map_wlan_ipq8074); >>> + ab->hw_params.svc_to_ce_map = >>> target_service_to_ce_map_wlan_ipq8074; >> >> I think you misunderstood my point about this, the point wasn't to >> copy the svc map to hw_params, but define it in hw_params: >> >> + { >> + .hw_rev = ATH11K_HW_IPQ6018, >> + .name = "ipq6018 hw1.0", >> + .fw = { >> + .dir = "IPQ6018/hw1.0", >> + .board_size = 256 * 1024, >> + .cal_size = 256 * 1024, >> + }, >> + .max_radios = 2, >> + .bdf_addr = 0x4ABC0000, >> + .hw_ops = &ipq6018_ops, >> + .svc_to_ce_map_len = >> ARRAY_SIZE(target_service_to_ce_map_wlan_ipq6018, >> + .svc_to_ce_map = target_service_to_ce_map_wlan_ipq6018, >> + }, >> >> That completely eliminates special case code based on the hardware ID >> in the driver. >> > The static array of structures target_service_to_ce_map_wlan_ipq6018[] > is defined in ahb.c and hw_params are initialised in core.c, this will > not work. no? You could move the map arrays to hw.c, as an example see how ath11k_hw_ring_mask_ipq8074 is implemented. That way we don't need any new hw_rev checks.
diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c index 7e9bfeaaf4d2..aa74d27e5871 100644 --- a/drivers/net/wireless/ath/ath11k/ahb.c +++ b/drivers/net/wireless/ath/ath11k/ahb.c @@ -152,7 +152,7 @@ static const struct ce_pipe_config target_ce_config_wlan[] = { * This table is derived from the CE_PCI TABLE, above. * It is passed to the Target at startup for use by firmware. */ -static const struct service_to_pipe target_service_to_ce_map_wlan[] = { +static const struct service_to_pipe target_service_to_ce_map_wlan_ipq8074[] = { { .service_id = __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO), .pipedir = __cpu_to_le32(PIPEDIR_OUT), /* out = UL = host -> target */ @@ -644,8 +644,8 @@ static void ath11k_ahb_init_qmi_ce_config(struct ath11k_base *ab) cfg->tgt_ce_len = ARRAY_SIZE(target_ce_config_wlan) - 1; cfg->tgt_ce = target_ce_config_wlan; - cfg->svc_to_ce_map_len = ARRAY_SIZE(target_service_to_ce_map_wlan); - cfg->svc_to_ce_map = target_service_to_ce_map_wlan; + cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len; + cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map; } static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab) @@ -853,8 +853,8 @@ static int ath11k_ahb_map_service_to_pipe(struct ath11k_base *ab, u16 service_id bool ul_set = false, dl_set = false; int i; - for (i = 0; i < ARRAY_SIZE(target_service_to_ce_map_wlan); i++) { - entry = &target_service_to_ce_map_wlan[i]; + for (i = 0; i < ab->hw_params.svc_to_ce_map_len; i++) { + entry = &ab->hw_params.svc_to_ce_map[i]; if (__le32_to_cpu(entry->service_id) != service_id) continue; @@ -950,6 +950,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev) goto err_hal_srng_deinit; } + ret = ath11k_init_hw_params(ab); + if (ret) { + ath11k_err(ab, "failed to get hw params %d\n", ret); + return ret; + } + + ab->hw_params.svc_to_ce_map_len = + ARRAY_SIZE(target_service_to_ce_map_wlan_ipq8074); + ab->hw_params.svc_to_ce_map = target_service_to_ce_map_wlan_ipq8074; + ath11k_ahb_init_qmi_ce_config(ab); ret = ath11k_core_init(ab); diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c index 38e830a2395b..0bf8bb1674dc 100644 --- a/drivers/net/wireless/ath/ath11k/core.c +++ b/drivers/net/wireless/ath/ath11k/core.c @@ -703,7 +703,7 @@ static void ath11k_core_restart(struct work_struct *work) complete(&ab->driver_recovery); } -static int ath11k_init_hw_params(struct ath11k_base *ab) +int ath11k_init_hw_params(struct ath11k_base *ab) { const struct ath11k_hw_params *hw_params = NULL; int i; @@ -746,12 +746,6 @@ int ath11k_core_init(struct ath11k_base *ab) } ab->tgt_rproc = prproc; - ret = ath11k_init_hw_params(ab); - if (ret) { - ath11k_err(ab, "failed to get hw params %d\n", ret); - return ret; - } - ret = ath11k_core_soc_create(ab); if (ret) { ath11k_err(ab, "failed to create soc core: %d\n", ret); diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index b6ccd9f93853..98b994984c25 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -852,6 +852,7 @@ struct ath11k_peer *ath11k_peer_find_by_id(struct ath11k_base *ab, int peer_id); int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab); int ath11k_core_init(struct ath11k_base *ath11k); void ath11k_core_deinit(struct ath11k_base *ath11k); +int ath11k_init_hw_params(struct ath11k_base *ab); struct ath11k_base *ath11k_core_alloc(struct device *dev, size_t priv_size, enum ath11k_bus bus); void ath11k_core_free(struct ath11k_base *ath11k); diff --git a/drivers/net/wireless/ath/ath11k/hw.h b/drivers/net/wireless/ath/ath11k/hw.h index f31d53f6adb8..388cfb42ef93 100644 --- a/drivers/net/wireless/ath/ath11k/hw.h +++ b/drivers/net/wireless/ath/ath11k/hw.h @@ -116,6 +116,8 @@ struct ath11k_hw_params { } fw; const struct ath11k_hw_ops *hw_ops; + const struct service_to_pipe *svc_to_ce_map; + int svc_to_ce_map_len; }; extern const struct ath11k_hw_ops ipq8074_ops;
No functional changes, added target ce service configurations to hw_params. Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org> --- V3: - added ce svc configs in hw_params drivers/net/wireless/ath/ath11k/ahb.c | 20 +++++++++++++++----- drivers/net/wireless/ath/ath11k/core.c | 8 +------- drivers/net/wireless/ath/ath11k/core.h | 1 + drivers/net/wireless/ath/ath11k/hw.h | 2 ++ 4 files changed, 19 insertions(+), 12 deletions(-)