| Message ID | 1447063362-27322-5-git-send-email-fengwei.yin@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Mon, Nov 09, 2015 at 05:02:41AM -0500, Yin, Fengwei wrote: > From: Andy Green <andy.green@linaro.org> > > From: Andy Green <andy.green@linaro.org> > > On wcn3620, firmware response to trigger_ba uses the new, larger > "v2" format > - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); > + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, > + wcn->hal_rsp_len); It's unclear from the changelog -- is it safe to call wcn36xx_smd_rsp_status_check_v2 on the 3660/3680 as well? Is wcn36xx_smd_rsp_status_check() still needed?
Hi Bob, On 2015/11/9 23:40, Bob Copeland wrote: > On Mon, Nov 09, 2015 at 05:02:41AM -0500, Yin, Fengwei wrote: >> From: Andy Green <andy.green@linaro.org> >> >> From: Andy Green <andy.green@linaro.org> >> >> On wcn3620, firmware response to trigger_ba uses the new, larger >> "v2" format > >> - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); >> + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, >> + wcn->hal_rsp_len); > > It's unclear from the changelog -- is it safe to call > wcn36xx_smd_rsp_status_check_v2 on the 3660/3680 as well? > > Is wcn36xx_smd_rsp_status_check() still needed? Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it may be related with firmware). In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If you could test on none-3620 platform (I don't have none-3620 plafrom), that will be great. I will send out patch v2 soon which drive the build error and the comments from you. Thanks a lot for reviewing. Regards Yin, Fengwei > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 10, 2015 at 03:08:53PM +0800, fengwei.yin wrote: > Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. > And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it > may be related with firmware). > > In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If > you could test on none-3620 platform (I don't have none-3620 plafrom), that > will be great. Sure, I'll take a look today or tomorrow on my hardware.
Hi Bob, On 2015/11/10 22:13, Bob Copeland wrote: > On Tue, Nov 10, 2015 at 03:08:53PM +0800, fengwei.yin wrote: >> Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. >> And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it >> may be related with firmware). >> >> In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If >> you could test on none-3620 platform (I don't have none-3620 plafrom), that >> will be great. > > Sure, I'll take a look today or tomorrow on my hardware. Thanks a lot. > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 9, 2015 at 7:40 AM, Bob Copeland <me@bobcopeland.com> wrote: > On Mon, Nov 09, 2015 at 05:02:41AM -0500, Yin, Fengwei wrote: >> From: Andy Green <andy.green@linaro.org> >> >> From: Andy Green <andy.green@linaro.org> >> >> On wcn3620, firmware response to trigger_ba uses the new, larger >> "v2" format > >> - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); >> + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, >> + wcn->hal_rsp_len); > > It's unclear from the changelog -- is it safe to call > wcn36xx_smd_rsp_status_check_v2 on the 3660/3680 as well? > > Is wcn36xx_smd_rsp_status_check() still needed? > I had to introduce this on one of my 3680 devices recently to silence the error described originally by Andy. So it not only seems safe but seems required. But still, based on how the code was written this doesn't seem to be the case on all versions of the firmware or all chips(?) Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On 2015/11/12 12:50, Bjorn Andersson wrote: > On Mon, Nov 9, 2015 at 7:40 AM, Bob Copeland <me@bobcopeland.com> wrote: >> On Mon, Nov 09, 2015 at 05:02:41AM -0500, Yin, Fengwei wrote: >>> From: Andy Green <andy.green@linaro.org> >>> >>> From: Andy Green <andy.green@linaro.org> >>> >>> On wcn3620, firmware response to trigger_ba uses the new, larger >>> "v2" format >> >>> - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); >>> + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, >>> + wcn->hal_rsp_len); >> >> It's unclear from the changelog -- is it safe to call >> wcn36xx_smd_rsp_status_check_v2 on the 3660/3680 as well? >> >> Is wcn36xx_smd_rsp_status_check() still needed? >> > > I had to introduce this on one of my 3680 devices recently to silence > the error described originally by Andy. So it not only seems safe but > seems required. But still, based on how the code was written this > doesn't seem to be the case on all versions of the firmware or all > chips(?) > Thanks for the information. It confirm my thought that the change sticks to new firmware instead of specific platform. But we couldn't tell which version of firmware need this new format. Andy's original change has two conditions to use the new format: 1. The platform is 3620. - this should be removed because you need the same change for 3680. And patch v2 already remove it. 2. The packet size from firmware is larger than old response size. I suppose this one works in most case. Regards Yin, Fengwei > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bob, On 2015?11?10? 22:13, Bob Copeland wrote: > On Tue, Nov 10, 2015 at 03:08:53PM +0800, fengwei.yin wrote: >> Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. >> And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it >> may be related with firmware). >> >> In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If >> you could test on none-3620 platform (I don't have none-3620 plafrom), that >> will be great. > > Sure, I'll take a look today or tomorrow on my hardware. > Ping... Regards Yin, Fengwei -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 19, 2015 at 11:20:14AM +0800, yfw wrote: > Hi Bob, > > On 2015?11?10? 22:13, Bob Copeland wrote: > >On Tue, Nov 10, 2015 at 03:08:53PM +0800, fengwei.yin wrote: > >>Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. > >>And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it > >>may be related with firmware). > >> > >>In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If > >>you could test on none-3620 platform (I don't have none-3620 plafrom), that > >>will be great. > > > >Sure, I'll take a look today or tomorrow on my hardware. > > > > Ping... Oops, sorry. I can confirm my hardware uses this format. Turns out, in my backports snapshot I was already carrying a patch for it: https://github.com/KrasnikovEugene/wcn36xx/commit/7d41270b8c3eee0f72713b4553ca047807c0a00e
Hi Bob, On 2015/11/20 1:41, Bob Copeland wrote: > On Thu, Nov 19, 2015 at 11:20:14AM +0800, yfw wrote: >> Hi Bob, >> >> On 2015?11?10? 22:13, Bob Copeland wrote: >>> On Tue, Nov 10, 2015 at 03:08:53PM +0800, fengwei.yin wrote: >>>> Even on 3620, not all cmds switch to wcn36xx_smd_rsp_status_check_v2. >>>> And I don't think wcn36xx_smd_rsp_status_check_v2 should bind to 3620 (it >>>> may be related with firmware). >>>> >>>> In patch v2, the wcn36xx_smd_rsp_status_check_v2 is not bind to 3620. If >>>> you could test on none-3620 platform (I don't have none-3620 plafrom), that >>>> will be great. >>> >>> Sure, I'll take a look today or tomorrow on my hardware. >>> >> >> Ping... > > Oops, sorry. I can confirm my hardware uses this format. > > Turns out, in my backports snapshot I was already carrying a patch for it: > > https://github.com/KrasnikovEugene/wcn36xx/commit/7d41270b8c3eee0f72713b4553ca047807c0a00e > Actually, this patch was using the v1 and DragonBoard needs v2 for trigger_ba. And I want to know whether the change from Andy could work on the platform which is using v1. Can you try this change on your platform? Thanks. Regards Yin, Fengwei -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 20, 2015 at 09:40:30AM +0800, fengwei.yin wrote: > Actually, this patch was using the v1 and DragonBoard needs v2 for trigger_ba. > And I want to know whether the change from Andy could work on the platform > which is using v1. > > Can you try this change on your platform? Thanks. I'm not sure I understand -- if you look at the right side of the diff for the patch "wcn36xx: Parse trigger_ba response properly", it is changing wcn36xx_smd_rsp_status_check() to wcn36xx_smd_trigger_ba_rsp(), whose definition is the same as wcn36xx_smd_rsp_station_check_v2() as far as I can tell. Or am I missing something?
On 2015/11/20 9:52, Bob Copeland wrote: > On Fri, Nov 20, 2015 at 09:40:30AM +0800, fengwei.yin wrote: >> Actually, this patch was using the v1 and DragonBoard needs v2 for trigger_ba. >> And I want to know whether the change from Andy could work on the platform >> which is using v1. >> >> Can you try this change on your platform? Thanks. > > I'm not sure I understand -- if you look at the right side of the diff > for the patch "wcn36xx: Parse trigger_ba response properly", it is changing > wcn36xx_smd_rsp_status_check() to wcn36xx_smd_trigger_ba_rsp(), whose > definition is the same as wcn36xx_smd_rsp_station_check_v2() as far as I can > tell. Or am I missing something? > Yes. Exactly. So your platform is using the v2 format as well. We may not need to care the v1 format too much. Thanks. Regards Yin, Fengwei -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Yin, Yes, it seems like response is FW specific other than chip spesific. 2015-11-12 6:37 GMT+00:00 fengwei.yin <fengwei.yin@linaro.org>: > Hi Bjorn, > > On 2015/11/12 12:50, Bjorn Andersson wrote: >> >> On Mon, Nov 9, 2015 at 7:40 AM, Bob Copeland <me@bobcopeland.com> wrote: >>> >>> On Mon, Nov 09, 2015 at 05:02:41AM -0500, Yin, Fengwei wrote: >>>> >>>> From: Andy Green <andy.green@linaro.org> >>>> >>>> From: Andy Green <andy.green@linaro.org> >>>> >>>> On wcn3620, firmware response to trigger_ba uses the new, larger >>>> "v2" format >>> >>> >>>> - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, >>>> wcn->hal_rsp_len); >>>> + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, >>>> + wcn->hal_rsp_len); >>> >>> >>> It's unclear from the changelog -- is it safe to call >>> wcn36xx_smd_rsp_status_check_v2 on the 3660/3680 as well? >>> >>> Is wcn36xx_smd_rsp_status_check() still needed? >>> >> >> I had to introduce this on one of my 3680 devices recently to silence >> the error described originally by Andy. So it not only seems safe but >> seems required. But still, based on how the code was written this >> doesn't seem to be the case on all versions of the firmware or all >> chips(?) >> > Thanks for the information. It confirm my thought that the change sticks > to new firmware instead of specific platform. > > But we couldn't tell which version of firmware need this new format. Andy's > original change has two conditions to use the new format: > 1. The platform is 3620. - this should be removed because you need the same > change for 3680. And patch v2 already remove it. > > 2. The packet size from firmware is larger than old response size. I suppose > this one works in most case. > > Regards > Yin, Fengwei > >> Regards, >> Bjorn >> >
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index a84c2cc..f2443a0 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -1968,7 +1968,8 @@ int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index) wcn36xx_err("Sending hal_trigger_ba failed\n"); goto out; } - ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len); + ret = wcn36xx_smd_rsp_status_check_v2(wcn, wcn->hal_buf, + wcn->hal_rsp_len); if (ret) { wcn36xx_err("hal_trigger_ba response failed err=%d\n", ret); goto out;