Re: [PATCH -next] soc: ixp4xx: qmgr: Use DEFINE_SPINLOCK() for spinlock
Yang Yingliang writes: > spinlock can be initialized automatically with DEFINE_SPINLOCK() > rather than explicitly calling spin_lock_init(). That's right. > Reported-by: Hulk Robot > Signed-off-by: Yang Yingliang Acked-by: Krzysztof Halasa > --- > drivers/soc/ixp4xx/ixp4xx-qmgr.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/soc/ixp4xx/ixp4xx-qmgr.c > b/drivers/soc/ixp4xx/ixp4xx-qmgr.c > index 8c968382cea7..dde3b668eb40 100644 > --- a/drivers/soc/ixp4xx/ixp4xx-qmgr.c > +++ b/drivers/soc/ixp4xx/ixp4xx-qmgr.c > @@ -16,7 +16,7 @@ > static struct qmgr_regs __iomem *qmgr_regs; > static int qmgr_irq_1; > static int qmgr_irq_2; > -static spinlock_t qmgr_lock; > +static DEFINE_SPINLOCK(qmgr_lock); > static u32 used_sram_bitmap[4]; /* 128 16-dword pages */ > static void (*irq_handlers[QUEUES])(void *pdev); > static void *irq_pdevs[QUEUES]; > @@ -434,7 +434,6 @@ static int ixp4xx_qmgr_probe(struct platform_device *pdev) > } > > used_sram_bitmap[0] = 0xF; /* 4 first pages reserved for config */ > - spin_lock_init(&qmgr_lock); > > dev_info(dev, "IXP4xx Queue Manager initialized.\n"); > return 0; -- Krzysztof Hałasa Sieć Badawcza Łukasiewicz Przemysłowy Instytut Automatyki i Pomiarów PIAP Al. Jerozolimskie 202, 02-486 Warszawa
Re: RFC: MEDIA: Driver for ON Semi AR0521 camera sensor
Laurent,
There is an additional issue:
>> +static const struct v4l2_subdev_video_ops ar0521_video_ops = {
>> +.g_frame_interval = ar0521_g_frame_interval,
>> +.s_frame_interval = ar0521_s_frame_interval,
>
> For raw sensors, frame intervals should be configured using
> V4L2_CID_HBLANK and V4L2_CID_VBLANK instead. These two operations should
> be dropped.
Unfortunately, I require a way to specify an exact timings for the
sensor. The frame interval gives me a way to do this, but it requires
setting HBLANK and VBLANK _and_ an extra register
(AR0521_REG_EXTRA_DELAY) which is specified in pixels, not lines.
E.g. the total number of pixels (active or otherwise) is not
(width + h blank) * (heigth + v blank)
but rather:
(width + h blank) * (heigth + v blank) + extra_delay
How do I do that with HBLANK/VBLANK controls?
BTW there are specific constraints on the pixel clock as well
(the active scanning must be as fast as possible, which means fast pixel
clock, minimum possible hblank and thus long vblank).
All comments are appreciated.
--
Krzysztof Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Re: RFC: dt-binding: media: document ON Semi AR0521 sensor bindings
Laurent Pinchart writes:
>> + reg:
>> +description: I2C bus address of the sensor device
>
> You can drop this, it's implicit for I2C devices.
Do you mean just dropping these two lines (and MaxItems: 1), and leaving
"reg" in "required" and in the example? E.g.:
...
required:
- compatible
- reg
- clocks
- clock-names
- port
additionalProperties: false
examples:
- |
#include
#include
i2c {
#address-cells = <1>;
#size-cells = <0>;
ar0521: camera-sensor@36 {
compatible = "onnn,ar0521";
reg = <0x36>;
pinctrl-names = "default";
...
It protests with:
Documentation/devicetree/bindings/media/i2c/onnn,ar0521.example.dt.yaml:
camera-sensor@36: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema:
/usr/src/linux/imx6/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
Thus I'm currently leaving it as is.
--
Krzysztof Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Re: RFC: MEDIA: Driver for ON Semi AR0521 camera sensor
Laurent Pinchart writes:
>> Is there a reliable way to include national unicode characters in the
>> kernel sources?
>
> It depends where. In comments it shouldn't be a problem. In C code, I
> don't think the compiler will be too happy.
I meant in comments, sure. And in stuff like MODULE_AUTHOR.
I know gcc will handle it correctly, but the problem is getting
the unicode characters right through mail.
> Signed-off-by means that you have the right to submit the code for
> upstreaming, so it should be included in patches under review too.
> Otherwise it's a waste of time for reviewers to review something that
> may never be resubmitted with an SoB line.
I know. I obviously have rights to upstream this code. The problem is
when I publish a patch with a SOB line, anyone can take it, "make
a derivative work" (so to speak), and submit as his own. The new patch
doesn't need to be an improvement, the changes from the original are not
even looked upon. Been there BTW.
>> +#define AR0521_WIDTH_MIN 8u
>
> We usually use an uppercase U suffix.
>> +#define AR0521_REG_RESET0x301A
>
> But lowercase hex values. I know, lots of tribal (and sometimes
> arbitrary) knowledge :-S
Right. Is there a consensus about it?
I use lowercase U because it contrasts with "uppercase" digits, and
uppercase hex letter for consistency with (decimal) digits, but I can
change it if it's only me.
>> +regs[0] = AR0521_REG_GREEN1_GAIN;
>> +regs[1] = green << 7 | analog;
>> +regs[2] = blue << 7 | analog;
>> +regs[3] = red << 7 | analog;
>> +regs[4] = green << 7 | analog;
>> +
>> +return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
>
> Passing the values in an array, with the first entry being a register
> address, is a really weird API. I'd recommend either using regmap (may
> be overkill here), or use a write function that takes the register
> address and value as separate arguments. If we want to avoid sending the
> register address for each write as a performance improvement, we'll have
> to figure out what a good API would be to do so, but more importantly,
> it would be good to have numbers to justify why this would be needed.
It's a slow I^2C device. Doing a single write transfer is faster than
a series of transfers, especially on a busy bus. Do I really have to
justify why I like a faster and more efficient code?
And it's not a big API, it's just a small internal driver subroutine.
Would splitting it to several ar0521_write_reg() be better, e.g. more
readable?
>> +static int ar0521_set_mode(struct ar0521_dev *sensor)
>> +{
>> +unsigned int speed_mod = 4 / lanes(sensor); /* 1 with 4 DDR lanes */
>> +u64 pix_clk;
>> +u32 pixels, pll2, num, denom, new_total_height, new_pixels;
>> +u16 total_width, total_height, x, y, pre, mult, pre2, mult2,
>> extra_delay;
>> +u16 regs[9];
>> +int ret;
>> +
>> +/* stop streaming first */
>> +ret = ar0521_write_reg(sensor, AR0521_REG_RESET,
>> AR0521_REG_RESET_DEFAULTS);
>
> set_format isn't supposed to stop streaming implicitly. It should
> instead return -EBUSY if the stream if running.
It doesn't stop permanently, it's restarted after the registers are
updated. No need for -EBUSY here.
>> +static int ar0521_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +int ret;
>> +
>> +mutex_lock(&sensor->lock);
>
> Could you please use runtime PM for power management, enabling the clock
> and regulators when starting streaming ?
>
> I forgot to mention in the review of the DT bindings that regulators
> should be specified in DT.
Why? The hw using this driver doesn't have capability to disable
regulators. If someone produces hw with means to control power, the sw
support can be trivially added. When I last checked, we didn't add
driver code for functionality for which no hw support exists, did we?
--
Krzysztof Hałasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Re: RFC: dt-binding: media: document ON Semi AR0521 sensor bindings
Laurent, Laurent Pinchart writes: >> +const: on-semi,ar0521 > > That's not the correct prefix for ON Semiconductor. See > Documentation/devicetree/bindings/vendor-prefixes.yaml. Or just the name > of this file :-) Right, just missed this one. Thanks for the comments. -- Krzysztof Halasa Sieć Badawcza Łukasiewicz Przemysłowy Instytut Automatyki i Pomiarów PIAP Al. Jerozolimskie 202, 02-486 Warszawa
RFC: MEDIA: Driver for ON Semi AR0521 camera sensor
Is there a reliable way to include national unicode characters in the
kernel sources?
For review only. This will be signed off when (if) the driver is
accepted.
diff --git a/MAINTAINERS b/MAINTAINERS
index bfc1b86e3e73..20514c00909b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1294,6 +1294,12 @@ S: Supported
W: http://www.aquantia.com
F: drivers/net/ethernet/aquantia/atlantic/aq_ptp*
+AR0521 ON SEMICONDUCTOR CAMERA SENSOR DRIVER
+M: Krzysztof Halasa
+L: linux-me...@vger.kernel.org
+S: Maintained
+F: drivers/media/i2c/ar0521.c
+
ARASAN NAND CONTROLLER DRIVER
M: Naga Sureshkumar Relli
L: linux-...@lists.infradead.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 2b9d81e4794a..b212af488b17 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -725,6 +725,16 @@ config VIDEO_APTINA_PLL
config VIDEO_CCS_PLL
tristate
+config VIDEO_AR0521
+ tristate "ON Semiconductor AR0521 sensor support"
+ depends on I2C && VIDEO_V4L2
+ help
+ This is a Video4Linux2 sensor driver for the ON Semiconductor
+ AR0521 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ar0521.
+
config VIDEO_HI556
tristate "Hynix Hi-556 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index a3149dce21bb..4b21beb95147 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_VIDEO_CX25840) += cx25840/
obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
+obj-$(CONFIG_VIDEO_AR0521) += ar0521.o
obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
new file mode 100644
index ..395264f1a558
--- /dev/null
+++ b/drivers/media/i2c/ar0521.c
@@ -0,0 +1,1087 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Siec Badawcza Lukasiewicz
+ * - Przemyslowy Instytut Automatyki i Pomiarow PIAP
+ * Written by Krzysztof Halasa
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* External clock (xclk) frequencies */
+#define AR0521_XCLK_RATE (27 * 1000 * 1000)
+#define AR0521_XCLK_MIN (10 * 1000 * 1000)
+#define AR0521_XCLK_MAX (48 * 1000 * 1000)
+
+/* PLL and PLL2 */
+#define AR0521_PLL_MIN (320 * 1000 * 1000)
+#define AR0521_PLL_MAX (1280 * 1000 * 1000)
+
+/* effective pixel clocks, the registers may be DDR */
+#define AR0521_PIXEL_CLOCK_MIN (168 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_MAX (414 * 1000 * 1000)
+
+#define AR0521_WIDTH_MIN 8u
+#define AR0521_WIDTH_MAX 2608u
+#define AR0521_HEIGHT_MIN 8u
+#define AR0521_HEIGHT_MAX 1958u
+
+// FIXME check them
+#define AR0521_WIDTH_BLANKING_MIN 572u
+#define AR0521_HEIGHT_BLANKING_MIN 28u /* must be even */
+#define AR0521_TOTAL_WIDTH_MIN 2968u
+
+/* AR0521 registers */
+#define AR0521_REG_VT_PIX_CLK_DIV 0x0300
+#define AR0521_REG_FRAME_LENGTH_LINES 0x0340
+
+#define AR0521_REG_CHIP_ID 0x3000
+#define AR0521_REG_COARSE_INTEGRATION_TIME 0x3012
+#define AR0521_REG_ROW_SPEED 0x3016
+#define AR0521_REG_EXTRA_DELAY 0x3018
+#define AR0521_REG_RESET 0x301A
+#define AR0521_REG_RESET_DEFAULTS 0x0238
+#define AR0521_REG_RESET_GROUP_PARAM_HOLD 0x8000
+#define AR0521_REG_RESET_STREAMBIT(2)
+#define AR0521_REG_RESET_RESTART BIT(1)
+#define AR0521_REG_RESET_INIT BIT(0)
+
+#define AR0521_REG_GREEN1_GAIN 0x3056
+#define AR0521_REG_BLUE_GAIN 0x3058
+#define AR0521_REG_RED_GAIN0x305A
+#define AR0521_REG_GREEN2_GAIN 0x305C
+#define AR0521_REG_GLOBAL_GAIN 0x305E
+
+#define AR0521_REG_HISPI_TEST_MODE 0x3066
+#define AR0521_REG_HISPI_TEST_MODE_LP11 0x0004
+
+#define AR0521_REG_TEST_PATTERN_MODE 0x3070
+
+#define AR0521_REG_SERIAL_FORMAT 0x31AE
+#define AR0521_REG_SERIAL_FORMAT_MIPI0x0200
+
+#define AR0521_REG_HISPI_CONTROL_STATUS0x31C6
+#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80
+
+#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_name)
+
+struct ar0521_ctrls {
+ struct v4l2_ctrl_handler handler;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *gain, *red_balance, *blue_balance;
+ struct v4l2_ctrl *test_pattern;
+};
+
+struct ar0
RFC: dt-binding: media: document ON Semi AR0521 sensor bindings
This file documents DT bindings for the AR0521 camera sensor driver.
Signed-off-by: Krzysztof Halasa
diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
new file mode 100644
index ..f649d4cbcb37
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ON Semiconductor AR0521 MIPI CSI-2 sensor
+
+maintainers:
+ - Krzysztof Halasa
+
+description: |-
+ The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
+ I2C-compatible control interface.
+
+properties:
+ compatible:
+const: on-semi,ar0521
+
+ reg:
+description: I2C bus address of the sensor device
+maxItems: 1
+
+ clocks:
+description: reference to the xclk clock
+maxItems: 1
+
+ clock-names:
+const: xclk
+
+ reset-gpios:
+description: active low reset GPIO
+maxItems: 1
+
+ port:
+type: object
+description: |
+ Output video port: 1, 2 or 4 lanes. See ../video-interfaces.txt.
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+#include
+
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+ar0521: camera-sensor@36 {
+compatible = "onnn,ar0521";
+reg = <0x36>;
+pinctrl-names = "default";
+pinctrl-0 = <&pinctrl_mipi_camera>;
+
+clocks = <&clks IMX6QDL_CLK_CKO>;
+clock-names = "xclk";
+
+reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
+
+port {
+ mipi_camera_to_mipi_csi2: endpoint {
+remote-endpoint = <&mipi_csi2_in>;
+data-lanes = <1 2 3 4>;
+};
+};
+};
+};
--
Krzysztof Halasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Re: [v2] Old platforms: bring out your dead
Arnd, Arnd Bergmann writes: > For these I received no reply yet. Again, these will stay for the moment > unless I get a reply, but if anyone has more information, please reply > here to document the status (adding a few more people to Cc): > > * cns3xxx -- added in 2010, last fixed in 2019, probably no users left The following is what I sent to you a week ago. I don't say whether CNS3xxx support should stay or not, of course. Subject: Re: cns3xxx PCIe domain support Arnd Bergmann writes: > For the cns3xxx case, I wonder if anyone actually cares. If > there are still users, the treewide change would make it trivial > to set it up right, while backporting would be harder. I noticed > that openwrt removed cns3xxx support in August with the > explanation that the platform is not used much anymore, > and I suspect that any users outside of openwrt stopped updating > their kernels long ago. I'm still using CNS3xxx-based Gateworks' boards (Laguna), with some custom patch set, but the last kernels are over 2 years old. I have some plan to update, but the probability it will happen very soon is rather low. I guess I will test and, if needed, fix it when the time comes. I'm not using them with OpenWrt, though. They are basically a platform for (the old, parallel, not express) mini-PCI cards and similar stuff. Nothing connected to the Internet etc. -- Krzysztof Halasa Sieć Badawcza Łukasiewicz Przemysłowy Instytut Automatyki i Pomiarów PIAP Al. Jerozolimskie 202, 02-486 Warszawa
Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices
Xie He writes: > The HDLC device is not actually prepending any header when it is used > with this driver. When the PVC device has prepended its header and > handed over the skb to the HDLC device, the HDLC device just hands it > over to the hardware driver for transmission without prepending any > header. That's correct. IIRC: - Cisco and PPP modes add 4 bytes - Frame Relay adds 4 (specific protocols - mostly IPv4) or 10 (general case) bytes. There is that pvcX->hdlcX transition which adds nothing (the header is already in place when the packet leaves pvcX device). - Raw mode adds nothing (IPv4 only, though it could be modified for both IPv4/v6 easily) - Ethernet (hdlc_raw_eth.c) adds normal Ethernet header. (I had been "unplugged" for some time). -- Krzysztof Halasa Sieć Badawcza Łukasiewicz Przemysłowy Instytut Automatyki i Pomiarów PIAP Al. Jerozolimskie 202, 02-486 Warszawa
Re: [PATCH net] drivers/net/wan/hdlc_cisco: Add hard_header_len
Hello Xie,
Xie He writes:
> This driver didn't set hard_header_len. This patch sets hard_header_len
> for it according to its header_ops->create function.
BTW it's 4 bytes long:
struct hdlc_header {
u8 address;
u8 control;
__be16 protocol;
}__packed;
OTOH hdlc_setup_dev() initializes hard_header_len to 16,
but in this case I guess 4 bytes are better.
Acked-by: Krzysztof Halasa
> Cc: Martin Schiller
> Signed-off-by: Xie He
> ---
> --- a/drivers/net/wan/hdlc_cisco.c
> +++ b/drivers/net/wan/hdlc_cisco.c
> @@ -370,6 +370,7 @@ static int cisco_ioctl(struct net_device *dev, struct
> ifreq *ifr)
> memcpy(&state(hdlc)->settings, &new_settings, size);
> spin_lock_init(&state(hdlc)->lock);
> dev->header_ops = &cisco_header_ops;
> + dev->hard_header_len = sizeof(struct hdlc_header);
> dev->type = ARPHRD_CISCO;
> call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
> netif_dormant_on(dev);
--
Krzysztof Halasa
Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
[PATCH] 802.11n IBSS: wlan0 stops receiving packets due to aggregation after sender reboot
Fix a bug where the mac80211 RX aggregation code sets a new aggregation
"session" at the remote station's request, but the head_seq_num
(the sequence number the receiver expects to receive) isn't reset.
Spotted on a pair of AR9580 in IBSS mode.
Signed-off-by: Krzysztof Halasa
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 4d1c335e06e5..775a51cc51c9 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -354,9 +354,11 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
*/
rcu_read_lock();
tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
- if (tid_rx && tid_rx->timeout == timeout)
+ if (tid_rx && tid_rx->timeout == timeout) {
+ tid_rx->ssn = start_seq_num;
+ tid_rx->head_seq_num = start_seq_num;
status = WLAN_STATUS_SUCCESS;
- else
+ } else
status = WLAN_STATUS_REQUEST_DECLINED;
rcu_read_unlock();
goto end;
--
Krzysztof Hałasa
ŁUKASIEWICZ Research Network
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
802.11n IBSS: wlan0 stops receiving packets due to aggregation after sender reboot
Johannes,
it seems I've encountered a bug in mac80211 RX aggregation handler.
The hw is a pair of stations using AR9580 (PCI ID 168c:0033) PCIe
adapters. Linux 5.4-rc4.
The driver shows the chip is Atheros AR9300 Rev:4.
I'm using (on both ends):
iw wlan0 set type ibss
ip link set wlan0 up
iw dev wlan0 ibss join $ESSID $FREQ HT20
The problem manifests itself after one of the stations is restarted
(or the ath9k driver is reloaded, or a station is out of range for
some time etc).
It appears that the mac80211 RX aggregation code sets a new aggregation
"session" at the remote station's request, but the head_seq_num
(the sequence number the receiver expects to receive) isn't reset.
I've added some debugging code to ___ieee80211_start_rx_ba_session()
and ieee80211_sta_manage_reorder_buf() and it produced the following:
Both stations boot and join the IBSS, packets get through:
[ 61.123131] AGG RX OK: ssn 1
[ 61.125346] SEQ OK: 1 vs 1
[ 61.125484] SEQ OK: 2 vs 2
[ 62.100841] SEQ OK: 3 vs 3
...
[ 180.124210] SEQ OK: 130 vs 130
[ 181.123888] SEQ OK: 131 vs 131
[ 182.126046] SEQ OK: 132 vs 132
Now I'm rebooting the remote station. It joins IBSS, packets can be seen
on mon0 monitoring interface (on the local station), but they aren't
arriving on wlan0:
[ 192.131102] SEQ BAD: 0 vs 133
[ 192.151243] AGG RX no change - OK: ssn 1
[ 192.242760] SEQ BAD: 1 vs 133
[ 193.133819] SEQ BAD: 2 vs 133
[ 193.272802] SEQ BAD: 3 vs 133
...
[ 421.272374] SEQ BAD: 130 vs 133
[ 421.303630] SEQ BAD: 131 vs 133
[ 422.327924] SEQ BAD: 132 vs 133
Then the sequence number catches up and the communication is
reestablished:
[ 423.167023] SEQ OK: 133 vs 133
[ 423.169061] SEQ OK: 134 vs 134
[ 423.351618] SEQ OK: 135 vs 135
I'll attach a patch in a separate mail but I'm not sure if it's
the optimal fix - one packet (the "SEQ BAD: 0 vs 133) is still dropped,
and I guess it won't work if the sender decides to not request
aggregation anymore.
Comments?
The debugging code:
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -354,9 +354,10 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
*/
rcu_read_lock();
tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
- if (tid_rx && tid_rx->timeout == timeout)
+ if (tid_rx && tid_rx->timeout == timeout) {
status = WLAN_STATUS_SUCCESS;
- else
+ printk(KERN_DEBUG "AGG RX no change - OK: ssn
%u\n", start_seq_num);
+ } else
status = WLAN_STATUS_REQUEST_DECLINED;
rcu_read_unlock();
goto end;
@@ -434,6 +437,7 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
tid_agg_rx->tid = tid;
tid_agg_rx->sta = sta;
status = WLAN_STATUS_SUCCESS;
+ printk(KERN_DEBUG "AGG RX OK: ssn %u\n", start_seq_num);
/* activate it for RX */
rcu_assign_pointer(sta->ampdu_mlme.tid_rx[tid], tid_agg_rx);
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1298,9 +1298,11 @@ static bool ieee80211_sta_manage_reorder_buf(struct
ieee80211_sub_if_data *sdata
/* frame with out of date sequence number */
if (ieee80211_sn_less(mpdu_seq_num, head_seq_num)) {
+ printk(KERN_DEBUG "SEQ BAD: %u vs %u\n", mpdu_seq_num,
head_seq_num);
dev_kfree_skb(skb);
goto out;
- }
+ } else
+ printk(KERN_DEBUG "SEQ OK: %u vs %u\n", mpdu_seq_num,
head_seq_num);
/*
* If frame the sequence number exceeds our buffering window
--
Krzysztof Hałasa
ŁUKASIEWICZ Research Network
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] 802.11n IBSS: wlan0 stops receiving packets due to aggregation after sender reboot
Fix a bug where the mac80211 RX aggregation code sets a new aggregation
"session" at the remote station's request, but the head_seq_num
(the sequence number the receiver expects to receive) isn't reset.
Spotted on a pair of AR9580 in IBSS mode.
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 4d1c335e06e5..775a51cc51c9 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -354,9 +354,11 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
*/
rcu_read_lock();
tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
- if (tid_rx && tid_rx->timeout == timeout)
+ if (tid_rx && tid_rx->timeout == timeout) {
+ tid_rx->ssn = start_seq_num;
+ tid_rx->head_seq_num = start_seq_num;
status = WLAN_STATUS_SUCCESS;
- else
+ } else
status = WLAN_STATUS_REQUEST_DECLINED;
rcu_read_unlock();
goto end;
--
Krzysztof Hałasa
ŁUKASIEWICZ Research Network
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] arch: arm: Kconfig: pedantic formatting
"Enrico Weigelt, metux IT consult" writes: > Formatting of Kconfig files doesn't look so pretty, so let the > Great White Handkerchief come around and clean it up. > > Signed-off-by: Enrico Weigelt, metux IT consult > --- > arch/arm/mach-ixp4xx/Kconfig | 32 ++--- Acked-by: Krzysztof Halasa Though I guess things like this one could go straight to "trivial" without all these Acked overkill. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
90ad2cbe88c22d0215225ab9594eeead0eb24fde changed the i.MX I2C bus driver
to use a notifier whenever the base clock ("ipg" - 66 MHz peripheral
clock) rate changes.
Unfortunately one can't use the container_of() macro this way - the
first argument has to point to a member of the bigger struct (last
argument). Merely pointing to the same value isn't enough (the clk
variable which has its address passed to the macro is the clk in
notifier_block, not the one in imx_i2c_struct, even though both pointers
point to the same clk struct).
This bug causes kernel panic when the IPG clock rate changes (e.g. if
any clock derived from IPG changes).
Signed-off-by: Krzysztof Halasa
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block
*nb,
unsigned long action, void *data)
{
struct clk_notifier_data *ndata = data;
- struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+ struct imx_i2c_struct *i2c_imx = container_of(nb,
struct imx_i2c_struct,
- clk);
+ clk_change_nb);
if (action & POST_RATE_CHANGE)
i2c_imx_set_clk(i2c_imx, ndata->new_rate);
Re: [PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
Hi Fabio,
Fabio Estevam writes:
> Please provide a commit log, giving some context to your fix.
Well, I hope Lucas could add something here. I am uncertain how it was
supposed to work, the ndata->clk (the pointer, not the clk pointed by
it) can't be at the same time a member of imx_i2c_struct, and I believe
the macro only does simple arithmetics to get to the outer struct.
@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block
*nb,
unsigned long action, void *data)
{
struct clk_notifier_data *ndata = data;
- struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+ struct imx_i2c_struct *i2c_imx = container_of(nb,
struct imx_i2c_struct,
- clk);
+ clk_change_nb);
> Is this a regression?
Probably (it went in between 4.16 and 4.17, commit id is
90ad2cbe88c22d0215225ab9594eeead0eb24fde). However this part may be
unused on many boards (apparently it only fires up if the "IPG" clock
rate changes), so it may not manifest itself. I only hit it when I added
a custom driver (using/requesting a special clock derived from IPG).
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] ARM i.MX: Fix a kernel panic in i2c_imx_clk_notifier_call().
Signed-off-by: Krzysztof Halasa
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -510,9 +510,9 @@ static int i2c_imx_clk_notifier_call(struct notifier_block
*nb,
unsigned long action, void *data)
{
struct clk_notifier_data *ndata = data;
- struct imx_i2c_struct *i2c_imx = container_of(&ndata->clk,
+ struct imx_i2c_struct *i2c_imx = container_of(nb,
struct imx_i2c_struct,
- clk);
+ clk_change_nb);
if (action & POST_RATE_CHANGE)
i2c_imx_set_clk(i2c_imx, ndata->new_rate);
Re: [PATCH 2/2] ARM: ixp4xx: include linux/mtd/platnand.h
Arnd Bergmann writes:
> The platform_nand_data structure definition is required for ixdp425
> but has now moved to a new header file:
>
> arch/arm/mach-ixp4xx/ixdp425-setup.c:98:15: error: variable
> 'ixdp425_flash_nand_data' has initializer but incomplete type
> static struct platform_nand_data ixdp425_flash_nand_data = {
>^~
> arch/arm/mach-ixp4xx/ixdp425-setup.c:99:3: error: 'struct platform_nand_data'
> has no member named 'chip'
>
> Fixes: 19cf5dfa3a31 ("mtd: rawnand: Move platform_nand_xxx definitions out of
> rawnand.h")
> Signed-off-by: Arnd Bergmann
> ---
> arch/arm/mach-ixp4xx/ixdp425-setup.c | 1 +
Right.
Acked-by: Krzysztof Halasa
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 01/12] ARM: ixp4xx: fix ioport_unmap definition
Arnd Bergmann writes: > An empty macro definition can cause unexpected behavior, in > case of the ixp4xx ioport_unmap, we get two warnings: > > drivers/net/wireless/marvell/libertas/if_cs.c: In function 'if_cs_release': > drivers/net/wireless/marvell/libertas/if_cs.c:826:3: error: suggest braces > around empty body in an 'if' statement [-Werror=empty-body] >ioport_unmap(card->iobase); > drivers/vfio/pci/vfio_pci_rdwr.c: In function 'vfio_pci_vga_rw': > drivers/vfio/pci/vfio_pci_rdwr.c:230:15: error: the omitted middle operand in > ?: will always be 'true', suggest explicit middle operand > [-Werror=parentheses] >is_ioport ? ioport_unmap(iomem) : iounmap(iomem); > > This uses an inline function to define the macro in a safer way. > > Signed-off-by: Arnd Bergmann > --- > arch/arm/mach-ixp4xx/include/mach/io.h | 11 +-- Acked-by: Krzysztof Halasa In fact, the old code contained another bug (missing parentheses): > --- a/arch/arm/mach-ixp4xx/include/mach/io.h > > -#define ioport_map(port, nr)((void __iomem*)(port + > PIO_OFFSET)) -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] ARM: remove duplicate 'const' annotations'
Arnd Bergmann writes:
> gcc-7 warns about some declarations that are more 'const' than necessary:
> --- a/arch/arm/mach-cns3xxx/core.c
> +++ b/arch/arm/mach-cns3xxx/core.c
> @@ -346,7 +346,7 @@ static struct usb_ohci_pdata cns3xxx_usb_ohci_pdata = {
> .power_off = csn3xxx_usb_power_off,
> };
>
> -static const struct of_dev_auxdata const cns3xxx_auxdata[] __initconst = {
> +static const struct of_dev_auxdata cns3xxx_auxdata[] __initconst = {
> { "intel,usb-ehci", CNS3XXX_USB_BASE, "ehci-platform",
> &cns3xxx_usb_ehci_pdata },
> { "intel,usb-ohci", CNS3XXX_USB_OHCI_BASE, "ohci-platform",
> &cns3xxx_usb_ohci_pdata },
> { "cavium,cns3420-ahci", CNS3XXX_SATA2_BASE, "ahci", NULL },
Acked-by: Krzysztof Hałasa
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [i.MX6 DRM IPUv3] Regression 4.9-rc5: greenish screen with YUV420 video
Philipp Zabel writes:
> I had already (accidentally) fixed this with 3fd8b292ae6b ("drm/imx:
> ipuv3-plane: merge ipu_plane_atomic_set_base into atomic_update").
Thanks, I've merged your pza/imx-drm/next and it now works.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 4.9.0-rc5] AR9300 calibration problems with antenna selected
miaoq...@codeaurora.org writes: >> rmmod ath9k >> modprobe ath9k >> iw dev wlan0 set type ibss >> iw phy phyX set antenna 2 > > 2 is a bad mask. We use bitmap, the valid masks are 1, 3, 7. Thanks for your response. I have two antenna connections (and a single antenna). Is it possible to select the secondary antenna connector only? How? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 01/15] net: phy: Add phy_ethtool_nway_reset
Hi,
Florian Fainelli writes:
> This function just calls into genphy_restart_aneg() to perform an
> autonegotation restart.
>
> +int phy_ethtool_nway_reset(struct net_device *ndev)
> +{
> + struct phy_device *phydev = ndev->phydev;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return genphy_restart_aneg(phydev);
> +}
and then you're using this function as .nway_reset of all netdev drivers
in question.
genphy_restart_aneg() deals directly with MII BMCR register:
int genphy_restart_aneg(struct phy_device *phydev)
{
int ctl = phy_read(phydev, MII_BMCR);
if (ctl < 0)
return ctl;
ctl |= BMCR_ANENABLE | BMCR_ANRESTART;
/* Don't isolate the PHY if we're negotiating */
ctl &= ~BMCR_ISOLATE;
return phy_write(phydev, MII_BMCR, ctl);
}
Will the above work if the PHY is something special, e.g. a "fixed"
connection, or some kind of fiber trx etc? Normally, the PHY driver
could provide its own config_aneg().
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[i.MX6 DRM IPUv3] Regression 4.9-rc5: greenish screen with YUV420 video
Hi, The following GStreamer pipeline causes screen to become green with v4.9-rc4+: gst-launch-1.0 udpsrc uri=udp://239.1.2.2:5100 reuse=true caps="application/x-rtp,media=(string)video,clock-rate=(int)9,encoding-name=(string)H264" ! rtph264depay ! h264parse ! v4l2video1dec capture-io-mode=dmabuf ! kmssink name=imx-drm sync=0 Reverting "drm/imx: ipuv3-plane: Skip setting u/vbo only when we don't need modeset", commit 81d553545a1510ff7c7c00cbc9b57d6172d411a4, fixes the problem. Hardware is Gateworks Ventana GW5400, i.MX6Q, HDMI output, no X (console only). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH 4.9.0-rc5] AR9300 calibration problems with antenna selected
Hi, I recently tried to select a single antenna on AR9300 and it works for 30 seconds only. The subsequent calibration makes the RX signal level to drop from the usual -30/-40 dBm to -70/-80 dBm, and the transmission practically stops. With the attached patch it works, though selecting the antenna doesn't seem to have any visible effect, at least with "iw wlanX station dump" (perhaps it works for TX). I'm using ad-hoc mode: rmmod ath9k modprobe ath9k iw dev wlan0 set type ibss iw phy phyX set antenna 2 ip link set up dev wlan0 iw dev wlan0 set bitrates legacy-2.4 18 iw dev wlan0 ibss join nameXXX freqYYY ip addr add ZZZ broadcast + dev wlan0 The card in question is Mikrotik (Routerboard) R11e-2HPnD mPCIe adapter: AR9580 Wireless Network Adapter (rev 01), ID 168c:0033, subsystem 19b6:d016. ieee80211 phy0: Atheros AR9300 Rev:4 mem=0xc0f4, irq=334 https://routerboard.com/R11e-2HPnD Linux 4.9.0-rc5. Is there a better way? Signed-off-by: Krzysztof Halasa diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index e9f32b5..7f17e5d 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -2245,7 +2245,7 @@ static int ath9k_set_antenna(struct ieee80211_hw *hw, u32 tx_ant, u32 rx_ant) return 0; /* AR9100 runs into calibration issues if not all rx chains are enabled */ - if (AR_SREV_9100(ah)) + if (AR_SREV_9100(ah) || AR_SREV_9300(ah)) ah->rxchainmask = 0x7; else ah->rxchainmask = fill_chainmask(ah->caps.rx_chainmask, rx_ant); -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Super Top USB SD card reader resets and corruption - XHCI
Hi, I came across a problem with an USB SD card reader apparently causing USB resets and corrupting data on the card. The reader in question is a very cheap: 14cd:1212 Super Top microSD card reader (SY-T18) The card is an old SDHC, 32 GB, Sandisk brand. The problems don't seem to exist now if I connect the reader to an EHCI-equipped machine (an old Core2 duo + ICH10). I can read and write MBs of data, however older logs suggest that this computer also had the issue in the past (with Linux 4.6.3-300.fc24.x86_64, with both EHCI and UHCI). I can't use it with an XHCI. Both: Intel Corp 8 Series/C220 Series Chipset Family USB xHCI (rev 04) (prog-if 30 [XHCI]) (SandyBridge laptop with only USB2.0 connectors) and: Intel Corp Sunrise Point-H USB 3.0 xHCI Controller (rev 31) (a Skylake + Z170 and USB3, though the reader is USB2) have the issue. I have spotted the following writted to the SD card, I guess it's some USB mass storage write command: 3d085000 55 53 42 43 ca 08 00 00 00 e0 01 00 00 00 0a 2a |USBC...*| 3d085010 00 00 1e 83 70 00 00 f0 00 00 00 00 00 00 00 b2 |p...| 3d085020 2d 00 38 00 a9 c8 f4 ab 10 8d cc 39 e1 8f 43 45 |-.89..CE| I think the problem doesn't manifest itself if I don't write (RO mount) to the SD card. However, even light writes seem to trigger the issue. The kernels are basically recent Fedora, 4.7.4-200.fc24.x86_64, 4.7.9-200.fc24.x86_64 but it seems e.g. 4.6.3-300.fc24.x86_64 was also affected. The one which doesn't cause problems now (EHCI) is 4.7.2-201.fc24.x86_64 (though EHCI did cause problems previously). Could it be faulty hw? What can I do with this? [80125.418020] usb 1-1: new high-speed USB device number 9 using xhci_hcd [80125.583307] usb 1-1: New USB device found, idVendor=14cd, idProduct=1212 [80125.583315] usb 1-1: New USB device strings: Mfr=1, Product=3, SerialNumber=2 [80125.583320] usb 1-1: Product: Mass Storage Device [80125.583324] usb 1-1: Manufacturer: Generic [80125.583327] usb 1-1: SerialNumber: 121220130416 [80125.585139] usb-storage 1-1:1.0: USB Mass Storage device detected [80125.585392] scsi host6: usb-storage 1-1:1.0 [80126.586670] scsi 6:0:0:0: Direct-Access Mass Storage Device 1.00 PQ: 0 ANSI: 0 CCS [80126.587547] sd 6:0:0:0: Attached scsi generic sg3 type 0 [80126.775260] sd 6:0:0:0: [sdc] 62333952 512-byte logical blocks: (31.9 GB/29.7 GiB) [80126.775504] sd 6:0:0:0: [sdc] Write Protect is off [80126.775510] sd 6:0:0:0: [sdc] Mode Sense: 03 00 00 00 [80126.775686] sd 6:0:0:0: [sdc] No Caching mode page found [80126.775696] sd 6:0:0:0: [sdc] Assuming drive cache: write through [80126.787956] sdc: sdc1 [80126.789309] sd 6:0:0:0: [sdc] Attached SCSI removable disk [80396.165947] usb 1-1: reset high-speed USB device number 9 using xhci_hcd [80408.251009] usb 1-1: USB disconnect, device number 9 [80408.256545] sd 6:0:0:0: [sdc] tag#0 FAILED Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK [80408.256555] sd 6:0:0:0: [sdc] tag#0 CDB: Write(10) 2a 00 00 02 82 50 00 00 f0 00 [80408.256560] blk_update_request: I/O error, dev sdc, sector 164432 [80408.256567] Buffer I/O error on dev sdc1, logical block 19530, lost async page write [80408.256578] Buffer I/O error on dev sdc1, logical block 19531, lost async page write [80408.256583] Buffer I/O error on dev sdc1, logical block 19532, lost async page write [80408.256587] Buffer I/O error on dev sdc1, logical block 19533, lost async page write [80408.256592] Buffer I/O error on dev sdc1, logical block 19534, lost async page write [80408.256596] Buffer I/O error on dev sdc1, logical block 19535, lost async page write [80408.256600] Buffer I/O error on dev sdc1, logical block 19536, lost async page write [80408.256604] Buffer I/O error on dev sdc1, logical block 19537, lost async page write [80408.256608] Buffer I/O error on dev sdc1, logical block 19538, lost async page write [80408.256611] Buffer I/O error on dev sdc1, logical block 19539, lost async page write [80408.340407] VFS: Dirty inode writeback failed for block device sdc1 (err=-5). [80408.618642] usb 1-1: new high-speed USB device number 10 using xhci_hcd [80408.783479] usb 1-1: New USB device found, idVendor=14cd, idProduct=1212 [80408.783487] usb 1-1: New USB device strings: Mfr=1, Product=3, SerialNumber=2 [80408.783492] usb 1-1: Product: Mass Storage Device [80408.783496] usb 1-1: Manufacturer: Generic [80408.784034] usb-storage 1-1:1.0: USB Mass Storage device detected [80408.784253] scsi host6: usb-storage 1-1:1.0 [80409.786335] scsi 6:0:0:0: Direct-Access Mass Storage Device 1.00 PQ: 0 ANSI: 0 CCS [80409.787089] sd 6:0:0:0: Attached scsi generic sg3 type 0 [80409.975129] sd 6:0:0:0: [sdc] 62333952 512-byte logical blocks: (31.9 GB/29.7 GiB) [80409.975380] sd 6:0:0:0: [sdc] Write Protect is off [80409.975389] sd 6:0:0:0: [sdc] Mode Sense: 03 00 00 00 [80409.976494] sd 6:0:0:0: [sdc] No Caching mode page found [80409.976503] sd 6:0:0:0: [sdc] Assuming drive cache: wri
Re: [PATCH v2] media: solo6x10: fix lockup by avoiding delayed register write
Andrey Utkin writes:
> --- a/drivers/media/pci/solo6x10/solo6x10.h
> +++ b/drivers/media/pci/solo6x10/solo6x10.h
> @@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev
> *solo_dev, int reg)
> static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
> u32 data)
> {
> + u16 val;
> +
> writel(data, solo_dev->reg_base + reg);
> + pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val);
> }
>
> static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
This is ok for now. I hope I will find some to refine this, so not all
register writes are done with the penalty - eventually.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH net-next 3/6] net: use core MTU range checking in WAN drivers
Jarod Wilson writes: > - set min/max_mtu in all hdlc drivers, remove hdlc_change_mtu > - sent max_mtu in lec driver, remove lec_change_mtu > drivers/net/wan/c101.c| 1 - > drivers/net/wan/hdlc.c| 11 ++- > drivers/net/wan/hdlc_fr.c | 3 ++- > drivers/net/wan/ixp4xx_hss.c | 1 - > drivers/net/wan/n2.c | 1 - > drivers/net/wan/pc300too.c| 1 - > drivers/net/wan/pci200syn.c | 1 - > drivers/net/wan/wanxl.c | 1 - > include/linux/hdlc.h | 2 -- Acked-by: Krzysztof Halasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Andrey Utkin writes: > Lockup happens only on 6010. In provided log you can see that 6110 > passes just fine right before 6010. Also if 6010 PCI ID is removed from > solo6x10 driver's devices list, the freeze doesn't happen. Probably explains why I don't see lockups :-) I will have a look. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Andrey Utkin writes: >> Can you share some details about the machine you are experiencing the >> problems on? CPU, chipset? I'd try to see if I can recreate the problem. > > See solo.txt.gz attached. Thanks. I can see you have quite a set of video devices there. I will see what I can do with this. BTW does the lookup occur on SOLO6010, 6110, or both? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Andrey Utkin writes: >> Does (only) adding the >> >> pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val); >> >> in solo_reg_write() help? > > Yes. > I have posted a patch with this change few days ago, I thought you have > noticed it. Well, I think you haven't sent me a copy. Anyway, it would be great to determine where exactly writes need a flush. Adding it everywhere is a bit suboptimal, one would think. Can you share some details about the machine you are experiencing the problems on? CPU, chipset? I'd try to see if I can recreate the problem. Alternatively, you could investigate yourself - at first you could put pci_read_config_word() at the end of subroutines (including return statements) using solo_reg_write(). And in that solo_p2m_dma_desc(), before wait_for_completion_timeout(). Then eliminate them using some sort of binary search to see which ones are required. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Andrey Utkin writes: > On Thu, Sep 22, 2016 at 10:51:37AM +0200, Krzysztof Hałasa wrote: >> I wonder if the following fixes the problem (completely untested). > > I have given this a run, and it still hangs. Does (only) adding the pci_read_config_word(solo_dev->pdev, PCI_STATUS, &val); in solo_reg_write() help? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Andrey Utkin writes:
> It happens in solo_disp_init at uploading default motion thresholds
> array.
>
> I've got a prints trace with solo6010-fix-lockup branch
> https://github.com/bluecherrydvr/linux/tree/solo6010-fix-lockup/drivers/media/pci/solo6x10
> the trace itself in jpg:
> https://decent.im:5281/upload/3793f393-e285-4514-83dd-bf08d1c8b4a2/e7ad898b-515b-4522-86a9-553daaeb0860.jpg
solo_motion_config() uses BM DMA and thus generates IRQ, this may be
indeed the ISR problem. BTW the IRQ debugging ("kernel hacking") should
catch it.
OTOH programming the DMA can be guilty as well.
I wonder if the following fixes the problem (completely untested).
diff --git a/drivers/media/pci/solo6x10/solo6x10-core.c
b/drivers/media/pci/solo6x10/solo6x10-core.c
index f50d072..2d4900e 100644
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -99,6 +99,7 @@ static irqreturn_t solo_isr(int irq, void *data)
{
struct solo_dev *solo_dev = data;
u32 status;
+ u16 tmp;
int i;
status = solo_reg_read(solo_dev, SOLO_IRQ_STAT);
@@ -129,6 +130,7 @@ static irqreturn_t solo_isr(int irq, void *data)
if (status & SOLO_IRQ_G723)
solo_g723_isr(solo_dev);
+ pci_read_config_word(solo_dev->pdev, PCI_STATUS, &tmp) // flush write
to SOLO_IRQ_STAT
return IRQ_HANDLED;
}
diff --git a/drivers/media/pci/solo6x10/solo6x10-p2m.c
b/drivers/media/pci/solo6x10/solo6x10-p2m.c
index 07c4e07..8a51d45 100644
--- a/drivers/media/pci/solo6x10/solo6x10-p2m.c
+++ b/drivers/media/pci/solo6x10/solo6x10-p2m.c
@@ -70,6 +70,7 @@ int solo_p2m_dma_desc(struct solo_dev *solo_dev,
unsigned int config = 0;
int ret = 0;
int p2m_id = 0;
+ u16 tmp;
/* Get next ID. According to Softlogic, 6110 has problems on !=0 P2M */
if (solo_dev->type != SOLO_DEV_6110 && multi_p2m) {
@@ -111,6 +112,7 @@ int solo_p2m_dma_desc(struct solo_dev *solo_dev,
desc[1].ctrl);
}
+ pci_read_config_word(solo_dev->pdev, PCI_STATUS, &tmp); // flush writes
timeout = wait_for_completion_timeout(&p2m_dev->completion,
solo_dev->p2m_jiffies);
> Indeed, targeted fixing would be more reasonable than making register
> r/w routines follow blocking fashion. But the driver is already complete
> and was known to be working, and I seems all places in code assume the
> blocking fashion of reg r/w, and changing that assumption may lead to
> covert bugs anywhere else, not just at probing, which may be hard to
> nail down.
The driver code doesn't have to assume anything about posted writes -
except at very specific places (as explained by Alan).
Normally, a CPU write to a register doesn't have to be flushed right
away. It would be much slower, especially if used extensively. Nobody
does anything alike since the end of the ISA bus.
The driver (and the card) can still see all operations in correct
order, in both cases.
The potential problem is a write being held in a buffer (and not making
it to the actual hardware). This may happen in ISR since the actual
write is deactivates the physical IRQ line. Otherwise the ISR terminates
and is immediately requested again - though this second call should
bring the IRQ down by reading the register (thus flushing the write
buffer) - so, while not very effective, it shouldn't lock up (but it's
a real bug worth fixing).
Also, I imagine a write to the DMA registers can be posted and the DMA
may not start in time. This shouldn't end in a lock up, either. Perhaps
a different bug is involved.
The other thing is BM DMA (card->RAM). All DMA transfers (initiated by
the card) are completed with an IRQ (either with success or failure).
This is potentially a problem as well, though it has nothing to do with
the patch in question. I guess the SOLO reads some descriptors or
something, and such writes are flushed this way.
> For now, I'll try setting pci_read_config_word() back instead of full
> revert. Does it need to be just in reg_write? No need for it in
> reg_read, right?
Sure, reg_read() doesn't write to the device.
It the patch doesn't fix the problem, what CPU and chipset are used by
the computer which exhibits the issue? Perhaps I have something similar
here and can reproduce it.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)
Hans Verkuil writes: > That was probably the reason for the pci_read_config_word in the reg_write > code. Try putting that back (and just that). Yes. I guess a single pci_read_config_word() would suffice. Though it would obviously be much better to identify the place in the driver which needs to have the write buffers flushed, and add a read() just there. The interrupt handler maybe (e.g. just before the return IRQ_HANDLED)? OTOH this may be some sort of timing problem, I mean the faster code may put too much stress on the SOLO chip. Doesn't happen here so I can't test the cure. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: Aw: BUG: Wrong dma queue handling in ixp4 driver
Hi Lino, "Lino Sanfilippo" writes: >> maybe I miss something, but the ixp4 ethernet driver seems to handle dma >> pools >> in a wrong way: In init_queues() it creates a dma pool for descriptors and >> then >> only allocates a single descriptor from this pool. The author seems to >> assume the whole >> table has been allocated already, since after that the complete pool size is >> zeroed: >> > > Sorry, I indeed missed something. The allocation is correct. A pool is not > required, though, > since only one chunk is allocated. Not really: there is one pool for all ports, but each port uses a separate desc_tab (allocated from that pool). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] [media] tw686x-kh: Delete an unnecessary check before the function call "video_unregister_device"
SF Markus Elfring writes: > The video_unregister_device() function tests whether its argument is NULL > and then returns immediately. Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring > --- > drivers/staging/media/tw686x-kh/tw686x-kh-video.c | 3 +-- Thanks. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"
Arnd Bergmann writes: > I'd start by copying the relevant nodes from > arch/arm/boot/dts/arm-realview-pb11mp.dts, which is the closest > I can think of. I've put together something completely untested > below. Thanks. I will try to handle that. 3+ weeks, unfortunately. Now, what do we do with the PCIe MRRS? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"
Arnd Bergmann writes: > Before that, we were always setting both mrrs and mps. As we don't know > who uses PCIE_BUS_PEER2PEER, maybe another option would be to add yet > another pcie_bus_config value for this particular quirk? It would be a safe approach. Or, maybe another non-pcie_bus_config thing, I don't know (so the pcie_bus_config is left for the user). > I started the DT conversion a long time ago (see the DT parsing in > arch/arm/mach-cns3xxx/core.c) but I never had any hardware to test > on, and it was at a time when we didn't even have DT support in all > the subsystems. > > I'd definitely help you get the rest of the DT support in place if > you can test it. This is now the only SMP platform and one of > the last users of GIC and l2x0 that does not use DT, so I'd love > to see that converted just so we can remove the legacy probing from > those drivers. Ok. Is there a DT skeleton file somewhere, so I can try to boot the board (without Laguna extras) in DT mode? At first, I only need CPU + RAM + console serial port. > Converting what we have in mainline should be fairly straightforward, > but there is more code in > target/linux/cns3xxx/files/arch/arm/mach-cns3xxx/laguna.c that requires > more work, in particular we need to come up with a way to handle > the laguna_net_data and laguna_info structures, which have some of > the same data that is normall in DT. I assume adding this to U-Boot should be acceptable (for Gateworks, too). They are already doing this to their i.MX6 line Ventana. > Also, the gpio driver doesn't > have a trivial conversion to DT and requires some work to define > a binding and implement that. GPIO is a bit less important ATM, since the boards can boot without it. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"
Arnd Bergmann writes:
> What exactly is the problem we are seeing, and is there a way to fix
> it on top of my patch? Are we perhaps just missing a call to
> pcie_bus_configure_settings()?
From: khal...@piap.pl (Krzysztof Halasa)
Subject: [PATCH] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM
DMA.
To: Bjorn Helgaas
Cc: Arnd Bergmann , linux-...@vger.kernel.org,
linux-kernel@vger.kernel.org
Date: Mon, 21 Mar 2016 10:39:52 +0100 (11 weeks, 2 days, 19 hours ago)
The platform in question is Cavium CNS3xxx, ARMv6.
A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
potential stack overflow") converted an explicit setting of
PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
read request) to:
+pcie_bus_config = PCIE_BUS_PEER2PEER;
with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size
value for the first device/function on bus 1, i.e. the device
plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not
perform the same setting on devices attached to a PCIe switch,
or multi-function devices.
The solution for this part fortunately is even easier: if we
just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
all PCIe devices in the system are limited to 128 byte MPS, which
in turn limits the MRRS to 128 bytes for all devices, and we
no longer even need to touch any devices."
The problem is the MRRS setting is never written to the hardware.
I propose the following, though I'm not sure if we can do this safely,
especially given the comments in probe.c. OTOH, this change may be
required in other (all?) cases when the user requests
PCIE_BUS_PEER2PEER.
On this Laguna GW-2388 the following patch fixes BM DMA with:
:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
the second lane from the CPU)
pci :00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128
pci :01:00.0: Max Payload Size set to 128/ 512 (was 128), Max Read Rq 128
pci 0001:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128
Signed-off-by: Krzysztof Hałasa
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
/* In the "safe" case, do not configure the MRRS. There appear to be
* issues with setting MRRS to 0 on a number of devices.
*/
- if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
+ if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
+ pcie_bus_config != PCIE_BUS_PEER2PEER)
return;
/* For Max performance, the MRRS must be set to the largest supported
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {
PCIE_BUS_DEFAULT, /* ensure MPS matches upstream bridge */
PCIE_BUS_SAFE, /* use largest MPS boot-time devices support */
PCIE_BUS_PERFORMANCE, /* use MPS and MRRS for best performance */
- PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */
+ PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */
};
extern enum pcie_bus_config_types pcie_bus_config;
> Note that cns3xxx is in a bit of an odd state, as only half of the
> platform code is even present in the kernel, and there is no effort
> to change that. As far as I know, the board that this was tested on
> is not present in the mainline kernel, and the board we support
> is a development system that few people even own at this point.
The boards I use (Gateworks Laguna) are basically equivalent to the
devel board (from the platform code POV).
The kernel lacks support for SMP and the Ethernet driver (and things
like GPIO), though there are patches available and I plan to integrate
them, when the existing issues are resolved.
Also, this is practically a non-DT arch but I guess a conversion to DT
would be a good thing as it would eliminate a need for board-specific
code. That's why there is no platform code for Laguna. Unfortunately
there is no DT file for CNS3xxx, and I'm not an DT expert.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"
Bjorn Helgaas writes: > This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1. > > Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna > GW-2388) because the MRRS setting is never written to the hardware. > > Signed-off-by: Bjorn Helgaas > CC: Arnd Bergmann > CC: Krzysztof Hałasa > --- > arch/arm/mach-cns3xxx/pcie.c | 71 > -- > 1 file changed, 41 insertions(+), 30 deletions(-) This, applied to v4.7-rc1, fixes the problem on my Laguna boards. Tested-by: Krzysztof Hałasa And as well Acked-by: Krzysztof Hałasa Thanks. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH REPOST] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.
Bjorn Helgaas writes: > It looks like 498a92d42596 merely fixed a warning, at the expense of > breaking DMA on Cavium. Reverting it would bring the warning back, but > that's better than broken DMA. Perhaps we should change PCIE_BUS_PEER2PEER to also write MRRS anyway. I realize the CNS3xxx patch is some sort of clever workaround, and that PCIE_BUS_PEER2PEER (which normally comes from kernel command line parameter "pcie_bus_peer2peer") was not exactly intended for this. But if one asks for "peer2peer" (which means limiting transfers to 128 bytes), how could it all work if the bus mastering read requests are not equally limited? BTW s/MRSS/MRRS/g -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 4/4] ARM: remove duplicate const qualifier
Eric Engestrom writes: > I can't confirm it (haven't tried), and don't care enough to do it :] > I guess I'm just dropping the patch then. Like I said, it can't hurt to > leave them in. Actually it may hurt (a little bit) - it makes the code less readable. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Tim, > So perhaps there is something else going on with the tw8689 > device/driver that is causing it to not work with MSI. We have an > avc8000 miniPCIe with tw8689 here and can test if you send me your > patches that enable tw8689 with msi. I think you can use this: http://git.linuxtv.org/hverkuil/media_tree.git/log/?h=tw686x > Regardless of MSI working in our tests we still disable it because of > it breaking legacy irqs and for performance reasons. So do I. However I'm also using Fedora 23 and this means I have to recompile the kernel, since they build it with MSI enabled. I think we should fix it, either by disabling in run time, or making it work. Performance-wise, I found it "surprising" that one can't have multiple MSIs with separate hw vector for each. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Lucas Stach writes: >> TW6869: PCI :04:00.0, IRQ 336, MMIO 0x110 >> TW686x :04:00.0: enabling device (0140 -> 0142) >> > I don't see whee the device even tries to use MSI IRQs. Even if the > infrastructure is enabled it opts to use legacy INTA. It only tries to use normal IRQ. > There is no upstream driver for this chip, so I don't know where to look > to find out if the driver tries to enable MSI. It's been posted on linux-media list... I added pci_enable_msi() to this driver and it didn't help. > Is what you are saying that if you enable MSI support in the kernel, it > breaks legacy IRQs? Precisely. However, MSI doesn't seem to work either. Could be a problem specific to this TW6869 card. Ventana GW5410 has 5 mPCIe slots, and (with MSI enabled in the system) those IRQs (non-MSI) don't work in any slot: 304: 0 0 0 0 PCI-MSI 0 Edge PCIe PME, aerdrv 336: 0 0 0 0 GPC 123 Level TW8689 in J7 slot 337: 0 0 0 0 GPC 122 Level TW8689 in J8, J10, J11 338: 0 0 0 0 GPC 121 Level TW8689 in J6) If I enable MSI on this card (adding pci_enable_msi()): 313: 0 0 0 0 PCI-MSI 9 Edge TW6869 in J7 slot The only way I can get it to work is by disabling MSI (system wide). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Lucas Stach writes: > Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI > IRQs before enabling them in the defconfig and they have been working > for me for a long time before that. Tested with i210 on Gateworks > Ventana. MSI never worked for me on Ventana. I have been using 4.2 extensively, and now I'm switching to 4.5 (which doesn't work either). Could it be a DTS (bridge) problem(?) On 4.5, trying to use it with TW6869 frame buffer and GW5410: TW6869: PCI :04:00.0, IRQ 336, MMIO 0x110 TW686x :04:00.0: enabling device (0140 -> 0142) CPU0 CPU1 CPU2 CPU3 16: 1165 1032 1271 1591 GIC-0 29 Edge twd 17:879387 1404606 GPC 55 Level i.MX Timer Tick 18: 6434 0 0 0 GPC 13 Level mxs-dma 19: 0 0 0 0 GPC 15 Level bch 21: 0 0 0 0 GPC 9 Level 13.gpu 22: 0 0 0 0 GPC 10 Level 134000.gpu 24: 0 0 0 0 GPC 120 Level mx6-pcie-msi 26: 0 0 0 0 GPC 26 Level 202.serial 30: 0 0 0 0 GPC 12 Level 204.vpu 240: 0 0 0 0 gpio-mxc 0 Edge 2198000.usdhc cd 280: 0 0 0 0 GPC 19 Level rtc alarm 286: 0 0 0 0 GPC 2 Level sdma 287: 0 0 0 0 GPC 43 Level 2184000.usb 288: 32 0 0 0 GPC 40 Level 2184200.usb 289: 2294 0 0 0 GIC-0 150 Level 2188000.ethernet 290: 0 0 0 0 GIC-0 151 Level 2188000.ethernet 291: 0 0 0 0 GPC 24 Level mmc0 292: 0 0 0 0 GPC 36 Level 21a.i2c 293: 0 0 0 0 GPC 37 Level 21a4000.i2c 294: 0 0 0 0 GPC 38 Level 21a8000.i2c 296: 1422 0 0 0 GPC 27 Level 21e8000.serial 297: 0 0 0 0 GPC 30 Level 21f4000.serial 300: 0 0 0 0 GPC 39 Level ahci-imx[220.sata] 301: 0 0 0 0 GPC 11 Level 2204000.gpu 304: 0 0 0 0 PCI-MSI 0 Edge PCIe PME, aerdrv 336: 0 0 0 0 GPC 123 Level TW6869 339: 0 0 0 0 IPU 457 Edge (null) 340: 0 0 0 0 IPU 451 Edge (null) 341: 0 0 0 0 IPU 457 Edge (null) 342: 0 0 0 0 IPU 451 Edge (null) IPI0: 0 0 0 0 CPU wakeup interrupts IPI1:183111 90 57 Timer broadcast interrupts IPI2:453 2091 6539 2088 Rescheduling interrupts IPI3: 37 32 29 23 Function call interrupts IPI4: 0 0 0 0 CPU stop interrupts IPI5: 0 0 0 1 IRQ work interrupts IPI6: 0 0 0 0 completion interrupts Err: 0 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 01:00.1 System peripheral: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:06.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:08.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba) 04:00.0 Multimedia controller: Techwell Inc. Device 6869 (rev 01) 08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057 PCI-E
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
> OTOH, we should fix it some day, making sure the DTS files are fixed > first: > > imx6qdl-apf6dev.dtsi: reset-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>; > imx6qdl-aristainetos2.dtsi: reset-gpio = <&gpio2 16 GPIO_ACTIVE_HIGH>; > imx6qdl-hummingboard.dtsi: reset-gpio = <&gpio3 4 0>; (I think RMK already > handles this) > imx6qdl-phytec-pfla02.dtsi: reset-gpio = <&gpio4 17 0>; > imx6qdl-sabresd.dtsi: reset-gpio = <&gpio7 12 0>; > imx6q-dmo-edmqmx6.dts: reset-gpio = <&gpio4 8 0>; > imx6q-novena.dts: reset-gpio = <&gpio3 29 GPIO_ACTIVE_HIGH>; > imx6q-tbs2910.dts: reset-gpio = <&gpio7 12 GPIO_ACTIVE_HIGH>; Or maybe we should simply change these to *_LOW, add that short patch from me, and forget about it. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Fabio Estevam writes: > In order to keep old dtb's working we should simply ignore the GPIO > flags passed in the 'reset-gpio' property. > > That's why we need a revert. Just sent a v2, BTW. OTOH, we should fix it some day, making sure the DTS files are fixed first: imx6qdl-apf6dev.dtsi: reset-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>; imx6qdl-aristainetos2.dtsi: reset-gpio = <&gpio2 16 GPIO_ACTIVE_HIGH>; imx6qdl-hummingboard.dtsi: reset-gpio = <&gpio3 4 0>; (I think RMK already handles this) imx6qdl-phytec-pfla02.dtsi: reset-gpio = <&gpio4 17 0>; imx6qdl-sabresd.dtsi: reset-gpio = <&gpio7 12 0>; imx6q-dmo-edmqmx6.dts: reset-gpio = <&gpio4 8 0>; imx6q-novena.dts: reset-gpio = <&gpio3 29 GPIO_ACTIVE_HIGH>; imx6q-tbs2910.dts: reset-gpio = <&gpio7 12 GPIO_ACTIVE_HIGH>; The original patch was a bad implementation of a good idea. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Tim Harvey writes: > It's not too easy to tell how many IMX6 boards incorrectly specify > their reset-gpio polarity. I don't know what the best way to determine > what boards use the IMX6 pcie host controller. Is there a dtc usage > that will display the compiled dtb's then we grep out 'compatible = > "fsl,imx6q-pcie"' to at least get the list of boards to inspect? I'm > curious if its just one or two boards that incorrectly specify the > polarity of their PCI reset. I guess, maybe 8 of them. Not counting those with out-of-tree DTS/DTB files. Something like: $ grep reset-gpio arch/arm/boot/dts/imx6* | grep -v phy-reset > I figured out it was the change to enable CONFIG_PCI_MSI in v4.5 that > is causing interrupts to fail for me. Right, a long standing issue. MSI never worked for me on i.MX6. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Tim Harvey writes: > ok - I'll respond there as I agree with the patch but not the wording > of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we > do define the polarity properly as active-low in Ventana dt's). Right, it's Ventana of course (I had been working with Laguna boards recently). > However, there seems to be another regression in 4.5 that's keeping > PCI working for me and I'm still bisecting that (using 802.11n access > points to test). Can you confirm that PCI works in v4.5 on IMX6 boards > with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted? I will check with my frame buffer and wifi cards. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
A recent commit 5c5fb40de8f14391a1238db05cef88754faf9229 stated:
Follows: linus/v4.4-rc2
Precedes: linus/v4.5-rc1
PCI: imx6: Add support for active-low reset GPIO
We previously used of_get_named_gpio(), which ignores the OF flags cell, so
the reset GPIO defaulted to "active high." This doesn't work on the Toradex
Apalis SoM with Ixora base board, which has an active-low reset GPIO.
Use devm_gpiod_get_optional() instead so we pay attention to the active
high/low flag. This also adds support for GPIOs described via ACPI.
The (now replaced) code doesn't support the above:
@@ -287,10 +287,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port
*pp)
usleep_range(200, 500);
/* Some boards don't have PCIe reset GPIO. */
-if (gpio_is_valid(imx6_pcie->reset_gpio)) {
-gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
+if (imx6_pcie->reset_gpio) {
+gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
msleep(100);
-gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
}
return 0;
If the reset_gpio setup code had ignored the flags (haven't checked
that), then clearly the resets were active-low (most reset lines are,
because they can be then driven with open-drain/collector output).
The gpiod_set_value*(0) activates reset, gpiod_set_value(1) -
deactivates.
Now we're told the setup code is now level-aware, but the above sequence
thus _deactivates_ reset for 100 ms, then _activates_ it again. It has
no chance to work, unless a board has a broken DTS file. A quick grep
shows that about half the IMX6 boards specify an active-low PCIe reset,
4 ask for active-high, and another 4 don't bother.
I wonder if all boards (except maybe that Toradex set) use an active-low
PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
works.
I'm not fixing individual DTS files because I don't really know, though
perhaps we should change them all to "active-low", since it would work
the same as before the 5c5fb40de8f14391a1238db05cef88754faf9229 change.
Confirmed to fix Gateworks Laguna GW54xx.
Without the patch, the following happens (as expected):
PCI host bridge /soc/pcie@0x0100 ranges:
No bus range found for /soc/pcie@0x0100, using [bus 00-ff]
IO 0x01f8..0x01f8 -> 0x
MEM 0x0100..0x01efffff -> 0x01000000
imx6q-pcie 1ffc000.pcie: phy link never came up
Signed-off-by: Krzysztof Hałasa
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fe60096..f17fb02 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -288,9 +288,9 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port
*pp)
/* Some boards don't have PCIe reset GPIO. */
if (imx6_pcie->reset_gpio) {
- gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
- msleep(100);
gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+ msleep(100);
+ gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
}
return 0;
Re: [PATCH] Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM
Bjorn Helgaas writes:
>> At least CNS3xxx doesn't boot. I haven't verified a couple of others,
>> but they may be broken as well.
>
> Good, thanks. Also (I should have asked this before), please include a
> "Fixes:" line so we know exactly when this broke and what stable kernels
> need the fix.
The problem started here:
Fixes: 8c7d14746abc ("ARM/PCI: Move to generic PCI domains")
It means the patch should be applicable starting with v4.0, and ...
indeed v4.0 with the patch boots correctly, while v4.0 without the patch
does not.
Thanks.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM
Hi Bjorn,
Bjorn Helgaas writes:
> On Tue, Mar 01, 2016 at 07:07:18AM +0100, Krzysztof Hałasa wrote:
>> Many ARM platforms use a wrapper:
>> /*
>> * Compatibility wrapper for older platforms that do not care about
>> * passing the parent device.
>> */
>> static inline void pci_common_init(struct hw_pci *hw)
>> {
>> pci_common_init_dev(NULL, hw);
>> }
>>
>> which means that pci_bus_assign_domain_nr() can be called without
>> a parent. This patch fixes the NULL pointer dereference.
>
> What exactly is the impact of this? Does this fix need to be in v4.5?
> It sounds like it should be, but I need a little more detailed
> justification, e.g., "platforms X, Y, Z don't boot at all without
> this change."
At least CNS3xxx doesn't boot. I haven't verified a couple of others,
but they may be broken as well.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] CNS3xxx: remove unused *_VIRT definitions
Signed-off-by: Krzysztof Hałasa diff --git a/arch/arm/mach-cns3xxx/cns3xxx.h b/arch/arm/mach-cns3xxx/cns3xxx.h index a0f5b60..a642ba5 100644 --- a/arch/arm/mach-cns3xxx/cns3xxx.h +++ b/arch/arm/mach-cns3xxx/cns3xxx.h @@ -162,13 +162,11 @@ #define CNS3XXX_L2C_BASE 0x9200 /* L2 Cache Control */ #define CNS3XXX_PCIE0_MEM_BASE 0xA000 /* PCIe Port 0 IO/Memory Space */ -#define CNS3XXX_PCIE0_MEM_BASE_VIRT0xE000 #define CNS3XXX_PCIE0_HOST_BASE0xAB00 /* PCIe Port 0 RC Base */ #define CNS3XXX_PCIE0_HOST_BASE_VIRT 0xE100 #define CNS3XXX_PCIE0_IO_BASE 0xAC00 /* PCIe Port 0 */ -#define CNS3XXX_PCIE0_IO_BASE_VIRT 0xE200 #define CNS3XXX_PCIE0_CFG0_BASE0xAD00 /* PCIe Port 0 CFG Type 0 */ #define CNS3XXX_PCIE0_CFG0_BASE_VIRT 0xE300 @@ -177,16 +175,13 @@ #define CNS3XXX_PCIE0_CFG1_BASE_VIRT 0xE400 #define CNS3XXX_PCIE0_MSG_BASE 0xAF00 /* PCIe Port 0 Message Space */ -#define CNS3XXX_PCIE0_MSG_BASE_VIRT0xE500 #define CNS3XXX_PCIE1_MEM_BASE 0xB000 /* PCIe Port 1 IO/Memory Space */ -#define CNS3XXX_PCIE1_MEM_BASE_VIRT0xE800 #define CNS3XXX_PCIE1_HOST_BASE0xBB00 /* PCIe Port 1 RC Base */ #define CNS3XXX_PCIE1_HOST_BASE_VIRT 0xE900 #define CNS3XXX_PCIE1_IO_BASE 0xBC00 /* PCIe Port 1 */ -#define CNS3XXX_PCIE1_IO_BASE_VIRT 0xEA00 #define CNS3XXX_PCIE1_CFG0_BASE0xBD00 /* PCIe Port 1 CFG Type 0 */ #define CNS3XXX_PCIE1_CFG0_BASE_VIRT 0xEB00 @@ -195,7 +190,6 @@ #define CNS3XXX_PCIE1_CFG1_BASE_VIRT 0xEC00 #define CNS3XXX_PCIE1_MSG_BASE 0xBF00 /* PCIe Port 1 Message Space */ -#define CNS3XXX_PCIE1_MSG_BASE_VIRT0xED00 /* * Testchip peripheral and fpga gic regions -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] CNS3xxx: remove unused *_VIRT definitions
diff --git a/arch/arm/mach-cns3xxx/cns3xxx.h b/arch/arm/mach-cns3xxx/cns3xxx.h index a0f5b60..a642ba5 100644 --- a/arch/arm/mach-cns3xxx/cns3xxx.h +++ b/arch/arm/mach-cns3xxx/cns3xxx.h @@ -162,13 +162,11 @@ #define CNS3XXX_L2C_BASE 0x9200 /* L2 Cache Control */ #define CNS3XXX_PCIE0_MEM_BASE 0xA000 /* PCIe Port 0 IO/Memory Space */ -#define CNS3XXX_PCIE0_MEM_BASE_VIRT0xE000 #define CNS3XXX_PCIE0_HOST_BASE0xAB00 /* PCIe Port 0 RC Base */ #define CNS3XXX_PCIE0_HOST_BASE_VIRT 0xE100 #define CNS3XXX_PCIE0_IO_BASE 0xAC00 /* PCIe Port 0 */ -#define CNS3XXX_PCIE0_IO_BASE_VIRT 0xE200 #define CNS3XXX_PCIE0_CFG0_BASE0xAD00 /* PCIe Port 0 CFG Type 0 */ #define CNS3XXX_PCIE0_CFG0_BASE_VIRT 0xE300 @@ -177,16 +175,13 @@ #define CNS3XXX_PCIE0_CFG1_BASE_VIRT 0xE400 #define CNS3XXX_PCIE0_MSG_BASE 0xAF00 /* PCIe Port 0 Message Space */ -#define CNS3XXX_PCIE0_MSG_BASE_VIRT0xE500 #define CNS3XXX_PCIE1_MEM_BASE 0xB000 /* PCIe Port 1 IO/Memory Space */ -#define CNS3XXX_PCIE1_MEM_BASE_VIRT0xE800 #define CNS3XXX_PCIE1_HOST_BASE0xBB00 /* PCIe Port 1 RC Base */ #define CNS3XXX_PCIE1_HOST_BASE_VIRT 0xE900 #define CNS3XXX_PCIE1_IO_BASE 0xBC00 /* PCIe Port 1 */ -#define CNS3XXX_PCIE1_IO_BASE_VIRT 0xEA00 #define CNS3XXX_PCIE1_CFG0_BASE0xBD00 /* PCIe Port 1 CFG Type 0 */ #define CNS3XXX_PCIE1_CFG0_BASE_VIRT 0xEB00 @@ -195,7 +190,6 @@ #define CNS3XXX_PCIE1_CFG1_BASE_VIRT 0xEC00 #define CNS3XXX_PCIE1_MSG_BASE 0xBF00 /* PCIe Port 1 Message Space */ -#define CNS3XXX_PCIE1_MSG_BASE_VIRT0xED00 /* * Testchip peripheral and fpga gic regions -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[PATCH] Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM
Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
pci_common_init_dev(NULL, hw);
}
which means that pci_bus_assign_domain_nr() can be called without
a parent. This patch fixes the NULL pointer dereference.
Signed-off-by: Krzysztof Hałasa
Cc: sta...@vger.kernel.org
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 602eb42..f89db3a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
static int use_dt_domains = -1;
- int domain = of_get_pci_domain_nr(parent->of_node);
+ int domain = -1;
+ if (parent)
+ domain = of_get_pci_domain_nr(parent->of_node);
/*
* Check DT domain and use_dt_domains values.
*
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes:
> Ok, so I guess what this means is that ixp4xx (or xscale in general)
> implements its big-endian mode by adding a byteswap on its DRAM
> and PCI interfaces in be32 mode, rather than by changing the behavior of
> the load/store operations (as be8 mode does) or by having the byteswap
> in its load/store pipeline or the top-level AHB bridge?
Hmm... IIRC, there is normally no swapping on DRAM bus. E.g. if you
write 0x12345678 to RAM and change endianness, it will still read as
0x12345678. The CPU will still be able to execute opcodes after
switching endianness, but byte-oriented data will be messed up.
PCI swaps in BE mode, so byte order is preserved but readl() must
"unswap" it.
> I'm still unsure about
> __indirect_readsl()/ioread32_rep()/insl()/readsl().
Indirect ops should behave the same as direct. I think I have tested
them at some point. The "string" operations don't have to swap (on PCI)
because the PCI bus controller does it for them (in BE mode).
> insl() does a double-swap on big-endian, which seems right, as we
> end up with four swaps total, preserving correct byte order.
static inline void insl(u32 io_addr, void *p, u32 count)
{
u32 *vaddr = p;
while (count--)
*vaddr++ = le32_to_cpu(inl(io_addr));
}
inl() does indirect input (preserving value, not byte order), so there seem
to be just one swap here (le32_to_cpu) preserving byte order.
> __raw_readsl() performs no swap, which would be correct for PCI
> (same swap on PCI and RAM, so byteorder is preserved),
No, a single swap on PCI, this means the byte order is preserved :-)
> but wrong
> for on-chip FIFO registers (one swap on RAM, no swap on MMIO).
But there aren't any such registers. Basically, almost all registers are
32-bit, even if they only hold an 8-bit value.
Exceptions such as 16550 UARTs are taken care of in platform structs
(using offset = 3).
> However, when CONFIG_IXP4XX_INDIRECT_PCI is set, both
> ioread32_rep() and readsl() call __indirect_readsl(), which
> in turn swaps the data once, so I think we actually need this patch:
>
> diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h
> b/arch/arm/mach-ixp4xx/include/mach/io.h
> @@ -100,7 +102,7 @@ static inline void __indirect_writesw(volatile void
> __iomem *bus_addr,
> const u16 *vaddr = p;
>
> while (count--)
> - writew(*vaddr++, bus_addr);
> + writew((u16 __force)cpu_to_le32(*vaddr++), bus_addr);
> }
...
> Does that make sense to you? This is essentially the same thing we already
> do for inw/inl/outw/outl.
Well, we may need something like this. It seems writesw() (and thus
__indirect_writesw()) etc. use le16 values (preserving byte order), so
the above should probably use le16_to_cpu() instead (and le32_to_cpu in
__indirect_writesl()). I think the only thing that can use it on my hw
is VIA PATA adapter (throught ioread32_rep() etc). I will have to dig it
up as well.
I wouldn't rather touch this stuff without verifying that it fixes
things up.
Thanks for looking into this.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > ixp4xx is really special in that it performs hardware swapping for > internal devices based on CPU endianess but not on PCI devices. Again, IXP4xx does not perform hardware (nor any other) swapping for registers of on-chip devices. The registers are connected 1:1, bit 0 to bit 0 etc. (Yes, IXP4xx can be optionally programmed for such swapping, depending on silicon revision, but it is not used in mainline kernel). The only hardware swapping happens on PCI bus (in BE mode), to be compatible with other platforms and non-IXP4xx-specific PCI drivers. > Coming back to the specific pxa25x_udc case: using __raw_* accessors > in the driver would possibly end up breaking the PXA25x machines in > the (very unlikely) case that someone wants to make it work with > big-endian kernels, assuming it does not have the same hardware > byteswap logic as ixp4xx. I'd expect both CPUs to behave in exactly the same manner, i.e., to not swap anything on the internal bus. If true, it would mean it should "just work" in both BE and LE modes (including BE mode on PXA, should it be actually possible). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > Both writes leave the CPU core within the spinlock but are not serialized > with anything else, so there is no ordering between the CPUs when they > enter the shared bus, other than having address before data. You'd > expect to see address0, data0, address1, data1, but it could also > be e.g. address0, address1, data1, data0. Ah, so it's a matter of flushing the write buffers before exiting the spinlock-protected code. > The point is more what the common case is. Almost all machines we support > have fixed-endian devices, and the drivers are correct when using readl() > or in_le32() but are not endian-safe when using __raw_readl(). Sure, e.g. PCI does it this way (eventually swapping the data lanes if needed). > Using __raw_readl() has the big danger of someone accidentally "fixing" > the driver to work like all the others in order to solve a theoretical > endian problem, while it should really be made obvious that the hardware > actively swaps its data on the bus. Sure - if this is the case. On-chip IXP4xx peripherals don't swap data at all (i.e., they match CPU endianess) - accessing their registers is like accessing a normal CPU register. That's why they don't use PCI-style readl() etc. - however a better name than __raw_* would probably help here. Using __raw_* in a PCI driver would be generally wrong. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH v2 01/15] arm: use of_platform_default_populate() to populate default bus
Kefeng Wang writes: > Use helper of_platform_default_populate() in linux/of_platform > when possible, instead of calling of_platform_populate() with > the default match table. > --- a/arch/arm/mach-cns3xxx/core.c > +++ b/arch/arm/mach-cns3xxx/core.c > @@ -395,8 +395,7 @@ static void __init cns3xxx_init(void) > > pm_power_off = cns3xxx_power_off; > > - of_platform_populate(NULL, of_default_bus_match_table, > -cns3xxx_auxdata, NULL); > + of_platform_default_populate(NULL, cns3xxx_auxdata, NULL); > } > Doesn't look wrong :-) Acked-by: Krzysztof Halasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > The barriers on a spinlock synchronize between CPUs but not an external > bus, so (on some architectures) a spinlock protecting an MMIO register > does not guarantee that two CPUs doing > > spin_lock(); > __raw_writel(address); > __raw_writel(data); > spin_unlock(); > > would cause pairs of address/data to be seen on the bus. > > Of course this is meaningless on ixp4xx, as there is only one CPU. I still don't get it. If the spinlocks synchronize between CPUs, there can only be one CPU (or core) doing the pair of raw_writel(), so how would it be possible to not get the address/data pair written out? IOW, how is it different from a system with a single CPU? > On powerpc, we have in_le32/in_be32 for SoC-internal register access, > while only PCI devices are allowed to be accessed using readl(). Yeah, this seems like a sane solution. > I would suggest using an ixp4xx specific set of accessors that comes down > to either readl() or ioread32_be(), depending on whether CONFIG_CPU_BIG_ENDIAN > is set. That makes it clear that there is a magic bus involved and that it > works on this platform but not in portable code. Hmm. This is actually the opposite - while there may be some magic (swapping) in readl() and friends, there is absolutely no magic in the __raw_readl() etc. They are essentially equivalent to *(volatile u32 *)ptr. This is constant and doesn't depend on endianess, PCI, anything. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > I consider the use of __raw_* accessors a bug, I don't think we should > ever do that because it hides how the hardware actually works, it doesn't > work with spinlocks, and it can lead to the compiler splitting up accesses > into byte sized ones (not on ARM with the current definition, but > possible in general). Well, then maybe we should fix them, or add another set. Why don't they work with spinlocks? To be honest, I remember this was already discussed a bit years ago. I think I proposed back then a set of read_le32 (which would be equivalent of current readl(), and could be named pci_readl() as well), read_be32, read_host (without swapping). The names could be better, though. > Almost all hardware is fixed-endian, so you have to use swapping accessors > when the CPU is the other way, except for device RAM and FIFO registers > that are always used to transfer a byte stream (see the definition of > readsl() and memcpy_fromio()). When you have hardware that adds byteswaps > on the bus interface, you typically end up with MMIO registers requiring > no swap (or double swap) and readsl()/memcpy_fromio()) suddenly requiring > a swap that is counterintuitive. Sure, but the __raw_* are used just to be sure there is absolutely no swapping. E.g. for IXP4xx, the registers never require swapping, thus readl() etc. are not suitable for this. What is needed here is simple "atomic" 32-bit straight to/from register (MM)IO (assuming 4-byte address alignment). If not __raw_* then what? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: >> Anyway, I think readl()/writel() do the right thing: in BE mode they >> swap PCI accesses and don't swap normal registers, in LE mode nothing is >> swapped. > > This seems to be true when CONFIG_IXP4XX_INDIRECT_PCI is set, but > not otherwise. For the indirect variant, writel() is a __raw_writel() > without barrier or byteswap on non-PCI memory, while with that > option disabled, we use the standard implementation that has both > a byteswap and a barrier. > > According to your description, that would mean the version without > indirect PCI access is broken and it appears to have been that way > since before the start of git history in 2.6.12. > > It's possible that nobody cared because all drivers for non-PCI > devices on ixp4xx (the on chip ones) just use __raw_readl or > direct pointer references. Well, it is possible. I recall I probably used __raw_* instead of readl()/writel() in qmgr/npe/Ethernet drivers for this very reason. I think Direct pointer references are only used for locations in RAM (DMA buffers), they have their own share of problems because the peripherals are always big endian. I think I still have an early IXP425 board with UDC connector so I will try to dig it up and test this code. I wonder if we should switch the driver to use __raw_*, too. The problem is almost nobody uses UDC with this CPU. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 2/3] ARM: ixp4xx: avoid warning about ioport_map() macro argument processing
Arnd Bergmann writes: > We get a harmless compiler warning when building the cs89x0 driver > for ixp4xx: > > drivers/net/ethernet/cirrus/cs89x0.c: In function 'cs89x0_ioport_probe': > drivers/net/ethernet/cirrus/cs89x0.c:1602:28: warning: suggest parentheses > around '+' in operand of '&' [-Wparentheses] > io_mem = ioport_map(ioport & ~3, NETCARD_IO_EXTENT); > > Simply adding parentheses in the macro avoids the warning > > Signed-off-by: Arnd Bergmann > --- > arch/arm/mach-ixp4xx/include/mach/io.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/3] ARM: ixp4xx: move indirect I/O into common-pci.c
Arnd Bergmann writes: > The pointer comparison in is_pci_memory() confuses gcc, so it > starts throwing bogus warnings about the use of possibly uninitialized > variables: > > drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset': > arch/arm/include/asm/io.h:101:2: error: 'px_is' may be used uninitialized in > this function [-Werror=maybe-uninitialized] > drivers/crypto/ixp4xx_crypto.c: In function 'aead_perform': > drivers/crypto/ixp4xx_crypto.c:1072:5: error: 'lastlen' may be used > uninitialized in this function [-Werror=maybe-uninitialized] > > The code is that gets warned about is correct, and we should not warn > about this. Moving the code into a .c file makes the object files > smaller and avoids the warnings. > > Signed-off-by: Arnd Bergmann > --- > arch/arm/mach-ixp4xx/common-pci.c | 98 > ++ > arch/arm/mach-ixp4xx/include/mach/io.h | 95 +++----- > 2 files changed, 104 insertions(+), 89 deletions(-) Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 1/3] ARM: ixp4xx: use normal prototype for {read,write}s{b,w,l}
Arnd Bergmann writes: > ixp4xx defines the arguments to its __indirect_writesb() and other > functions as pointers to fixed-size data. This is not necessarily > wrong, and it works most of the time, but it causes warnings in > at least one driver: > > drivers/net/ethernet/smsc/smc91x.c: In function 'smc_rcv': > drivers/net/ethernet/smsc/smc91x.c:495:21: error: passing argument 2 of > '__indirect_readsw' from incompatible pointer type > [-Werror=incompatible-pointer-types] >SMC_PULL_DATA(lp, data, packet_len - 4); > > All other definitions of the same functions pass void pointers, > so doing the same here avoids the warnings. > > Signed-off-by: Arnd Bergmann > --- > arch/arm/mach-ixp4xx/include/mach/io.h | 23 +-- > 1 file changed, 17 insertions(+), 6 deletions(-) Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] ARM: drop unused Makefile.boot of Multiplatform SoCs
Masahiro Yamada writes: > The variable "MACHINE" is empty if CONFIG_ARCH_MULTIPLATFORM=y, > so these Makefile.boot files are never included. > > Signed-off-by: Masahiro Yamada > --- > > arch/arm/mach-cns3xxx/Makefile.boot | 3 --- Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > The unclear part here is for IXP4xx, which supports both big-endian > and little-endian configurations. So far, the driver has done > no byteswap in either case. I suspect that is wrong and it would > actually need to swap in one or the other case, but I don't know > which. If at all, I guess it should swap in LE mode. But it's far from certain. > It's also possible that there is some magic setting in > the chip that makes the endianess of the MMIO register match the > CPU, and in that case, the code actually does the right thing > for all configurations, both before and after this patch. This is IMHO most probable. Actually, the IXP4xx is "natural" in BE mode (except for PCI) and normally in LE mode it's order-coherent, meaning 32-bit "integer" accesses need no swapping, but 8-bit and (mostly unused) 16-bit transfers need swapping. Anyway, I think readl()/writel() do the right thing: in BE mode they swap PCI accesses and don't swap normal registers, in LE mode nothing is swapped. LE data-coherent mode (which has never landed in the official kernel) is a bit different, but still, readl()/writel() do the right thing. I remember the "string" (block) functions preserve 8-bit ordering, and thus 32-bit values transfered using them may need swapping. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
Arnd Bergmann writes: > This addresses both issues by moving all the definitions into the > pxa25x_udc driver itself. It turns out the only difference between > them was 'UDCCS_IO_ROF', and that could well be a mistake when it > was incorrectly copied from pxa25x to ixp4xx. > > Signed-off-by: Arnd Bergmann Both the programming manual and the Intel's library code say it's bit 2. Must be a mistake in the old code. Unfortunately I think we won't be able to check it on real hardware, the UDC on IXP4xx boards isn't usually connected. Also, this bug wouldn't likely affect operation (unless there is an overflow event, which would be currently undetected). I guess we as well very safely change it to 1 << 2. For the IXP4xx part, Acked-by: Krzysztof Halasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] ARM: ixp4xx: fix read{b,w,l} return types
Arnd Bergmann writes: > On ixp4xx, the readl() function returns an 'unsigned long' output > when indirect I/O is used. This is unlike any other platform, and > it causes lots of harmless compiler warnings, such as: > > drivers/ata/libahci.c: In function 'ahci_show_host_version': > drivers/ata/libahci.c:254:22: warning: format '%x' expects argument of type > 'unsigned int', but argument 3 has type 'long unsigned int' [-Wformat=] > drivers/block/mtip32xx/mtip32xx.c: In function 'mtip_hw_read_registers': > drivers/block/mtip32xx/mtip32xx.c:2602:31: warning: format '%X' expects > argument of type 'unsigned int', but argument 3 has type 'long unsigned int' > [-Wformat=] > drivers/block/cciss.c: In function 'print_cfg_table': > drivers/block/cciss.c:3845:25: warning: format '%d' expects argument of type > 'int', but argument 4 has type 'long unsigned int' [-Wformat=] > > This changes all six of the ixp4xx specific I/O read functions > to return the same types that we have in the normal asm/io.h, > to avoid the warnings. Thanks for fixing this. Acked-by: Krzysztof Halasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ixp4xx_eth: Delete an unnecessary check before the function call "dma_pool_destroy"
SF Markus Elfring writes:
> The dma_pool_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
> --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
> +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> @@ -1192,7 +1192,7 @@ static void destroy_queues(struct port *port)
> port->desc_tab = NULL;
> }
>
> - if (!ports_open && dma_pool) {
> + if (!ports_open) {
> dma_pool_destroy(dma_pool);
> dma_pool = NULL;
> }
Acked-by: Krzysztof Halasa
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ixp4xx_hss: Delete an unnecessary check before the function call "dma_pool_destroy"
SF Markus Elfring writes:
> The dma_pool_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
> --- a/drivers/net/wan/ixp4xx_hss.c
> +++ b/drivers/net/wan/ixp4xx_hss.c
> @@ -1037,7 +1037,7 @@ static void destroy_hdlc_queues(struct port *port)
> port->desc_tab = NULL;
> }
>
> - if (!ports_open && dma_pool) {
> + if (!ports_open) {
> dma_pool_destroy(dma_pool);
> dma_pool = NULL;
> }
Acked-by: Krzysztof Halasa
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ARM: cns3xxx: pci: avoid potential stack overflow
Hi, Arnd Bergmann writes: > With those two changes in place, we no longer need the fake > pci_sys_data/pci_bus structures for faking config space writes, > and the stack usage goes down as well. > arch/arm/mach-cns3xxx/pcie.c | 71 > + I'm ATM unable to test this change, but will do that at some point. Meanwhile, I guess there is nothing I can say against this patch. Thanks. Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: H264 headers generation for driver
Andrey Utkin writes: [H.264 headers] > I guess that one acceptable way is to pre-generate all headers for all > needed cases and ship them inlined; for correctness checking purpose, > it is possible to ship also a script or additional source code file > which is able to generate same headers. I think these are good ideas. BTW the SOLO6110 at the moment also has pregenerated SPS/PPS headers (whole NALs actually). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] defines modified to match the 80-char rule
Joe Perches writes: >> So, could we have the localized readability when it makes sense, >> and the default rules when nothing else applies? > > Then the question becomes how local. The answer seems easy to me: as much as it's needed to get the best readability. If that means 4 or 2 custom formatted lines, great. Why not? It should be the maintainer's call anyway. It's (s)he who has to work with the code in question most. >> OTOH I think the 80 columns rule should go. > > I have no issue with that. > There should be some limit though. Sure, the max readability should set the limit. Though I would say one shouldn't exceed maybe 132 chars (this is what old VT100 had, and this is also supported on PC since some 8-bit XT-BUS VGA cards). 132 chars also allow for full 80-char "console output text" lines. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] defines modified to match the 80-char rule
Joe Perches writes: > Nah, you're not an extremist, you're just preferring narrowly > localized readability over global consistency. > > That's fine and all, until you come up to LCONSOLE_ERROR_MSG > type use which blows the nice old formatting up. > > So what I suggested is just a simple consistency thing. So, could we have the localized readability when it makes sense, and the default rules when nothing else applies? OTOH I think the 80 columns rule should go. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] defines modified to match the 80-char rule
Joe Perches writes: > -#define LCONSOLE(mask, format, ...) CDEBUG(D_CONSOLE | (mask), format, ## > __VA_ARGS__) > -#define LCONSOLE_INFO(format, ...) CDEBUG_LIMIT(D_CONSOLE, format, ## > __VA_ARGS__) > -#define LCONSOLE_WARN(format, ...) CDEBUG_LIMIT(D_CONSOLE | D_WARNING, > format, ## __VA_ARGS__) > -#define LCONSOLE_ERROR_MSG(errnum, format, ...) CDEBUG_LIMIT(D_CONSOLE | > D_ERROR, \ > -"%x-%x: " format, errnum, LERRCHKSUM(errnum), ## > __VA_ARGS__) > -#define LCONSOLE_ERROR(format, ...) LCONSOLE_ERROR_MSG(0x00, format, ## > __VA_ARGS__) > +#define LCONSOLE(mask, fmt, ...) \ > + CDEBUG(D_CONSOLE | (mask), fmt, ##__VA_ARGS__) > +#define LCONSOLE_INFO(fmt, ...) > \ > + CDEBUG_LIMIT(D_CONSOLE, fmt, ##__VA_ARGS__) > +#define LCONSOLE_WARN(fmt, ...) > \ > + CDEBUG_LIMIT(D_CONSOLE | D_WARNING, fmt, ##__VA_ARGS__) > +#define LCONSOLE_ERROR_MSG(errnum, fmt, ...) \ > + CDEBUG_LIMIT(D_CONSOLE | D_ERROR, "%x-%x: " fmt,\ > + errnum, LERRCHKSUM(errnum), ##__VA_ARGS__) I don't find it better, actually I think it's much harder to read. Maybe that's just me. Call me extremist, but I think I could even like the following :-) #define CWARN(format, ...)CDEBUG_LIMIT(D_WARNING, format, ## __VA_ARGS__) #define CERROR(format, ...)CDEBUG_LIMIT(D_ERROR,format, ## __VA_ARGS__) #define CNETERR(format, ...)CDEBUG_LIMIT(D_NETERROR, format, ## __VA_ARGS__) #define CEMERG(format, ...)CDEBUG_LIMIT(D_EMERG,format, ## __VA_ARGS__) -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] defines modified to match the 80-char rule
Mario Bambagini writes:
> Defines have been written in more than one line to match the 80-character
> rule. This error has been fixed 6 times in this file.
> The file is fully compliant with respect to the coding rules now.
Rules, maybe. But is it better, i.e., more readable?
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
> @@ -233,14 +233,20 @@ do {
> \
> #define CNETERR(format, a...)CDEBUG_LIMIT(D_NETERROR, format, ## a)
> #define CEMERG(format, ...) CDEBUG_LIMIT(D_EMERG, format, ## __VA_ARGS__)
>
> -#define LCONSOLE(mask, format, ...) CDEBUG(D_CONSOLE | (mask), format, ##
> __VA_ARGS__)
> -#define LCONSOLE_INFO(format, ...) CDEBUG_LIMIT(D_CONSOLE, format, ##
> __VA_ARGS__)
> -#define LCONSOLE_WARN(format, ...) CDEBUG_LIMIT(D_CONSOLE | D_WARNING,
> format, ## __VA_ARGS__)
> -#define LCONSOLE_ERROR_MSG(errnum, format, ...) CDEBUG_LIMIT(D_CONSOLE |
> D_ERROR, \
> -"%x-%x: " format, errnum, LERRCHKSUM(errnum), ##
> __VA_ARGS__)
> -#define LCONSOLE_ERROR(format, ...) LCONSOLE_ERROR_MSG(0x00, format, ##
> __VA_ARGS__)
> +#define LCONSOLE(mask, format, ...) \
> + CDEBUG(D_CONSOLE | (mask), format, ## __VA_ARGS__)
> +#define LCONSOLE_INFO(format, ...) \
> + CDEBUG_LIMIT(D_CONSOLE, format, ## __VA_ARGS__)
> +#define LCONSOLE_WARN(format, ...) \
> + CDEBUG_LIMIT(D_CONSOLE | D_WARNING, format, ## __VA_ARGS__)
> +#define LCONSOLE_ERROR_MSG(errnum, format, ...)
> \
> + CDEBUG_LIMIT(D_CONSOLE | D_ERROR, "%x-%x: " format, \
> + errnum, LERRCHKSUM(errnum), ## __VA_ARGS__)
> +#define LCONSOLE_ERROR(format, ...) \
> + LCONSOLE_ERROR_MSG(0x00, format, ## __VA_ARGS__)
>
> -#define LCONSOLE_EMERG(format, ...) CDEBUG(D_CONSOLE | D_EMERG, format, ##
> __VA_ARGS__)
> +#define LCONSOLE_EMERG(format, ...) \
> + CDEBUG(D_CONSOLE | D_EMERG, format, ## __VA_ARGS__)
>
> int libcfs_debug_msg(struct libcfs_debug_msg_data *msgdata,
> const char *format1, ...)
... I don't think so. Perhaps if I wasn't using the bleading edge tech
132-column digital flat LCD screen, I would see this differently (Emacs
isn't perfect when displaying long lines on IBM monochrome display
adapter, even with the intelligent-long-lines-wrap package).
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Coding style details (checkpatch)
Joe Perches writes:
> #define VDREG8(a0) ((const u16[]){\
> a0 + 0x000, a0 + 0x010, a0 +0x020, a0 + 0x030, \
> a0 + 0x100, a0 + 0x110, a0 +0x120, a0 + 0x130})
>
> as "const u16[]" is a $Type but "const u16[]" is not.
>
> Still, as written, the code seems fragile as MACRO[index]
> allows index to be any value, maybe larger than the array.
Right.
XXX[8] is meant as an additional mental check. Not very effective,
though I think certain GCCs can issue a warning for obvious
out-of-bounds accesses (probably with both [] and [8]).
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
Re: Coding style details (checkpatch)
Joe Perches writes:
> It might be better to use some base + index macro
> as it could be smaller object code.
>
> Something like:
>
> #define REG_NO(base, multiplier, index) (base + (multiplier * index))
>
> reg_write(vc->dev, REG_NO(0x10, 1, vc->ch), dma_cfg);
> or
>
> #define VDMA_CHANNEL_CONFIG 0x10
>
> reg_write(vc->dev, REG_NO(VDMA_CHANNEL_CONFIG, 1, vc->ch), dma_cfg);
Wouldn't work, the register map is a bit messy.
E.g.
#define DMA_PAGE_TABLE0_ADDR((const u16[8]){0x08, 0xD0, 0xD2, 0xD4, 0xD6,
0xD8, 0xDA, 0xDC})
#define DMA_PAGE_TABLE1_ADDR((const u16[8]){0x09, 0xD1, 0xD3, 0xD5, 0xD7,
0xD9, 0xDB, 0xDD})
also
#define VDREG8(a0) ((const u16[8]){ \
a0 + 0x000, a0 + 0x010, a0 +0x020, a0 + 0x030, \
a0 + 0x100, a0 + 0x110, a0 +0x120, a0 + 0x130})
#define VIDSTAT VDREG8(0x100)
#define BRIGHT VDREG8(0x101)
#define CONTRASTVDREG8(0x102)
etc.
One would have to remember (writing .c code) which scheme applies to
which access. The tables, while probably less than optimal WRT CPU
cycles used, are consistent, and the addressing details are grouped in
one place - the *regs.h file.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Add PCI ID and quirk for Intersil/Techwell TW686[4589] AV capture cards.
Bjorn Helgaas writes: >> Intersil/Techwell TW686[4589]-based video capture cards have an empty >> (zero) class code. Fix it. >> > Applied to pci/misc for v4.2, with minor tweak as below to use > PCI_CLASS_MULTIMEDIA_OTHER instead of a bare number. Let me know > if you see any issues with this. Thanks! Can't see any, thanks a lot. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
Re: Coding style details (checkpatch)
Joe Perches writes:
> How is the macro used?
> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3, a0 + 4, a0 + 5,
a0 + 6, a0 + 7})
#define REG8_2(a0) ((const u16[8]){a0, a0 + 2, a0 + 4, a0 + 6, a0 + 8, a0 +
0xA, a0 + 0xC, a0 + 0xE})
#define REG8_8(a0) ((const u16[8]){a0, a0 + 8, a0 + 0x10, a0 + 0x18, a0 + 0x20,
a0 + 0x28, a0 + 0x30, a0 + 0x38})
#define VDMA_CHANNEL_CONFIG REG8_1(0x10)
#define ADMA_P_ADDR REG8_2(0x18)
#define ADMA_B_ADDR REG8_2(0x19)
#define INTL_HBAR_CTRL REG8_1(0x30)
#define VIDEO_FIELD_CTRLREG8_1(0x39)
#define HSCALER_CTRLREG8_1(0x42)
#define VIDEO_SIZE REG8_1(0x4A)
#define VIDEO_SIZE_F2 REG8_1(0x52)
#define MD_CONF REG8_1(0x60)
#define MD_INIT REG8_1(0x68)
#define MD_MAP0 REG8_1(0x70)
#define VDMA_P_ADDR REG8_8(0x80) /* not used in DMA SG mode */
#define VDMA_WHPREG8_8(0x81)
#define VDMA_B_ADDR REG8_8(0x82)
#define VDMA_F2_P_ADDR REG8_8(0x84)
#define VDMA_F2_WHP REG8_8(0x85)
#define VDMA_F2_B_ADDR REG8_8(0x86)
then
reg_write(vc->dev, VDMA_CHANNEL_CONFIG[vc->ch], dma_cfg);
reg_write(dev, SAT_U[ch], ctrl->val);
reg_write(dev, SAT_V[ch], ctrl->val);
etc.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/
Re: Coding style details (checkpatch)
Frans Klaver writes: > Ah, right. One might say that this is a false positive, but that's up > to Joe or Andy I guess. > > It may be valid C code, but I think this construction is slightly > funky to begin with. > > Which file is this? A new file, not yet sent anywhere. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
Re: Coding style details (checkpatch)
Frans Klaver writes:
>> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
>>
>> vs
>>
>> #define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
>> ^
>
> The prescribed style is to have no space between cast and castee. So,
> the top option.
Thanks. That's what I thought. It looks that checkpatch doesn't like
this:
ERROR: space required before the open brace '{'
+#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
Does this qualify as the "false positive"?
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MEDIA PCI Kconfig ANALOG_TV -> CAMERA
Hi, I noticed certain cards are currently under MEDIA_ANALOG_TV_SUPPORT but it seems they are frame grabbers (with CVBS, Svideo etc. inputs) rather than TV receivers (with analog TV tuners). MEDIA_CAMERA_SUPPORT maybe isn't the best name (only "meye" driver seems to drive a real camera in a laptop) but it at least doesn't select the TUNERs. Perhaps the following patch would make sense. Signed-off-by: Krzysztof Hałasa --- a/drivers/media/pci/Kconfig +++ b/drivers/media/pci/Kconfig @@ -11,16 +11,16 @@ if MEDIA_PCI_SUPPORT if MEDIA_CAMERA_SUPPORT comment "Media capture support" source "drivers/media/pci/meye/Kconfig" +source "drivers/media/pci/solo6x10/Kconfig" source "drivers/media/pci/sta2x11/Kconfig" +source "drivers/media/pci/tw68/Kconfig" +source "drivers/media/pci/zoran/Kconfig" endif if MEDIA_ANALOG_TV_SUPPORT comment "Media capture/analog TV support" source "drivers/media/pci/ivtv/Kconfig" -source "drivers/media/pci/zoran/Kconfig" source "drivers/media/pci/saa7146/Kconfig" -source "drivers/media/pci/solo6x10/Kconfig" -source "drivers/media/pci/tw68/Kconfig" endif if MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Coding style details
Hi,
a simple question: which style is preferred?
#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
vs
#define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
^
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add PCI ID and quirk for Intersil/Techwell TW686[4589] AV capture cards.
Intersil/Techwell TW686[4589]-based video capture cards have an empty
(zero) class code. Fix it.
Signed-off-by: Krzysztof Hałasa
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3662,6 +3662,26 @@ DECLARE_PCI_FIXUP_HEADER(0x1283, 0x8892,
quirk_use_pcie_bridge_dma_alias);
DECLARE_PCI_FIXUP_HEADER(0x8086, 0x244e, quirk_use_pcie_bridge_dma_alias);
/*
+ * Intersil/Techwell TW686[4589]-based video capture cards have an empty (zero)
+ * class code. Fix it.
+ */
+
+static void quirk_tw686x_class(struct pci_dev *pdev)
+{
+ /* Use "Multimedia controller" class */
+ pdev->class = 0x048001;
+ dev_info(&pdev->dev, "TW686x PCI class workaround enabled\n");
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6864, PCI_CLASS_NOT_DEFINED, 0,
+ quirk_tw686x_class);
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6865, PCI_CLASS_NOT_DEFINED, 0,
+ quirk_tw686x_class);
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6868, PCI_CLASS_NOT_DEFINED, 0,
+ quirk_tw686x_class);
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x1797, 0x6869, PCI_CLASS_NOT_DEFINED, 0,
+ quirk_tw686x_class);
+
+/*
* AMD has indicated that the devices below do not support peer-to-peer
* in any system where they are found in the southbridge with an AMD
* IOMMU in the system. Multifunction devices that do not support
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: cns3xxx: pci: avoid potential stack overflow
Arnd Bergmann writes:
> The cns3xxx_pcie_hw_init function uses excessive kernel
> stack space because of a hack that puts a fake struct
> pci_sys_data and struct pci_bus on the stack in order to
> call the generic pci_bus_read_config accessors, which causes
> a warning in ARM allmodconfig builds:
>
> arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is
> larger than 1024 bytes
>
> This rewrites the code in question to use a private
> implementation of the config space access for the same
> purpose, getting rid of the local variables and the
> warning in the process. As part of this, we have to
> use an open-coded version of pci_bus_find_capability(),
> which unfortunately complicates the implementation.
Wouldn't it be better to simply use static structs for this purpose?
The hack isn't pretty, though.
> + regs = cnspci->cfg0_regs + (PCI_DEVFN(1, 0) << 12);
> +
Some comment about would be helpful. Such as this:
> - bus.number = 1; /* directly connected PCIe device */
^
> + pos = cns3xxx_pci_raw_read_config(regs, PCI_CAPABILITY_LIST, 1);
> + while (cns3xxx_pci_raw_read_config(regs, pos, 1) != PCI_CAP_ID_EXP)
> + pos = cns3xxx_pci_raw_read_config(regs, pos +
> PCI_CAP_LIST_NEXT, 1);
> +
I wonder if this can fail (i.e., no PCI_CAP_ID_EXP capability).
> + dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
> + dc &= ~(0x3 << 12); /* Clear Device Control Register [14:12] */
> + cns3xxx_pci_raw_write_config(regs, pos + PCI_EXP_DEVCTL, 2, dc);
> + dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
> + if (!(dc & (0x3 << 12)))
> + pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n");
> +
This seems to revert 367dc4b75f4349d5363bc3ebdc030939db944786. Why do
you want to do it?
--
Krzysztof Halasa
Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: On register r/w macros/procedures of drivers/media/pci
Andrey Utkin writes: > Please check first digit. I mean _5_864, in your post there's 6869. Ok, I just thought it may be a typo. I can now see 5864 is a H.264 encoder, while 686x are simpler frame-grabbers only. Sorry for the noise. -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: On register r/w macros/procedures of drivers/media/pci
Andrey Utkin writes: > I am starting a work on driver for techwell tw5864 media grabber&encoder. If this is tw6864 then I have a driver mostly completed. Actually I'm using tw6869 but I think this is very similar (4 channels instead of 8 and PCI instead of PCIe). I have 6864s but haven't yet tried using them for trivial reasons. This is BTW completely different chip (family) than the old tw68x. -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/15] ARM: cns3xxx: don't export static symbol
Arnd Bergmann writes: > Looks good. Krzysztof, do you want to pick this up and send me a > pull request together with other patches, or should I apply this > to the arm-soc fixes directly? The latter, please. Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/11] ARM: ixp4xx: fix {in,out}s{bwl} data types
Arnd Bergmann writes: > Most platforms use void pointer arguments in these functions, but > ixp4xx does not, which triggers lots of warnings in device drivers like: > > net/ethernet/8390/ne2k-pci.c: In function 'ne2k_pci_get_8390_hdr': > net/ethernet/8390/ne2k-pci.c:503:3: warning: passing argument 2 of 'insw' > from incompatible pointer type >insw(NE_BASE + NE_DATAPORT, hdr, sizeof(struct e8390_pkt_hdr)>>1); >^ > In file included from include/asm/io.h:214:0, > from /git/arm-soc/include/linux/io.h:22, > from /git/arm-soc/include/linux/pci.h:31, > from net/ethernet/8390/ne2k-pci.c:48: > mach-ixp4xx/include/mach/io.h:316:91: note: expected 'u16 *' but argument is > of type 'struct e8390_pkt_hdr *' > static inline void insw(u32 io_addr, u16 *vaddr, u32 count) > > Fixing the drivers seems hopeless, so this changes the ixp4xx code > to do the same as the others to avoid the warnings. Acked-by: Krzysztof Halasa -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net:wan:Change location of debug printk statement in the function,hss_hdlc_poll
Nicholas Krause writes: > This changes the location of the printk for montioring if the stucture pointer > desc of type structure desc has a error count due to failing in the switch > statement for checking it's status from the default switch case in this switch > to the area of the function for checking if there is a error count. Due to > this > this part of the function can now be uncommented as it's now needed for > printing > out important information if the structure pointer,desc has a error count for > debugging purposes. This error count seems to be secondary to the desc->status. It's isn't normally logged (RX errors on serial links are quite normal) and only the "default" case, normally not seen, would be logged (to indicate unhandled condition). In the "normal" error path, the driver ignores error count, because there could be several errors counted for a single erroneous frame (or non-frame). Also the patch seems to remove the beginning of a preprocessor directive (#if) without removing its end (#endif). I'd be surprised if it compiles. > --- a/drivers/net/wan/ixp4xx_hss.c > +++ b/drivers/net/wan/ixp4xx_hss.c > @@ -697,7 +697,6 @@ static int hss_hdlc_poll(struct napi_struct *napi, int > budget) > } > > desc = rx_desc_ptr(port, n); > -#if 0 /* FIXME - error_count counts modulo 256, perhaps we should use it */ > if (desc->error_count) > printk(KERN_DEBUG "%s: hss_hdlc_poll status 0x%02X" > " errors %u\n", dev->name, desc->status, > @@ -735,9 +734,7 @@ static int hss_hdlc_poll(struct napi_struct *napi, int > budget) > dev->stats.rx_length_errors++; > dev->stats.rx_errors++; > break; > - default:/* FIXME - remove printk */ > - netdev_err(dev, "hss_hdlc_poll: status 0x%02X errors > %u\n", > -desc->status, desc->error_count); > + default: > dev->stats.rx_errors++; > } -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/16] ARM: cns3xxx: convert PCI to use generic config accesses
Rob Herring writes: > Convert the cns3xxx PCI driver to use the generic config access functions. > > This changes accesses from __raw_readl/__raw_writel to readl/writel. > > arch/arm/mach-cns3xxx/pcie.c | 52 > +--- This looks fine: Acked-by: Krzysztof Hałasa -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Fix unbalanced mutex in dma_pool_create().
dma_pool_create() needs to unlock the mutex in error case.
The bug has been present starting with v3.16-rc1 (cc6b664a).
Signed-off-by: Krzysztof Hałasa
Cc: sta...@vger.kernel.org
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -174,11 +174,11 @@ struct dma_pool *dma_pool_create(const char *name, struct
device *dev,
mutex_lock(&pools_lock);
if (list_empty(&dev->dma_pools) &&
device_create_file(dev, &dev_attr_pools)) {
kfree(retval);
- return NULL;
+ retval = NULL;
} else
list_add(&retval->pools, &dev->dma_pools);
mutex_unlock(&pools_lock);
return retval;
--
Krzysztof Halasa
Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] CNS3xxx: Fix PCIe early iotable_init().
Arnd Bergmann writes: > Applied to next/fixes-non-critical. > > Since the problem is not new and you didn't mark the patch as 'stable', > this seems to be the right place. Let us know if you need backports > to stable kernels, also fo the other patch. Sure, BTW there is also that problem with PCI bridge configuration or something (maybe on certain boards only) which I can't fix at the moment, but will do eventually. So absolutely no rush with this/these patch(es) alone. I just want to make CNS3xxx some valid platform (the same with IXP4xx and I also have some i.MX6 stuff) but it has to wait for now. Thanks, -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH2] CNS3xxx: Fix a WARN() related to IRQ allocation.
WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_basesfrom cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq from init_IRQ+0x24/0x2c
init_IRQ from start_kernel+0x1a8/0x338
start_kernel from 0x2000806c
The problem is that 64 CNS3xxx CPU interrupts, starting at 32, are
allocated by the ARM platform-independent code (as requested by
machine_desc->nr_irqs = 96), and then the GIC code tries to allocate
them again.
Tested on Gateworks Laguna board, masqueraded as CNS3420VB.
Signed-off-by: Krzysztof Hałasa
--- a/arch/arm/mach-cns3xxx/cns3420vb.c
+++ b/arch/arm/mach-cns3xxx/cns3420vb.c
@@ -246,7 +246,6 @@ static void __init cns3420_map_io(void)
MACHINE_START(CNS3420VB, "Cavium Networks CNS3420 Validation Board")
.atag_offset= 0x100,
- .nr_irqs= NR_IRQS_CNS3XXX,
.map_io = cns3420_map_io,
.init_irq = cns3xxx_init_irq,
.init_time = cns3xxx_timer_init,
--- a/arch/arm/mach-cns3xxx/core.c
+++ b/arch/arm/mach-cns3xxx/core.c
@@ -400,7 +400,6 @@ static const char *cns3xxx_dt_compat[] __initdata = {
DT_MACHINE_START(CNS3XXX_DT, "Cavium Networks CNS3xxx")
.dt_compat = cns3xxx_dt_compat,
- .nr_irqs= NR_IRQS_CNS3XXX,
.map_io = cns3xxx_map_io,
.init_irq = cns3xxx_init_irq,
.init_time = cns3xxx_timer_init,
--
Krzysztof Halasa
Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH2] CNS3xxx: Fix a WARN() related to IRQ allocation.
WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8() Cannot allocate irq_descs @ IRQ16, assuming pre-allocated Backtrace: gic_init_basesfrom cns3xxx_init_irq+0x24/0x34 cns3xxx_init_irq from init_IRQ+0x24/0x2c init_IRQ from start_kernel+0x1a8/0x338 start_kernel from 0x2000806c The problem is that 64 CNS3xxx CPU interrupts, starting at 32, are allocated by the ARM platform-independent code (as requested by machine_desc->nr_irqs = 96), and then the GIC code tries to allocate them again. Tested on Gateworks Laguna board, masqueraded as CNS3420VB. Signed-off-by: Krzysztof Hałasa -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] CNS3xxx: Fix PCIe early iotable_init().
khal...@piap.pl (Krzysztof Hałasa) writes: > This patch fixes the following BUG: > >> kernel BUG at mm/vmalloc.c:1132! >> PC is at vm_area_add_early+0x20/0x84 >> LR is at add_static_vm_early+0xc/0x60 >> >> The problem is cns3xxx_pcie_init() (device_initcall) calls the "early" >> iotable_init(). That's obviously: Signed-off-by: Krzysztof Hałasa -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] CNS3xxx: Fix PCIe early iotable_init().
This patch fixes the following BUG:
> kernel BUG at mm/vmalloc.c:1132!
> PC is at vm_area_add_early+0x20/0x84
> LR is at add_static_vm_early+0xc/0x60
>
> The problem is cns3xxx_pcie_init() (device_initcall) calls the "early"
> iotable_init().
Instead of merely calling the PCIe iotable_init() from
machine_desc->map_io(), this patch adds the required mappings to the
main CNS3xxx mapping table. This means we don't convert .pfn back to
virtual addresses in pcie.c. The size of the address space consumed for
PCIe control is reduced from 96 MiB (6 * 16 MiB) to about 32 MiB (this
doesn't include MMIO address space required by PCI devices):
- Size of the PCIe "host" mapping is reduced from 16 MiB to 4 KiB.
It's a PCI configuration address space for the local (on-chip) PCIe
bridge.
- Size of the "CFG0" mapping is reduced from 16 MiB to 64 KiB. It's for
Type 0 Configuration accesses, i.e., accesses to the single device
(with possible "functions") on the other end of the PCIe link.
We really only need a 4 KiB page at 0x8000 (see the offset
calculation in cns3xxx_pci_cfg_base()). There is still a potential
problem with PCI bus numbers > 0xF, are they supported?
- The code in cns3xxx_pci_cfg_base() and cns3xxx_pcie_hw_init() should
be clearer now.
- The maximum address space allocated for PCI MMIO is now correctly
shown in /proc/iomem as 176 MiB (per each of the two PCI "domains"
- previously only 16 MiB were reserved).
This patch has been tested on Gateworks Laguna board, masqueraded as
CNS3420VB.
--- a/arch/arm/mach-cns3xxx/core.c
+++ b/arch/arm/mach-cns3xxx/core.c
@@ -47,6 +47,38 @@ static struct map_desc cns3xxx_io_desc[] __initdata = {
.pfn= __phys_to_pfn(CNS3XXX_PM_BASE),
.length = SZ_4K,
.type = MT_DEVICE,
+#ifdef CONFIG_PCI
+ }, {
+ .virtual= CNS3XXX_PCIE0_HOST_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE0_HOST_BASE),
+ .length = SZ_4K,
+ .type = MT_DEVICE,
+ }, {
+ .virtual= CNS3XXX_PCIE0_CFG0_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE0_CFG0_BASE),
+ .length = SZ_64K, /* really 4 KiB at offset 32 KiB */
+ .type = MT_DEVICE,
+ }, {
+ .virtual= CNS3XXX_PCIE0_CFG1_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE0_CFG1_BASE),
+ .length = SZ_16M,
+ .type = MT_DEVICE,
+ }, {
+ .virtual= CNS3XXX_PCIE1_HOST_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE1_HOST_BASE),
+ .length = SZ_4K,
+ .type = MT_DEVICE,
+ }, {
+ .virtual= CNS3XXX_PCIE1_CFG0_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE1_CFG0_BASE),
+ .length = SZ_64K, /* really 4 KiB at offset 32 KiB */
+ .type = MT_DEVICE,
+ }, {
+ .virtual= CNS3XXX_PCIE1_CFG1_BASE_VIRT,
+ .pfn= __phys_to_pfn(CNS3XXX_PCIE1_CFG1_BASE),
+ .length = SZ_16M,
+ .type = MT_DEVICE,
+#endif
},
};
--- a/arch/arm/mach-cns3xxx/pcie.c
+++ b/arch/arm/mach-cns3xxx/pcie.c
@@ -23,15 +23,10 @@
#include "cns3xxx.h"
#include "core.h"
-enum cns3xxx_access_type {
- CNS3XXX_HOST_TYPE = 0,
- CNS3XXX_CFG0_TYPE,
- CNS3XXX_CFG1_TYPE,
- CNS3XXX_NUM_ACCESS_TYPES,
-};
-
struct cns3xxx_pcie {
- struct map_desc cfg_bases[CNS3XXX_NUM_ACCESS_TYPES];
+ void __iomem *host_regs; /* PCI config registers for host bridge */
+ void __iomem *cfg0_regs; /* PCI Type 0 config registers */
+ void __iomem *cfg1_regs; /* PCI Type 1 config registers */
unsigned int irqs[2];
struct resource res_io;
struct resource res_mem;
@@ -66,7 +61,6 @@ static void __iomem *cns3xxx_pci_cfg_base(struct pci_bus *bus,
int busno = bus->number;
int slot = PCI_SLOT(devfn);
int offset;
- enum cns3xxx_access_type type;
void __iomem *base;
/* If there is no link, just show the CNS PCI bridge. */
@@ -78,17 +72,21 @@ static void __iomem *cns3xxx_pci_cfg_base(struct pci_bus
*bus,
* we still want to access it. For this to work, we must place
* the first device on the same bus as the CNS PCI bridge.
*/
- if (busno == 0) {
- if (slot > 1)
- return NULL;
- type = slot;
- } else {
- type = CNS3XXX_CFG1_TYPE;
- }
+ if (busno == 0) { /* directly connected PCIe bus */
+ switch (slot) {
+ case 0: /* host bridge device, function 0 only */
+ base = cnspci->host_regs;
+
Re: [PATCH] CNS3xxx: Fix PCIe early iotable_init().
Arnd Bergmann writes: > Patch looks good, but please add the changeset description from > your first patch. I wonder if another approach would be better. I don't like the .pfn messing and the bugs that function introduces. -- Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
