Michal Kazior <michal.kaz...@tieto.com> 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 <michal.kaz...@tieto.com>
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 ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel