[ath9k-devel] [PATCH v2 01/10] ath10k: decouple pci init/deinit logic

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@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.kazior@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