* [PATCH] rtlwifi: Initialize power-setting callback for USB devices
From: Larry Finger @ 2013-06-28 14:12 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Larry Finger, netdev, Richard Genoud, Stable
Commit a269913c5 entitled "rtlwifi: Rework rtl_lps_leave() and
rtl_lps_enter() to use work queue" has two bugs for USB drivers.
Firstly, the work queue in question was not initialized. Secondly,
the callback routine used by this queue is contained within the
file used for PCI devices. As a result, it is not available for
architectures without PCI hardware.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Reported-by: Richard Genoud <richard.genoud@gmail.com>
Tested-by: Richard Genoud <richard.genoud@gmail.com>
Cc: Richard Genoud <richard.genoud@gmail.com>
Cc: Stable <stable@vger.kernel.org> [3.10]
---
John,
Ideally, this patch should be pushed to the 3.10 stream; however, at this late time
that is not possible. Please push it for 3.11 and the Cc to stable will get it
into 3.10.X.
Larry
---
drivers/net/wireless/rtlwifi/pci.c | 13 -------------
drivers/net/wireless/rtlwifi/ps.c | 12 ++++++++++++
drivers/net/wireless/rtlwifi/ps.h | 1 +
drivers/net/wireless/rtlwifi/usb.c | 2 ++
4 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
index c97e9d3..e70b4ff 100644
--- a/drivers/net/wireless/rtlwifi/pci.c
+++ b/drivers/net/wireless/rtlwifi/pci.c
@@ -1008,19 +1008,6 @@ static void _rtl_pci_prepare_bcn_tasklet(struct ieee80211_hw *hw)
return;
}
-static void rtl_lps_change_work_callback(struct work_struct *work)
-{
- struct rtl_works *rtlworks =
- container_of(work, struct rtl_works, lps_change_work);
- struct ieee80211_hw *hw = rtlworks->hw;
- struct rtl_priv *rtlpriv = rtl_priv(hw);
-
- if (rtlpriv->enter_ps)
- rtl_lps_enter(hw);
- else
- rtl_lps_leave(hw);
-}
-
static void _rtl_pci_init_trx_var(struct ieee80211_hw *hw)
{
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
diff --git a/drivers/net/wireless/rtlwifi/ps.c b/drivers/net/wireless/rtlwifi/ps.c
index 884bcea..71e917d 100644
--- a/drivers/net/wireless/rtlwifi/ps.c
+++ b/drivers/net/wireless/rtlwifi/ps.c
@@ -611,6 +611,18 @@ void rtl_swlps_rf_sleep(struct ieee80211_hw *hw)
MSECS(sleep_intv * mac->vif->bss_conf.beacon_int - 40));
}
+void rtl_lps_change_work_callback(struct work_struct *work)
+{
+ struct rtl_works *rtlworks =
+ container_of(work, struct rtl_works, lps_change_work);
+ struct ieee80211_hw *hw = rtlworks->hw;
+ struct rtl_priv *rtlpriv = rtl_priv(hw);
+
+ if (rtlpriv->enter_ps)
+ rtl_lps_enter(hw);
+ else
+ rtl_lps_leave(hw);
+}
void rtl_swlps_wq_callback(void *data)
{
diff --git a/drivers/net/wireless/rtlwifi/ps.h b/drivers/net/wireless/rtlwifi/ps.h
index 4d682b7..88bd76e 100644
--- a/drivers/net/wireless/rtlwifi/ps.h
+++ b/drivers/net/wireless/rtlwifi/ps.h
@@ -49,5 +49,6 @@ void rtl_swlps_rf_awake(struct ieee80211_hw *hw);
void rtl_swlps_rf_sleep(struct ieee80211_hw *hw);
void rtl_p2p_ps_cmd(struct ieee80211_hw *hw, u8 p2p_ps_state);
void rtl_p2p_info(struct ieee80211_hw *hw, void *data, unsigned int len);
+void rtl_lps_change_work_callback(struct work_struct *work);
#endif
diff --git a/drivers/net/wireless/rtlwifi/usb.c b/drivers/net/wireless/rtlwifi/usb.c
index a3532e0..1feebdc 100644
--- a/drivers/net/wireless/rtlwifi/usb.c
+++ b/drivers/net/wireless/rtlwifi/usb.c
@@ -1070,6 +1070,8 @@ int rtl_usb_probe(struct usb_interface *intf,
spin_lock_init(&rtlpriv->locks.usb_lock);
INIT_WORK(&rtlpriv->works.fill_h2c_cmd,
rtl_fill_h2c_cmd_work_callback);
+ INIT_WORK(&rtlpriv->works.lps_change_work,
+ rtl_lps_change_work_callback);
rtlpriv->usb_data_index = 0;
init_completion(&rtlpriv->firmware_loading_complete);
--
1.8.1.4
^ permalink raw reply related
* Re: rtl8192cu: slow path warning
From: Maxime Ripard @ 2013-06-28 13:53 UTC (permalink / raw)
To: Larry Finger
Cc: Richard Genoud, Chaoming Li, John W. Linville, linux-wireless,
linux-kernel
In-Reply-To: <51CC8284.9070301@lwfinger.net>
[-- Attachment #1: Type: text/plain, Size: 673 bytes --]
Hi Larry,
On Thu, Jun 27, 2013 at 01:20:52PM -0500, Larry Finger wrote:
> A revised patch for the warning is attached. The fix relatively
> simple and the only difference between this one and the earlier
> version is that routine rtl_lps_change_work_callback() was moved
> from pci.c to ps.c. As a result, it will be available for ARM and
> other architectures without a PCI bus.
I just tested it on top of 3.10-rc7 with an imx28, and it works fine.
You can add my
Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: RTL8192CU on ARM not working
From: Maxime Ripard @ 2013-06-28 13:40 UTC (permalink / raw)
To: Richard Genoud
Cc: Larry.Finger, chaoming_li, linville, linux-wireless, netdev,
linux-kernel, linux-arm-kernel, Alexandre Belloni, Brian Lilly,
Brent-Crosby, Jim Wall, Thomas Petazzoni
In-Reply-To: <CACQ1gAiHVxjnt2qx5Y9dEMAbZs5Ki_wEHLhOjDRcPc+xjBU76A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4313 bytes --]
Hi Richard, Larry,
On Thu, Jun 27, 2013 at 02:13:31PM +0200, Richard Genoud wrote:
> 2013/6/27 Maxime Ripard <maxime.ripard@free-electrons.com>:
> > Hi everyone,
> >
> > I'm currently trying to use a RTL8192CU on an ARM (Freescale imx28,
> > armv5) platform, using 3.10-rc3.
> >
> > Trouble is, while the chip is correctly detected and you can use iw on it
> > without any problem it seems, once you start an association to an access
> > point, the association goes on, seems to associate, displaying a WARN()
> > message [1] and then, after what looks like a random amount of time (could
> > be right away, could be after a few minutes), deassociate [2].
> >
> > During the time where it's associated, we never seem to transmit any
> > packets, while iw reports packets being sent, I guess we can assume that
> > they are actually never transmitted as well [3].
> >
> > What seems odd to me as well is that the signal power reported for the access
> > point is excessively high when using iw scan (10 dbm), and once connected, the
> > signal strength is -64dbm, which makes quite a huge difference.
> >
> > Do you have any suggestions on how to solve this issue?
> >
> > Thanks,
> > Maxime
> >
> >
> > [1]:
> > # iw wlan0 connect FreeWifi 2447
> > # [ 485.010858] wlan0: authenticate with f4:ca:e5:c9:f5:91
> > [ 485.065514] wlan0: send auth to f4:ca:e5:c9:f5:91 (try 1/3)
> > [ 485.075963] wlan0: authenticated
> > [ 485.088915] wlan0: associate with f4:ca:e5:c9:f5:91 (try 1/3)
> > [ 485.117137] wlan0: RX AssocResp from f4:ca:e5:c9:f5:91 (capab=0x401 status=0 aid=1)
> > [ 485.130607] wlan0: associated
> > [ 486.917555] ------------[ cut here ]------------
> > [ 486.922277] WARNING: at kernel/workqueue.c:1365 __queue_work+0x1f0/0x2f4()
> > [ 486.929175] Modules linked in:
> > [ 486.932284] CPU: 0 PID: 615 Comm: kworker/0:2 Not tainted 3.10.0-rc3 #2
> > [ 486.938958] Workqueue: rtl92c_usb rtl_watchdog_wq_callback
> > [ 486.944548] [<c00147dc>] (unwind_backtrace+0x0/0xf0) from [<c00120a0>] (show_stack+0x10/0x14)
> > [ 486.953132] [<c00120a0>] (show_stack+0x10/0x14) from [<c001d2ec>] (warn_slowpath_common+0x4c/0x68)
> > [ 486.962140] [<c001d2ec>] (warn_slowpath_common+0x4c/0x68) from [<c001d324>] (warn_slowpath_null+0x1c/0x24)
> > [ 486.971844] [<c001d324>] (warn_slowpath_null+0x1c/0x24) from [<c003769c>] (__queue_work+0x1f0/0x2f4)
> > [ 486.981025] [<c003769c>] (__queue_work+0x1f0/0x2f4) from [<c0037830>] (queue_work_on+0x80/0x88)
> > [ 486.989777] [<c0037830>] (queue_work_on+0x80/0x88) from [<c0267ca4>] (rtl_watchdog_wq_callback+0x5dc/0x8bc)
> > [ 486.999572] [<c0267ca4>] (rtl_watchdog_wq_callback+0x5dc/0x8bc) from [<c0038cbc>] (process_one_work+0x1c0/0x4c8)
> > [ 487.009795] [<c0038cbc>] (process_one_work+0x1c0/0x4c8) from [<c0039684>] (worker_thread+0x140/0x3ac)
> > [ 487.019065] [<c0039684>] (worker_thread+0x140/0x3ac) from [<c003f914>] (kthread+0xa4/0xb0)
> > [ 487.027381] [<c003f914>] (kthread+0xa4/0xb0) from [<c000f0c0>] (ret_from_fork+0x14/0x34)
> > [ 487.035498] ---[ end trace 93341a0c249e647e ]---
> >
> >
> > [2]:
> >
> > # [ 786.086127] wlan0: deauthenticated from f4:ca:e5:c9:f5:91 (Reason: 2)
> > [ 786.126368] cfg80211: Calling CRDA to update world regulatory domain
> >
> >
> > [3]:
> > # iw wlan0 station dump
> > Station f4:ca:e5:c9:f5:91 (on wlan0)
> > inactive time: 55430 ms
> > rx bytes: 77826
> > rx packets: 1026
> > tx packets: 2
> > tx retries: 0
> > tx failed: 0
> > signal: -64 dBm
> > signal avg: -63 dBm
> > tx bitrate: 1.0 MBit/s
> > authorized: yes
> > authenticated: yes
> > preamble: long
> > WMM/WME: yes
> > MFP: no
> > TDLS peer: no
>
> Hi Maxime,
>
> You should have a look at https://lkml.org/lkml/2013/6/11/300
> (in short, use 3.10-rc7)
>
> The warning is still there, but I managed use this device on ARM
> (sam9g35) as a client.
Indeed, the client mode works way better with 3.10-rc7.
Thanks to you two!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-28 13:21 UTC (permalink / raw)
To: mturquette
Cc: balbi, Mark Rutland, grant.likely@linaro.org,
rob.herring@calxeda.com, devicetree-discuss@lists.ozlabs.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org, linux-wireless@vger.kernel.org,
linux-arm@vger.kernel.org
In-Reply-To: <20130628121859.GP11297@arwen.pp.htv.fi>
On Fri, 2013-06-28 at 15:18 +0300, Felipe Balbi wrote:
> On Fri, Jun 28, 2013 at 03:13:52PM +0300, Luciano Coelho wrote:
> > On Fri, 2013-06-28 at 14:41 +0300, Felipe Balbi wrote:
> > > On Fri, Jun 28, 2013 at 02:22:11PM +0300, Luciano Coelho wrote:
> > > > On Fri, 2013-06-28 at 13:31 +0300, Luciano Coelho wrote:
> > > > > (fixed Mike's address)
> > > > >
> > > > > On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> > > > > > On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > > > > > > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > > > > > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > > > > > > +Optional properties:
> > > > > > > > > +--------------------
> > > > > > > > > +
> > > > > > > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > > > > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > > > > > > + following:
> > > > > > > > > + 0 = 19.2 MHz
> > > > > > > > > + 1 = 26.0 MHz
> > > > > > > > > + 2 = 38.4 MHz
> > > > > > > > > + 3 = 52.0 MHz
> > > > > > > > > + 4 = 38.4 MHz, XTAL
> > > > > > > > > + 5 = 26.0 MHz, XTAL
> > > > > > > > > +
> > > > > > > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > > > > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > > > > > > + following:
> > > > > > > > > + 0 = 19.200 MHz
> > > > > > > > > + 1 = 26.000 MHz
> > > > > > > > > + 2 = 38.400 MHz
> > > > > > > > > + 3 = 52.000 MHz
> > > > > > > > > + 4 = 16.368 MHz
> > > > > > > > > + 5 = 32.736 MHz
> > > > > > > > > + 6 = 16.800 MHz
> > > > > > > > > + 7 = 33.600 MHz
> > > > > > > >
> > > > > > > > This looks suspiciously like what we have the common clock bindings for:
> > > > > > > >
> > > > > > > > refclk {
> > > > > > > > compatible = "fixed-clock";
> > > > > > > > #clock-cells = <0>;
> > > > > > > > clock-frequency = <19200000>;
> > > > > > > > }
> > > > > > > >
> > > > > > > > wilink {
> > > > > > > > compatible = "ti,wilink7";
> > > > > > > > interrupt-parent = <&some_interrupt_controller>;
> > > > > > > > interrupts = <0 1 1>;
> > > > > > > > clocks = <&refclk>, <&refclk>;
> > > > > > > > clock-names = "refclk", "txoclk";
> > > > > > > > };
> > > > > > > >
> > > > > > > > Could you not use them?
> > > > > > >
> > > > > > > Hmmm... this actually does look good. But these are internal clocks in
> > > > > > > the modules, they cannot be accessed from outside. Does it make sense
> > > > > > > to register them with the clock framework?
> > > > > >
> > > > > > Given we already have a common way of describing clocks, I think it
> > > > > > makes sense to use it -- people already understand the common bindings,
> > > > > > and it's less code to add add to the kernel. I don't think the fact
> > > > > > these clocks are internal should prevent us from describing them as we
> > > > > > would an external clock.
> > > > >
> > > > > Yes, I agree with you. Thanks for the suggestion! I think it will look
> > > > > much better. And now that I dug a bit more into the code, I can see
> > > > > that there are only structs being populated, so there shouldn't be any
> > > > > other side-effects.
> > > >
> > > > Hmmm, one thing that escaped me. Besides the frequency, I also need a
> > > > boolean that tells if the clock is XTAL or not. I can't figure out how
> > > > to pass this if I use the generic clock framework. Any suggestions?
> > >
> > > Could you use clock-output-names for that ?
> > >
> > > XTAL clock:
> > >
> > > refclk {
> > > compatible = "fixed-clock";
> > > #clock cells = <0>;
> > > clock-frequency = <19200000>;
> > > clock-output-names = "xtal";
> > > };
> > >
> > > non-XTAL clock:
> > >
> > > refclk {
> > > compatible = "fixed-clock";
> > > #clock cells = <0>;
> > > clock-frequency = <19200000>;
> > > clock-output-names = "osc"; /* any better name ? */
> > > };
> >
> > This starts looking a bit hacky. Using the output name as a flag is not
> > very pretty.
> >
> > I think it would be better to have a separate flag for it in the wlan
> > node. Like an optional "refclock-xtal" boolean or something. The
> > downside of this is that we would be adding information about the clock
> > details in the wilink node. :(
> >
> > OTOH, we could add a flag to the generic clock binding? A new optional
> > boolean that tells whether the clock is XTAL or not:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock cells = <0>;
> > clock-frequency = <19200000>;
> > clock-xtal;
> > };
> >
> > Do you think that would make sense?
>
> sure, that looks alright to me. Surely there are other devices out there
> who want to know if the clock comes from a crystal or not ?!?
Mike, what do you think about this idea? If it sounds okay to you, I can
cook up a patch adding this flag.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Felipe Balbi @ 2013-06-28 12:18 UTC (permalink / raw)
To: Luciano Coelho
Cc: balbi, mturquette, Mark Rutland, grant.likely@linaro.org,
rob.herring@calxeda.com, devicetree-discuss@lists.ozlabs.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org, linux-wireless@vger.kernel.org,
linux-arm@vger.kernel.org
In-Reply-To: <1372421632.21065.60.camel@cumari.coelho.fi>
[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]
On Fri, Jun 28, 2013 at 03:13:52PM +0300, Luciano Coelho wrote:
> On Fri, 2013-06-28 at 14:41 +0300, Felipe Balbi wrote:
> > On Fri, Jun 28, 2013 at 02:22:11PM +0300, Luciano Coelho wrote:
> > > On Fri, 2013-06-28 at 13:31 +0300, Luciano Coelho wrote:
> > > > (fixed Mike's address)
> > > >
> > > > On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> > > > > On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > > > > > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > > > > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > > > > > +Optional properties:
> > > > > > > > +--------------------
> > > > > > > > +
> > > > > > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > > > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > > > > > + following:
> > > > > > > > + 0 = 19.2 MHz
> > > > > > > > + 1 = 26.0 MHz
> > > > > > > > + 2 = 38.4 MHz
> > > > > > > > + 3 = 52.0 MHz
> > > > > > > > + 4 = 38.4 MHz, XTAL
> > > > > > > > + 5 = 26.0 MHz, XTAL
> > > > > > > > +
> > > > > > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > > > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > > > > > + following:
> > > > > > > > + 0 = 19.200 MHz
> > > > > > > > + 1 = 26.000 MHz
> > > > > > > > + 2 = 38.400 MHz
> > > > > > > > + 3 = 52.000 MHz
> > > > > > > > + 4 = 16.368 MHz
> > > > > > > > + 5 = 32.736 MHz
> > > > > > > > + 6 = 16.800 MHz
> > > > > > > > + 7 = 33.600 MHz
> > > > > > >
> > > > > > > This looks suspiciously like what we have the common clock bindings for:
> > > > > > >
> > > > > > > refclk {
> > > > > > > compatible = "fixed-clock";
> > > > > > > #clock-cells = <0>;
> > > > > > > clock-frequency = <19200000>;
> > > > > > > }
> > > > > > >
> > > > > > > wilink {
> > > > > > > compatible = "ti,wilink7";
> > > > > > > interrupt-parent = <&some_interrupt_controller>;
> > > > > > > interrupts = <0 1 1>;
> > > > > > > clocks = <&refclk>, <&refclk>;
> > > > > > > clock-names = "refclk", "txoclk";
> > > > > > > };
> > > > > > >
> > > > > > > Could you not use them?
> > > > > >
> > > > > > Hmmm... this actually does look good. But these are internal clocks in
> > > > > > the modules, they cannot be accessed from outside. Does it make sense
> > > > > > to register them with the clock framework?
> > > > >
> > > > > Given we already have a common way of describing clocks, I think it
> > > > > makes sense to use it -- people already understand the common bindings,
> > > > > and it's less code to add add to the kernel. I don't think the fact
> > > > > these clocks are internal should prevent us from describing them as we
> > > > > would an external clock.
> > > >
> > > > Yes, I agree with you. Thanks for the suggestion! I think it will look
> > > > much better. And now that I dug a bit more into the code, I can see
> > > > that there are only structs being populated, so there shouldn't be any
> > > > other side-effects.
> > >
> > > Hmmm, one thing that escaped me. Besides the frequency, I also need a
> > > boolean that tells if the clock is XTAL or not. I can't figure out how
> > > to pass this if I use the generic clock framework. Any suggestions?
> >
> > Could you use clock-output-names for that ?
> >
> > XTAL clock:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock cells = <0>;
> > clock-frequency = <19200000>;
> > clock-output-names = "xtal";
> > };
> >
> > non-XTAL clock:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock cells = <0>;
> > clock-frequency = <19200000>;
> > clock-output-names = "osc"; /* any better name ? */
> > };
>
> This starts looking a bit hacky. Using the output name as a flag is not
> very pretty.
>
> I think it would be better to have a separate flag for it in the wlan
> node. Like an optional "refclock-xtal" boolean or something. The
> downside of this is that we would be adding information about the clock
> details in the wilink node. :(
>
> OTOH, we could add a flag to the generic clock binding? A new optional
> boolean that tells whether the clock is XTAL or not:
>
> refclk {
> compatible = "fixed-clock";
> #clock cells = <0>;
> clock-frequency = <19200000>;
> clock-xtal;
> };
>
> Do you think that would make sense?
sure, that looks alright to me. Surely there are other devices out there
who want to know if the clock comes from a crystal or not ?!?
cheers
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-28 12:13 UTC (permalink / raw)
To: balbi, mturquette
Cc: Mark Rutland, grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org
In-Reply-To: <20130628114129.GO11297@arwen.pp.htv.fi>
On Fri, 2013-06-28 at 14:41 +0300, Felipe Balbi wrote:
> On Fri, Jun 28, 2013 at 02:22:11PM +0300, Luciano Coelho wrote:
> > On Fri, 2013-06-28 at 13:31 +0300, Luciano Coelho wrote:
> > > (fixed Mike's address)
> > >
> > > On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> > > > On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > > > > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > > > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > > > > +Optional properties:
> > > > > > > +--------------------
> > > > > > > +
> > > > > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > > > > + following:
> > > > > > > + 0 = 19.2 MHz
> > > > > > > + 1 = 26.0 MHz
> > > > > > > + 2 = 38.4 MHz
> > > > > > > + 3 = 52.0 MHz
> > > > > > > + 4 = 38.4 MHz, XTAL
> > > > > > > + 5 = 26.0 MHz, XTAL
> > > > > > > +
> > > > > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > > > > + following:
> > > > > > > + 0 = 19.200 MHz
> > > > > > > + 1 = 26.000 MHz
> > > > > > > + 2 = 38.400 MHz
> > > > > > > + 3 = 52.000 MHz
> > > > > > > + 4 = 16.368 MHz
> > > > > > > + 5 = 32.736 MHz
> > > > > > > + 6 = 16.800 MHz
> > > > > > > + 7 = 33.600 MHz
> > > > > >
> > > > > > This looks suspiciously like what we have the common clock bindings for:
> > > > > >
> > > > > > refclk {
> > > > > > compatible = "fixed-clock";
> > > > > > #clock-cells = <0>;
> > > > > > clock-frequency = <19200000>;
> > > > > > }
> > > > > >
> > > > > > wilink {
> > > > > > compatible = "ti,wilink7";
> > > > > > interrupt-parent = <&some_interrupt_controller>;
> > > > > > interrupts = <0 1 1>;
> > > > > > clocks = <&refclk>, <&refclk>;
> > > > > > clock-names = "refclk", "txoclk";
> > > > > > };
> > > > > >
> > > > > > Could you not use them?
> > > > >
> > > > > Hmmm... this actually does look good. But these are internal clocks in
> > > > > the modules, they cannot be accessed from outside. Does it make sense
> > > > > to register them with the clock framework?
> > > >
> > > > Given we already have a common way of describing clocks, I think it
> > > > makes sense to use it -- people already understand the common bindings,
> > > > and it's less code to add add to the kernel. I don't think the fact
> > > > these clocks are internal should prevent us from describing them as we
> > > > would an external clock.
> > >
> > > Yes, I agree with you. Thanks for the suggestion! I think it will look
> > > much better. And now that I dug a bit more into the code, I can see
> > > that there are only structs being populated, so there shouldn't be any
> > > other side-effects.
> >
> > Hmmm, one thing that escaped me. Besides the frequency, I also need a
> > boolean that tells if the clock is XTAL or not. I can't figure out how
> > to pass this if I use the generic clock framework. Any suggestions?
>
> Could you use clock-output-names for that ?
>
> XTAL clock:
>
> refclk {
> compatible = "fixed-clock";
> #clock cells = <0>;
> clock-frequency = <19200000>;
> clock-output-names = "xtal";
> };
>
> non-XTAL clock:
>
> refclk {
> compatible = "fixed-clock";
> #clock cells = <0>;
> clock-frequency = <19200000>;
> clock-output-names = "osc"; /* any better name ? */
> };
This starts looking a bit hacky. Using the output name as a flag is not
very pretty.
I think it would be better to have a separate flag for it in the wlan
node. Like an optional "refclock-xtal" boolean or something. The
downside of this is that we would be adding information about the clock
details in the wilink node. :(
OTOH, we could add a flag to the generic clock binding? A new optional
boolean that tells whether the clock is XTAL or not:
refclk {
compatible = "fixed-clock";
#clock cells = <0>;
clock-frequency = <19200000>;
clock-xtal;
};
Do you think that would make sense?
--
Luca.
^ permalink raw reply
* [PATCH] iwl3945: better skb management in rx path
From: Eric Dumazet @ 2013-06-28 12:03 UTC (permalink / raw)
To: John W. Linville; +Cc: Steinar H. Gunderson, linux-wireless, netdev
From: Eric Dumazet <edumazet@google.com>
Steinar reported reallocations of skb->head with IPv6, leading to
a warning in skb_try_coalesce()
It turns out iwl3945 has several problems :
1) skb->truesize is underestimated.
We really consume PAGE_SIZE bytes for a fragment,
not the frame length.
2) 128 bytes of initial headroom is a bit low and forces reallocations.
3) We can avoid consuming a full page for small enough frames.
Reported-by: Steinar H. Gunderson <sesse@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Note : compiled only, I do not have this hardware.
drivers/net/wireless/iwlegacy/3945.c | 30 +++++++++++++++----------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/iwlegacy/3945.c b/drivers/net/wireless/iwlegacy/3945.c
index c092033..ad7294f 100644
--- a/drivers/net/wireless/iwlegacy/3945.c
+++ b/drivers/net/wireless/iwlegacy/3945.c
@@ -483,14 +483,13 @@ il3945_pass_packet_to_mac80211(struct il_priv *il, struct il_rx_buf *rxb,
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)IL_RX_DATA(pkt);
struct il3945_rx_frame_hdr *rx_hdr = IL_RX_HDR(pkt);
struct il3945_rx_frame_end *rx_end = IL_RX_END(pkt);
- u16 len = le16_to_cpu(rx_hdr->len);
+ u32 len = le16_to_cpu(rx_hdr->len);
struct sk_buff *skb;
__le16 fc = hdr->frame_control;
+ u32 fraglen = PAGE_SIZE << il->hw_params.rx_page_order;
/* We received data from the HW, so stop the watchdog */
- if (unlikely
- (len + IL39_RX_FRAME_SIZE >
- PAGE_SIZE << il->hw_params.rx_page_order)) {
+ if (unlikely(len + IL39_RX_FRAME_SIZE > fraglen)) {
D_DROP("Corruption detected!\n");
return;
}
@@ -506,26 +505,33 @@ il3945_pass_packet_to_mac80211(struct il_priv *il, struct il_rx_buf *rxb,
D_INFO("Woke queues - frame received on passive channel\n");
}
- skb = dev_alloc_skb(128);
+ skb = dev_alloc_skb(256);
if (!skb) {
IL_ERR("dev_alloc_skb failed\n");
return;
}
if (!il3945_mod_params.sw_crypto)
- il_set_decrypted_flag(il, (struct ieee80211_hdr *)rxb_addr(rxb),
+ il_set_decrypted_flag(il, (struct ieee80211_hdr *)pkt,
le32_to_cpu(rx_end->status), stats);
- skb_add_rx_frag(skb, 0, rxb->page,
- (void *)rx_hdr->payload - (void *)pkt, len,
- len);
-
+ /* If frame is small enough to fit into skb->head, copy it
+ * and do not consume a full page
+ */
+ if (len <= 256) {
+ skb_copy_to_linear_data(skb, rx_hdr->payload, len);
+ skb->tail += len;
+ } else {
+ skb_add_rx_frag(skb, 0, rxb->page,
+ (void *)rx_hdr->payload - (void *)pkt, len,
+ fraglen);
+ il->alloc_rxb_page--;
+ rxb->page = NULL;
+ }
il_update_stats(il, false, fc, len);
memcpy(IEEE80211_SKB_RXCB(skb), stats, sizeof(*stats));
ieee80211_rx(il->hw, skb);
- il->alloc_rxb_page--;
- rxb->page = NULL;
}
#define IL_DELAY_NEXT_SCAN_AFTER_ASSOC (HZ*6)
^ permalink raw reply related
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Felipe Balbi @ 2013-06-28 11:41 UTC (permalink / raw)
To: Luciano Coelho
Cc: Mark Rutland, mturquette, grant.likely@linaro.org,
rob.herring@calxeda.com, devicetree-discuss@lists.ozlabs.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org, linux-wireless@vger.kernel.org,
linux-arm@vger.kernel.org
In-Reply-To: <1372418531.21065.53.camel@cumari.coelho.fi>
[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]
On Fri, Jun 28, 2013 at 02:22:11PM +0300, Luciano Coelho wrote:
> On Fri, 2013-06-28 at 13:31 +0300, Luciano Coelho wrote:
> > (fixed Mike's address)
> >
> > On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> > > On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > > > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > > > +Optional properties:
> > > > > > +--------------------
> > > > > > +
> > > > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > > > + following:
> > > > > > + 0 = 19.2 MHz
> > > > > > + 1 = 26.0 MHz
> > > > > > + 2 = 38.4 MHz
> > > > > > + 3 = 52.0 MHz
> > > > > > + 4 = 38.4 MHz, XTAL
> > > > > > + 5 = 26.0 MHz, XTAL
> > > > > > +
> > > > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > > > + following:
> > > > > > + 0 = 19.200 MHz
> > > > > > + 1 = 26.000 MHz
> > > > > > + 2 = 38.400 MHz
> > > > > > + 3 = 52.000 MHz
> > > > > > + 4 = 16.368 MHz
> > > > > > + 5 = 32.736 MHz
> > > > > > + 6 = 16.800 MHz
> > > > > > + 7 = 33.600 MHz
> > > > >
> > > > > This looks suspiciously like what we have the common clock bindings for:
> > > > >
> > > > > refclk {
> > > > > compatible = "fixed-clock";
> > > > > #clock-cells = <0>;
> > > > > clock-frequency = <19200000>;
> > > > > }
> > > > >
> > > > > wilink {
> > > > > compatible = "ti,wilink7";
> > > > > interrupt-parent = <&some_interrupt_controller>;
> > > > > interrupts = <0 1 1>;
> > > > > clocks = <&refclk>, <&refclk>;
> > > > > clock-names = "refclk", "txoclk";
> > > > > };
> > > > >
> > > > > Could you not use them?
> > > >
> > > > Hmmm... this actually does look good. But these are internal clocks in
> > > > the modules, they cannot be accessed from outside. Does it make sense
> > > > to register them with the clock framework?
> > >
> > > Given we already have a common way of describing clocks, I think it
> > > makes sense to use it -- people already understand the common bindings,
> > > and it's less code to add add to the kernel. I don't think the fact
> > > these clocks are internal should prevent us from describing them as we
> > > would an external clock.
> >
> > Yes, I agree with you. Thanks for the suggestion! I think it will look
> > much better. And now that I dug a bit more into the code, I can see
> > that there are only structs being populated, so there shouldn't be any
> > other side-effects.
>
> Hmmm, one thing that escaped me. Besides the frequency, I also need a
> boolean that tells if the clock is XTAL or not. I can't figure out how
> to pass this if I use the generic clock framework. Any suggestions?
Could you use clock-output-names for that ?
XTAL clock:
refclk {
compatible = "fixed-clock";
#clock cells = <0>;
clock-frequency = <19200000>;
clock-output-names = "xtal";
};
non-XTAL clock:
refclk {
compatible = "fixed-clock";
#clock cells = <0>;
clock-frequency = <19200000>;
clock-output-names = "osc"; /* any better name ? */
};
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-28 11:22 UTC (permalink / raw)
To: Mark Rutland
Cc: mturquette, grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org
In-Reply-To: <1372415514.21065.50.camel@cumari.coelho.fi>
On Fri, 2013-06-28 at 13:31 +0300, Luciano Coelho wrote:
> (fixed Mike's address)
>
> On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> > On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > > +Optional properties:
> > > > > +--------------------
> > > > > +
> > > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > > + following:
> > > > > + 0 = 19.2 MHz
> > > > > + 1 = 26.0 MHz
> > > > > + 2 = 38.4 MHz
> > > > > + 3 = 52.0 MHz
> > > > > + 4 = 38.4 MHz, XTAL
> > > > > + 5 = 26.0 MHz, XTAL
> > > > > +
> > > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > > + following:
> > > > > + 0 = 19.200 MHz
> > > > > + 1 = 26.000 MHz
> > > > > + 2 = 38.400 MHz
> > > > > + 3 = 52.000 MHz
> > > > > + 4 = 16.368 MHz
> > > > > + 5 = 32.736 MHz
> > > > > + 6 = 16.800 MHz
> > > > > + 7 = 33.600 MHz
> > > >
> > > > This looks suspiciously like what we have the common clock bindings for:
> > > >
> > > > refclk {
> > > > compatible = "fixed-clock";
> > > > #clock-cells = <0>;
> > > > clock-frequency = <19200000>;
> > > > }
> > > >
> > > > wilink {
> > > > compatible = "ti,wilink7";
> > > > interrupt-parent = <&some_interrupt_controller>;
> > > > interrupts = <0 1 1>;
> > > > clocks = <&refclk>, <&refclk>;
> > > > clock-names = "refclk", "txoclk";
> > > > };
> > > >
> > > > Could you not use them?
> > >
> > > Hmmm... this actually does look good. But these are internal clocks in
> > > the modules, they cannot be accessed from outside. Does it make sense
> > > to register them with the clock framework?
> >
> > Given we already have a common way of describing clocks, I think it
> > makes sense to use it -- people already understand the common bindings,
> > and it's less code to add add to the kernel. I don't think the fact
> > these clocks are internal should prevent us from describing them as we
> > would an external clock.
>
> Yes, I agree with you. Thanks for the suggestion! I think it will look
> much better. And now that I dug a bit more into the code, I can see
> that there are only structs being populated, so there shouldn't be any
> other side-effects.
Hmmm, one thing that escaped me. Besides the frequency, I also need a
boolean that tells if the clock is XTAL or not. I can't figure out how
to pass this if I use the generic clock framework. Any suggestions?
--
Luca.
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Mark Rutland @ 2013-06-28 10:39 UTC (permalink / raw)
To: Luciano Coelho
Cc: linux-doc@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
rob.herring@calxeda.com, grant.likely@linaro.org,
linux-omap@vger.kernel.org, mturquette@linaro.org
In-Reply-To: <1372413215.21065.41.camel@cumari.coelho.fi>
[resending again with the doubly corrected address for Mike Turquette,
apologies for the spam]
On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > +Optional properties:
> > > +--------------------
> > > +
> > > +- refclock: the internal WLAN reference clock frequency (required for
> > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.2 MHz
> > > + 1 = 26.0 MHz
> > > + 2 = 38.4 MHz
> > > + 3 = 52.0 MHz
> > > + 4 = 38.4 MHz, XTAL
> > > + 5 = 26.0 MHz, XTAL
> > > +
> > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.200 MHz
> > > + 1 = 26.000 MHz
> > > + 2 = 38.400 MHz
> > > + 3 = 52.000 MHz
> > > + 4 = 16.368 MHz
> > > + 5 = 32.736 MHz
> > > + 6 = 16.800 MHz
> > > + 7 = 33.600 MHz
> >
> > This looks suspiciously like what we have the common clock bindings for:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <19200000>;
> > }
> >
> > wilink {
> > compatible = "ti,wilink7";
> > interrupt-parent = <&some_interrupt_controller>;
> > interrupts = <0 1 1>;
> > clocks = <&refclk>, <&refclk>;
> > clock-names = "refclk", "txoclk";
> > };
> >
> > Could you not use them?
>
> Hmmm... this actually does look good. But these are internal clocks in
> the modules, they cannot be accessed from outside. Does it make sense
> to register them with the clock framework?
Given we already have a common way of describing clocks, I think it
makes sense to use it -- people already understand the common bindings,
and it's less code to add add to the kernel. I don't think the fact
these clocks are internal should prevent us from describing them as we
would an external clock.
Perhaps Mike Turquette [Cc'd] has an opinion on the matter.
Thanks,
Mark.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Mark Rutland @ 2013-06-28 10:33 UTC (permalink / raw)
To: Luciano Coelho
Cc: linux-doc@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
rob.herring@calxeda.com, grant.likely@linaro.org,
linux-omap@vger.kernel.org, "mturquette
In-Reply-To: <1372413215.21065.41.camel@cumari.coelho.fi>
[resending with the correct address for Mike Turquette]
On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > +Optional properties:
> > > +--------------------
> > > +
> > > +- refclock: the internal WLAN reference clock frequency (required for
> > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.2 MHz
> > > + 1 = 26.0 MHz
> > > + 2 = 38.4 MHz
> > > + 3 = 52.0 MHz
> > > + 4 = 38.4 MHz, XTAL
> > > + 5 = 26.0 MHz, XTAL
> > > +
> > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.200 MHz
> > > + 1 = 26.000 MHz
> > > + 2 = 38.400 MHz
> > > + 3 = 52.000 MHz
> > > + 4 = 16.368 MHz
> > > + 5 = 32.736 MHz
> > > + 6 = 16.800 MHz
> > > + 7 = 33.600 MHz
> >
> > This looks suspiciously like what we have the common clock bindings for:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <19200000>;
> > }
> >
> > wilink {
> > compatible = "ti,wilink7";
> > interrupt-parent = <&some_interrupt_controller>;
> > interrupts = <0 1 1>;
> > clocks = <&refclk>, <&refclk>;
> > clock-names = "refclk", "txoclk";
> > };
> >
> > Could you not use them?
>
> Hmmm... this actually does look good. But these are internal clocks in
> the modules, they cannot be accessed from outside. Does it make sense
> to register them with the clock framework?
Given we already have a common way of describing clocks, I think it
makes sense to use it -- people already understand the common bindings,
and it's less code to add add to the kernel. I don't think the fact
these clocks are internal should prevent us from describing them as we
would an external clock.
Perhaps Mike Turquette [Cc'd] has an opinion on the matter.
Thanks,
Mark.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-28 10:31 UTC (permalink / raw)
To: Mark Rutland, mturquette
Cc: grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org
In-Reply-To: <20130628102145.GB2139@e106331-lin.cambridge.arm.com>
(fixed Mike's address)
On Fri, 2013-06-28 at 11:21 +0100, Mark Rutland wrote:
> On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> > On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > > +Optional properties:
> > > > +--------------------
> > > > +
> > > > +- refclock: the internal WLAN reference clock frequency (required for
> > > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > > + following:
> > > > + 0 = 19.2 MHz
> > > > + 1 = 26.0 MHz
> > > > + 2 = 38.4 MHz
> > > > + 3 = 52.0 MHz
> > > > + 4 = 38.4 MHz, XTAL
> > > > + 5 = 26.0 MHz, XTAL
> > > > +
> > > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > > + following:
> > > > + 0 = 19.200 MHz
> > > > + 1 = 26.000 MHz
> > > > + 2 = 38.400 MHz
> > > > + 3 = 52.000 MHz
> > > > + 4 = 16.368 MHz
> > > > + 5 = 32.736 MHz
> > > > + 6 = 16.800 MHz
> > > > + 7 = 33.600 MHz
> > >
> > > This looks suspiciously like what we have the common clock bindings for:
> > >
> > > refclk {
> > > compatible = "fixed-clock";
> > > #clock-cells = <0>;
> > > clock-frequency = <19200000>;
> > > }
> > >
> > > wilink {
> > > compatible = "ti,wilink7";
> > > interrupt-parent = <&some_interrupt_controller>;
> > > interrupts = <0 1 1>;
> > > clocks = <&refclk>, <&refclk>;
> > > clock-names = "refclk", "txoclk";
> > > };
> > >
> > > Could you not use them?
> >
> > Hmmm... this actually does look good. But these are internal clocks in
> > the modules, they cannot be accessed from outside. Does it make sense
> > to register them with the clock framework?
>
> Given we already have a common way of describing clocks, I think it
> makes sense to use it -- people already understand the common bindings,
> and it's less code to add add to the kernel. I don't think the fact
> these clocks are internal should prevent us from describing them as we
> would an external clock.
Yes, I agree with you. Thanks for the suggestion! I think it will look
much better. And now that I dug a bit more into the code, I can see
that there are only structs being populated, so there shouldn't be any
other side-effects.
> Perhaps Mike Turquette [Cc'd] has an opinion on the matter.
Experts' opinions are appreciated. :)
--
Luca.
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Mark Rutland @ 2013-06-28 10:21 UTC (permalink / raw)
To: Luciano Coelho
Cc: grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org,
mturquette
In-Reply-To: <1372413215.21065.41.camel@cumari.coelho.fi>
On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote:
> On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> > On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > > +Optional properties:
> > > +--------------------
> > > +
> > > +- refclock: the internal WLAN reference clock frequency (required for
> > > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.2 MHz
> > > + 1 = 26.0 MHz
> > > + 2 = 38.4 MHz
> > > + 3 = 52.0 MHz
> > > + 4 = 38.4 MHz, XTAL
> > > + 5 = 26.0 MHz, XTAL
> > > +
> > > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > > + following:
> > > + 0 = 19.200 MHz
> > > + 1 = 26.000 MHz
> > > + 2 = 38.400 MHz
> > > + 3 = 52.000 MHz
> > > + 4 = 16.368 MHz
> > > + 5 = 32.736 MHz
> > > + 6 = 16.800 MHz
> > > + 7 = 33.600 MHz
> >
> > This looks suspiciously like what we have the common clock bindings for:
> >
> > refclk {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <19200000>;
> > }
> >
> > wilink {
> > compatible = "ti,wilink7";
> > interrupt-parent = <&some_interrupt_controller>;
> > interrupts = <0 1 1>;
> > clocks = <&refclk>, <&refclk>;
> > clock-names = "refclk", "txoclk";
> > };
> >
> > Could you not use them?
>
> Hmmm... this actually does look good. But these are internal clocks in
> the modules, they cannot be accessed from outside. Does it make sense
> to register them with the clock framework?
Given we already have a common way of describing clocks, I think it
makes sense to use it -- people already understand the common bindings,
and it's less code to add add to the kernel. I don't think the fact
these clocks are internal should prevent us from describing them as we
would an external clock.
Perhaps Mike Turquette [Cc'd] has an opinion on the matter.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-28 9:53 UTC (permalink / raw)
To: Mark Rutland
Cc: grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org
In-Reply-To: <20130628093848.GA2139@e106331-lin.cambridge.arm.com>
On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote:
> On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> > +Optional properties:
> > +--------------------
> > +
> > +- refclock: the internal WLAN reference clock frequency (required for
> > + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> > + following:
> > + 0 = 19.2 MHz
> > + 1 = 26.0 MHz
> > + 2 = 38.4 MHz
> > + 3 = 52.0 MHz
> > + 4 = 38.4 MHz, XTAL
> > + 5 = 26.0 MHz, XTAL
> > +
> > +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> > + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> > + following:
> > + 0 = 19.200 MHz
> > + 1 = 26.000 MHz
> > + 2 = 38.400 MHz
> > + 3 = 52.000 MHz
> > + 4 = 16.368 MHz
> > + 5 = 32.736 MHz
> > + 6 = 16.800 MHz
> > + 7 = 33.600 MHz
>
> This looks suspiciously like what we have the common clock bindings for:
>
> refclk {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <19200000>;
> }
>
> wilink {
> compatible = "ti,wilink7";
> interrupt-parent = <&some_interrupt_controller>;
> interrupts = <0 1 1>;
> clocks = <&refclk>, <&refclk>;
> clock-names = "refclk", "txoclk";
> };
>
> Could you not use them?
Hmmm... this actually does look good. But these are internal clocks in
the modules, they cannot be accessed from outside. Does it make sense
to register them with the clock framework?
--
Luca.
^ permalink raw reply
* [PATCH] cfg80211: fix bugs in new SME implementation
From: Johannes Berg @ 2013-06-28 9:41 UTC (permalink / raw)
To: linux-wireless
When splitting the SME implementation from the MLME code,
I introduced a few bugs:
* association failures no longer sent a connect-failure event
* getting disassociated from the AP caused deauth to be sent
but state wasn't cleaned up, leading to warnings
* authentication failures weren't cleaned up properly, causing
new connection attempts to warn and fail
Fix these bugs.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
net/wireless/sme.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 1d3cfb1..81c8a10 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -34,8 +34,10 @@ struct cfg80211_conn {
CFG80211_CONN_SCAN_AGAIN,
CFG80211_CONN_AUTHENTICATE_NEXT,
CFG80211_CONN_AUTHENTICATING,
+ CFG80211_CONN_AUTH_FAILED,
CFG80211_CONN_ASSOCIATE_NEXT,
CFG80211_CONN_ASSOCIATING,
+ CFG80211_CONN_ASSOC_FAILED,
CFG80211_CONN_DEAUTH,
CFG80211_CONN_CONNECTED,
} state;
@@ -164,6 +166,8 @@ static int cfg80211_conn_do_work(struct wireless_dev *wdev)
NULL, 0,
params->key, params->key_len,
params->key_idx, NULL, 0);
+ case CFG80211_CONN_AUTH_FAILED:
+ return -ENOTCONN;
case CFG80211_CONN_ASSOCIATE_NEXT:
BUG_ON(!rdev->ops->assoc);
wdev->conn->state = CFG80211_CONN_ASSOCIATING;
@@ -188,10 +192,17 @@ static int cfg80211_conn_do_work(struct wireless_dev *wdev)
WLAN_REASON_DEAUTH_LEAVING,
false);
return err;
+ case CFG80211_CONN_ASSOC_FAILED:
+ cfg80211_mlme_deauth(rdev, wdev->netdev, params->bssid,
+ NULL, 0,
+ WLAN_REASON_DEAUTH_LEAVING, false);
+ return -ENOTCONN;
case CFG80211_CONN_DEAUTH:
cfg80211_mlme_deauth(rdev, wdev->netdev, params->bssid,
NULL, 0,
WLAN_REASON_DEAUTH_LEAVING, false);
+ /* free directly, disconnected event already sent */
+ cfg80211_sme_free(wdev);
return 0;
default:
return 0;
@@ -371,7 +382,7 @@ bool cfg80211_sme_rx_assoc_resp(struct wireless_dev *wdev, u16 status)
return true;
}
- wdev->conn->state = CFG80211_CONN_DEAUTH;
+ wdev->conn->state = CFG80211_CONN_ASSOC_FAILED;
schedule_work(&rdev->conn_work);
return false;
}
@@ -383,7 +394,13 @@ void cfg80211_sme_deauth(struct wireless_dev *wdev)
void cfg80211_sme_auth_timeout(struct wireless_dev *wdev)
{
- cfg80211_sme_free(wdev);
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+
+ if (!wdev->conn)
+ return;
+
+ wdev->conn->state = CFG80211_CONN_AUTH_FAILED;
+ schedule_work(&rdev->conn_work);
}
void cfg80211_sme_disassoc(struct wireless_dev *wdev)
@@ -399,7 +416,13 @@ void cfg80211_sme_disassoc(struct wireless_dev *wdev)
void cfg80211_sme_assoc_timeout(struct wireless_dev *wdev)
{
- cfg80211_sme_disassoc(wdev);
+ struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
+
+ if (!wdev->conn)
+ return;
+
+ wdev->conn->state = CFG80211_CONN_ASSOC_FAILED;
+ schedule_work(&rdev->conn_work);
}
static int cfg80211_sme_connect(struct wireless_dev *wdev,
--
1.8.0
^ permalink raw reply related
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Mark Rutland @ 2013-06-28 9:38 UTC (permalink / raw)
To: Luciano Coelho
Cc: grant.likely@linaro.org, rob.herring@calxeda.com,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-wireless@vger.kernel.org, linux-arm@vger.kernel.org
In-Reply-To: <1372149330-24335-1-git-send-email-coelho@ti.com>
On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote:
> Add device tree bindings documentation for the TI WiLink modules.
> Currently only the WLAN part of the WiLink6, WiLink7 and WiLink8
> modules is supported.
>
> Signed-off-by: Luciano Coelho <coelho@ti.com>
> ---
>
> I created a new directory under net to contain wireless bindings documentation.
>
> The actual implementation in the driver will follow separately.
>
> .../devicetree/bindings/net/wireless/ti-wilink.txt | 46 ++++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/wireless/ti-wilink.txt
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt b/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt
> new file mode 100644
> index 0000000..d8e8bfbb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt
> @@ -0,0 +1,46 @@
> +TI WiLink Wireless Modules Device Tree Bindings
> +===============================================
> +
> +The WiLink modules provide wireless connectivity, such as WLAN,
> +Bluetooth, FM and NFC.
> +
> +There are several different modules available, which can be grouped by
> +their generation: WiLink6, WiLink7 and WiLink8. WiLink4 is not
> +currently supported with device tree.
> +
> +Currently, only the WLAN portion of the modules is supported with
> +device tree.
> +
> +Required properties:
> +--------------------
> +
> +- compatible: should be "ti,wilink6", "ti,wilink7" or "ti,wilink8"
> +- interrupt-parent: the interrupt controller
> +- interrupts: out-of-band WLAN interrupt
> + See the interrupt controller's bindings documentation for
> + detailed definition.
> +
> +Optional properties:
> +--------------------
> +
> +- refclock: the internal WLAN reference clock frequency (required for
> + WiLink6 and WiLink7; not used for WiLink8). Must be one of the
> + following:
> + 0 = 19.2 MHz
> + 1 = 26.0 MHz
> + 2 = 38.4 MHz
> + 3 = 52.0 MHz
> + 4 = 38.4 MHz, XTAL
> + 5 = 26.0 MHz, XTAL
> +
> +- tcxoclock: the internal WLAN TCXO clock frequency (required for
> + WiLink7 not used for WiLink6 and WiLink8). Must be one of the
> + following:
> + 0 = 19.200 MHz
> + 1 = 26.000 MHz
> + 2 = 38.400 MHz
> + 3 = 52.000 MHz
> + 4 = 16.368 MHz
> + 5 = 32.736 MHz
> + 6 = 16.800 MHz
> + 7 = 33.600 MHz
This looks suspiciously like what we have the common clock bindings for:
refclk {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <19200000>;
}
wilink {
compatible = "ti,wilink7";
interrupt-parent = <&some_interrupt_controller>;
interrupts = <0 1 1>;
clocks = <&refclk>, <&refclk>;
clock-names = "refclk", "txoclk";
};
Could you not use them?
Thanks,
Mark.
^ permalink raw reply
* Re: rtl8192cu: slow path warning
From: Richard GENOUD @ 2013-06-28 8:58 UTC (permalink / raw)
To: Larry Finger
Cc: Chaoming Li, John W. Linville, linux-wireless, linux-kernel,
Maxime Ripard
In-Reply-To: <51CC8284.9070301@lwfinger.net>
On [jeu., 27.06.2013 13:20:52], Larry Finger wrote:
> On 06/27/2013 02:33 AM, Richard Genoud wrote:
> >Yes, of course, you can add my
> >Reported-by: Richard Genoud <richard.genoud@gmail.com>
> >
> >But the patch doesn't compile on my platform ( since I'm on ARM, I
> >haven't got a PCI bus, so rtlwifi/pci.c is not compiled ) :
> >
> >ERROR: "rtl_lps_change_work_callback"
> >[drivers/net/wireless/rtlwifi/rtlwifi.ko] undefined!
> >
> >
> >Best regards,
> >Richard.
> >
> >PS: duckducking the warning, I found this automated report also :
> >https://retrace.fedoraproject.org/faf/reports/142038/
>
> A revised patch for the warning is attached. The fix relatively
> simple and the only difference between this one and the earlier
> version is that routine rtl_lps_change_work_callback() was moved
> from pci.c to ps.c. As a result, it will be available for ARM and
> other architectures without a PCI bus.
>
> Larry
>
>
> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/pci.c
> ===================================================================
> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/pci.c
> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/pci.c
> @@ -1008,19 +1008,6 @@ static void _rtl_pci_prepare_bcn_tasklet
> return;
> }
>
> -static void rtl_lps_change_work_callback(struct work_struct *work)
> -{
> - struct rtl_works *rtlworks =
> - container_of(work, struct rtl_works, lps_change_work);
> - struct ieee80211_hw *hw = rtlworks->hw;
> - struct rtl_priv *rtlpriv = rtl_priv(hw);
> -
> - if (rtlpriv->enter_ps)
> - rtl_lps_enter(hw);
> - else
> - rtl_lps_leave(hw);
> -}
> -
> static void _rtl_pci_init_trx_var(struct ieee80211_hw *hw)
> {
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/ps.h
> ===================================================================
> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/ps.h
> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/ps.h
> @@ -49,5 +49,6 @@ void rtl_swlps_rf_awake(struct ieee80211
> void rtl_swlps_rf_sleep(struct ieee80211_hw *hw);
> void rtl_p2p_ps_cmd(struct ieee80211_hw *hw, u8 p2p_ps_state);
> void rtl_p2p_info(struct ieee80211_hw *hw, void *data, unsigned int len);
> +void rtl_lps_change_work_callback(struct work_struct *work);
>
> #endif
> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/usb.c
> ===================================================================
> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/usb.c
> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/usb.c
> @@ -1070,6 +1070,8 @@ int rtl_usb_probe(struct usb_interface *
> spin_lock_init(&rtlpriv->locks.usb_lock);
> INIT_WORK(&rtlpriv->works.fill_h2c_cmd,
> rtl_fill_h2c_cmd_work_callback);
> + INIT_WORK(&rtlpriv->works.lps_change_work,
> + rtl_lps_change_work_callback);
>
> rtlpriv->usb_data_index = 0;
> init_completion(&rtlpriv->firmware_loading_complete);
> Index: wireless-testing-save/drivers/net/wireless/rtlwifi/ps.c
> ===================================================================
> --- wireless-testing-save.orig/drivers/net/wireless/rtlwifi/ps.c
> +++ wireless-testing-save/drivers/net/wireless/rtlwifi/ps.c
> @@ -611,6 +611,18 @@ void rtl_swlps_rf_sleep(struct ieee80211
> MSECS(sleep_intv * mac->vif->bss_conf.beacon_int - 40));
> }
>
> +void rtl_lps_change_work_callback(struct work_struct *work)
> +{
> + struct rtl_works *rtlworks =
> + container_of(work, struct rtl_works, lps_change_work);
> + struct ieee80211_hw *hw = rtlworks->hw;
> + struct rtl_priv *rtlpriv = rtl_priv(hw);
> +
> + if (rtlpriv->enter_ps)
> + rtl_lps_enter(hw);
> + else
> + rtl_lps_leave(hw);
> +}
>
> void rtl_swlps_wq_callback(void *data)
> {
Tested the patch on 3.10-rc7, ARM at91sam9g35, usb key TP-link
TL-WN725N.
I haven't seen the WARNING any more.
Tests done: (DHCP request)
client with no security
client WEP
client WPA
AP with no security
AP WEP
AP+WAP test failed (bust it was like that before the patch also)
Tested-by: Richard Genoud <richard.genoud@gmail.com>
^ permalink raw reply
* [PATCH 1/2] nl80211: enable HT overrides for ibss
From: Simon Wunderlich @ 2013-06-28 8:39 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
Signed-off-by: Mathias Kretschmer <mathias.kretschmer@fokus.fraunhofer.de>
---
include/net/cfg80211.h | 5 +++++
net/wireless/nl80211.c | 13 +++++++++++++
2 files changed, 18 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7b0730a..836b7e2 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1592,6 +1592,9 @@ struct cfg80211_disassoc_request {
* user space. Otherwise, port is marked authorized by default.
* @basic_rates: bitmap of basic rates to use when creating the IBSS
* @mcast_rate: per-band multicast rate index + 1 (0: disabled)
+ * @ht_capa: HT Capabilities over-rides. Values set in ht_capa_mask
+ * will be used in ht_capa. Un-supported values will be ignored.
+ * @ht_capa_mask: The bits of ht_capa which are to be used.
*/
struct cfg80211_ibss_params {
u8 *ssid;
@@ -1605,6 +1608,8 @@ struct cfg80211_ibss_params {
bool privacy;
bool control_port;
int mcast_rate[IEEE80211_NUM_BANDS];
+ struct ieee80211_ht_cap ht_capa;
+ struct ieee80211_ht_cap ht_capa_mask;
};
/**
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7dc3343..d74230c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6344,6 +6344,19 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
return err;
}
+ if (info->attrs[NL80211_ATTR_HT_CAPABILITY_MASK])
+ memcpy(&ibss.ht_capa_mask,
+ nla_data(info->attrs[NL80211_ATTR_HT_CAPABILITY_MASK]),
+ sizeof(ibss.ht_capa_mask));
+
+ if (info->attrs[NL80211_ATTR_HT_CAPABILITY]) {
+ if (!info->attrs[NL80211_ATTR_HT_CAPABILITY_MASK])
+ return -EINVAL;
+ memcpy(&ibss.ht_capa,
+ nla_data(info->attrs[NL80211_ATTR_HT_CAPABILITY]),
+ sizeof(ibss.ht_capa));
+ }
+
if (info->attrs[NL80211_ATTR_MCAST_RATE] &&
!nl80211_parse_mcast_rate(rdev, ibss.mcast_rate,
nla_get_u32(info->attrs[NL80211_ATTR_MCAST_RATE])))
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/2] mac80211: enable HT overrides for ibss
From: Simon Wunderlich @ 2013-06-28 8:39 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <1372408799-25651-1-git-send-email-siwu@hrz.tu-chemnitz.de>
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
Signed-off-by: Mathias Kretschmer <mathias.kretschmer@fokus.fraunhofer.de>
---
net/mac80211/ht.c | 53 +++++++++++++++++++++++++++++++-------------
net/mac80211/ibss.c | 18 +++++++++++++--
net/mac80211/ieee80211_i.h | 3 +++
3 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index f83534f..529bf58 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -19,13 +19,14 @@
#include "ieee80211_i.h"
#include "rate.h"
-static void __check_htcap_disable(struct ieee80211_sub_if_data *sdata,
+static void __check_htcap_disable(struct ieee80211_ht_cap *ht_capa,
+ struct ieee80211_ht_cap *ht_capa_mask,
struct ieee80211_sta_ht_cap *ht_cap,
u16 flag)
{
__le16 le_flag = cpu_to_le16(flag);
- if (sdata->u.mgd.ht_capa_mask.cap_info & le_flag) {
- if (!(sdata->u.mgd.ht_capa.cap_info & le_flag))
+ if (ht_capa_mask->cap_info & le_flag) {
+ if (!(ht_capa->cap_info & le_flag))
ht_cap->cap &= ~flag;
}
}
@@ -33,13 +34,30 @@ static void __check_htcap_disable(struct ieee80211_sub_if_data *sdata,
void ieee80211_apply_htcap_overrides(struct ieee80211_sub_if_data *sdata,
struct ieee80211_sta_ht_cap *ht_cap)
{
- u8 *scaps = (u8 *)(&sdata->u.mgd.ht_capa.mcs.rx_mask);
- u8 *smask = (u8 *)(&sdata->u.mgd.ht_capa_mask.mcs.rx_mask);
+ struct ieee80211_ht_cap *ht_capa, *ht_capa_mask;
+ u8 *scaps, *smask;
int i;
if (!ht_cap->ht_supported)
return;
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ ht_capa = &sdata->u.mgd.ht_capa;
+ ht_capa_mask = &sdata->u.mgd.ht_capa_mask;
+ break;
+ case NL80211_IFTYPE_ADHOC:
+ ht_capa = &sdata->u.ibss.ht_capa;
+ ht_capa_mask = &sdata->u.ibss.ht_capa_mask;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ return;
+ }
+
+ scaps = (u8 *)(&ht_capa->mcs.rx_mask);
+ smask = (u8 *)(&ht_capa_mask->mcs.rx_mask);
+
/* NOTE: If you add more over-rides here, update register_hw
* ht_capa_mod_msk logic in main.c as well.
* And, if this method can ever change ht_cap.ht_supported, fix
@@ -55,28 +73,32 @@ void ieee80211_apply_htcap_overrides(struct ieee80211_sub_if_data *sdata,
}
/* Force removal of HT-40 capabilities? */
- __check_htcap_disable(sdata, ht_cap, IEEE80211_HT_CAP_SUP_WIDTH_20_40);
- __check_htcap_disable(sdata, ht_cap, IEEE80211_HT_CAP_SGI_40);
+ __check_htcap_disable(ht_capa, ht_capa_mask, ht_cap,
+ IEEE80211_HT_CAP_SUP_WIDTH_20_40);
+ __check_htcap_disable(ht_capa, ht_capa_mask, ht_cap,
+ IEEE80211_HT_CAP_SGI_40);
/* Allow user to disable SGI-20 (SGI-40 is handled above) */
- __check_htcap_disable(sdata, ht_cap, IEEE80211_HT_CAP_SGI_20);
+ __check_htcap_disable(ht_capa, ht_capa_mask, ht_cap,
+ IEEE80211_HT_CAP_SGI_20);
/* Allow user to disable the max-AMSDU bit. */
- __check_htcap_disable(sdata, ht_cap, IEEE80211_HT_CAP_MAX_AMSDU);
+ __check_htcap_disable(ht_capa, ht_capa_mask, ht_cap,
+ IEEE80211_HT_CAP_MAX_AMSDU);
/* Allow user to decrease AMPDU factor */
- if (sdata->u.mgd.ht_capa_mask.ampdu_params_info &
+ if (ht_capa_mask->ampdu_params_info &
IEEE80211_HT_AMPDU_PARM_FACTOR) {
- u8 n = sdata->u.mgd.ht_capa.ampdu_params_info
- & IEEE80211_HT_AMPDU_PARM_FACTOR;
+ u8 n = ht_capa->ampdu_params_info &
+ IEEE80211_HT_AMPDU_PARM_FACTOR;
if (n < ht_cap->ampdu_factor)
ht_cap->ampdu_factor = n;
}
/* Allow the user to increase AMPDU density. */
- if (sdata->u.mgd.ht_capa_mask.ampdu_params_info &
+ if (ht_capa_mask->ampdu_params_info &
IEEE80211_HT_AMPDU_PARM_DENSITY) {
- u8 n = (sdata->u.mgd.ht_capa.ampdu_params_info &
+ u8 n = (ht_capa->ampdu_params_info &
IEEE80211_HT_AMPDU_PARM_DENSITY)
>> IEEE80211_HT_AMPDU_PARM_DENSITY_SHIFT;
if (n > ht_cap->ampdu_density)
@@ -112,7 +134,8 @@ bool ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_sub_if_data *sdata,
* we advertised a restricted capability set to. Override
* our own capabilities and then use those below.
*/
- if (sdata->vif.type == NL80211_IFTYPE_STATION &&
+ if ((sdata->vif.type == NL80211_IFTYPE_STATION ||
+ sdata->vif.type == NL80211_IFTYPE_ADHOC) &&
!test_sta_flag(sta, WLAN_STA_TDLS_PEER))
ieee80211_apply_htcap_overrides(sdata, &own_cap);
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index ea7b9c2..25e6998 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -179,8 +179,12 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
chandef.width != NL80211_CHAN_WIDTH_5 &&
chandef.width != NL80211_CHAN_WIDTH_10 &&
sband->ht_cap.ht_supported) {
- pos = ieee80211_ie_build_ht_cap(pos, &sband->ht_cap,
- sband->ht_cap.cap);
+ struct ieee80211_sta_ht_cap ht_cap;
+
+ memcpy(&ht_cap, &sband->ht_cap, sizeof(ht_cap));
+ ieee80211_apply_htcap_overrides(sdata, &ht_cap);
+
+ pos = ieee80211_ie_build_ht_cap(pos, &ht_cap, ht_cap.cap);
/*
* Note: According to 802.11n-2009 9.13.3.1, HT Protection
* field and RIFS Mode are reserved in IBSS mode, therefore
@@ -1051,6 +1055,11 @@ int ieee80211_ibss_join(struct ieee80211_sub_if_data *sdata,
memcpy(sdata->u.ibss.ssid, params->ssid, params->ssid_len);
sdata->u.ibss.ssid_len = params->ssid_len;
+ memcpy(&sdata->u.ibss.ht_capa, ¶ms->ht_capa,
+ sizeof(sdata->u.ibss.ht_capa));
+ memcpy(&sdata->u.ibss.ht_capa_mask, ¶ms->ht_capa_mask,
+ sizeof(sdata->u.ibss.ht_capa_mask));
+
/*
* 802.11n-2009 9.13.3.1: In an IBSS, the HT Protection field is
* reserved, but an HT STA shall protect HT transmissions as though
@@ -1131,6 +1140,11 @@ int ieee80211_ibss_leave(struct ieee80211_sub_if_data *sdata)
presp = rcu_dereference_protected(ifibss->presp,
lockdep_is_held(&sdata->wdev.mtx));
RCU_INIT_POINTER(sdata->u.ibss.presp, NULL);
+
+ /* on the next assoc, re-program HT parameters */
+ memset(&ifibss->ht_capa, 0, sizeof(ifibss->ht_capa));
+ memset(&ifibss->ht_capa_mask, 0, sizeof(ifibss->ht_capa_mask));
+
sdata->vif.bss_conf.ibss_joined = false;
sdata->vif.bss_conf.ibss_creator = false;
sdata->vif.bss_conf.enable_beacon = false;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8412a30..683751a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -509,6 +509,9 @@ struct ieee80211_if_ibss {
/* probe response/beacon for IBSS */
struct beacon_data __rcu *presp;
+ struct ieee80211_ht_cap ht_capa; /* configured ht-cap over-rides */
+ struct ieee80211_ht_cap ht_capa_mask; /* Valid parts of ht_capa */
+
spinlock_t incomplete_lock;
struct list_head incomplete_stations;
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] ath10k: minimally handle new channel width enumeration values
From: Kalle Valo @ 2013-06-28 7:10 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless, ath9k-devel
In-Reply-To: <1372355538-19284-1-git-send-email-linville@tuxdriver.com>
"John W. Linville" <linville@tuxdriver.com> writes:
> From: "John W. Linville" <linville@tuxdriver.com>
>
> CC drivers/net/wireless/ath/ath10k/mac.o
> drivers/net/wireless/ath/ath10k/mac.c: In function ‘chan_to_phymode’:
> drivers/net/wireless/ath/ath10k/mac.c:229:3: warning: enumeration value ‘NL80211_CHAN_WIDTH_5’ not handled in switch [-Wswitch]
> drivers/net/wireless/ath/ath10k/mac.c:229:3: warning: enumeration value ‘NL80211_CHAN_WIDTH_10’ not handled in switch [-Wswitch]
> drivers/net/wireless/ath/ath10k/mac.c:247:3: warning: enumeration value ‘NL80211_CHAN_WIDTH_5’ not handled in switch [-Wswitch]
> drivers/net/wireless/ath/ath10k/mac.c:247:3: warning: enumeration value ‘NL80211_CHAN_WIDTH_10’ not handled in switch [-Wswitch]
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
Looks good, thanks for fixing this while I'm on vacation. Strangely
enough we even had few warm days here in Finland! :)
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Nishanth Menon @ 2013-06-27 19:56 UTC (permalink / raw)
To: Luciano Coelho
Cc: grant.likely, rob.herring, devicetree-discuss, linux-doc, lkml,
linux-wireless, Tony Lindgren, linux-omap,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1372362418.21065.25.camel@cumari.coelho.fi>
On 06/27/2013 02:46 PM, Luciano Coelho wrote:
> On Thu, 2013-06-27 at 14:12 -0500, Nishanth Menon wrote:
[...]
>> Indexes to another entity is always a maintenance burden in the longer
>> run and needs judicious control. If it is possible to avoid it by
>> selecting the right parameters, I am a hard advocate for the same.
>
> I tend to agree. But you need a balance. In theory you're right. But
> I think if you take the real world example, it is over-engineering.
>
> Anyway, if you *really* think this needs to be changed, I think we're in
> a deadlock here and I'd like to hear other people's opinions. I don't
> mind making the change, but I'm still not convinced it is worth it,
> since it just adds complexity.
>
> And hey, this is too much bikeshedding for such a small detail.
Lol :)
Alrite, if no one else is complaining, I am not going to block it either.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-27 19:46 UTC (permalink / raw)
To: Nishanth Menon
Cc: grant.likely, rob.herring, devicetree-discuss, linux-doc, lkml,
linux-wireless, Tony Lindgren, linux-omap,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <51CC8E80.5030206@ti.com>
On Thu, 2013-06-27 at 14:12 -0500, Nishanth Menon wrote:
> On 06/27/2013 01:51 PM, Luciano Coelho wrote:
> > On Thu, 2013-06-27 at 08:39 -0500, Nishanth Menon wrote:
> >> On Thu, Jun 27, 2013 at 8:30 AM, Luciano Coelho <coelho@ti.com> wrote:
> >>> On Thu, 2013-06-27 at 08:23 -0500, Nishanth Menon wrote:
> >>>> On 06/27/2013 08:19 AM, Luciano Coelho wrote:
> >>>>> On Thu, 2013-06-27 at 08:15 -0500, Nishanth Menon wrote:
> >>>>>> On Thu, Jun 27, 2013 at 7:58 AM, Luciano Coelho <coelho@ti.com> wrote:
> >>>>>>> For the actual DTS files, I could add a wilink.dtsi with enumerations
> >>>>>>> for these values so they could be used in the node definitions. But I'm
> >>>>>>> not sure it's going to be that valuable in the end.
> >>>>>> The way GPIO HIGH was defined might help to provide guidance I think :)
> >>>>>
> >>>>> Where? As far as I can see, the GPIO flags are defined in a bitmap.
> >>>>
> >>>> include/dt-bindings/gpio/gpio.h
> >>>
> >>> Thanks! I don't see these macros used anywhere, though.
> >> umm... I'd think you have'nt looked deep enough / lists :)
> >
> > If you mean mailing lists, you're right, I didn't. I just did a git
> > grep for the macros and didn't find any users.
> git grep "GPIO_ACTIVE_[HIGH|LOW]" arch/arm/boot/dts/|wc -l
> 344
> on next-20130626. anyways, besides the point.
>
>
> >>> This seems to be a completely different thing. This is the header that
> >>> contains the helper functions to get GPIO-related device tree nodes,
> >>> isn't it?
> >> That is true, but it also contains the flag for level which needs to
> >> be in sync with the gpio.h dts header.
> >>>> just a hint. not saying frequencies were defined in header. for systems
> >>>> that define frequencies - example cpufreq OPPs, clock node usage, we do
> >>>> not use indexing to frequency, instead, that is the responsibility of
> >>>> driver to convert frequency back to required index.
> >>>> git grep frequency Documentation/devicetree/bindings gives you how the
> >>>> precedence looks like.
> >>>>
> >>>> Personally, if given a choice, I'd go with actual frequencies rather
> >>>> than indexes.
> >>>
> >>> If I do that, I need to add also a separate flag to define whether the
> >>> XTAL clock is used or not. For instance, we have 26MHz and 26MHz
> >>> crystal; and 38.4MHz and 38.4MHz crystal...
> >> Yes, you would have to. at the same time, it is easy for module maker
> >> to provide dtsi corresponding to exact h/w representation on his
> >> module using wilink chip without being worried about weird index value
> >> whose meaning is hidden in binding
> >
> > The module makers need to know about the bindings and read the document
> > before they specify the node in DTS. I think for clarity, a comment in
> > the DTS file stating the actual frequency is good enough. Simpler and
> > more efficient (in terms of DT binary size and module code size) than
> > adding the actual frequencies.
> >
> >
> >> On the flip side, It also allows driver to reject frequencies /
> >> configurations that are not supported by the corresponding chip.
> >
> > It's actually much easier to reject frequencies that are not valid with
> > the enumeration. "if (refclock > 5) { bail_out(); }". If I need to
> > check every frequency, I need to add an array of valid frequencies and
> > so on. Waste of code, IMHO.
> >
> >
> >> As I said, just my 2 cents. Personally, I'd like dts files to be as
> >> readable as c files without having to dig through bindings document.
> >
> > As I said before, for readability, adding a comment with the frequency
> > is good enough. This is what I did for PandaES's DTS (not sent out
> > yet):
> >
> > wlan {
> > compatible = "ti,wilink6";
> > interrupt-parent = <&gpio2>;
> > interrupts = <21 0x4>; /* gpio line 53, high level triggered */
> > refclock = <2>; /* 38.4 MHz */
> > };
> >
> > Looks more readable to me than:
> >
> > wlan {
> > compatible = "ti,wilink6";
> > interrupt-parent = <&gpio2>;
> > interrupts = <21 0x4>; /* gpio line 53, high level triggered */
> > refclock = <38400>;
> > refclock_xtal = <0>;
> > };
> >
> > The macro idea sounds better to me, but in this case this code is so
> > simple that I don't think it's really worth it.
> >
> > And, from another point of view, I see this as only a specification of
> > the module's details. The frequency value is not really used anywhere
> > outside the module, so we don't see it. I don't think there's a good
> > reason to expose the actual frequency to the kernel (and parse it back
> > to the values the firmware needs), since nothing else inside the kernel
> > will care about it.
> Overview: we are adding bindings for
> Documentation/devicetree/bindings/net/wireless/ti-wilink.txt Which I
> believe is intended to be generic.
>
> Current frequencies supported for tcxoclock is the following for WiLink7
> + 0 = 19.200 MHz
> + 1 = 26.000 MHz
> + 2 = 38.400 MHz
> + 3 = 52.000 MHz
> + 4 = 16.368 MHz
> + 5 = 32.736 MHz
> + 6 = 16.800 MHz
> + 7 = 33.600 MHz
> Say wilink9 comes along and redefines this map OR introduces support for
> 20MHz support making the map 0-8, you'd no longer be able to support
> this map. or say a new update of firmware magically changes this mapping
> or something unexpected.
No problem with adding 20MHz support. Look at 6 = 16.800 MHz. That's
already out of order, so why would 20MHz have to change the mapping?
It is true that stupid changes happen in the firmware from time to time.
But if it happens, the translation could still be done in the driver.
> If the translation and validation is done in the driver, it is trivial
> to handle without redefining the binding and breaking older dtbs (if
> relevant)
The validation can still be done in the driver. I don't think anything
has to break here. The bindings document is the de-facto specifications
of this stuff. If something in the lower layers (ie. firmware) breaks,
the driver can do the translation without having to change anything in
the DTS.
> Indexes to another entity is always a maintenance burden in the longer
> run and needs judicious control. If it is possible to avoid it by
> selecting the right parameters, I am a hard advocate for the same.
I tend to agree. But you need a balance. In theory you're right. But
I think if you take the real world example, it is over-engineering.
Anyway, if you *really* think this needs to be changed, I think we're in
a deadlock here and I'd like to hear other people's opinions. I don't
mind making the change, but I'm still not convinced it is worth it,
since it just adds complexity.
And hey, this is too much bikeshedding for such a small detail.
--
Luca.
^ permalink raw reply
* Re: Fwd: Re: TP-Link 8200ND - rtl8192cu module not loading / working
From: Xose Vazquez Perez @ 2013-06-27 19:45 UTC (permalink / raw)
To: Larry Finger; +Cc: shiki.biomernok, linux-wireless
In-Reply-To: <51CC5541.8010003@lwfinger.net>
On 06/27/2013 05:07 PM, Larry Finger wrote:
> On 06/27/2013 08:34 AM, Xose Vazquez Perez wrote:
>> But it looks like rtl8192cu wasn't tested enough with 2T2R(RTL8192CUS)
>> devices.
> The code in wireless-testing works fine on my Edimax EW-7811Un, which reports as an RTL8192CUS.
But Edimax EW-7811Un is a 1T1R device.
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Nishanth Menon @ 2013-06-27 19:12 UTC (permalink / raw)
To: Luciano Coelho
Cc: grant.likely, rob.herring, devicetree-discuss, linux-doc, lkml,
linux-wireless, Tony Lindgren, linux-omap,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1372359090.21065.12.camel@cumari.coelho.fi>
On 06/27/2013 01:51 PM, Luciano Coelho wrote:
> On Thu, 2013-06-27 at 08:39 -0500, Nishanth Menon wrote:
>> On Thu, Jun 27, 2013 at 8:30 AM, Luciano Coelho <coelho@ti.com> wrote:
>>> On Thu, 2013-06-27 at 08:23 -0500, Nishanth Menon wrote:
>>>> On 06/27/2013 08:19 AM, Luciano Coelho wrote:
>>>>> On Thu, 2013-06-27 at 08:15 -0500, Nishanth Menon wrote:
>>>>>> On Thu, Jun 27, 2013 at 7:58 AM, Luciano Coelho <coelho@ti.com> wrote:
>>>>>>> For the actual DTS files, I could add a wilink.dtsi with enumerations
>>>>>>> for these values so they could be used in the node definitions. But I'm
>>>>>>> not sure it's going to be that valuable in the end.
>>>>>> The way GPIO HIGH was defined might help to provide guidance I think :)
>>>>>
>>>>> Where? As far as I can see, the GPIO flags are defined in a bitmap.
>>>>
>>>> include/dt-bindings/gpio/gpio.h
>>>
>>> Thanks! I don't see these macros used anywhere, though.
>> umm... I'd think you have'nt looked deep enough / lists :)
>
> If you mean mailing lists, you're right, I didn't. I just did a git
> grep for the macros and didn't find any users.
git grep "GPIO_ACTIVE_[HIGH|LOW]" arch/arm/boot/dts/|wc -l
344
on next-20130626. anyways, besides the point.
>>> This seems to be a completely different thing. This is the header that
>>> contains the helper functions to get GPIO-related device tree nodes,
>>> isn't it?
>> That is true, but it also contains the flag for level which needs to
>> be in sync with the gpio.h dts header.
>>>> just a hint. not saying frequencies were defined in header. for systems
>>>> that define frequencies - example cpufreq OPPs, clock node usage, we do
>>>> not use indexing to frequency, instead, that is the responsibility of
>>>> driver to convert frequency back to required index.
>>>> git grep frequency Documentation/devicetree/bindings gives you how the
>>>> precedence looks like.
>>>>
>>>> Personally, if given a choice, I'd go with actual frequencies rather
>>>> than indexes.
>>>
>>> If I do that, I need to add also a separate flag to define whether the
>>> XTAL clock is used or not. For instance, we have 26MHz and 26MHz
>>> crystal; and 38.4MHz and 38.4MHz crystal...
>> Yes, you would have to. at the same time, it is easy for module maker
>> to provide dtsi corresponding to exact h/w representation on his
>> module using wilink chip without being worried about weird index value
>> whose meaning is hidden in binding
>
> The module makers need to know about the bindings and read the document
> before they specify the node in DTS. I think for clarity, a comment in
> the DTS file stating the actual frequency is good enough. Simpler and
> more efficient (in terms of DT binary size and module code size) than
> adding the actual frequencies.
>
>
>> On the flip side, It also allows driver to reject frequencies /
>> configurations that are not supported by the corresponding chip.
>
> It's actually much easier to reject frequencies that are not valid with
> the enumeration. "if (refclock > 5) { bail_out(); }". If I need to
> check every frequency, I need to add an array of valid frequencies and
> so on. Waste of code, IMHO.
>
>
>> As I said, just my 2 cents. Personally, I'd like dts files to be as
>> readable as c files without having to dig through bindings document.
>
> As I said before, for readability, adding a comment with the frequency
> is good enough. This is what I did for PandaES's DTS (not sent out
> yet):
>
> wlan {
> compatible = "ti,wilink6";
> interrupt-parent = <&gpio2>;
> interrupts = <21 0x4>; /* gpio line 53, high level triggered */
> refclock = <2>; /* 38.4 MHz */
> };
>
> Looks more readable to me than:
>
> wlan {
> compatible = "ti,wilink6";
> interrupt-parent = <&gpio2>;
> interrupts = <21 0x4>; /* gpio line 53, high level triggered */
> refclock = <38400>;
> refclock_xtal = <0>;
> };
>
> The macro idea sounds better to me, but in this case this code is so
> simple that I don't think it's really worth it.
>
> And, from another point of view, I see this as only a specification of
> the module's details. The frequency value is not really used anywhere
> outside the module, so we don't see it. I don't think there's a good
> reason to expose the actual frequency to the kernel (and parse it back
> to the values the firmware needs), since nothing else inside the kernel
> will care about it.
Overview: we are adding bindings for
Documentation/devicetree/bindings/net/wireless/ti-wilink.txt Which I
believe is intended to be generic.
Current frequencies supported for tcxoclock is the following for WiLink7
+ 0 = 19.200 MHz
+ 1 = 26.000 MHz
+ 2 = 38.400 MHz
+ 3 = 52.000 MHz
+ 4 = 16.368 MHz
+ 5 = 32.736 MHz
+ 6 = 16.800 MHz
+ 7 = 33.600 MHz
Say wilink9 comes along and redefines this map OR introduces support for
20MHz support making the map 0-8, you'd no longer be able to support
this map. or say a new update of firmware magically changes this mapping
or something unexpected.
If the translation and validation is done in the driver, it is trivial
to handle without redefining the binding and breaking older dtbs (if
relevant)
Indexes to another entity is always a maintenance burden in the longer
run and needs judicious control. If it is possible to avoid it by
selecting the right parameters, I am a hard advocate for the same.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
From: Luciano Coelho @ 2013-06-27 18:51 UTC (permalink / raw)
To: Nishanth Menon
Cc: grant.likely, rob.herring, devicetree-discuss, linux-doc, lkml,
linux-wireless, Tony Lindgren, linux-omap,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAGo_u6pWZX9gbHgBae=gcWk8i6-TnhxH1TdM9QdEDLTow6Pn5A@mail.gmail.com>
On Thu, 2013-06-27 at 08:39 -0500, Nishanth Menon wrote:
> On Thu, Jun 27, 2013 at 8:30 AM, Luciano Coelho <coelho@ti.com> wrote:
> > On Thu, 2013-06-27 at 08:23 -0500, Nishanth Menon wrote:
> >> On 06/27/2013 08:19 AM, Luciano Coelho wrote:
> >> > On Thu, 2013-06-27 at 08:15 -0500, Nishanth Menon wrote:
> >> >> On Thu, Jun 27, 2013 at 7:58 AM, Luciano Coelho <coelho@ti.com> wrote:
> >> >>> For the actual DTS files, I could add a wilink.dtsi with enumerations
> >> >>> for these values so they could be used in the node definitions. But I'm
> >> >>> not sure it's going to be that valuable in the end.
> >> >> The way GPIO HIGH was defined might help to provide guidance I think :)
> >> >
> >> > Where? As far as I can see, the GPIO flags are defined in a bitmap.
> >>
> >> include/dt-bindings/gpio/gpio.h
> >
> > Thanks! I don't see these macros used anywhere, though.
> umm... I'd think you have'nt looked deep enough / lists :)
If you mean mailing lists, you're right, I didn't. I just did a git
grep for the macros and didn't find any users.
> >> And corresponding kernel header:
> >> include/linux/of_gpio.h
> >
> > This seems to be a completely different thing. This is the header that
> > contains the helper functions to get GPIO-related device tree nodes,
> > isn't it?
> That is true, but it also contains the flag for level which needs to
> be in sync with the gpio.h dts header.
> >> just a hint. not saying frequencies were defined in header. for systems
> >> that define frequencies - example cpufreq OPPs, clock node usage, we do
> >> not use indexing to frequency, instead, that is the responsibility of
> >> driver to convert frequency back to required index.
> >> git grep frequency Documentation/devicetree/bindings gives you how the
> >> precedence looks like.
> >>
> >> Personally, if given a choice, I'd go with actual frequencies rather
> >> than indexes.
> >
> > If I do that, I need to add also a separate flag to define whether the
> > XTAL clock is used or not. For instance, we have 26MHz and 26MHz
> > crystal; and 38.4MHz and 38.4MHz crystal...
> Yes, you would have to. at the same time, it is easy for module maker
> to provide dtsi corresponding to exact h/w representation on his
> module using wilink chip without being worried about weird index value
> whose meaning is hidden in binding
The module makers need to know about the bindings and read the document
before they specify the node in DTS. I think for clarity, a comment in
the DTS file stating the actual frequency is good enough. Simpler and
more efficient (in terms of DT binary size and module code size) than
adding the actual frequencies.
> On the flip side, It also allows driver to reject frequencies /
> configurations that are not supported by the corresponding chip.
It's actually much easier to reject frequencies that are not valid with
the enumeration. "if (refclock > 5) { bail_out(); }". If I need to
check every frequency, I need to add an array of valid frequencies and
so on. Waste of code, IMHO.
> As I said, just my 2 cents. Personally, I'd like dts files to be as
> readable as c files without having to dig through bindings document.
As I said before, for readability, adding a comment with the frequency
is good enough. This is what I did for PandaES's DTS (not sent out
yet):
wlan {
compatible = "ti,wilink6";
interrupt-parent = <&gpio2>;
interrupts = <21 0x4>; /* gpio line 53, high level triggered */
refclock = <2>; /* 38.4 MHz */
};
Looks more readable to me than:
wlan {
compatible = "ti,wilink6";
interrupt-parent = <&gpio2>;
interrupts = <21 0x4>; /* gpio line 53, high level triggered */
refclock = <38400>;
refclock_xtal = <0>;
};
The macro idea sounds better to me, but in this case this code is so
simple that I don't think it's really worth it.
And, from another point of view, I see this as only a specification of
the module's details. The frequency value is not really used anywhere
outside the module, so we don't see it. I don't think there's a good
reason to expose the actual frequency to the kernel (and parse it back
to the values the firmware needs), since nothing else inside the kernel
will care about it.
--
Luca.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox