Michal Kazior <[email protected]> writes:
> Split logic that prepares the device for BMI
> phase/cleans up related resources.
>
> This is necessary for ath10k to be able to restart
> hw on the fly without reloading the module.
>
> Signed-off-by: Michal Kazior <[email protected]>
Few comments:
> --- a/drivers/net/wireless/ath/ath10k/hif.h
> +++ b/drivers/net/wireless/ath/ath10k/hif.h
> @@ -46,8 +46,11 @@ struct ath10k_hif_ops {
> void *request, u32 request_len,
> void *response, u32 *response_len);
>
> + /* Post BMI phase, after FW is loaded. Starts regular operation */
> int (*start)(struct ath10k *ar);
>
> + /* Clean up what start() did. This does not revert to BMI phase. If
> + * desired so, call deinit() and init() */
> void (*stop)(struct ath10k *ar);
>
> int (*map_service_to_pipe)(struct ath10k *ar, u16 service_id,
> @@ -70,6 +73,13 @@ struct ath10k_hif_ops {
> struct ath10k_hif_cb *callbacks);
>
> u16 (*get_free_queue_number)(struct ath10k *ar, u8 pipe_id);
> +
> + /* Power up the device and enter BMI transfer mode for FW download */
> + int (*init)(struct ath10k *ar);
> +
> + /* Power down the device and free up resources. stop() must be called
> + * before this if start() was called earlier */
> + void (*deinit)(struct ath10k *ar);
I think terminology is mixed here as well. To me init() does here a lot
more than other init() functions in ath10k. Should we rename these to
power_up() and power_down(), as how your documentation already uses
those terms?
So when booting the firwmware we call:
hif_power_up()
hif_start()
and when we want to kill the firmware we do:
hif_stop()
hif_power_down()
[...]
> +static int ath10k_pci_hif_init(struct ath10k *ar)
> +{
> + int ret;
> +
> + /*
> + * Bring the target up cleanly.
> + *
> + * The target may be in an undefined state with an AUX-powered Target
> + * and a Host in WoW mode. If the Host crashes, loses power, or is
> + * restarted (without unloading the driver) then the Target is left
> + * (aux) powered and running. On a subsequent driver load, the Target
> + * is in an unexpected state. We try to catch that here in order to
> + * reset the Target and retry the probe.
> + */
> + ath10k_pci_device_reset(ar);
> +
> + ret = ath10k_pci_reset_target(ar);
> + if (ret)
> + goto err;
> +
> + if (ath10k_target_ps) {
> + ath10k_dbg(ATH10K_DBG_PCI, "on-chip power save enabled\n");
> + } else {
> + /* Force AWAKE forever */
> + ath10k_dbg(ATH10K_DBG_PCI, "on-chip power save disabled\n");
> + ath10k_do_pci_wake(ar);
> + }
> +
> + ret = ath10k_pci_ce_init(ar);
> + if (ret)
> + goto err_ps;
I noticed ath10k_pci_ce_init() also allocs host memory etc. If possible,
in the future we might want to refactor the function into two:
ath10k_pci_ce_init() and ath10k_pci_ce_start(). And the former would be
called only from ath10k_pci_probe(). That way we would not need to do
any memory allocation during start time.
But no need to refactor it right now, this is good enough for the first
implementation.
--
Kalle Valo
_______________________________________________
ath9k-devel mailing list
[email protected]
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
