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

2013-06-14 Thread Michal Kazior 
On 13/06/13 20:09, Kalle Valo wrote:
 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()

 [...]

You're right. I'll fix that.


 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.

There's also some an asymmetry here.

We should be enabling/disabling irqs when we start/stop, shouldn't we? 
But then again we need interrupts _before_ start() for BMI phase, while 
it's a also a good idea to disable them upon stop(). We do stop CE 
interrupts by poking up device registers, but we don't unregister irq 
handlers. There might be a race in there that we're not aware 
of/observed yet.

I'm thinking of having only 3 functions: start_bmi_phase, 
start_htc_phase, stop (that would handle either state transition). But 
that would require the aforementioned proper alloc/init/setup separation 
to be done first.


-- Pozdrawiam / Best regards, Michal Kazior.
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

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

2013-06-14 Thread Kalle Valo 
Michal Kazior michal.kaz...@tieto.com writes:

 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.

 There's also some an asymmetry here.

 We should be enabling/disabling irqs when we start/stop, shouldn't we?
 But then again we need interrupts _before_ start() for BMI phase,
 while it's a also a good idea to disable them upon stop(). We do stop
 CE interrupts by poking up device registers, but we don't unregister
 irq handlers. There might be a race in there that we're not aware
 of/observed yet.

Hehe, our emails crossed. I was thinking exactly the same issue in my
previous email. So I agree, disabling interrupts from host would be good
to have. But that's again for the future.

 I'm thinking of having only 3 functions: start_bmi_phase,
 start_htc_phase, stop (that would handle either state transition). But
 that would require the aforementioned proper alloc/init/setup
 separation to be done first.

Yeah, we can discuss about this later.

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

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

2013-06-13 Thread Kalle Valo 
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

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

2013-06-12 Thread Michal Kazior 
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
---
 drivers/net/wireless/ath/ath10k/hif.h |   20 ++
 drivers/net/wireless/ath/ath10k/pci.c |  110 +
 2 files changed, 91 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/hif.h 
b/drivers/net/wireless/ath/ath10k/hif.h
index 010e265..3a257d1 100644
--- 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);
 };
 
 
@@ -134,4 +144,14 @@ static inline u16 ath10k_hif_get_free_queue_number(struct 
ath10k *ar,
return ar-hif.ops-get_free_queue_number(ar, pipe_id);
 }
 
+static inline int ath10k_hif_init(struct ath10k *ar)
+{
+   return ar-hif.ops-init(ar);
+}
+
+static inline void ath10k_hif_deinit(struct ath10k *ar)
+{
+   ar-hif.ops-deinit(ar);
+}
+
 #endif /* _HIF_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
b/drivers/net/wireless/ath/ath10k/pci.c
index 06124a4..97f80e5 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -54,6 +54,8 @@ static int ath10k_pci_post_rx_pipe(struct hif_ce_pipe_info 
*pipe_info,
 int num);
 static void ath10k_pci_rx_pipe_cleanup(struct hif_ce_pipe_info *pipe_info);
 static void ath10k_pci_stop_ce(struct ath10k *ar);
+static void ath10k_pci_device_reset(struct ath10k *ar);
+static int ath10k_pci_reset_target(struct ath10k *ar);
 
 static const struct ce_attr host_ce_config_wlan[] = {
/* host-target HTC control and raw streams */
@@ -1755,6 +1757,66 @@ static void ath10k_pci_fw_interrupt_handler(struct 
ath10k *ar)
ath10k_pci_sleep(ar);
 }
 
+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;
+
+   ret = ath10k_pci_init_config(ar);
+   if (ret)
+   goto err_ce;
+
+   ret = ath10k_pci_wake_target_cpu(ar);
+   if (ret) {
+   ath10k_err(could not wake up target CPU (%d)\n, ret);
+   goto err_ce;
+   }
+
+   return 0;
+
+err_ce:
+   ath10k_pci_ce_deinit(ar);
+err_ps:
+   if (!ath10k_target_ps)
+   ath10k_do_pci_sleep(ar);
+err:
+   return ret;
+}
+
+static void ath10k_pci_hif_deinit(struct ath10k *ar)
+{
+   ath10k_pci_ce_deinit(ar);
+   if (!ath10k_target_ps)
+   ath10k_do_pci_sleep(ar);
+}
+
 static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.send_head  = ath10k_pci_hif_send_head,
.exchange_bmi_msg   = ath10k_pci_hif_exchange_bmi_msg,
@@ -1765,6 +1827,8 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.send_complete_check= ath10k_pci_hif_send_complete_check,
.set_callbacks  = ath10k_pci_hif_set_callbacks,
.get_free_queue_number  =