On Sat, 23 Mar 2019 at 08:20, Arend Van Spriel <arend.vanspriel@xxxxxxxxxxxx> wrote: > > * resending with corrected email address from Kalle > -------------------------------------------------------------------- > + Michał Thanks! > On 3/22/2019 8:25 PM, Christian Lamparter wrote: > > On Friday, March 22, 2019 7:58:40 PM CET Tomislav Požega wrote: > >> When chip reset is done before the chip is checked if supported > >> there will be crash. Previous behaviour caused bootloops on > >> Archer C7 v1 units, this patch allows clean device boot without > >> excluding ath10k driver. Can you elaborate more a bit? What kind of crashes are you seeing? What does the bootloop look like? Do you have uart connected to diagnose? Didn't C7 v1 have the old QCA9880 hw v1 which isn't really supported by ath10k? I recall the v1 chip was really buggy and required hammering registers sometimes to get things working. [...] > Looking at the commit subject makes me suspicious whether this is a > proper fix. [...] > It seems to me the chip reset was done explicitly *before* reading the > chipid for a reason. > > """ > ath10k: reset chip before reading chip_id in probe > There are some very rare cases with some hardware > configuration that the device doesn't init quickly > enough in which case reading chip_id yielded 0. > This caused driver to subsequently fail to setup > the device. > > Signed-off-by: Michal Kazior <michal.kazior@xxxxxxxxx> > Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> > """ Good catch, Arend! > Might be the ath10k_pci_chip_reset() function needs to be modified to > work properly for Archer C7 v1 units. When driver boots up hardware is in an unknown state. It could've been power cycled or it could've remained powered via pcie aux while host cpu went through a warm reset (eg. watchdog). For what it's worth it could still be beaconing if that is what it was doing before host cpu reset. Therefore the device can be in a messy state and it should be reset before talking to it or else you risk reading garbage. I don't recall details about this particular commit but given the timeline of commits at the time this change suggests this was related to supporting qca61x4. In other words your change risks breaking some qca61x4 chips. Michał
