[PATCH v4 4/4] hwmon: lm75: Handle broken device nodes gracefully
There is a logical flaw in lm75_probe() function introduced in
e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Note, that of_device_get_match_data() returns NULL when no match
is found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.
arch/powerpc/boot/dts/fsl/p2041rdb.dts:
lm75b@48 {
compatible = "nxp,lm75a";
reg = <0x48>;
};
In this case, the sensor is mistakenly considered as ADT75 variant.
Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov
---
drivers/hwmon/lm75.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 3e4374aa2f99..cd2cda4f557a 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -567,10 +567,17 @@ static int lm75_probe(struct i2c_client *client)
int status, err;
enum lm75_type kind;
- if (client->dev.of_node)
- kind = (enum lm75_type)of_device_get_match_data(&client->dev);
- else
+ if (dev->of_node) {
+ const struct of_device_id *match =
+ of_match_device(dev->driver->of_match_table, dev);
+
+ if (!match)
+ return -ENODEV;
+
+ kind = (enum lm75_type)(match->data);
+ } else {
kind = i2c_match_id(lm75_ids, client)->driver_data;
+ }
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
--
2.26.2
[PATCH v4 1/4] hwmon: lm75: Add lm75 to of_match list
Currently, many boards use just 'lm75' as a compatible string.
Signed-off-by: Matwey V. Kornilov
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 1 +
drivers/hwmon/lm75.c | 4
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index 96eed5cc7841..aec8edd1e0c6 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -13,6 +13,7 @@ maintainers:
properties:
compatible:
enum:
+ - lm75
- adi,adt75
- dallas,ds1775
- dallas,ds75
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..08cde1c446db 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -667,6 +667,10 @@ static const struct i2c_device_id lm75_ids[] = {
MODULE_DEVICE_TABLE(i2c, lm75_ids);
static const struct of_device_id __maybe_unused lm75_of_match[] = {
+ {
+ .compatible = "lm75",
+ .data = (void *)lm75
+ },
{
.compatible = "adi,adt75",
.data = (void *)adt75
--
2.26.2
[PATCH v4 2/4] hwmon: lm75: Add nxp,lm75a to of_match list
NXP LM75A is compatible with original LM75A while it has improved
11-bit precision.
https://www.nxp.com/docs/en/data-sheet/LM75A.pdf
Signed-off-by: Matwey V. Kornilov
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 1 +
drivers/hwmon/lm75.c | 11 +++
2 files changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index aec8edd1e0c6..8c3848f4c277 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -22,6 +22,7 @@ properties:
- national,lm75
- national,lm75a
- national,lm75b
+ - nxp,lm75a
- maxim,max6625
- maxim,max6626
- maxim,max31725
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 08cde1c446db..9c54c7d86771 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -33,6 +33,7 @@ enum lm75_type { /* keep sorted in alphabetical
order */
lm75,
lm75a,
lm75b,
+ nxp_lm75,
max6625,
max6626,
max31725,
@@ -182,6 +183,11 @@ static const struct lm75_params device_params[] = {
.default_resolution = 11,
.default_sample_time = MSEC_PER_SEC / 10,
},
+ [nxp_lm75] = {
+ .default_resolution = 11,
+ .default_sample_time = MSEC_PER_SEC / 10,
+ .resolution_limits = 9,
+ },
[max6625] = {
.default_resolution = 9,
.default_sample_time = MSEC_PER_SEC / 7,
@@ -644,6 +650,7 @@ static const struct i2c_device_id lm75_ids[] = {
{ "lm75", lm75, },
{ "lm75a", lm75a, },
{ "lm75b", lm75b, },
+ { "nxp_lm75a", nxp_lm75, },
{ "max6625", max6625, },
{ "max6626", max6626, },
{ "max31725", max31725, },
@@ -703,6 +710,10 @@ static const struct of_device_id __maybe_unused
lm75_of_match[] = {
.compatible = "national,lm75b",
.data = (void *)lm75b
},
+ {
+ .compatible = "nxp,lm75a",
+ .data = (void *)nxp_lm75
+ },
{
.compatible = "maxim,max6625",
.data = (void *)max6625
--
2.26.2
[PATCH v4 0/4] hwmon: lm75: Handle broken device nodes gracefully
This series is to fix a logical issue in lm75 driver. The actual fix is in the last patch. Other patches are present in order not to break existing users. Matwey V. Kornilov (4): hwmon: lm75: Add lm75 to of_match list hwmon: lm75: Add nxp,lm75a to of_match list hwmon: lm75: Add ti,lm75 to of_match list hwmon: lm75: Handle broken device nodes gracefully .../devicetree/bindings/hwmon/lm75.yaml | 3 ++ drivers/hwmon/lm75.c | 32 +-- 2 files changed, 32 insertions(+), 3 deletions(-) -- 2.26.2
[PATCH v4 3/4] hwmon: lm75: Add ti,lm75 to of_match list
Currently, armada-388-helios4.dts and nuvoton-npcm730-kudo.dts use
"ti,lm75" compatible string.
TI LM75A/B are compatible with original LM75A
https://www.ti.com/lit/ds/symlink/lm75a.pdf
https://www.ti.com/lit/ds/symlink/lm75b.pdf
Signed-off-by: Matwey V. Kornilov
---
Documentation/devicetree/bindings/hwmon/lm75.yaml | 1 +
drivers/hwmon/lm75.c | 4
2 files changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/lm75.yaml
b/Documentation/devicetree/bindings/hwmon/lm75.yaml
index 8c3848f4c277..721e77ce4390 100644
--- a/Documentation/devicetree/bindings/hwmon/lm75.yaml
+++ b/Documentation/devicetree/bindings/hwmon/lm75.yaml
@@ -32,6 +32,7 @@ properties:
- st,stds75
- st,stlm75
- microchip,tcn75
+ - ti,lm75
- ti,tmp100
- ti,tmp101
- ti,tmp105
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 9c54c7d86771..3e4374aa2f99 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -750,6 +750,10 @@ static const struct of_device_id __maybe_unused
lm75_of_match[] = {
.compatible = "microchip,tcn75",
.data = (void *)tcn75
},
+ {
+ .compatible = "ti,lm75",
+ .data = (void *)lm75a
+ },
{
.compatible = "ti,tmp100",
.data = (void *)tmp100
--
2.26.2
[PATCH v3] hwmon: lm75: Handle broken device nodes gracefully
There is a logical flaw in lm75_probe() function introduced in
commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Note, that of_device_get_match_data() returns NULL when no match
is found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.
arch/powerpc/boot/dts/fsl/p2041rdb.dts:
lm75b@48 {
compatible = "nxp,lm75a";
reg = <0x48>;
};
In this case, the sensor is mistakenly considered as ADT75 variant.
Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov
---
Changes since v2:
* fixed typo in the message
* fixed brackets
drivers/hwmon/lm75.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..53882c334a0d 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
int status, err;
enum lm75_type kind;
- if (client->dev.of_node)
- kind = (enum lm75_type)of_device_get_match_data(&client->dev);
- else
+ if (dev->of_node) {
+ const struct of_device_id *match =
+ of_match_device(dev->driver->of_match_table, dev);
+
+ if (!match)
+ return -ENODEV;
+
+ kind = (enum lm75_type)(match->data);
+ } else {
kind = i2c_match_id(lm75_ids, client)->driver_data;
+ }
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
--
2.26.2
[PATCH v2] hwmon: lm75: Handle broken device nodes gracefully
There is a logical flaw in lm75_probe() function introduced in
commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Note, that of_device_get_match_data() returns NULL when no match
found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.
arch/powerpc/boot/dts/fsl/p2041rdb.dts:
lm75b@48 {
compatible = "nxp,lm75a";
reg = <0x48>;
};
In this case, the sensor is mistakenly considered as ADT75 variant.
Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov
---
drivers/hwmon/lm75.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..130ad5042107 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -561,9 +561,15 @@ static int lm75_probe(struct i2c_client *client)
int status, err;
enum lm75_type kind;
- if (client->dev.of_node)
- kind = (enum lm75_type)of_device_get_match_data(&client->dev);
- else
+ if (dev->of_node) {
+ const struct of_device_id *match =
+ of_match_device(dev->driver->of_match_table, dev);
+
+ if (!match)
+ return -ENODEV;
+
+ kind = (enum lm75_type)(match->data);
+ } else
kind = i2c_match_id(lm75_ids, client)->driver_data;
if (!i2c_check_functionality(client->adapter,
--
2.26.2
[PATCH 1/2] hwmon: lm75: Add NXP LM75A to of_match list
NXP LM75A is compatible with original LM75A while it has improved
11-bit precision.
https://www.nxp.com/docs/en/data-sheet/LM75A.pdf
Signed-off-by: Matwey V. Kornilov
---
drivers/hwmon/lm75.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 3aa7f9454f57..37dc903ebf54 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -699,6 +699,10 @@ static const struct of_device_id __maybe_unused
lm75_of_match[] = {
.compatible = "national,lm75b",
.data = (void *)lm75b
},
+ {
+ .compatible = "nxp,lm75a",
+ .data = (void *)lm75b
+ },
{
.compatible = "maxim,max6625",
.data = (void *)max6625
--
2.26.2
[PATCH 2/2] hwmon: lm75: Add another name for NXP LM75B to of_match list
NXP LM75B is compatible with original LM75A while it has improved
11-bit precision.
https://www.nxp.com/docs/en/data-sheet/LM75B.pdf
NXP LM75B support was added in
799fc6021430 ("hwmon: (lm75) Add support for the NXP LM75B")
OF compatible string "national,lm75b" for LM75B was created in
e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Since the previous commit introduces "nxp,lm75a" compatible string
for LM75A, there is a reason to add another alias for LM75B.
Signed-off-by: Matwey V. Kornilov
---
drivers/hwmon/lm75.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 37dc903ebf54..6cd951e062f0 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -703,6 +703,10 @@ static const struct of_device_id __maybe_unused
lm75_of_match[] = {
.compatible = "nxp,lm75a",
.data = (void *)lm75b
},
+ {
+ .compatible = "nxp,lm75b",
+ .data = (void *)lm75b
+ },
{
.compatible = "maxim,max6625",
.data = (void *)max6625
--
2.26.2
[PATCH] hwmon: lm75: Use zero lm75_type for lm75
There is a logical flaw in lm75_probe() function introduced in
e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Note, that of_device_get_match_data() returns NULL when no match
found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.
arch/powerpc/boot/dts/fsl/p2041rdb.dts:
lm75b@48 {
compatible = "nxp,lm75a";
reg = <0x48>;
};
In this case, the sensor is mistakenly considered as ADT75 variant.
The simplest way to handle this issue is to make the LM75 code
zero.
Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov
---
drivers/hwmon/lm75.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..3aa7f9454f57 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -25,12 +25,12 @@
*/
enum lm75_type { /* keep sorted in alphabetical order */
+ lm75 = 0, /* except of lm75 which is default fallback */
adt75,
ds1775,
ds75,
ds7505,
g751,
- lm75,
lm75a,
lm75b,
max6625,
--
2.26.2
[PATCH] media: pwc: Use correct device for DMA
This fixes the following newly introduced warning:
[ 15.518253] [ cut here ]
[ 15.518941] WARNING: CPU: 0 PID: 246 at
/home/matwey/lab/linux/kernel/dma/mapping.c:149 dma_map_page_attrs+0x1a8/0x1d0
[ 15.520634] Modules linked in: pwc videobuf2_vmalloc videobuf2_memops
videobuf2_v4l2 videobuf2_common videodev mc efivarfs
[ 15.522335] CPU: 0 PID: 246 Comm: v4l2-test Not tainted 5.11.0-rc1+ #1
[ 15.523281] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0
02/06/2015
[ 15.524438] RIP: 0010:dma_map_page_attrs+0x1a8/0x1d0
[ 15.525135] Code: 10 5b 5d 41 5c 41 5d c3 4d 89 d0 eb d7 4d 89 c8 89 e9 48
89 da e8 68 29 00 00 eb d1 48 89 f2 48 2b 50 18 48 89 d0 eb 83 0f 0b <0f> 0b 48
c7 c0 ff ff ff ff eb b8 48 89 d9 48 8b 40 40 e8 61 69 d2
[ 15.527938] RSP: 0018:a2694047bca8 EFLAGS: 00010246
[ 15.528716] RAX: RBX: 2580 RCX:
[ 15.529782] RDX: RSI: cdce000ecc00 RDI: a0b4bdb888a0
[ 15.530849] RBP: 0002 R08: 0002 R09:
[ 15.531881] R10: 0004 R11: 0002d8c0 R12:
[ 15.532911] R13: a0b4bdb88800 R14: a0b48382 R15: a0b4bdb888a0
[ 15.533942] FS: 7fc5fbb5e4c0() GS:a0b4fc00()
knlGS:
[ 15.535141] CS: 0010 DS: ES: CR0: 80050033
[ 15.535988] CR2: 7fc5fb6ea138 CR3: 03812000 CR4: 001506f0
[ 15.537025] Call Trace:
[ 15.537425] start_streaming+0x2e9/0x4b0 [pwc]
[ 15.538143] vb2_start_streaming+0x5e/0x110 [videobuf2_common]
[ 15.538989] vb2_core_streamon+0x107/0x140 [videobuf2_common]
[ 15.539831] __video_do_ioctl+0x18f/0x4a0 [videodev]
[ 15.540670] video_usercopy+0x13a/0x5b0 [videodev]
[ 15.541349] ? video_put_user+0x230/0x230 [videodev]
[ 15.542096] ? selinux_file_ioctl+0x143/0x200
[ 15.542752] v4l2_ioctl+0x40/0x50 [videodev]
[ 15.543360] __x64_sys_ioctl+0x89/0xc0
[ 15.543930] do_syscall_64+0x33/0x40
[ 15.58] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 15.545236] RIP: 0033:0x7fc5fb671587
[ 15.545780] Code: b3 66 90 48 8b 05 11 49 2c 00 64 c7 00 26 00 00 00 48 c7
c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d e1 48 2c 00 f7 d8 64 89 01 48
[ 15.548486] RSP: 002b:7fff0f71f038 EFLAGS: 0246 ORIG_RAX:
0010
[ 15.549578] RAX: ffda RBX: 0003 RCX: 7fc5fb671587
[ 15.550664] RDX: 7fff0f71f060 RSI: 40045612 RDI: 0003
[ 15.551706] RBP: R08: R09:
[ 15.552738] R10: R11: 0246 R12: 7fff0f71f060
[ 15.553817] R13: 7fff0f71f1d0 R14: 00de1270 R15:
[ 15.554914] ---[ end trace 7be03122966c2486 ]---
Fixes: 1161db6776bd ("media: usb: pwc: Don't use coherent DMA buffers for ISO
transfer")
Signed-off-by: Matwey V. Kornilov
---
drivers/media/usb/pwc/pwc-if.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 61869636ec61..5e3339cc31c0 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -155,16 +155,17 @@ static const struct video_device pwc_template = {
/***/
/* Private functions */
-static void *pwc_alloc_urb_buffer(struct device *dev,
+static void *pwc_alloc_urb_buffer(struct usb_device *dev,
size_t size, dma_addr_t *dma_handle)
{
+ struct device *dmadev = dev->bus->sysdev;
void *buffer = kmalloc(size, GFP_KERNEL);
if (!buffer)
return NULL;
- *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
- if (dma_mapping_error(dev, *dma_handle)) {
+ *dma_handle = dma_map_single(dmadev, buffer, size, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dmadev, *dma_handle)) {
kfree(buffer);
return NULL;
}
@@ -172,12 +173,14 @@ static void *pwc_alloc_urb_buffer(struct device *dev,
return buffer;
}
-static void pwc_free_urb_buffer(struct device *dev,
+static void pwc_free_urb_buffer(struct usb_device *dev,
size_t size,
void *buffer,
dma_addr_t dma_handle)
{
- dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
+ struct device *dmadev = dev->bus->sysdev;
+
+ dma_unmap_single(dmadev, dma_handle, size, DMA_FROM_DEVICE);
kfree(buffer);
}
@@ -282,6 +285,7 @@ static void pwc_frame_complete(struct pwc_device *pdev)
static void pwc_isoc_handler(struct urb *urb)
{
struct pwc_device *pdev = (struct pwc_device *)urb->context
Re: [PATCH v2] ARM: dts: zynq: Fix ethernet PHY for v5 schematics
Hi Anton,
I hope you are doing good. Could you please check this patch, since
you are initial author of zynq-zturn.dts and I suppose you do have the
"v4" board variant to test.
вт, 28 апр. 2020 г. в 13:04, Matwey V. Kornilov :
>
> There are at least two different versions existing for MYIR Zturn:
>
> * v4 schematics has Atheros AR8035 PHY at 0b000
> http://www.myirtech.com/download/Zynq7000/Z-TURNBOARD_schematic.pdf
> * v5 schematics has Micrel KSZ9031 PHY at 0b011
> v5 schematics available at DVD disk supplied with the board
>
> Specify both PHYs to make ethernet interface working for any board
> revision. This commit relies on of_mdiobus_register() behavior.
> When phy-handle is missed, every nested PHY node is considered,
> while ENODEVs are ignored.
>
> Before the patch:
>
> [ 28.295002] macb e000b000.ethernet eth0: Could not attach PHY (-19)
>
> After the patch:
>
> [ 28.257365] macb e000b000.ethernet eth0: PHY
> [e000b000.ethernet-:00] driver [Micrel KSZ9031 Gigabit PHY] (irq=POLL)
> [ 28.257384] macb e000b000.ethernet eth0: configuring for phy/rgmii-id link
> mode
>
> Signed-off-by: Matwey V. Kornilov
Cc: Anton Gerasimov
> ---
> Changes since v1:
> - reworded commit message
>
> arch/arm/boot/dts/zynq-zturn.dts | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/zynq-zturn.dts
> b/arch/arm/boot/dts/zynq-zturn.dts
> index 5ec616ebca08..07da9cf60d02 100644
> --- a/arch/arm/boot/dts/zynq-zturn.dts
> +++ b/arch/arm/boot/dts/zynq-zturn.dts
> @@ -67,10 +67,17 @@
> &gem0 {
> status = "okay";
> phy-mode = "rgmii-id";
> - phy-handle = <ðernet_phy>;
>
> - ethernet_phy: ethernet-phy@0 {
> - reg = <0x0>;
> + ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + max-speed = <1000>;
> + };
> +
> + ethernet-phy@3 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <3>;
> + max-speed = <1000>;
> };
> };
>
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
[PATCH v2] ARM: dts: zynq: Fix ethernet PHY for v5 schematics
There are at least two different versions existing for MYIR Zturn:
* v4 schematics has Atheros AR8035 PHY at 0b000
http://www.myirtech.com/download/Zynq7000/Z-TURNBOARD_schematic.pdf
* v5 schematics has Micrel KSZ9031 PHY at 0b011
v5 schematics available at DVD disk supplied with the board
Specify both PHYs to make ethernet interface working for any board
revision. This commit relies on of_mdiobus_register() behavior.
When phy-handle is missed, every nested PHY node is considered,
while ENODEVs are ignored.
Before the patch:
[ 28.295002] macb e000b000.ethernet eth0: Could not attach PHY (-19)
After the patch:
[ 28.257365] macb e000b000.ethernet eth0: PHY [e000b000.ethernet-:00]
driver [Micrel KSZ9031 Gigabit PHY] (irq=POLL)
[ 28.257384] macb e000b000.ethernet eth0: configuring for phy/rgmii-id link
mode
Signed-off-by: Matwey V. Kornilov
---
Changes since v1:
- reworded commit message
arch/arm/boot/dts/zynq-zturn.dts | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/zynq-zturn.dts b/arch/arm/boot/dts/zynq-zturn.dts
index 5ec616ebca08..07da9cf60d02 100644
--- a/arch/arm/boot/dts/zynq-zturn.dts
+++ b/arch/arm/boot/dts/zynq-zturn.dts
@@ -67,10 +67,17 @@
&gem0 {
status = "okay";
phy-mode = "rgmii-id";
- phy-handle = <ðernet_phy>;
- ethernet_phy: ethernet-phy@0 {
- reg = <0x0>;
+ ethernet-phy@0 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ max-speed = <1000>;
+ };
+
+ ethernet-phy@3 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <3>;
+ max-speed = <1000>;
};
};
--
2.16.4
Re: [PATCH v2 0/6] musb: Improve performance for hub-attached webcams
пн, 9 сент. 2019 г. в 19:33, Matwey V. Kornilov :
>
> вт, 2 июл. 2019 г. в 20:33, Bin Liu :
> >
> > Matwey,
> >
> > On Tue, Jul 02, 2019 at 08:29:03PM +0300, Matwey V. Kornilov wrote:
> > > Ping?
> >
> > I was offline and just got back. I will review it soon. Sorry for the
> > delay.
>
> Ping?
>
Ping?
> >
> > -Bin.
> >
> > >
> > > пт, 14 июн. 2019 г. в 19:47, Matwey V. Kornilov :
> > > >
> > > > The series is concerned to issues with isochronous transfer while
> > > > streaming the USB webcam data. I discovered the issue first time
> > > > when attached PWC USB webcam to AM335x-based BeagleBone Black SBC.
> > > > It appeared that the root issue was in numerous missed IN requests
> > > > during isochronous transfer where each missing leaded to the frame
> > > > drop. Since every IN request is triggered in MUSB driver
> > > > individually, it is important to queue the send IN request as
> > > > earlier as possible when the previous IN completed. At the same
> > > > time the URB giveback handler of the device driver has also to be
> > > > called there, that leads to arbitrarily delay depending on the
> > > > device driver performance. The details with the references are
> > > > described in [1].
> > > >
> > > > The issue has two parts:
> > > >
> > > > 1) peripheral driver URB callback performance
> > > > 2) MUSB host driver performance
> > > >
> > > > It appeared that the first part is related to the wrong memory
> > > > allocation strategy in the most USB webcam drivers. Non-cached
> > > > memory is used in assumption that coherent DMA memory leads to
> > > > the better performance than non-coherent memory in conjunction with
> > > > the proper synchronization. Yet the assumption might be valid for
> > > > x86 platforms some time ago, the issue was fixed for PWC driver in:
> > > >
> > > > 1161db6776bd ("media: usb: pwc: Don't use coherent DMA buffers for
> > > > ISO transfer")
> > > >
> > > > that leads to 3.5x performance gain. The more generic fix for this
> > > > common issue are coming for the rest drivers [2].
> > > >
> > > > The patch allowed successfully running full-speed USB PWC webcams
> > > > attached directly to BeagleBone Black USB port.
> > > >
> > > > However, the second part of the issue is still present for
> > > > peripheral device attached through the high-speed USB hub due to
> > > > its 125us frame time. The patch series is intended to reorganize
> > > > musb_advance_schedule() to allow host to send IN request quicker.
> > > >
> > > > The patch series is organized as the following. First three patches
> > > > improve readability of the existing code in
> > > > musb_advance_schedule(). Patches 4 and 5 introduce updated
> > > > signature for musb_start_urb(). The last patch introduce new
> > > > code-path in musb_advance_schedule() which allows for faster
> > > > response.
> > > >
> > > > References:
> > > >
> > > > [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> > > > [2] https://www.spinics.net/lists/linux-media/msg144279.html
> > > >
> > > > Changes since v1:
> > > > - Patch 6 was redone to keep URB giveback order and stop transmission
> > > > at
> > > >erroneous URB.
> > > >
> > > > Matwey V. Kornilov (6):
> > > > usb: musb: Use USB_DIR_IN when calling musb_advance_schedule()
> > > > usb: musb: Introduce musb_qh_empty() helper function
> > > > usb: musb: Introduce musb_qh_free() helper function
> > > > usb: musb: Rename musb_start_urb() to musb_start_next_urb()
> > > > usb: musb: Introduce musb_start_urb()
> > > > usb: musb: Decrease URB starting latency in musb_advance_schedule()
> > > >
> > > > drivers/usb/musb/musb_host.c | 132
> > > > ---
> > > > drivers/usb/musb/musb_host.h | 1 +
> > > > 2 files changed, 86 insertions(+), 47 deletions(-)
> > > >
> > > > --
> > > > 2.16.4
> > > >
> > >
> > >
> > > --
> > > With best regards,
> > > Matwey V. Kornilov
>
>
>
> --
> With best regards,
> Matwey V. Kornilov
--
With best regards,
Matwey V. Kornilov
Re: [PATCH v2 0/6] musb: Improve performance for hub-attached webcams
вт, 2 июл. 2019 г. в 20:33, Bin Liu :
>
> Matwey,
>
> On Tue, Jul 02, 2019 at 08:29:03PM +0300, Matwey V. Kornilov wrote:
> > Ping?
>
> I was offline and just got back. I will review it soon. Sorry for the
> delay.
Ping?
>
> -Bin.
>
> >
> > пт, 14 июн. 2019 г. в 19:47, Matwey V. Kornilov :
> > >
> > > The series is concerned to issues with isochronous transfer while
> > > streaming the USB webcam data. I discovered the issue first time
> > > when attached PWC USB webcam to AM335x-based BeagleBone Black SBC.
> > > It appeared that the root issue was in numerous missed IN requests
> > > during isochronous transfer where each missing leaded to the frame
> > > drop. Since every IN request is triggered in MUSB driver
> > > individually, it is important to queue the send IN request as
> > > earlier as possible when the previous IN completed. At the same
> > > time the URB giveback handler of the device driver has also to be
> > > called there, that leads to arbitrarily delay depending on the
> > > device driver performance. The details with the references are
> > > described in [1].
> > >
> > > The issue has two parts:
> > >
> > > 1) peripheral driver URB callback performance
> > > 2) MUSB host driver performance
> > >
> > > It appeared that the first part is related to the wrong memory
> > > allocation strategy in the most USB webcam drivers. Non-cached
> > > memory is used in assumption that coherent DMA memory leads to
> > > the better performance than non-coherent memory in conjunction with
> > > the proper synchronization. Yet the assumption might be valid for
> > > x86 platforms some time ago, the issue was fixed for PWC driver in:
> > >
> > > 1161db6776bd ("media: usb: pwc: Don't use coherent DMA buffers for
> > > ISO transfer")
> > >
> > > that leads to 3.5x performance gain. The more generic fix for this
> > > common issue are coming for the rest drivers [2].
> > >
> > > The patch allowed successfully running full-speed USB PWC webcams
> > > attached directly to BeagleBone Black USB port.
> > >
> > > However, the second part of the issue is still present for
> > > peripheral device attached through the high-speed USB hub due to
> > > its 125us frame time. The patch series is intended to reorganize
> > > musb_advance_schedule() to allow host to send IN request quicker.
> > >
> > > The patch series is organized as the following. First three patches
> > > improve readability of the existing code in
> > > musb_advance_schedule(). Patches 4 and 5 introduce updated
> > > signature for musb_start_urb(). The last patch introduce new
> > > code-path in musb_advance_schedule() which allows for faster
> > > response.
> > >
> > > References:
> > >
> > > [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> > > [2] https://www.spinics.net/lists/linux-media/msg144279.html
> > >
> > > Changes since v1:
> > > - Patch 6 was redone to keep URB giveback order and stop transmission at
> > >erroneous URB.
> > >
> > > Matwey V. Kornilov (6):
> > > usb: musb: Use USB_DIR_IN when calling musb_advance_schedule()
> > > usb: musb: Introduce musb_qh_empty() helper function
> > > usb: musb: Introduce musb_qh_free() helper function
> > > usb: musb: Rename musb_start_urb() to musb_start_next_urb()
> > > usb: musb: Introduce musb_start_urb()
> > > usb: musb: Decrease URB starting latency in musb_advance_schedule()
> > >
> > > drivers/usb/musb/musb_host.c | 132
> > > ---
> > > drivers/usb/musb/musb_host.h | 1 +
> > > 2 files changed, 86 insertions(+), 47 deletions(-)
> > >
> > > --
> > > 2.16.4
> > >
> >
> >
> > --
> > With best regards,
> > Matwey V. Kornilov
--
With best regards,
Matwey V. Kornilov
[PATCH] power: reset: reboot-mode: Fix author email format
Closing angle bracket was missing.
Signed-off-by: Matwey V. Kornilov
---
drivers/power/reset/reboot-mode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/reset/reboot-mode.c
b/drivers/power/reset/reboot-mode.c
index 06ff035b57f5..b4076b10b893 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -190,6 +190,6 @@ void devm_reboot_mode_unregister(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_reboot_mode_unregister);
-MODULE_AUTHOR("Andy Yan ");
MODULE_DESCRIPTION("System reboot mode core library");
MODULE_LICENSE("GPL v2");
--
2.16.4
Re: [PATCH v2 0/6] musb: Improve performance for hub-attached webcams
Ping?
пт, 14 июн. 2019 г. в 19:47, Matwey V. Kornilov :
>
> The series is concerned to issues with isochronous transfer while
> streaming the USB webcam data. I discovered the issue first time
> when attached PWC USB webcam to AM335x-based BeagleBone Black SBC.
> It appeared that the root issue was in numerous missed IN requests
> during isochronous transfer where each missing leaded to the frame
> drop. Since every IN request is triggered in MUSB driver
> individually, it is important to queue the send IN request as
> earlier as possible when the previous IN completed. At the same
> time the URB giveback handler of the device driver has also to be
> called there, that leads to arbitrarily delay depending on the
> device driver performance. The details with the references are
> described in [1].
>
> The issue has two parts:
>
> 1) peripheral driver URB callback performance
> 2) MUSB host driver performance
>
> It appeared that the first part is related to the wrong memory
> allocation strategy in the most USB webcam drivers. Non-cached
> memory is used in assumption that coherent DMA memory leads to
> the better performance than non-coherent memory in conjunction with
> the proper synchronization. Yet the assumption might be valid for
> x86 platforms some time ago, the issue was fixed for PWC driver in:
>
> 1161db6776bd ("media: usb: pwc: Don't use coherent DMA buffers for ISO
> transfer")
>
> that leads to 3.5x performance gain. The more generic fix for this
> common issue are coming for the rest drivers [2].
>
> The patch allowed successfully running full-speed USB PWC webcams
> attached directly to BeagleBone Black USB port.
>
> However, the second part of the issue is still present for
> peripheral device attached through the high-speed USB hub due to
> its 125us frame time. The patch series is intended to reorganize
> musb_advance_schedule() to allow host to send IN request quicker.
>
> The patch series is organized as the following. First three patches
> improve readability of the existing code in
> musb_advance_schedule(). Patches 4 and 5 introduce updated
> signature for musb_start_urb(). The last patch introduce new
> code-path in musb_advance_schedule() which allows for faster
> response.
>
> References:
>
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> [2] https://www.spinics.net/lists/linux-media/msg144279.html
>
> Changes since v1:
> - Patch 6 was redone to keep URB giveback order and stop transmission at
>erroneous URB.
>
> Matwey V. Kornilov (6):
> usb: musb: Use USB_DIR_IN when calling musb_advance_schedule()
> usb: musb: Introduce musb_qh_empty() helper function
> usb: musb: Introduce musb_qh_free() helper function
> usb: musb: Rename musb_start_urb() to musb_start_next_urb()
> usb: musb: Introduce musb_start_urb()
> usb: musb: Decrease URB starting latency in musb_advance_schedule()
>
> drivers/usb/musb/musb_host.c | 132
> ---
> drivers/usb/musb/musb_host.h | 1 +
> 2 files changed, 86 insertions(+), 47 deletions(-)
>
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
[PATCH v2 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule()
Previously, the algorithm was the following:
1. giveback current URB
2. if current qh is not empty
then start next URB
3. if current qh is empty
then dispose the qh, find next qh if any, and start URB.
It may take a while to run urb->callback inside URB giveback which is
run synchronously in musb. In order to improve the latency we rearrange
the function behaviour for the case when qh is not empty: next URB is
started before URB giveback. When qh is empty or current URB has an
error then the behaviour is intentionally kept in order
a) not to break existing inter qh scheduling: URB giveback could
potentionally enqueue other URB to the empty qh preventing it from being
disposed;
b) allow the class driver to cancel outstanding URBs in the queue.
Correct URB giveback order is guaranteed as the following. For each
qh there can be at most three ready URBs processing by the driver.
Indeed, every ready URB can send at most one URB in
musb_advance_schedule(), and in the worst case scenario we have the
following ready URBs:
1) URB in the giveback lock protected section inside musb_giveback()
2) URB waiting at the giveback lock acqusition in musb_giveback()
3) URB waiting at the controller lock acqusition in the glue layer
interrput handler
Here URB #2 and URB #3 are triggered by URB #1 and URB #2
correspondingly when they passed through musb_advance_schedule().
Since URB #3 is waiting before musb_advance_schedule(), no other new
URBs will be sent until URB#1 is finished, URB#2 goes to the giveback
lock protected section, and URB#3 goes to the controller lock protected
musb_advance_schedule().
Before this patch, time spent in urb->callback led to the following
glitches between the host and a hub during isoc transfer (line 4):
11.624492 d= 0.000124 [130.6 + 1.050] [ 4] SPLIT
11.624492 d= 0.00 [130.6 + 1.467] [ 3] IN : 3.5
11.624493 d= 0.00 [130.6 + 1.967] [ 37] DATA0: aa 08 [skipped...]
11.625617 d= 0.001124 [131.7 + 1.050] [ 4] SPLIT
11.625617 d= 0.00 [131.7 + 1.467] [ 3] IN : 3.5
11.625867 d= 0.000250 [132.1 + 1.050] [ 4] SPLIT
11.625867 d= 0.00 [132.1 + 1.467] [ 3] IN : 3.5
11.625868 d= 0.01 [132.1 + 1.983] [ 3] DATA0: 00 00
11.626617 d= 0.000749 [132.7 + 1.050] [ 4] SPLIT
11.626617 d= 0.00 [132.7 + 1.467] [ 3] IN : 3.5
11.626867 d= 0.000250 [133.1 + 1.050] [ 4] SPLIT
11.626867 d= 0.00 [133.1 + 1.467] [ 3] IN : 3.5
11.626868 d= 0.00 [133.1 + 1.967] [ 3] DATA0: 00 00
After the hub, they look as the following and may lead to broken
perepherial transfer (as in case of PWC based webcam):
11.332004 d= 0.000997 [ 30.0 + 3.417] [ 3] IN : 5.5
11.332007 d= 0.03 [ 30.0 + 6.833] [800] DATA0: 8a 1c [skipped...]
11.334004 d= 0.001997 [ 32.0 + 3.417] [ 3] IN : 5.5
11.334007 d= 0.03 [ 32.0 + 6.750] [ 3] DATA0: 00 00
11.335004 d= 0.000997 [ 33 + 3.417] [ 3] IN : 5.5
11.335007 d= 0.03 [ 33 + 6.750] [ 3] DATA0: 00 00
Removing this glitches makes us able to successfully run 10fps
video stream from the webcam attached via USB hub. That was
previously impossible.
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 36
drivers/usb/musb/musb_host.h | 1 +
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index ed99ecd4e63a..5c43996f2de5 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -85,6 +85,11 @@ static bool musb_qh_empty(struct musb_qh *qh)
return list_empty(&qh->hep->urb_list);
}
+static bool musb_qh_singular(struct musb_qh *qh)
+{
+ return list_is_singular(&qh->hep->urb_list);
+}
+
static void musb_qh_unlink_hep(struct musb_qh *qh)
{
if (!qh->hep)
@@ -301,15 +306,24 @@ musb_start_next_urb(struct musb *musb, int is_in, struct
musb_qh *qh)
}
/* Context: caller owns controller lock, IRQs are blocked */
-static void musb_giveback(struct musb *musb, struct urb *urb, int status)
+static void musb_giveback(struct musb *musb, struct musb_qh *qh, struct urb
*urb, int status)
__releases(musb->lock)
__acquires(musb->lock)
{
trace_musb_urb_gb(musb, urb);
+ /*
+* This line is protected by the controller lock: at most
+* one thread waiting on the giveback lock.
+*/
+ spin_lock(&qh->giveback_lock);
usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
+
spin_unlock(&musb->lock);
+
usb_hcd_giveback_urb(musb->hcd, urb, status);
+ spin_unlock(&qh->giveback_lock);
+
spin_lock(&musb->lock);
}
@@ -362,8 +376,21 @@ static void musb_advance_schedule(struct musb *musb,
struct urb *urb,
break;
}
+ if (ready && !musb_qh_singular(qh) &
[PATCH v2 3/6] usb: musb: Introduce musb_qh_free() helper function
Reduce the following similar snippets by using musb_qh_free().
qh->hep->hcpriv = NULL;
list_del(&qh->ring);
kfree(qh);
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 66 +---
1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 37aa9f6155d9..5d23c950a21b 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -85,6 +85,21 @@ static bool musb_qh_empty(struct musb_qh *qh)
return list_empty(&qh->hep->urb_list);
}
+static void musb_qh_unlink_hep(struct musb_qh *qh)
+{
+ if (!qh->hep)
+ return;
+
+ qh->hep->hcpriv = NULL;
+}
+
+static void musb_qh_free(struct musb_qh *qh)
+{
+ musb_qh_unlink_hep(qh);
+ list_del(&qh->ring);
+ kfree(qh);
+}
+
/*
* Clear TX fifo. Needed to avoid BABBLE errors.
*/
@@ -348,7 +363,7 @@ static void musb_advance_schedule(struct musb *musb, struct
urb *urb,
* invalidate qh as soon as list_empty(&hep->urb_list)
*/
if (musb_qh_empty(qh)) {
- struct list_head*head;
+ struct list_head*head = NULL;
struct dma_controller *dma = musb->dma_controller;
if (is_in) {
@@ -367,34 +382,22 @@ static void musb_advance_schedule(struct musb *musb,
struct urb *urb,
/* Clobber old pointers to this qh */
musb_ep_set_qh(ep, is_in, NULL);
- qh->hep->hcpriv = NULL;
- switch (qh->type) {
+ /* USB_ENDPOINT_XFER_CONTROL and USB_ENDPOINT_XFER_BULK: fifo
+* policy for these lists, except that NAKing should rotate
+* a qh to the end (for fairness).
+* USB_ENDPOINT_XFER_ISOC and USB_ENDPOINT_XFER_INT: this is
+* where periodic bandwidth should be de-allocated if it's
+* tracked and allocated; and where we'd update the schedule
+* tree...
+*/
+ if (qh->mux == 1
+ && (qh->type == USB_ENDPOINT_XFER_CONTROL || qh->type ==
USB_ENDPOINT_XFER_BULK))
+ head = qh->ring.prev;
- case USB_ENDPOINT_XFER_CONTROL:
- case USB_ENDPOINT_XFER_BULK:
- /* fifo policy for these lists, except that NAKing
-* should rotate a qh to the end (for fairness).
-*/
- if (qh->mux == 1) {
- head = qh->ring.prev;
- list_del(&qh->ring);
- kfree(qh);
- qh = first_qh(head);
- break;
- }
- /* fall through */
+ musb_qh_free(qh);
- case USB_ENDPOINT_XFER_ISOC:
- case USB_ENDPOINT_XFER_INT:
- /* this is where periodic bandwidth should be
-* de-allocated if it's tracked and allocated;
-* and where we'd update the schedule tree...
-*/
- kfree(qh);
- qh = NULL;
- break;
- }
+ qh = head ? first_qh(head) : NULL;
}
if (qh != NULL && qh->is_ready) {
@@ -2435,11 +2438,8 @@ static int musb_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
/* If nothing else (usually musb_giveback) is using it
* and its URB list has emptied, recycle this qh.
*/
- if (ready && musb_qh_empty(qh)) {
- qh->hep->hcpriv = NULL;
- list_del(&qh->ring);
- kfree(qh);
- }
+ if (ready && musb_qh_empty(qh))
+ musb_qh_free(qh);
} else
ret = musb_cleanup_urb(urb, qh);
done:
@@ -2493,9 +2493,7 @@ musb_h_disable(struct usb_hcd *hcd, struct
usb_host_endpoint *hep)
while (!musb_qh_empty(qh))
musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
- hep->hcpriv = NULL;
- list_del(&qh->ring);
- kfree(qh);
+ musb_qh_free(qh);
}
exit:
spin_unlock_irqrestore(&musb->lock, flags);
--
2.16.4
[PATCH v2 0/6] musb: Improve performance for hub-attached webcams
The series is concerned to issues with isochronous transfer while
streaming the USB webcam data. I discovered the issue first time
when attached PWC USB webcam to AM335x-based BeagleBone Black SBC.
It appeared that the root issue was in numerous missed IN requests
during isochronous transfer where each missing leaded to the frame
drop. Since every IN request is triggered in MUSB driver
individually, it is important to queue the send IN request as
earlier as possible when the previous IN completed. At the same
time the URB giveback handler of the device driver has also to be
called there, that leads to arbitrarily delay depending on the
device driver performance. The details with the references are
described in [1].
The issue has two parts:
1) peripheral driver URB callback performance
2) MUSB host driver performance
It appeared that the first part is related to the wrong memory
allocation strategy in the most USB webcam drivers. Non-cached
memory is used in assumption that coherent DMA memory leads to
the better performance than non-coherent memory in conjunction with
the proper synchronization. Yet the assumption might be valid for
x86 platforms some time ago, the issue was fixed for PWC driver in:
1161db6776bd ("media: usb: pwc: Don't use coherent DMA buffers for ISO
transfer")
that leads to 3.5x performance gain. The more generic fix for this
common issue are coming for the rest drivers [2].
The patch allowed successfully running full-speed USB PWC webcams
attached directly to BeagleBone Black USB port.
However, the second part of the issue is still present for
peripheral device attached through the high-speed USB hub due to
its 125us frame time. The patch series is intended to reorganize
musb_advance_schedule() to allow host to send IN request quicker.
The patch series is organized as the following. First three patches
improve readability of the existing code in
musb_advance_schedule(). Patches 4 and 5 introduce updated
signature for musb_start_urb(). The last patch introduce new
code-path in musb_advance_schedule() which allows for faster
response.
References:
[1] https://www.spinics.net/lists/linux-usb/msg165735.html
[2] https://www.spinics.net/lists/linux-media/msg144279.html
Changes since v1:
- Patch 6 was redone to keep URB giveback order and stop transmission at
erroneous URB.
Matwey V. Kornilov (6):
usb: musb: Use USB_DIR_IN when calling musb_advance_schedule()
usb: musb: Introduce musb_qh_empty() helper function
usb: musb: Introduce musb_qh_free() helper function
usb: musb: Rename musb_start_urb() to musb_start_next_urb()
usb: musb: Introduce musb_start_urb()
usb: musb: Decrease URB starting latency in musb_advance_schedule()
drivers/usb/musb/musb_host.c | 132 ---
drivers/usb/musb/musb_host.h | 1 +
2 files changed, 86 insertions(+), 47 deletions(-)
--
2.16.4
[PATCH v2 2/6] usb: musb: Introduce musb_qh_empty() helper function
Use musb_qh_empty() instead of &qh->hep->urb_list to avoid code
duplicating.
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 3ffea6a5e022..37aa9f6155d9 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -80,6 +80,11 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
struct urb *urb, int is_out,
u8 *buf, u32 offset, u32 len);
+static bool musb_qh_empty(struct musb_qh *qh)
+{
+ return list_empty(&qh->hep->urb_list);
+}
+
/*
* Clear TX fifo. Needed to avoid BABBLE errors.
*/
@@ -342,7 +347,7 @@ static void musb_advance_schedule(struct musb *musb, struct
urb *urb,
/* reclaim resources (and bandwidth) ASAP; deschedule it, and
* invalidate qh as soon as list_empty(&hep->urb_list)
*/
- if (list_empty(&qh->hep->urb_list)) {
+ if (musb_qh_empty(qh)) {
struct list_head*head;
struct dma_controller *dma = musb->dma_controller;
@@ -2430,7 +2435,7 @@ static int musb_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
/* If nothing else (usually musb_giveback) is using it
* and its URB list has emptied, recycle this qh.
*/
- if (ready && list_empty(&qh->hep->urb_list)) {
+ if (ready && musb_qh_empty(qh)) {
qh->hep->hcpriv = NULL;
list_del(&qh->ring);
kfree(qh);
@@ -2475,7 +2480,7 @@ musb_h_disable(struct usb_hcd *hcd, struct
usb_host_endpoint *hep)
/* Then nuke all the others ... and advance the
* queue on hw_ep (e.g. bulk ring) when we're done.
*/
- while (!list_empty(&hep->urb_list)) {
+ while (!musb_qh_empty(qh)) {
urb = next_urb(qh);
urb->status = -ESHUTDOWN;
musb_advance_schedule(musb, urb, qh->hw_ep, is_in);
@@ -2485,7 +2490,7 @@ musb_h_disable(struct usb_hcd *hcd, struct
usb_host_endpoint *hep)
* other transfers, and since !qh->is_ready nothing
* will activate any of these as it advances.
*/
- while (!list_empty(&hep->urb_list))
+ while (!musb_qh_empty(qh))
musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
hep->hcpriv = NULL;
--
2.16.4
[PATCH v2 4/6] usb: musb: Rename musb_start_urb() to musb_start_next_urb()
In the following commit we introduce new musb_start_urb() function
which will be able to start arbitrary urb. In order to have
intuitive function names we rename musb_start_urb() to
musb_start_next_urb().
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 5d23c950a21b..3a202a2a521d 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -213,7 +213,7 @@ static struct musb_qh *musb_ep_get_qh(struct musb_hw_ep
*ep, int is_in)
* Context: controller locked, irqs blocked
*/
static void
-musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh)
+musb_start_next_urb(struct musb *musb, int is_in, struct musb_qh *qh)
{
u32 len;
void __iomem*mbase = musb->mregs;
@@ -403,7 +403,7 @@ static void musb_advance_schedule(struct musb *musb, struct
urb *urb,
if (qh != NULL && qh->is_ready) {
musb_dbg(musb, "... next ep%d %cX urb %p",
hw_ep->epnum, is_in ? 'R' : 'T', next_urb(qh));
- musb_start_urb(musb, is_in, qh);
+ musb_start_next_urb(musb, is_in, qh);
}
}
@@ -1001,7 +1001,7 @@ static void musb_bulk_nak_timeout(struct musb *musb,
struct musb_hw_ep *ep,
}
if (next_qh)
- musb_start_urb(musb, is_in, next_qh);
+ musb_start_next_urb(musb, is_in, next_qh);
}
}
@@ -2141,7 +2141,7 @@ static int musb_schedule(
qh->hw_ep = hw_ep;
qh->hep->hcpriv = qh;
if (idle)
- musb_start_urb(musb, is_in, qh);
+ musb_start_next_urb(musb, is_in, qh);
return 0;
}
--
2.16.4
[PATCH v2 1/6] usb: musb: Use USB_DIR_IN when calling musb_advance_schedule()
Use USB_DIR_IN instead of 1 when calling musb_advance_schedule(). This is consistent with the rest of musb_host.c code and impoves the readability. Signed-off-by: Matwey V. Kornilov --- drivers/usb/musb/musb_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c index eb308ec35c66..3ffea6a5e022 100644 --- a/drivers/usb/musb/musb_host.c +++ b/drivers/usb/musb/musb_host.c @@ -1195,7 +1195,7 @@ irqreturn_t musb_h_ep0_irq(struct musb *musb) /* call completion handler if done */ if (complete) - musb_advance_schedule(musb, urb, hw_ep, 1); + musb_advance_schedule(musb, urb, hw_ep, USB_DIR_IN); done: return retval; } -- 2.16.4
[PATCH v2 5/6] usb: musb: Introduce musb_start_urb()
This function allows us to start arbitrary urb.
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 3a202a2a521d..ed99ecd4e63a 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -213,11 +213,10 @@ static struct musb_qh *musb_ep_get_qh(struct musb_hw_ep
*ep, int is_in)
* Context: controller locked, irqs blocked
*/
static void
-musb_start_next_urb(struct musb *musb, int is_in, struct musb_qh *qh)
+musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh, struct urb
*urb)
{
u32 len;
void __iomem*mbase = musb->mregs;
- struct urb *urb = next_urb(qh);
void*buf = urb->transfer_buffer;
u32 offset = 0;
struct musb_hw_ep *hw_ep = qh->hw_ep;
@@ -293,6 +292,14 @@ musb_start_next_urb(struct musb *musb, int is_in, struct
musb_qh *qh)
}
}
+static void
+musb_start_next_urb(struct musb *musb, int is_in, struct musb_qh *qh)
+{
+ struct urb *urb = next_urb(qh);
+
+ musb_start_urb(musb, is_in, qh, urb);
+}
+
/* Context: caller owns controller lock, IRQs are blocked */
static void musb_giveback(struct musb *musb, struct urb *urb, int status)
__releases(musb->lock)
--
2.16.4
Re: [PATCH 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule()
вт, 30 апр. 2019 г. в 18:31, Bin Liu :
>
> Hi Greg and all devs,
>
> On Wed, Apr 03, 2019 at 09:53:10PM +0300, Matwey V. Kornilov wrote:
> > Previously, the algorithm was the following:
> >
> > 1. giveback current URB
> > 2. if current qh is not empty
> > then start next URB
> > 3. if current qh is empty
> > then dispose the qh, find next qh if any, and start URB.
> >
> > It may take a while to run urb->callback inside URB giveback which is
> > run synchronously in musb. In order to improve the latency we rearrange
> > the function behaviour for the case when qh is not empty: next URB is
> > started before URB giveback. When qh is empty then the behaviour is
> > intentionally kept in order not to break existing inter qh scheduling:
> > URB giveback could potentionally enqueue other URB to the empty qh
> > preventing it from being disposed.
>
> This patch changes the sequence of urb giveback in musb.
>
> before after
> -- -
> 1. giveback current urb 1. start next urb if qh != empty
> 2. start next urb if qh != empty2. giveback current urb
>
> I see there is a potential that the urb giveback could be out of order,
> for example, if urb giveback in BH and the next urb finishes before BH
> runs.
Could you please give more details? Frankly speaking, I am not sure
that I understand the reordering issue origin correctly.
I see in the existing implementation that the function call order is
the following:
1. glue interrupt handler (for instance dsps_interrupt() in my am335x
case) holds musb->lock;
2. musb_interrupt()
3. musb_host_rx() (or *_tx())
4. musb_advance_schedule()
5. musb_giveback() releases and reacquires musb->lock around:
6. usb_hcd_giveback_urb()
So, when musb_giveback() is called inside musb_advance_schedule() then
the second instance of musb_advance_schedule() can be started
simultaneously when the following interrupt is being handled at other
CPU core. And we can see two usb_hcd_giveback_urb() running
concurrently.
Is it correct?
>
> If this potential is possible, is it a problem for any class driver?
>
> Thanks,
> -Bin.
>
> >
> > Before this patch, time spent in urb->callback led to the following
> > glitches between the host and a hub during isoc transfer (line 4):
> >
> > 11.624492 d= 0.000124 [130.6 + 1.050] [ 4] SPLIT
> > 11.624492 d= 0.00 [130.6 + 1.467] [ 3] IN : 3.5
> > 11.624493 d= 0.00 [130.6 + 1.967] [ 37] DATA0: aa 08 [skipped...]
> > 11.625617 d= 0.001124 [131.7 + 1.050] [ 4] SPLIT
> > 11.625617 d= 0.00 [131.7 + 1.467] [ 3] IN : 3.5
> > 11.625867 d= 0.000250 [132.1 + 1.050] [ 4] SPLIT
> > 11.625867 d= 0.00 [132.1 + 1.467] [ 3] IN : 3.5
> > 11.625868 d= 0.01 [132.1 + 1.983] [ 3] DATA0: 00 00
> > 11.626617 d= 0.000749 [132.7 + 1.050] [ 4] SPLIT
> > 11.626617 d= 0.00 [132.7 + 1.467] [ 3] IN : 3.5
> > 11.626867 d= 0.000250 [133.1 + 1.050] [ 4] SPLIT
> > 11.626867 d= 0.00 [133.1 + 1.467] [ 3] IN : 3.5
> > 11.626868 d= 0.00 [133.1 + 1.967] [ 3] DATA0: 00 00
> >
> > After the hub, they look as the following and may lead to broken
> > perepherial transfer (as in case of PWC based webcam):
> >
> > 11.332004 d= 0.000997 [ 30.0 + 3.417] [ 3] IN : 5.5
> > 11.332007 d= 0.03 [ 30.0 + 6.833] [800] DATA0: 8a 1c [skipped...]
> > 11.334004 d= 0.001997 [ 32.0 + 3.417] [ 3] IN : 5.5
> > 11.334007 d= 0.03 [ 32.0 + 6.750] [ 3] DATA0: 00 00
> > 11.335004 d= 0.000997 [ 33 + 3.417] [ 3] IN : 5.5
> > 11.335007 d= 0.03 [ 33 + 6.750] [ 3] DATA0: 00 00
> >
> > Removing this glitches makes us able to successfully run 10fps
> > video stream from the webcam attached via USB hub. That was
> > previously impossible.
> >
> > Signed-off-by: Matwey V. Kornilov
> > ---
> > drivers/usb/musb/musb_host.c | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > index ed99ecd4e63a..75be92873b5b 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -85,6 +85,11 @@ static bool musb_qh_empty(struct musb_qh *qh)
> > return list_empty(&qh->hep->urb_list);
> > }
> >
> > +static bool musb_qh_singular(struct musb_qh *qh)
> > +{
> > + return list_is_singular(&qh->hep->urb_list);
> > +}
> > +
> > static void musb_
[PATCH v2] usb: musb: Fix urb->hcpriv value
urb->hcpriv != NULL is used to indicate that the URB is queued [1].
Also see __usb_hcd_giveback_urb() and usb_hcd_submit_urb() for
the reference.
In this code path, the URB is actually queued and valid qh is hep->hcpriv.
[1] https://lkml.org/lkml/2019/1/25/750
Fixes: 714bc5ef3eda ("musb: potential use after free")
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index b59ce9ad14ce..a60d52c7e112 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2302,7 +2302,7 @@ static int musb_urb_enqueue(
* odd, rare, error prone, but legal.
*/
kfree(qh);
- qh = NULL;
+ qh = hep->hcpriv;
ret = 0;
} else
ret = musb_schedule(musb, qh,
--
2.16.4
Re: [PATCH] usb: musb: Fix potential NULL dereference
сб, 26 янв. 2019 г. в 00:37, Alan Stern :
>
> On Fri, 25 Jan 2019, Bin Liu wrote:
>
> > On Thu, Jan 24, 2019 at 09:47:02PM +0300, Matwey V. Kornilov wrote:
> > > By the way, why do we need to store the qh in urb->hcpriv?
> > > qh can always be accessible through urb->ep->hcpriv
> > > Wouldn't it be better to drop entire urb->hcpriv usage?
> >
> > I am not sure why. The code is there since the first commit in a decade
> > ago. But I tend to agree with you.
> >
> > In a quick search for urb->hcpriv and urb->ep->hcpriv, based on the
> > usage in core/hcd.c, it seems to me that urb->hcpriv should not be
> > changed in each controller driver, but I see both have been used in most
> > controller drivers. I will leave this to others to educate me.
>
> In some of the older HCDs, urb->hcpriv != NULL is used to indicate that
> urb is on an endpoint queue. Perhaps that usage was copied.
>
> Alan Stern
>
Hi,
Thank you for the explanation. I think that It would be great to
document it somewhere. Such a purpose for variable named `hcpriv' is
not entirely obvious.
Now it is clear to me, why __usb_hcd_giveback_urb() sets urb->hcpriv to NULL.
Returning to my initial patch. I think that it is still valid, though
I admit that the commit message must be rewritten.
In this code path we successfully queued the new urb, so urb->hcpriv
should be NOT NULL indicating that the urb is queued according to Alan
explanation.
musb_urb_enqueue(), musb_host.c line 2345:
if (ret == 0) {
urb->hcpriv = qh;
/* FIXME set urb->start_frame for iso/intr, it's tested in
* musb_start_urb(), but otherwise only konicawc cares ...
*/
}
--
With best regards,
Matwey V. Kornilov
Re: [PATCH] usb: musb: Fix potential NULL dereference
By the way, why do we need to store the qh in urb->hcpriv?
qh can always be accessible through urb->ep->hcpriv
Wouldn't it be better to drop entire urb->hcpriv usage?
ср, 23 янв. 2019 г. в 20:52, Matwey V. Kornilov :
>
> We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
> hep->hcpriv in this code path.
>
> Fixes: 714bc5ef3eda ("musb: potential use after free")
> Signed-off-by: Matwey V. Kornilov
> ---
> drivers/usb/musb/musb_host.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c6118a962d37..6f267716768e 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
> * odd, rare, error prone, but legal.
> */
> kfree(qh);
> - qh = NULL;
> + qh = hep->hcpriv;
> ret = 0;
> } else
> ret = musb_schedule(musb, qh,
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
[PATCH] usb: musb: Fix potential NULL dereference
We assign "urb->hcpriv = qh;" a few lines down. The valid qh for the urb is
hep->hcpriv in this code path.
Fixes: 714bc5ef3eda ("musb: potential use after free")
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c6118a962d37..6f267716768e 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2336,7 +2336,7 @@ static int musb_urb_enqueue(
* odd, rare, error prone, but legal.
*/
kfree(qh);
- qh = NULL;
+ qh = hep->hcpriv;
ret = 0;
} else
ret = musb_schedule(musb, qh,
--
2.16.4
Re: [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
ср, 12 дек. 2018 г. в 20:41, Ezequiel Garcia : > > On Wed, 12 Dec 2018 at 14:27, Laurent Pinchart > wrote: > > > > Hi Matwey, > > > > Thank you for the patches. > > > > For the whole series, > > > > Reviewed-by: Laurent Pinchart > > > > Thanks Laurent. > > Matwey, given your detailed analysis of this issue, > and given you have pwc hardware to test, I think > you should consider co-maintaining this driver. > Well, It would be great if I could help. Is there some guide how to apply? > Thanks, > -- > Ezequiel García, VanguardiaSur > www.vanguardiasur.com.ar -- With best regards, Matwey V. Kornilov
Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
ср, 12 дек. 2018 г. в 17:01, Steven Rostedt : > > On Wed, 12 Dec 2018 08:56:22 -0500 > Steven Rostedt wrote: > > > Can someone please take this patch or at least say what's wrong with it > > if you have a problem? > > > > Matwey has been patiently pinging us once every other week for over a > > month asking for a reply. I've already given my Reviewed-by from a > > tracing perspective. > > > > Ignoring patches is not a friendly gesture. > > > > Nevermind, it appears that v5 is still under discussion. > > Matwey, does v6 address the comments made in v5? Hi, v6 addresses the comments made by Laurent Pinchart on Oct, 31: https://www.spinics.net/lists/linux-media/msg142216.html namely, dma_sync_single_for_device() is introduced in the proper place > > -- Steve -- With best regards, Matwey V. Kornilov
Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
пт, 30 нояб. 2018 г. в 15:20, Matwey V. Kornilov :
>
> ср, 21 нояб. 2018 г. в 21:15, Matwey V. Kornilov :
> >
> > пт, 9 нояб. 2018 г. в 22:03, Matwey V. Kornilov :
> > >
> > > DMA cocherency slows the transfer down on systems without hardware
> > > coherent DMA.
> > > Instead we use noncocherent DMA memory and explicit sync at data receive
> > > handler.
> > >
> > > Based on previous commit the following performance benchmarks have been
> > > carried out. Average memcpy() data transfer rate (rate) and handler
> > > completion time (time) have been measured when running video stream at
> > > 640x480 resolution at 10fps.
> > >
> > > x86_64 based system (Intel Core i5-3470). This platform has hardware
> > > coherent DMA support and proposed change doesn't make big difference here.
> > >
> > > * kmalloc:rate = (2.0 +- 0.4) GBps
> > >time = (5.0 +- 3.0) usec
> > > * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
> > >time = (3.5 +- 3.0) usec
> > >
> > > We see that the measurements agree within error ranges in this case.
> > > So theoretically predicted performance downgrade cannot be reliably
> > > measured here.
> > >
> > > armv7l based system (TI AM335x BeagleBone Black @ 300MHz). This platform
> > > has no hardware coherent DMA support. DMA coherence is implemented via
> > > disabled page caching that slows down memcpy() due to memory controller
> > > behaviour.
> > >
> > > * kmalloc:rate = ( 94 +- 4) MBps
> > >time = (101 +- 4) usec
> > > * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> > >time = (341 +- 2) usec
> > >
> > > Note, that quantative difference leads (this commit leads to 3.3 times
> > > acceleration) to qualitative behavior change in this case. As it was
> > > stated before, the video stream cannot be successfully received at AM335x
> > > platforms with MUSB based USB host controller due to performance issues
> > > [1].
> > >
> > > [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> > >
> > > Signed-off-by: Matwey V. Kornilov
> >
> > Ping
>
> Ping
Ping
>
> >
> > > ---
> > > drivers/media/usb/pwc/pwc-if.c | 62
> > > +-
> > > 1 file changed, 49 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/pwc/pwc-if.c
> > > b/drivers/media/usb/pwc/pwc-if.c
> > > index 53c111bd5a22..a81fb319b339 100644
> > > --- a/drivers/media/usb/pwc/pwc-if.c
> > > +++ b/drivers/media/usb/pwc/pwc-if.c
> > > @@ -159,6 +159,32 @@ static const struct video_device pwc_template = {
> > >
> > > /***/
> > > /* Private functions */
> > >
> > > +static void *pwc_alloc_urb_buffer(struct device *dev,
> > > + size_t size, dma_addr_t *dma_handle)
> > > +{
> > > + void *buffer = kmalloc(size, GFP_KERNEL);
> > > +
> > > + if (!buffer)
> > > + return NULL;
> > > +
> > > + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> > > + if (dma_mapping_error(dev, *dma_handle)) {
> > > + kfree(buffer);
> > > + return NULL;
> > > + }
> > > +
> > > + return buffer;
> > > +}
> > > +
> > > +static void pwc_free_urb_buffer(struct device *dev,
> > > + size_t size,
> > > + void *buffer,
> > > + dma_addr_t dma_handle)
> > > +{
> > > + dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
> > > + kfree(buffer);
> > > +}
> > > +
> > > static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device
> > > *pdev)
> > > {
> > > unsigned long flags = 0;
> > > @@ -306,6 +332,11 @@ static void pwc_isoc_handler(struct urb *urb)
> > > /* Reset ISOC error counter. We did get here, after all. */
> > > pdev->visoc_errors = 0;
> > >
> > > + dma_sync_single_for_cpu(&urb->dev->dev,
> > > + urb->transfer_dma,
&
[PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()
There were reports that PWC-based webcams don't work at some
embedded ARM platforms. [1] Isochronous transfer handler seems to
work too long leading to the issues in MUSB USB host subsystem.
Also note, that urb->giveback() handlers are still called with
disabled interrupts. In order to be able to measure performance of
PWC driver, traces are introduced in URB handler section.
[1] https://www.spinics.net/lists/linux-usb/msg165735.html
Signed-off-by: Matwey V. Kornilov
---
drivers/media/usb/pwc/pwc-if.c | 7 +
include/trace/events/pwc.h | 65 ++
2 files changed, 72 insertions(+)
create mode 100644 include/trace/events/pwc.h
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 72704f4d5330..53c111bd5a22 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -76,6 +76,9 @@
#include "pwc-dec23.h"
#include "pwc-dec1.h"
+#define CREATE_TRACE_POINTS
+#include
+
/* Function prototypes and driver templates */
/* hotplug device table support */
@@ -260,6 +263,8 @@ static void pwc_isoc_handler(struct urb *urb)
int i, fst, flen;
unsigned char *iso_buf = NULL;
+ trace_pwc_handler_enter(urb, pdev);
+
if (urb->status == -ENOENT || urb->status == -ECONNRESET ||
urb->status == -ESHUTDOWN) {
PWC_DEBUG_OPEN("URB (%p) unlinked %ssynchronously.\n",
@@ -348,6 +353,8 @@ static void pwc_isoc_handler(struct urb *urb)
}
handler_end:
+ trace_pwc_handler_exit(urb, pdev);
+
i = usb_submit_urb(urb, GFP_ATOMIC);
if (i != 0)
PWC_ERROR("Error (%d) re-submitting urb in
pwc_isoc_handler.\n", i);
diff --git a/include/trace/events/pwc.h b/include/trace/events/pwc.h
new file mode 100644
index ..a2da764a3b41
--- /dev/null
+++ b/include/trace/events/pwc.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if !defined(_TRACE_PWC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PWC_H
+
+#include
+#include
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pwc
+
+TRACE_EVENT(pwc_handler_enter,
+ TP_PROTO(struct urb *urb, struct pwc_device *pdev),
+ TP_ARGS(urb, pdev),
+ TP_STRUCT__entry(
+ __field(struct urb*, urb)
+ __field(struct pwc_frame_buf*, fbuf)
+ __field(int, urb__status)
+ __field(u32, urb__actual_length)
+ __field(int, fbuf__filled)
+ __string(name, pdev->v4l2_dev.name)
+ ),
+ TP_fast_assign(
+ __entry->urb = urb;
+ __entry->fbuf = pdev->fill_buf;
+ __entry->urb__status = urb->status;
+ __entry->urb__actual_length = urb->actual_length;
+ __entry->fbuf__filled = (pdev->fill_buf
+? pdev->fill_buf->filled : 0);
+ __assign_str(name, pdev->v4l2_dev.name);
+ ),
+ TP_printk("dev=%s (fbuf=%p filled=%d) urb=%p (status=%d
actual_length=%u)",
+ __get_str(name),
+ __entry->fbuf,
+ __entry->fbuf__filled,
+ __entry->urb,
+ __entry->urb__status,
+ __entry->urb__actual_length)
+);
+
+TRACE_EVENT(pwc_handler_exit,
+ TP_PROTO(struct urb *urb, struct pwc_device *pdev),
+ TP_ARGS(urb, pdev),
+ TP_STRUCT__entry(
+ __field(struct urb*, urb)
+ __field(struct pwc_frame_buf*, fbuf)
+ __field(int, fbuf__filled)
+ __string(name, pdev->v4l2_dev.name)
+ ),
+ TP_fast_assign(
+ __entry->urb = urb;
+ __entry->fbuf = pdev->fill_buf;
+ __entry->fbuf__filled = pdev->fill_buf->filled;
+ __assign_str(name, pdev->v4l2_dev.name);
+ ),
+ TP_printk(" dev=%s (fbuf=%p filled=%d) urb=%p",
+ __get_str(name),
+ __entry->fbuf,
+ __entry->fbuf__filled,
+ __entry->urb)
+);
+
+#endif /* _TRACE_PWC_H */
+
+/* This part must be outside protection */
+#include
--
2.16.4
Re: [PATCH v5 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
вт, 21 авг. 2018 г. в 20:06, Matwey V. Kornilov :
>
> DMA cocherency slows the transfer down on systems without hardware
> coherent DMA.
> Instead we use noncocherent DMA memory and explicit sync at data receive
> handler.
>
> Based on previous commit the following performance benchmarks have been
> carried out. Average memcpy() data transfer rate (rate) and handler
> completion time (time) have been measured when running video stream at
> 640x480 resolution at 10fps.
>
> x86_64 based system (Intel Core i5-3470). This platform has hardware
> coherent DMA support and proposed change doesn't make big difference here.
>
> * kmalloc:rate = (2.0 +- 0.4) GBps
>time = (5.0 +- 3.0) usec
> * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
>time = (3.5 +- 3.0) usec
>
> We see that the measurements agree within error ranges in this case.
> So theoretically predicted performance downgrade cannot be reliably
> measured here.
>
> armv7l based system (TI AM335x BeagleBone Black @ 300MHz). This platform
> has no hardware coherent DMA support. DMA coherence is implemented via
> disabled page caching that slows down memcpy() due to memory controller
> behaviour.
>
> * kmalloc:rate = (114 +- 5) MBps
>time = (84 +- 4) usec
> * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
>time = (341 +- 2) usec
>
> Note, that quantative difference leads (this commit leads to 4 times
> acceleration) to qualitative behavior change in this case. As it was
> stated before, the video stream cannot be successfully received at AM335x
> platforms with MUSB based USB host controller due to performance issues
> [1].
>
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
>
> Signed-off-by: Matwey V. Kornilov
Ping
> ---
> drivers/media/usb/pwc/pwc-if.c | 57
> --
> 1 file changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 72d2897a4b9f..1360722ab423 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -159,6 +159,32 @@ static const struct video_device pwc_template = {
> /***/
> /* Private functions */
>
> +static void *pwc_alloc_urb_buffer(struct device *dev,
> + size_t size, dma_addr_t *dma_handle)
> +{
> + void *buffer = kmalloc(size, GFP_KERNEL);
> +
> + if (!buffer)
> + return NULL;
> +
> + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, *dma_handle)) {
> + kfree(buffer);
> + return NULL;
> + }
> +
> + return buffer;
> +}
> +
> +static void pwc_free_urb_buffer(struct device *dev,
> + size_t size,
> + void *buffer,
> + dma_addr_t dma_handle)
> +{
> + dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
> + kfree(buffer);
> +}
> +
> static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device *pdev)
> {
> unsigned long flags = 0;
> @@ -306,6 +332,11 @@ static void pwc_isoc_handler(struct urb *urb)
> /* Reset ISOC error counter. We did get here, after all. */
> pdev->visoc_errors = 0;
>
> + dma_sync_single_for_cpu(&urb->dev->dev,
> + urb->transfer_dma,
> + urb->transfer_buffer_length,
> + DMA_FROM_DEVICE);
> +
> /* vsync: 0 = don't copy data
> 1 = sync-hunt
> 2 = synched
> @@ -428,16 +459,15 @@ static int pwc_isoc_init(struct pwc_device *pdev)
> urb->dev = udev;
> urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> - urb->transfer_buffer = usb_alloc_coherent(udev,
> - ISO_BUFFER_SIZE,
> - GFP_KERNEL,
> - &urb->transfer_dma);
> + urb->transfer_buffer_length = ISO_BUFFER_SIZE;
> + urb->transfer_buffer = pwc_alloc_urb_buffer(&udev->dev,
> +
> urb->transfer_buffer_length,
> +
Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-08-17 20:44 GMT+03:00 Matwey V. Kornilov : > пт, 10 авг. 2018 г. в 17:27, Alan Stern : >> >> On Fri, 10 Aug 2018, Laurent Pinchart wrote: >> >> > > > Aren't you're missing a dma_sync_single_for_device() call before >> > > > submitting the URB ? IIRC that's required for correct operation of the >> > > > DMA >> > > > mapping API on some platforms, depending on the cache architecture. The >> > > > additional sync can affect performances, so it would be useful to >> > > > re-run >> > > > the perf test. >> > > >> > > This was already discussed: >> > > >> > > https://lkml.org/lkml/2018/7/23/1051 >> > > >> > > I rely on Alan's reply: >> > > >> > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write >> > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is >> > > > not needed. >> > >> > I fully agree that the CPU should not write to the buffer. However, I think >> > the sync call is still needed. It's been a long time since I touched this >> > area, but IIRC, some cache architectures (VIVT ?) require both cache clean >> > before the transfer and cache invalidation after the transfer. On platforms >> > where no cache management operation is needed before the transfer in the >> > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be >> > no-ops >> > (and if they're not, it's a bug of the DMA mapping implementation). >> >> In general, I agree that the cache has to be clean before a transfer >> starts. This means some sort of mapping operation (like >> dma_sync_*_for-device) is indeed required at some point between the >> allocation and the first transfer. >> >> For subsequent transfers, however, the cache is already clean and it >> will remain clean because the CPU will not do any writes to the buffer. >> (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers >> following the first should not need any dma_sync_*_for_device. >> >> If you don't accept this reasoning then you should ask the people who >> wrote DMA-API-HOWTO.txt. They certainly will know more about this >> issue than I do. > > Laurent, > > I have not seen any data corruption or glitches without > dma_sync_single_for_device() on ARM and x86. > It takes additional ~20usec for dma_sync_single_for_device() to run on > ARM. So It would be great to ensure that we can reliable save another > 15% running time. DMA-API-HOWTO.txt has and example with my_card_interrupt_handler() function where it says that "CPU should not write to DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is not needed here." I've found that, for instance, drivers/crypto/caam/caamrng.c follows DMA-API-HOWTO.txt and don't use dma_sync_single_for_device() in the same case as we have in pwc. > >> >> Alan Stern >> > > > -- > With best regards, > Matwey V. Kornilov -- With best regards, Matwey V. Kornilov
Re: [PATCH 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-07-24 23:55 GMT+03:00 Alan Stern : > On Tue, 24 Jul 2018, Matwey V. Kornilov wrote: > >> 2018-07-23 21:57 GMT+03:00 Alan Stern : >> > On Mon, 23 Jul 2018, Matwey V. Kornilov wrote: >> > >> >> I've tried to strategies: >> >> >> >> 1) Use dma_unmap and dma_map inside the handler (I suppose this is >> >> similar to how USB core does when there is no URB_NO_TRANSFER_DMA_MAP) >> > >> > Yes. >> > >> >> 2) Use sync_cpu and sync_device inside the handler (and dma_map only >> >> once at memory allocation) >> >> >> >> It is interesting that dma_unmap/dma_map pair leads to the lower >> >> overhead (+1us) than sync_cpu/sync_device (+2us) at x86_64 platform. >> >> At armv7l platform using dma_unmap/dma_map leads to ~50 usec in the >> >> handler, and sync_cpu/sync_device - ~65 usec. >> >> >> >> However, I am not sure is it mandatory to call >> >> dma_sync_single_for_device for FROM_DEVICE direction? >> > >> > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write >> > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is >> > not needed. >> >> Well, I measured the following at armv7l. The handler execution time >> (URB_NO_TRANSFER_DMA_MAP is used for all cases): >> >> 1) coherent DMA: ~3000 usec (pwc is not functional) >> 2) explicit dma_unmap and dma_map in the handler: ~52 usec >> 3) explicit dma_sync_single_for_cpu (no dma_sync_single_for_device): ~56 usec >> >> So, I suppose that unfortunately Tomasz suggestion doesn't work. There >> is no performance improvement when dma_sync_single is used. >> >> At x86_64 the following happens: >> >> 1) coherent DMA: ~2 usec >> 2) explicit dma_unmap and dma_map in the handler: ~3.5 usec >> 3) explicit dma_sync_single_for_cpu (no dma_sync_single_for_device): ~4 usec >> >> So, whats to do next? Personally, I think that DMA streaming API >> introduces not so great overhead. >> Does anybody happy with turning to streaming DMA or I'll introduce >> module-level switch as Ezequiel suggested? > > How about using the dma_unmap and dma_map calls in the USB core? If > they add the same overhead as putting them in the handler, I think it > would be acceptable for x86_64. Sure, that is the simplest way to implement streaming API. > > It certainly is odd that the dma_sync_single APIs take longer than > simply mapping and unmapping. Well. I am surprised too. Probably, it is related to that only 9560 bytes are used for each URB. It is only three memory pages. > > Alan Stern > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
2018-06-06 22:15 GMT+03:00 Giulio Benetti :
> Il 06/06/2018 20:55, Matwey V. Kornilov ha scritto:
>>
>> 2018-06-06 16:11 GMT+03:00 Andy Shevchenko
>> :
>>>
>>> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>>>>
>>>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>>>>>
>>>>> On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>>>>>>
>>>>>> em485 gets lost during
>>>>>>
>>>>>> Copy em485 to final uart port.
>>>>>>
>>>>>
>>>>> Is it needed at all?
>>>>>
>>>>> The individual driver decides either to use software emulation (and
>>>>> calls explicitly serial8250_em485_init() for that) or do HW assisted
>>>>> stuff.
>>>>
>>>>
>>>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>>>> against local struct uart_8250_port uart = {};
>>>> Inside serial8250_register_8250_port() not all uart fields are
>>>> copied(em485 too).
>>>> So after probe, em485 is NULL.
>>>>
>>>> Another way could be to call dw8250_rs485_config() against real uart
>>>> port, after calling serial8250_register_8250_port(),
>>>> would it make sense?
>>>
>>>
>>> Look at OMAP case closely. They have a callback to configure RS485 which
>>> is called in uart_set_rs485_config() which is called whenever user
>>> space does TIOCGRS485 IOCTL.
>>>
>>> So, it's completely driven by user space which makes sense by my
>>> opinion.
>>
>>
>> AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
>> device tree option (see bindings/serial/rs485.txt for reference).
>
>
> Yes, I want to add support for "rs485-enabled-at-boot-time" property,
> maybe I had to write better commit log and a cover letter. Sorry.
>
>> I suppose it is only important for use-case when rs485 used as slave
>> (peripheral role).
>
>
> It's important also for master, because RTS, if RTS_AFTER_SEND is set,
> remains not asserted(trasnmission) until userspace opens that serial port.
>
And it makes no sense, because how are you going to transmit and
receive not opening the port and not running an application?
It is master who transmits the data first. So, before all systems
going to work you have to open the port from userspace.
In case of slave this of course makes sense.
> Sorry again for not explaining myself well.
>
>
> --
> Giulio Benetti
> CTO
>
> MICRONOVA SRL
> Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
> Tel. 049/8931563 - Fax 049/8931346
> Cod.Fiscale - P.IVA 02663420285
> Capitale Sociale € 26.000 i.v.
> Iscritta al Reg. Imprese di Padova N. 02663420285
> Numero R.E.A. 258642
--
With best regards,
Matwey V. Kornilov
Re: [PATCH 1/4] serial: 8250: Copy em485 from port to real port.
2018-06-06 16:11 GMT+03:00 Andy Shevchenko :
> On Wed, 2018-06-06 at 14:15 +0200, Giulio Benetti wrote:
>> Il 06/06/2018 13:56, Andy Shevchenko ha scritto:
>> > On Wed, 2018-06-06 at 11:49 +0200, Giulio Benetti wrote:
>> > > em485 gets lost during
>> > >
>> > > Copy em485 to final uart port.
>> > >
>> >
>> > Is it needed at all?
>> >
>> > The individual driver decides either to use software emulation (and
>> > calls explicitly serial8250_em485_init() for that) or do HW assisted
>> > stuff.
>>
>> In 8250_dw.c, during probe(), I need to call dw8250_rs485_config()
>> against local struct uart_8250_port uart = {};
>> Inside serial8250_register_8250_port() not all uart fields are
>> copied(em485 too).
>> So after probe, em485 is NULL.
>>
>> Another way could be to call dw8250_rs485_config() against real uart
>> port, after calling serial8250_register_8250_port(),
>> would it make sense?
>
> Look at OMAP case closely. They have a callback to configure RS485 which
> is called in uart_set_rs485_config() which is called whenever user
> space does TIOCGRS485 IOCTL.
>
> So, it's completely driven by user space which makes sense by my
> opinion.
AFAIU, Giulio wants to add support for rs485-enabled-at-boot-time
device tree option (see bindings/serial/rs485.txt for reference).
I suppose it is only important for use-case when rs485 used as slave
(peripheral role).
>
> --
> Andy Shevchenko
> Intel Finland Oy
--
With best regards,
Matwey V. Kornilov
Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
2018-06-04 21:50 GMT+03:00 Giulio Benetti :
> Il 04/06/2018 19:40, Matwey V. Kornilov ha scritto:
>>
>> 01.06.2018 15:40, Giulio Benetti пишет:
>>>
>>> Some 8250 ports only have TEMT interrupt, so current implementation
>>> can't work for ports without it. The only chance to make it work is to
>>> loop-read on LSR register.
>>>
>>> With NO TEMT interrupt check if both TEMT and THRE are set looping on
>>> LSR register.
>>>
>>> Signed-off-by: Giulio Benetti
>>> ---
>>> drivers/tty/serial/8250/8250.h | 2 +-
>>> drivers/tty/serial/8250/8250_dw.c | 2 +-
>>> drivers/tty/serial/8250/8250_omap.c | 2 +-
>>> drivers/tty/serial/8250/8250_port.c | 26 ++
>>> include/linux/serial_8250.h | 1 +
>>> 5 files changed, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250.h
>>> b/drivers/tty/serial/8250/8250.h
>>> index ebfb0bd5bef5..5c6ae5f69432 100644
>>> --- a/drivers/tty/serial/8250/8250.h
>>> +++ b/drivers/tty/serial/8250/8250.h
>>> @@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
>>> void serial8250_rpm_get_tx(struct uart_8250_port *p);
>>> void serial8250_rpm_put_tx(struct uart_8250_port *p);
>>> -int serial8250_em485_init(struct uart_8250_port *p);
>>> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
>>> void serial8250_em485_destroy(struct uart_8250_port *p);
>>> static inline void serial8250_out_MCR(struct uart_8250_port *up, int
>>> value)
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>>> b/drivers/tty/serial/8250/8250_dw.c
>>> index 0f8b4da03d4e..888280ff5451 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
>>> * are idempotent
>>> */
>>> if (rs485->flags & SER_RS485_ENABLED) {
>>> - int ret = serial8250_em485_init(up);
>>> + int ret = serial8250_em485_init(up, false);
>>> if (ret) {
>>> rs485->flags &= ~SER_RS485_ENABLED;
>>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>>> b/drivers/tty/serial/8250/8250_omap.c
>>> index 624b501fd253..ab483c8b30c8 100644
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port
>>> *port,
>>> * are idempotent
>>> */
>>> if (rs485->flags & SER_RS485_ENABLED) {
>>> - int ret = serial8250_em485_init(up);
>>> + int ret = serial8250_em485_init(up, true);
>>> if (ret) {
>>> rs485->flags &= ~SER_RS485_ENABLED;
>>> diff --git a/drivers/tty/serial/8250/8250_port.c
>>> b/drivers/tty/serial/8250/8250_port.c
>>> index 95833cbc4338..e14badbbf181 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>> /**
>>>*serial8250_em485_init() - put uart_8250_port into rs485 emulating
>>>*@p: uart_8250_port port instance
>>> + * @p: bool specify if 8250 port has TEMT interrupt or not
>>>*
>>>*The function is used to start rs485 software emulating on the
>>>*&struct uart_8250_port* @p. Namely, RTS is switched before/after
>>>*transmission. The function is idempotent, so it is safe to call
>>> it
>>>*multiple times.
>>>*
>>> - * The caller MUST enable interrupt on empty shift register before
>>> - * calling serial8250_em485_init(). This interrupt is not a part of
>>> - * 8250 standard, but implementation defined.
>>> + * If has_temt_isr is passed as true, the caller MUST enable
>>> interrupt
>>> + * on empty shift register before calling serial8250_em485_init().
>>> + * This interrupt is not a part of 8250 standard, but implementation
>>> defined.
>>>*
>>>*The function is supposed to be called from .rs485_config callback
>>>*or from any other callback protected with p->port.lock spinlock.
>>> @@ -616,7 +617,7
Re: [PATCH 4/8] serial: 8250: Handle case port doesn't have TEMT interrupt using em485.
01.06.2018 15:40, Giulio Benetti пишет:
> Some 8250 ports only have TEMT interrupt, so current implementation
> can't work for ports without it. The only chance to make it work is to
> loop-read on LSR register.
>
> With NO TEMT interrupt check if both TEMT and THRE are set looping on
> LSR register.
>
> Signed-off-by: Giulio Benetti
> ---
> drivers/tty/serial/8250/8250.h | 2 +-
> drivers/tty/serial/8250/8250_dw.c | 2 +-
> drivers/tty/serial/8250/8250_omap.c | 2 +-
> drivers/tty/serial/8250/8250_port.c | 26 ++
> include/linux/serial_8250.h | 1 +
> 5 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index ebfb0bd5bef5..5c6ae5f69432 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -136,7 +136,7 @@ void serial8250_rpm_put(struct uart_8250_port *p);
> void serial8250_rpm_get_tx(struct uart_8250_port *p);
> void serial8250_rpm_put_tx(struct uart_8250_port *p);
>
> -int serial8250_em485_init(struct uart_8250_port *p);
> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr);
> void serial8250_em485_destroy(struct uart_8250_port *p);
>
> static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index 0f8b4da03d4e..888280ff5451 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -330,7 +330,7 @@ static int dw8250_rs485_config(struct uart_port *p,
>* are idempotent
>*/
> if (rs485->flags & SER_RS485_ENABLED) {
> - int ret = serial8250_em485_init(up);
> + int ret = serial8250_em485_init(up, false);
>
> if (ret) {
> rs485->flags &= ~SER_RS485_ENABLED;
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index 624b501fd253..ab483c8b30c8 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -725,7 +725,7 @@ static int omap_8250_rs485_config(struct uart_port *port,
>* are idempotent
>*/
> if (rs485->flags & SER_RS485_ENABLED) {
> - int ret = serial8250_em485_init(up);
> + int ret = serial8250_em485_init(up, true);
>
> if (ret) {
> rs485->flags &= ~SER_RS485_ENABLED;
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index 95833cbc4338..e14badbbf181 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -599,15 +599,16 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
> /**
> * serial8250_em485_init() - put uart_8250_port into rs485 emulating
> * @p: uart_8250_port port instance
> + * @p: bool specify if 8250 port has TEMT interrupt or not
> *
> * The function is used to start rs485 software emulating on the
> * &struct uart_8250_port* @p. Namely, RTS is switched before/after
> * transmission. The function is idempotent, so it is safe to call it
> * multiple times.
> *
> - * The caller MUST enable interrupt on empty shift register before
> - * calling serial8250_em485_init(). This interrupt is not a part of
> - * 8250 standard, but implementation defined.
> + * If has_temt_isr is passed as true, the caller MUST enable interrupt
> + * on empty shift register before calling serial8250_em485_init().
> + * This interrupt is not a part of 8250 standard, but implementation
> defined.
> *
> * The function is supposed to be called from .rs485_config callback
> * or from any other callback protected with p->port.lock spinlock.
> @@ -616,7 +617,7 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
> *
> * Return 0 - success, -errno - otherwise
> */
> -int serial8250_em485_init(struct uart_8250_port *p)
> +int serial8250_em485_init(struct uart_8250_port *p, bool has_temt_isr)
> {
> if (p->em485)
> return 0;
> @@ -633,6 +634,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
> p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
> p->em485->port = p;
> p->em485->active_timer = NULL;
> + p->em485->has_temt_isr = has_temt_isr;
> serial8250_em485_rts_after_send(p);
>
> return 0;
> @@ -1517,11 +1519,19 @@ static inline void __stop_tx(struct uart_8250_port *p)
> /*
>* To provide required timeing and allow FIFO transfer,
>* __stop_tx_rs485() must be called only when both FIFO and
> - * shift register are empty. It is for device driver to enable
> - * interrupt on TEMT.
> + * shift register are empty. If 8250 port supports it,
> + * it is for device driver to enable interrupt on TEMT.
> + * Otherwise must loop-read un
Re: [PATCH 0/8] serial: 8250: Add 485 emulation to 8250_dw.
2018-06-04 13:12 GMT+03:00 Andy Shevchenko : > On Fri, 2018-06-01 at 14:40 +0200, Giulio Benetti wrote: >> Need to handle rs485 with 8250_dw port. >> >> Use existing em485 emulation layer for 8250 taking care to fix some >> bug >> and taking care especially of RTS_AFTER_SEND case. >> > > I don't see in Cc list author of rs485 code, who might be interested in > this. > > So, added him here. > Hi, Andy Nice to see you there. I will look through the code later. I always thought that DW has its own hardware rs485 support. >> Giulio Benetti (8): >> serial: 8250_dw: add em485 support >> serial: 8250_dw: allow enable rs485 at boot time >> serial: 8250: Copy em485 from port to real port. >> serial: 8250: Handle case port doesn't have TEMT interrupt using >> em485. >> serial: 8250_dw: treat rpm suspend with -EBUSY if RS485 ON and >> RTS_AFTER_SEND >> serial: 8250: Copy mctrl when register port. >> serial: 8250: Make em485_rts_after_send() set mctrl according to rts >> state. >> serial: core: Mask mctrl with TIOCM_RTS too if rs485 on and >> RTS_AFTER_SEND set. >> >> drivers/tty/serial/8250/8250.h | 2 +- >> drivers/tty/serial/8250/8250_core.c | 2 ++ >> drivers/tty/serial/8250/8250_dw.c | 41 >> - >> drivers/tty/serial/8250/8250_omap.c | 2 +- >> drivers/tty/serial/8250/8250_port.c | 33 --- >> drivers/tty/serial/serial_core.c| 12 ++++- >> include/linux/serial_8250.h | 1 + >> 7 files changed, 79 insertions(+), 14 deletions(-) >> > > -- > Andy Shevchenko > Intel Finland Oy > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-11-16 19:32 GMT+03:00 Bin Liu : > Hi, > > On Wed, Nov 15, 2017 at 06:19:08PM +0300, Matwey V. Kornilov wrote: >> The issue is also present in 4.9.60-ti-r75 >> >> 2017-11-04 17:05 GMT+03:00 Matwey V. Kornilov : >> > Hi Bin, >> > >> > I've just checked that the issue is still present in 4.13.10. > > I am not aware of any work having been done for this yet. Well, please tell If I can help somehow. > > Regards, > -Bin. > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
The issue is also present in 4.9.60-ti-r75
2017-11-04 17:05 GMT+03:00 Matwey V. Kornilov :
> Hi Bin,
>
> I've just checked that the issue is still present in 4.13.10.
>
> 2017-04-27 13:20 GMT+03:00 Matwey V. Kornilov :
>> This commit changes the order of actions undertaken in
>> musb_advance_schedule() in order to overcome issue with broken
>> isochronous transfer [1].
>>
>> There is no harm to split musb_giveback into two pieces. The first
>> unlinks finished urb, the second givebacks it. The issue here that
>> givebacking may be quite time-consuming due to urb->complete() call.
>> As it happens in case of pwc-driven web cameras. It may take about 0.5
>> ms to call __musb_giveback() that calls urb->callback() internally.
>> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
>> musb_start_urb() for the next urb will be too late to produce physical
>> IN packet. Since auto req is not used by this module the exchange
>> would be as the following:
>>
>> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
>> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0: [skipped]
>> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
>> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
>>
>> It is known that missed IN in isochronous mode makes some
>> perepherial broken. For instance, pwc-driven or uvc-driven
>> web cameras.
>> In order to workaround this issue we postpone calling
>> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> next urb if there is the next urb pending in queue.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>>
>> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to
>> handle urb return in bottom half"")
>> Signed-off-by: Matwey V. Kornilov
>> ---
>> drivers/usb/musb/musb_host.c | 54
>> +---
>> 1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index ac3a4952abb4..b590c2555dab 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct
>> musb_qh *qh)
>> }
>> }
>>
>> -/* Context: caller owns controller lock, IRQs are blocked */
>> -static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
>> __releases(musb->lock)
>> __acquires(musb->lock)
>> {
>> - trace_musb_urb_gb(musb, urb);
>> -
>> - usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> spin_unlock(&musb->lock);
>> usb_hcd_giveback_urb(musb->hcd, urb, status);
>> spin_lock(&musb->lock);
>> }
>>
>> +/* Context: caller owns controller lock, IRQs are blocked */
>> +static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +{
>> + trace_musb_urb_gb(musb, urb);
>> +
>> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> + __musb_giveback(musb, urb, status);
>> +}
>> +
>> /* For bulk/interrupt endpoints only */
>> static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
>> struct urb *urb)
>> @@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb,
>> struct urb *urb,
>> struct musb_hw_ep *ep = qh->hw_ep;
>> int ready = qh->is_ready;
>> int status;
>> + int postponed_giveback = 0;
>>
>> status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
>>
>> @@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb,
>> struct urb *urb,
>> break;
>> }
>>
>> - qh->is_ready = 0;
>> - musb_giveback(musb, urb, status);
>> - qh->is_ready = ready;
>> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> +
>> + /* It may take about 0.5 ms to call __musb_giveback() that
>> +* calls urb->callback() internally. Under specific circumstances
>> +* setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the
>> +* next urb will be too late to produce physical IN packet. Since
>> +* auto req is not used by this modu
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
Hi Bin,
I've just checked that the issue is still present in 4.13.10.
2017-04-27 13:20 GMT+03:00 Matwey V. Kornilov :
> This commit changes the order of actions undertaken in
> musb_advance_schedule() in order to overcome issue with broken
> isochronous transfer [1].
>
> There is no harm to split musb_giveback into two pieces. The first
> unlinks finished urb, the second givebacks it. The issue here that
> givebacking may be quite time-consuming due to urb->complete() call.
> As it happens in case of pwc-driven web cameras. It may take about 0.5
> ms to call __musb_giveback() that calls urb->callback() internally.
> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
> musb_start_urb() for the next urb will be too late to produce physical
> IN packet. Since auto req is not used by this module the exchange
> would be as the following:
>
> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0: [skipped]
> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
>
> It is known that missed IN in isochronous mode makes some
> perepherial broken. For instance, pwc-driven or uvc-driven
> web cameras.
> In order to workaround this issue we postpone calling
> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
> next urb if there is the next urb pending in queue.
>
> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>
> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to
> handle urb return in bottom half"")
> Signed-off-by: Matwey V. Kornilov
> ---
> drivers/usb/musb/musb_host.c | 54
> +---
> 1 file changed, 46 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index ac3a4952abb4..b590c2555dab 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct
> musb_qh *qh)
> }
> }
>
> -/* Context: caller owns controller lock, IRQs are blocked */
> -static void musb_giveback(struct musb *musb, struct urb *urb, int status)
> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
> __releases(musb->lock)
> __acquires(musb->lock)
> {
> - trace_musb_urb_gb(musb, urb);
> -
> - usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
> spin_unlock(&musb->lock);
> usb_hcd_giveback_urb(musb->hcd, urb, status);
> spin_lock(&musb->lock);
> }
>
> +/* Context: caller owns controller lock, IRQs are blocked */
> +static void musb_giveback(struct musb *musb, struct urb *urb, int status)
> +{
> + trace_musb_urb_gb(musb, urb);
> +
> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
> + __musb_giveback(musb, urb, status);
> +}
> +
> /* For bulk/interrupt endpoints only */
> static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
> struct urb *urb)
> @@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb,
> struct urb *urb,
> struct musb_hw_ep *ep = qh->hw_ep;
> int ready = qh->is_ready;
> int status;
> + int postponed_giveback = 0;
>
> status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
>
> @@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb,
> struct urb *urb,
> break;
> }
>
> - qh->is_ready = 0;
> - musb_giveback(musb, urb, status);
> - qh->is_ready = ready;
> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
> +
> + /* It may take about 0.5 ms to call __musb_giveback() that
> +* calls urb->callback() internally. Under specific circumstances
> +* setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the
> +* next urb will be too late to produce physical IN packet. Since
> +* auto req is not used by this module the exchange would be as the
> +* following:
> +*
> +* [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN :
> 4.5
> +* [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0:
> [skipped]
> +* [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN :
> 4.5
> +* [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0:
> 00 00
> +*
> +
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-29 17:24 GMT+03:00 Matwey V. Kornilov :
> 2017-04-29 11:16 GMT+03:00 Matwey V. Kornilov :
>> 2017-04-28 16:30 GMT+03:00 Bin Liu :
>>> On Fri, Apr 28, 2017 at 04:15:09PM +0300, Matwey V. Kornilov wrote:
>>>> 2017-04-28 15:43 GMT+03:00 Bin Liu :
>>>> > On Fri, Apr 28, 2017 at 03:13:55PM +0300, Matwey V. Kornilov wrote:
>>>> >> which i
>>>> >>
>>>> >> 2017-04-28 14:58 GMT+03:00 Bin Liu :
>>>> >> > On Fri, Apr 28, 2017 at 10:04:30AM +0300, Matwey V. Kornilov wrote:
>>>> >> >> 2017-04-27 20:13 GMT+03:00 Bin Liu :
>>>> >> >> > On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>>>> >> >> >> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>>>> >> >> >> > Hi Matwey,
>>>> >> >> >> >
>>>> >> >> >> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov
>>>> >> >> >> > wrote:
>>>> >> >> >> >> This commit changes the order of actions undertaken in
>>>> >> >> >> >> musb_advance_schedule() in order to overcome issue with broken
>>>> >> >> >> >> isochronous transfer [1].
>>>> >> >> >> >>
>>>> >> >> >> >> There is no harm to split musb_giveback into two pieces. The
>>>> >> >> >> >> first
>>>> >> >> >> >> unlinks finished urb, the second givebacks it. The issue here
>>>> >> >> >> >> that
>>>> >> >> >> >> givebacking may be quite time-consuming due to urb->complete()
>>>> >> >> >> >> call.
>>>> >> >> >> >> As it happens in case of pwc-driven web cameras. It may take
>>>> >> >> >> >> about 0.5
>>>> >> >> >> >> ms to call __musb_giveback() that calls urb->callback()
>>>> >> >> >> >> internally.
>>>> >> >> >> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in
>>>> >> >> >> >> subsequent
>>>> >> >> >> >> musb_start_urb() for the next urb will be too late to produce
>>>> >> >> >> >> physical
>>>> >> >> >> >> IN packet. Since auto req is not used by this module the
>>>> >> >> >> >> exchange
>>>> >> >> >> >> would be as the following:
>>>> >> >> >> >>
>>>> >> >> >> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN
>>>> >> >> >> >> : 4.5
>>>> >> >> >> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800]
>>>> >> >> >> >> DATA0: [skipped]
>>>> >> >> >> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN
>>>> >> >> >> >> : 4.5
>>>> >> >> >> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3]
>>>> >> >> >> >> DATA0: 00 00
>>>> >> >> >> >>
>>>> >> >> >> >> It is known that missed IN in isochronous mode makes some
>>>> >> >> >> >> perepherial broken. For instance, pwc-driven or uvc-driven
>>>> >> >> >> >> web cameras.
>>>> >> >> >> >> In order to workaround this issue we postpone calling
>>>> >> >> >> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>>>> >> >> >> >> next urb if there is the next urb pending in queue.
>>>> >> >> >> >>
>>>> >> >> >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>>>> >> >> >> >>
>>>> >> >> >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable
>>>> >> >> >> >> HCD_BH flag to handle urb return in bottom half"")
>>>> >> >> >> >> Signed-off-by: Matwey V. Kornilov
>>>> >> >>
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-29 11:16 GMT+03:00 Matwey V. Kornilov :
> 2017-04-28 16:30 GMT+03:00 Bin Liu :
>> On Fri, Apr 28, 2017 at 04:15:09PM +0300, Matwey V. Kornilov wrote:
>>> 2017-04-28 15:43 GMT+03:00 Bin Liu :
>>> > On Fri, Apr 28, 2017 at 03:13:55PM +0300, Matwey V. Kornilov wrote:
>>> >> which i
>>> >>
>>> >> 2017-04-28 14:58 GMT+03:00 Bin Liu :
>>> >> > On Fri, Apr 28, 2017 at 10:04:30AM +0300, Matwey V. Kornilov wrote:
>>> >> >> 2017-04-27 20:13 GMT+03:00 Bin Liu :
>>> >> >> > On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>>> >> >> >> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>>> >> >> >> > Hi Matwey,
>>> >> >> >> >
>>> >> >> >> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov
>>> >> >> >> > wrote:
>>> >> >> >> >> This commit changes the order of actions undertaken in
>>> >> >> >> >> musb_advance_schedule() in order to overcome issue with broken
>>> >> >> >> >> isochronous transfer [1].
>>> >> >> >> >>
>>> >> >> >> >> There is no harm to split musb_giveback into two pieces. The
>>> >> >> >> >> first
>>> >> >> >> >> unlinks finished urb, the second givebacks it. The issue here
>>> >> >> >> >> that
>>> >> >> >> >> givebacking may be quite time-consuming due to urb->complete()
>>> >> >> >> >> call.
>>> >> >> >> >> As it happens in case of pwc-driven web cameras. It may take
>>> >> >> >> >> about 0.5
>>> >> >> >> >> ms to call __musb_giveback() that calls urb->callback()
>>> >> >> >> >> internally.
>>> >> >> >> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in
>>> >> >> >> >> subsequent
>>> >> >> >> >> musb_start_urb() for the next urb will be too late to produce
>>> >> >> >> >> physical
>>> >> >> >> >> IN packet. Since auto req is not used by this module the
>>> >> >> >> >> exchange
>>> >> >> >> >> would be as the following:
>>> >> >> >> >>
>>> >> >> >> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN
>>> >> >> >> >> : 4.5
>>> >> >> >> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800]
>>> >> >> >> >> DATA0: [skipped]
>>> >> >> >> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN
>>> >> >> >> >> : 4.5
>>> >> >> >> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3]
>>> >> >> >> >> DATA0: 00 00
>>> >> >> >> >>
>>> >> >> >> >> It is known that missed IN in isochronous mode makes some
>>> >> >> >> >> perepherial broken. For instance, pwc-driven or uvc-driven
>>> >> >> >> >> web cameras.
>>> >> >> >> >> In order to workaround this issue we postpone calling
>>> >> >> >> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>>> >> >> >> >> next urb if there is the next urb pending in queue.
>>> >> >> >> >>
>>> >> >> >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>>> >> >> >> >>
>>> >> >> >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable
>>> >> >> >> >> HCD_BH flag to handle urb return in bottom half"")
>>> >> >> >> >> Signed-off-by: Matwey V. Kornilov
>>> >> >> >> >
>>> >> >> >> > Thanks for the effort of working on this long standing issue, I
>>> >> >> >> > know you
>>> >> >> >> > have spent alot of time on it, but what I am thinking is
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-28 16:30 GMT+03:00 Bin Liu :
> On Fri, Apr 28, 2017 at 04:15:09PM +0300, Matwey V. Kornilov wrote:
>> 2017-04-28 15:43 GMT+03:00 Bin Liu :
>> > On Fri, Apr 28, 2017 at 03:13:55PM +0300, Matwey V. Kornilov wrote:
>> >> which i
>> >>
>> >> 2017-04-28 14:58 GMT+03:00 Bin Liu :
>> >> > On Fri, Apr 28, 2017 at 10:04:30AM +0300, Matwey V. Kornilov wrote:
>> >> >> 2017-04-27 20:13 GMT+03:00 Bin Liu :
>> >> >> > On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>> >> >> >> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>> >> >> >> > Hi Matwey,
>> >> >> >> >
>> >> >> >> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov
>> >> >> >> > wrote:
>> >> >> >> >> This commit changes the order of actions undertaken in
>> >> >> >> >> musb_advance_schedule() in order to overcome issue with broken
>> >> >> >> >> isochronous transfer [1].
>> >> >> >> >>
>> >> >> >> >> There is no harm to split musb_giveback into two pieces. The
>> >> >> >> >> first
>> >> >> >> >> unlinks finished urb, the second givebacks it. The issue here
>> >> >> >> >> that
>> >> >> >> >> givebacking may be quite time-consuming due to urb->complete()
>> >> >> >> >> call.
>> >> >> >> >> As it happens in case of pwc-driven web cameras. It may take
>> >> >> >> >> about 0.5
>> >> >> >> >> ms to call __musb_giveback() that calls urb->callback()
>> >> >> >> >> internally.
>> >> >> >> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in
>> >> >> >> >> subsequent
>> >> >> >> >> musb_start_urb() for the next urb will be too late to produce
>> >> >> >> >> physical
>> >> >> >> >> IN packet. Since auto req is not used by this module the exchange
>> >> >> >> >> would be as the following:
>> >> >> >> >>
>> >> >> >> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN :
>> >> >> >> >> 4.5
>> >> >> >> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0:
>> >> >> >> >> [skipped]
>> >> >> >> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN :
>> >> >> >> >> 4.5
>> >> >> >> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0:
>> >> >> >> >> 00 00
>> >> >> >> >>
>> >> >> >> >> It is known that missed IN in isochronous mode makes some
>> >> >> >> >> perepherial broken. For instance, pwc-driven or uvc-driven
>> >> >> >> >> web cameras.
>> >> >> >> >> In order to workaround this issue we postpone calling
>> >> >> >> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> >> >> >> >> next urb if there is the next urb pending in queue.
>> >> >> >> >>
>> >> >> >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>> >> >> >> >>
>> >> >> >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable
>> >> >> >> >> HCD_BH flag to handle urb return in bottom half"")
>> >> >> >> >> Signed-off-by: Matwey V. Kornilov
>> >> >> >> >
>> >> >> >> > Thanks for the effort of working on this long standing issue, I
>> >> >> >> > know you
>> >> >> >> > have spent alot of time on it, but what I am thinking is instead
>> >> >> >> > of
>> >> >> >> > workaround the problem we need to understand the root cause - why
>> >> >> >> > uvc-video takes longer to exec the urb callback, why only am335x
>> >> >> >> > reported this issue. This is on my backlo
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-28 15:43 GMT+03:00 Bin Liu :
> On Fri, Apr 28, 2017 at 03:13:55PM +0300, Matwey V. Kornilov wrote:
>> which i
>>
>> 2017-04-28 14:58 GMT+03:00 Bin Liu :
>> > On Fri, Apr 28, 2017 at 10:04:30AM +0300, Matwey V. Kornilov wrote:
>> >> 2017-04-27 20:13 GMT+03:00 Bin Liu :
>> >> > On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>> >> >> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>> >> >> > Hi Matwey,
>> >> >> >
>> >> >> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote:
>> >> >> >> This commit changes the order of actions undertaken in
>> >> >> >> musb_advance_schedule() in order to overcome issue with broken
>> >> >> >> isochronous transfer [1].
>> >> >> >>
>> >> >> >> There is no harm to split musb_giveback into two pieces. The first
>> >> >> >> unlinks finished urb, the second givebacks it. The issue here that
>> >> >> >> givebacking may be quite time-consuming due to urb->complete() call.
>> >> >> >> As it happens in case of pwc-driven web cameras. It may take about
>> >> >> >> 0.5
>> >> >> >> ms to call __musb_giveback() that calls urb->callback() internally.
>> >> >> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in
>> >> >> >> subsequent
>> >> >> >> musb_start_urb() for the next urb will be too late to produce
>> >> >> >> physical
>> >> >> >> IN packet. Since auto req is not used by this module the exchange
>> >> >> >> would be as the following:
>> >> >> >>
>> >> >> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
>> >> >> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0:
>> >> >> >> [skipped]
>> >> >> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
>> >> >> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00
>> >> >> >> 00
>> >> >> >>
>> >> >> >> It is known that missed IN in isochronous mode makes some
>> >> >> >> perepherial broken. For instance, pwc-driven or uvc-driven
>> >> >> >> web cameras.
>> >> >> >> In order to workaround this issue we postpone calling
>> >> >> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> >> >> >> next urb if there is the next urb pending in queue.
>> >> >> >>
>> >> >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>> >> >> >>
>> >> >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH
>> >> >> >> flag to handle urb return in bottom half"")
>> >> >> >> Signed-off-by: Matwey V. Kornilov
>> >> >> >
>> >> >> > Thanks for the effort of working on this long standing issue, I know
>> >> >> > you
>> >> >> > have spent alot of time on it, but what I am thinking is instead of
>> >> >> > workaround the problem we need to understand the root cause - why
>> >> >> > uvc-video takes longer to exec the urb callback, why only am335x
>> >> >> > reported this issue. This is on my backlog, just seems never got time
>> >> >> > for it...
>> >> >>
>> >> >> Have you tried other SoCs with Invetra MUSB?
>> >> >
>> >> > That is the plan, I got an A20 board, but haven't bring it up yet.
>> >> >
>> >> >>
>> >> >> >
>> >> >> > Ideally MUSB driver should be just using HCD_BH flag and let the
>> >> >> > core to
>> >> >> > handle the urb callback, regardless the usb transfer types.
>> >> >>
>> >> >> I think the only reason why everything worked before with HCD_BH is
>> >> >> that execution of urb->callback() was placed after musb_start(). The
>> >> >> order of operations matters.
>> >> >> However, you said that something was also wrong with HCD_BH and o
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
which i
2017-04-28 14:58 GMT+03:00 Bin Liu :
> On Fri, Apr 28, 2017 at 10:04:30AM +0300, Matwey V. Kornilov wrote:
>> 2017-04-27 20:13 GMT+03:00 Bin Liu :
>> > On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>> >> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>> >> > Hi Matwey,
>> >> >
>> >> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote:
>> >> >> This commit changes the order of actions undertaken in
>> >> >> musb_advance_schedule() in order to overcome issue with broken
>> >> >> isochronous transfer [1].
>> >> >>
>> >> >> There is no harm to split musb_giveback into two pieces. The first
>> >> >> unlinks finished urb, the second givebacks it. The issue here that
>> >> >> givebacking may be quite time-consuming due to urb->complete() call.
>> >> >> As it happens in case of pwc-driven web cameras. It may take about 0.5
>> >> >> ms to call __musb_giveback() that calls urb->callback() internally.
>> >> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
>> >> >> musb_start_urb() for the next urb will be too late to produce physical
>> >> >> IN packet. Since auto req is not used by this module the exchange
>> >> >> would be as the following:
>> >> >>
>> >> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
>> >> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0:
>> >> >> [skipped]
>> >> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
>> >> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
>> >> >>
>> >> >> It is known that missed IN in isochronous mode makes some
>> >> >> perepherial broken. For instance, pwc-driven or uvc-driven
>> >> >> web cameras.
>> >> >> In order to workaround this issue we postpone calling
>> >> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> >> >> next urb if there is the next urb pending in queue.
>> >> >>
>> >> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>> >> >>
>> >> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag
>> >> >> to handle urb return in bottom half"")
>> >> >> Signed-off-by: Matwey V. Kornilov
>> >> >
>> >> > Thanks for the effort of working on this long standing issue, I know you
>> >> > have spent alot of time on it, but what I am thinking is instead of
>> >> > workaround the problem we need to understand the root cause - why
>> >> > uvc-video takes longer to exec the urb callback, why only am335x
>> >> > reported this issue. This is on my backlog, just seems never got time
>> >> > for it...
>> >>
>> >> Have you tried other SoCs with Invetra MUSB?
>> >
>> > That is the plan, I got an A20 board, but haven't bring it up yet.
>> >
>> >>
>> >> >
>> >> > Ideally MUSB driver should be just using HCD_BH flag and let the core to
>> >> > handle the urb callback, regardless the usb transfer types.
>> >>
>> >> I think the only reason why everything worked before with HCD_BH is
>> >> that execution of urb->callback() was placed after musb_start(). The
>> >> order of operations matters.
>> >> However, you said that something was also wrong with HCD_BH and other
>> >> peripherals.
>> >
>> > HCD_BH flag cause some issues which are docummented in the commit log of
>> > f551e1352983.
>> > But even with HCD_BH flag, it didn't work for uvc webcams, it still misses
>> > IN tokens. It might helps pwc webcams though.
>>
>> pwc webcams work with HCD_BH fine indeed.
>
> yeah, this is what you told long time ago.
>
>>
>> >
>> >> > The MUSB drivers are already messy and complicated enough for
>> >> > maintenance, I'd like to understand the root cause of the delay first
>> >> > before decide how to solve the issue.
>> >> >
>> >>
>> >> I feel from playing with OpenVizsla that REQPKT should be set well in
>> >> advance. So, time window to set the flag is a
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-27 20:13 GMT+03:00 Bin Liu :
> On Thu, Apr 27, 2017 at 07:26:31PM +0300, Matwey V. Kornilov wrote:
>> 2017-04-27 18:35 GMT+03:00 Bin Liu :
>> > Hi Matwey,
>> >
>> > On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote:
>> >> This commit changes the order of actions undertaken in
>> >> musb_advance_schedule() in order to overcome issue with broken
>> >> isochronous transfer [1].
>> >>
>> >> There is no harm to split musb_giveback into two pieces. The first
>> >> unlinks finished urb, the second givebacks it. The issue here that
>> >> givebacking may be quite time-consuming due to urb->complete() call.
>> >> As it happens in case of pwc-driven web cameras. It may take about 0.5
>> >> ms to call __musb_giveback() that calls urb->callback() internally.
>> >> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
>> >> musb_start_urb() for the next urb will be too late to produce physical
>> >> IN packet. Since auto req is not used by this module the exchange
>> >> would be as the following:
>> >>
>> >> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
>> >> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0: [skipped]
>> >> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
>> >> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
>> >>
>> >> It is known that missed IN in isochronous mode makes some
>> >> perepherial broken. For instance, pwc-driven or uvc-driven
>> >> web cameras.
>> >> In order to workaround this issue we postpone calling
>> >> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> >> next urb if there is the next urb pending in queue.
>> >>
>> >> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>> >>
>> >> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to
>> >> handle urb return in bottom half"")
>> >> Signed-off-by: Matwey V. Kornilov
>> >
>> > Thanks for the effort of working on this long standing issue, I know you
>> > have spent alot of time on it, but what I am thinking is instead of
>> > workaround the problem we need to understand the root cause - why
>> > uvc-video takes longer to exec the urb callback, why only am335x
>> > reported this issue. This is on my backlog, just seems never got time
>> > for it...
>>
>> Have you tried other SoCs with Invetra MUSB?
>
> That is the plan, I got an A20 board, but haven't bring it up yet.
>
>>
>> >
>> > Ideally MUSB driver should be just using HCD_BH flag and let the core to
>> > handle the urb callback, regardless the usb transfer types.
>>
>> I think the only reason why everything worked before with HCD_BH is
>> that execution of urb->callback() was placed after musb_start(). The
>> order of operations matters.
>> However, you said that something was also wrong with HCD_BH and other
>> peripherals.
>
> HCD_BH flag cause some issues which are docummented in the commit log of
> f551e1352983.
> But even with HCD_BH flag, it didn't work for uvc webcams, it still misses
> IN tokens. It might helps pwc webcams though.
pwc webcams work with HCD_BH fine indeed.
>
>> > The MUSB drivers are already messy and complicated enough for
>> > maintenance, I'd like to understand the root cause of the delay first
>> > before decide how to solve the issue.
>> >
>>
>> I feel from playing with OpenVizsla that REQPKT should be set well in
>> advance. So, time window to set the flag is actually smaller than 1
>> ms.
>> urb->callback() is never takes longer than 0.4 ms for pwc driver, but
>> INs are skipped.
>
> Setting REQPKT in advance might be the solution, but I'd like to
> understand why only Isoch transfer shows such issue, and why only am335x
> reports this issue. The later concerns me more if this would be a
> system issue not only in usb module.
0.4 ms is about 10 CPU cycles given that CPU is running at 275Mhz
(which is the lowest cpufreq). Long time ago, I run pwc webcam with
SIS Vortex86 at 200Mhz It worked fine. I would not say that it is
extraordinary value.
Do you think that somewhere CPU cycles are wasted globally for some reason?
>
>> At the same time musb_host doesn't utilize AutoReq here. I think many
>> other USB host controllers (OHCI?) just rely on hardware to send IN
>> packets in time.
>
> For Isoch transfer, MUSB has to be programmed for every transaction, but
> urb callback takes too long with spinlock held, which cause MUSB misses
> issuing IN tokens.
>
> Regards,
> -Bin.
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-27 18:35 GMT+03:00 Bin Liu :
> Hi Matwey,
>
> On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote:
>> This commit changes the order of actions undertaken in
>> musb_advance_schedule() in order to overcome issue with broken
>> isochronous transfer [1].
>>
>> There is no harm to split musb_giveback into two pieces. The first
>> unlinks finished urb, the second givebacks it. The issue here that
>> givebacking may be quite time-consuming due to urb->complete() call.
>> As it happens in case of pwc-driven web cameras. It may take about 0.5
>> ms to call __musb_giveback() that calls urb->callback() internally.
>> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
>> musb_start_urb() for the next urb will be too late to produce physical
>> IN packet. Since auto req is not used by this module the exchange
>> would be as the following:
>>
>> [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
>> [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0: [skipped]
>> [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
>> [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
>>
>> It is known that missed IN in isochronous mode makes some
>> perepherial broken. For instance, pwc-driven or uvc-driven
>> web cameras.
>> In order to workaround this issue we postpone calling
>> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
>> next urb if there is the next urb pending in queue.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
>>
>> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to
>> handle urb return in bottom half"")
>> Signed-off-by: Matwey V. Kornilov
>
> Thanks for the effort of working on this long standing issue, I know you
> have spent alot of time on it, but what I am thinking is instead of
> workaround the problem we need to understand the root cause - why
> uvc-video takes longer to exec the urb callback, why only am335x
> reported this issue. This is on my backlog, just seems never got time
> for it...
Have you tried other SoCs with Invetra MUSB?
>
> Ideally MUSB driver should be just using HCD_BH flag and let the core to
> handle the urb callback, regardless the usb transfer types.
I think the only reason why everything worked before with HCD_BH is
that execution of urb->callback() was placed after musb_start(). The
order of operations matters.
However, you said that something was also wrong with HCD_BH and other
peripherals.
>
> The MUSB drivers are already messy and complicated enough for
> maintenance, I'd like to understand the root cause of the delay first
> before decide how to solve the issue.
>
I feel from playing with OpenVizsla that REQPKT should be set well in
advance. So, time window to set the flag is actually smaller than 1
ms.
urb->callback() is never takes longer than 0.4 ms for pwc driver, but
INs are skipped.
At the same time musb_host doesn't utilize AutoReq here. I think many
other USB host controllers (OHCI?) just rely on hardware to send IN
packets in time.
> Regards,
> -Bin.
>
>> ---
>> drivers/usb/musb/musb_host.c | 54
>> +---
>> 1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index ac3a4952abb4..b590c2555dab 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct
>> musb_qh *qh)
>> }
>> }
>>
>> -/* Context: caller owns controller lock, IRQs are blocked */
>> -static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
>> __releases(musb->lock)
>> __acquires(musb->lock)
>> {
>> - trace_musb_urb_gb(musb, urb);
>> -
>> - usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> spin_unlock(&musb->lock);
>> usb_hcd_giveback_urb(musb->hcd, urb, status);
>> spin_lock(&musb->lock);
>> }
>>
>> +/* Context: caller owns controller lock, IRQs are blocked */
>> +static void musb_giveback(struct musb *musb, struct urb *urb, int status)
>> +{
>> + trace_musb_urb_gb(musb, urb);
>> +
>> + usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>> + __musb_giveback(musb, urb, status);
>> +}
>> +
>> /* For bulk/interrupt endpoints only */
>> static
[PATCH] usb: musb: musb_host: Introduce postponed URB giveback
This commit changes the order of actions undertaken in
musb_advance_schedule() in order to overcome issue with broken
isochronous transfer [1].
There is no harm to split musb_giveback into two pieces. The first
unlinks finished urb, the second givebacks it. The issue here that
givebacking may be quite time-consuming due to urb->complete() call.
As it happens in case of pwc-driven web cameras. It may take about 0.5
ms to call __musb_giveback() that calls urb->callback() internally.
Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
musb_start_urb() for the next urb will be too late to produce physical
IN packet. Since auto req is not used by this module the exchange
would be as the following:
[] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
[T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0: [skipped]
[] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
[] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00 00
It is known that missed IN in isochronous mode makes some
perepherial broken. For instance, pwc-driven or uvc-driven
web cameras.
In order to workaround this issue we postpone calling
urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
next urb if there is the next urb pending in queue.
[1] https://www.spinics.net/lists/linux-usb/msg145747.html
Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to
handle urb return in bottom half"")
Signed-off-by: Matwey V. Kornilov
---
drivers/usb/musb/musb_host.c | 54 +---
1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index ac3a4952abb4..b590c2555dab 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct
musb_qh *qh)
}
}
-/* Context: caller owns controller lock, IRQs are blocked */
-static void musb_giveback(struct musb *musb, struct urb *urb, int status)
+static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
__releases(musb->lock)
__acquires(musb->lock)
{
- trace_musb_urb_gb(musb, urb);
-
- usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
spin_unlock(&musb->lock);
usb_hcd_giveback_urb(musb->hcd, urb, status);
spin_lock(&musb->lock);
}
+/* Context: caller owns controller lock, IRQs are blocked */
+static void musb_giveback(struct musb *musb, struct urb *urb, int status)
+{
+ trace_musb_urb_gb(musb, urb);
+
+ usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
+ __musb_giveback(musb, urb, status);
+}
+
/* For bulk/interrupt endpoints only */
static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
struct urb *urb)
@@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb, struct
urb *urb,
struct musb_hw_ep *ep = qh->hw_ep;
int ready = qh->is_ready;
int status;
+ int postponed_giveback = 0;
status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
@@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb,
struct urb *urb,
break;
}
- qh->is_ready = 0;
- musb_giveback(musb, urb, status);
- qh->is_ready = ready;
+ usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
+
+ /* It may take about 0.5 ms to call __musb_giveback() that
+* calls urb->callback() internally. Under specific circumstances
+* setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the
+* next urb will be too late to produce physical IN packet. Since
+* auto req is not used by this module the exchange would be as the
+* following:
+*
+* [] 7.220456 d= 0.000997 [182 + 3.667] [ 3] IN : 4.5
+* [T ] 7.220459 d= 0.03 [182 + 7.000] [800] DATA0:
[skipped]
+* [] 7.222456 d= 0.001997 [184 + 3.667] [ 3] IN : 4.5
+* [] 7.222459 d= 0.03 [184 + 7.000] [ 3] DATA0: 00
00
+*
+* It is known that missed IN in isochronous mode makes some
+* perepherial broken. For instance, pwc-driven or uvc-driven
+* web cameras.
+* In order to workaround this issue we postpone calling
+* urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
+* next urb if there is the next urb pending in queue.
+*/
+ if (is_in && qh->type == USB_ENDPOINT_XFER_ISOC
+ && !list_empty(&qh->hep->urb_list)) {
+ postponed_giveback = 1;
+ } else {
+ qh->is_ready = 0;
+ __musb_giveback(musb, urb, status);
+
Re: [PATCH v3] tty: serial: 8250: add MOXA Smartio MUE boards support
Hi, I am currently reading misc stuff in serial/8250/. What is the reason why 8250_moxa ever exists? I mean, it seems that 8250_pci module does the same as 8250_moxa does. 2016-03-19 12:07 GMT+03:00 Mathieu OTHACEHE : > Hi, > > Sorry about late reply. > > No I haven't planned to do it soon. Your board is supported by mxser driver > (drivers/tty/mxser.c). > I think it would be nice to move everything from mxser.c to 8250_moxa.c. > > The problem is mxser supports ISA and PCI boards and I don't have the > hardware to test it. > > Mathieu -- With best regards, Matwey V. Kornilov http://blog.matwey.name xmpp://0x2...@jabber.ru
[PATCH] serial: 8250: moxa: Store num_ports in brd
When struct moxa8250_board is allocated, then num_ports should be initialized in order to use it later in moxa8250_remove. Signed-off-by: Matwey V. Kornilov --- drivers/tty/serial/8250/8250_moxa.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/tty/serial/8250/8250_moxa.c b/drivers/tty/serial/8250/8250_moxa.c index 26eb539..d5069b2 100644 --- a/drivers/tty/serial/8250/8250_moxa.c +++ b/drivers/tty/serial/8250/8250_moxa.c @@ -68,6 +68,7 @@ static int moxa8250_probe(struct pci_dev *pdev, const struct pci_device_id *id) sizeof(unsigned int) * nr_ports, GFP_KERNEL); if (!brd) return -ENOMEM; + brd->num_ports = nr_ports; memset(&uart, 0, sizeof(struct uart_8250_port)); -- 2.6.6
[PATCH] igb: Explicitly select page 0 at initialization
The functions igb_read_phy_reg_gs40g/igb_write_phy_reg_gs40g (which were
removed in 2a3cdea) explicitly selected the required page at every phy_reg
access. Currently, igb_get_phy_id_82575 relays on the fact that page 0 is
already selected. The assumption is not fulfilled for my Lex 3I380CW
motherboard with integrated dual i211 based gigabit ethernet. This leads to igb
initialization failure and network interfaces are not working:
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
igb: Copyright (c) 2007-2014 Intel Corporation.
igb: probe of :01:00.0 failed with error -2
igb: probe of :02:00.0 failed with error -2
In order to fix it, we explicitly select page 0 before first access to phy
registers.
See also: https://bugzilla.suse.com/show_bug.cgi?id=1009911
See also: http://www.lex.com.tw/products/pdf/3I380A&3I380CW.pdf
Fixes: 2a3cdea ("igb: Remove GS40G specific defines/functions")
Cc: # 4.5+
Signed-off-by: Matwey V. Kornilov
---
drivers/net/ethernet/intel/igb/e1000_82575.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c
b/drivers/net/ethernet/intel/igb/e1000_82575.c
index a61447f..1264a36 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -246,6 +246,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
E1000_STATUS_FUNC_SHIFT;
/* Set phy->phy_addr and phy->id. */
+ igb_write_phy_reg_82580(hw, I347AT4_PAGE_SELECT, 0);
ret_val = igb_get_phy_id_82575(hw);
if (ret_val)
return ret_val;
--
2.1.4
Re: [PATCH v2 0/2] serial: Add IrDA support to 8250_dw driver
I like this idea. OMAP UART (8250_omap) also can be switched to IrDA mode, but I don't know anyone who use it. 2016-10-28 20:04 GMT+03:00 Ed Blake : > This patch set adds IrDA support to the 8250_dw driver. The first patch > exposes > the set_ldisc() function in the 8250 driver so it can be overridden, and the > second patch adds a set_ldisc() function to the 8250_dw driver which enables / > disables IrDA SIR mode if supported. > > v2: > The 8250_dw set_ldisc() function no longer tries to enable/disable IrDA SIR > mode if not supported by the hardware configuration. > > Ed Blake (2): > serial: 8250: Expose set_ldisc function > serial: 8250_dw: Add support for IrDA SIR mode > > drivers/tty/serial/8250/8250_core.c | 3 +++ > drivers/tty/serial/8250/8250_dw.c | 24 > drivers/tty/serial/8250/8250_port.c | 12 ++-- > include/linux/serial_8250.h | 4 > include/linux/serial_core.h | 2 ++ > 5 files changed, 43 insertions(+), 2 deletions(-) > > -- > 1.9.1 > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH 0967/1285] Replace numeric parameter like 0444 with macro
Hello,
I believe that 0644 is shorter and easier to read and understand than
the long list of macros like S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
where it is easier to miss something.
2016-08-02 15:03 GMT+03:00 Baole Ni :
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access permission.
> As we know, these numeric value for access permission have had the
> corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.
>
> Signed-off-by: Chuansheng Liu
> Signed-off-by: Baole Ni
> ---
> drivers/tty/serial/8250/8250_core.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index 0fbd7c0..9309ec1 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1188,17 +1188,17 @@ module_exit(serial8250_exit);
> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
>
> -module_param(share_irqs, uint, 0644);
> +module_param(share_irqs, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices
> (unsafe)");
>
> -module_param(nr_uarts, uint, 0644);
> +module_param(nr_uarts, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-"
> __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>
> -module_param(skip_txen_test, uint, 0644);
> +module_param(skip_txen_test, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init
> time");
>
> #ifdef CONFIG_SERIAL_8250_RSA
> -module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
> +module_param_array(probe_rsa, ulong, &probe_rsa_count, S_IRUSR | S_IRGRP |
> S_IROTH);
> MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
> #endif
> MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
> --
> 2.9.2
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH 0967/1285] Replace numeric parameter like 0444 with macro
And actually S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH can be simplified
to S_IRUGO | S_IWUSR, not?
I think we need to have and use dedicated special macros for most
common magic octal permissions like 0755 or 0644.
2016-08-02 16:19 GMT+03:00 Matwey V. Kornilov :
> Hello,
>
> I believe that 0644 is shorter and easier to read and understand than
> the long list of macros like S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
> where it is easier to miss something.
>
> 2016-08-02 15:03 GMT+03:00 Baole Ni :
>> I find that the developers often just specified the numeric value
>> when calling a macro which is defined with a parameter for access permission.
>> As we know, these numeric value for access permission have had the
>> corresponding macro,
>> and that using macro can improve the robustness and readability of the code,
>> thus, I suggest replacing the numeric parameter with the macro.
>>
>> Signed-off-by: Chuansheng Liu
>> Signed-off-by: Baole Ni
>> ---
>> drivers/tty/serial/8250/8250_core.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c
>> b/drivers/tty/serial/8250/8250_core.c
>> index 0fbd7c0..9309ec1 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -1188,17 +1188,17 @@ module_exit(serial8250_exit);
>> MODULE_LICENSE("GPL");
>> MODULE_DESCRIPTION("Generic 8250/16x50 serial driver");
>>
>> -module_param(share_irqs, uint, 0644);
>> +module_param(share_irqs, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>> MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices
>> (unsafe)");
>>
>> -module_param(nr_uarts, uint, 0644);
>> +module_param(nr_uarts, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>> MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-"
>> __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>>
>> -module_param(skip_txen_test, uint, 0644);
>> +module_param(skip_txen_test, uint, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>> MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init
>> time");
>>
>> #ifdef CONFIG_SERIAL_8250_RSA
>> -module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
>> +module_param_array(probe_rsa, ulong, &probe_rsa_count, S_IRUSR | S_IRGRP |
>> S_IROTH);
>> MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
>> #endif
>> MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
>> --
>> 2.9.2
>>
>
>
>
> --
> With best regards,
> Matwey V. Kornilov.
> Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
> 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
kernel/time/ntp.c: possible unit inconsistency
Hello,
I think I found minor inconsistency in measurement units between ntpd
and linux kernel. Though, I am not sure completely.
I've failed to reach ntp mail list because lists.ntp.org is down for
me for several days.
My principal concern is about `maxerror' quantity.
kernel/time/ntp.c has time_maxerror variable which is can be get/put
from/to userspace using ntp_adjtime call.
The single point where the variable is altered by the linux kernel is
second_overflow() function in kernel/time/ntp.c:456.
Here the following happens:
/* Bump the maxerror field */
time_maxerror += MAXFREQ / NSEC_PER_USEC;
if (time_maxerror > NTP_PHASE_LIMIT) {
time_maxerror = NTP_PHASE_LIMIT;
time_status |= STA_UNSYNC;
}
The line assumes that time_maxerror is always measured in the units of usec.
At the same time, if we get ntp-4.2.8p8 sources and look at
ntpdc/ntpdc_ops.c:2955 function kerninfo() or
ntpd/ntp_control.c:2359 function ctl_putsys()
then we found that ntpd expects that ntp_adjtime returns maxerror
either in usec (if STA_NANO is not used) or in nsec (if STA_NANO is
used).
So, there can be the case when kernel measures maxerror in usec and
ntpd does in nsec.
--
With best regards,
Matwey V. Kornilov
http://blog.matwey.name
xmpp://0x2...@jabber.ru
Re: [PATCH] 8250: Hypervisors always export working 16550A UARTs.
2016-04-29 1:18 GMT+03:00 Richard W.M. Jones : > [This is an opinionated patch, mainly for discussion.] > > I'm trying to reduce the time taken in the kernel in initcalls, with > my aim being to reduce the current ~700ms spent in initcalls before > userspace, down to something like 100ms. All times on my Broadwell-U > laptop, under virtualization. The purpose of this is to be able to > launch VMs around containers with minimal overhead, like Intel Clear > Containers, but using standard distro kernels and qemu. > > Currently the kernel spends 25ms inspecting the UART that we passed to > it from qemu to find out whether it's an 8250/16550/16550A perhaps > with a non-working FIFO or other quirks. Well, it isn't -- it's a > working emulated 16550A, with a FIFO and no quirks, and if it isn't, > we should fix qemu. > > So the patch detects if we're running virtualized (perhaps it should > only check for qemu/KVM?) and if so, shortcuts the tests. Does anybody know, whether it is possible to pass through real hardware serial port to a guest? It seems to be as simple as to pass through an interrupt and memory IO ports. > > Rich. > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH v2] tty/serial/8250: fix RS485 half-duplex RX
2016-03-24 11:03 GMT+03:00 :
> From: Yegor Yefremov
>
> When in half-duplex mode RX will be disabled before TX, but not
> enabled after deactivating transmitter. This patch enables
> UART_IER_RLSI and UART_IER_RDI interrupts after TX is over.
>
> Cc: Matwey V. Kornilov
> Signed-off-by: Yegor Yefremov
> Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Acked-by: Matwey V. Kornilov
> ---
> Changes:
> v2: change subject and add 'Fixes' tag
>
> drivers/tty/serial/8250/8250_port.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index e213da0..00ad263 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1403,9 +1403,18 @@ static void __do_stop_tx_rs485(struct uart_8250_port
> *p)
> /*
> * Empty the RX FIFO, we are not interested in anything
> * received during the half-duplex transmission.
> +* Enable previously disabled RX interrupts.
> */
> - if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> serial8250_clear_fifos(p);
> +
> + serial8250_rpm_get(p);
> +
> + p->ier |= UART_IER_RLSI | UART_IER_RDI;
> + serial_port_out(&p->port, UART_IER, p->ier);
> +
> + serial8250_rpm_put(p);
> + }
> }
>
> static void serial8250_em485_handle_stop_tx(unsigned long arg)
> --
> 2.1.4
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH v3] tty: serial: 8250: add MOXA Smartio MUE boards support
2016-03-19 12:07 GMT+03:00 Mathieu OTHACEHE : > Hi, > > Sorry about late reply. > > No I haven't planned to do it soon. Your board is supported by mxser driver > (drivers/tty/mxser.c). > I think it would be nice to move everything from mxser.c to 8250_moxa.c. Last time when I tried to run my moxa PCI board with 3.1(?) and mxser it deadlocked my PC from time to time. 8250_moxa implementation is much more cleaner. > > The problem is mxser supports ISA and PCI boards and I don't have the > hardware to test it. > > Mathieu -- With best regards, Matwey V. Kornilov http://blog.matwey.name xmpp://0x2...@jabber.ru
Re: [PATCH v3] tty: serial: 8250: add MOXA Smartio MUE boards support
24.02.2016 22:10, Mathieu OTHACEHE пишет:
Add support for :
- CP-102E: 2 ports RS232 PCIE card
- CP-102EL: 2 ports RS232 PCIE card
- CP-132EL: 2 ports RS422/485 PCIE card
- CP-114EL: 4 ports RS232/422/485 PCIE card
- CP-104EL-A: 4 ports RS232 PCIE card
- CP-168EL-A: 8 ports RS232 PCIE card
- CP-118EL-A: 8 ports RS232/422/485 PCIE card
- CP-118E-A: 8 ports RS422/485 PCIE card
- CP-138E-A: 8 ports RS422/485 PCIE card
- CP-134EL-A: 4 ports RS422/485 PCIE card
- CP-116E-A (A): 8 ports RS232/422/485 PCIE card
- CP-116E-A (B): 8 ports RS232/422/485 PCIE card
This patch is based on information extracted from
vendor mxupcie driver available on MOXA website.
I was able to test it on a CP-168EL-A on PC.
Hi,
Are you currently planning to add RS485 support to this module?
I mean TIOCGRS485/TIOCSRS485.
I have old MOXA CP-132U PCI-based board in may labs.
What do you think, should 8250_moxa also support PCI-based boards?
Signed-off-by: Mathieu OTHACEHE
---
Hi,
Here is v3 of the patch, it fixes problems pointed out by
last Andy review.
Thanks,
Mathieu
Changelog:
V3:
* Add supported boards to 8250_pci blacklist
* Use MOXA_DEVICE macro to simplify pci_device_id declaration
V2:
* Move everything to 8250_moxa.c
* Use PCI_DEVICE macro
* Group driver_data by port number as this is the only singularity between
boards.
drivers/tty/serial/8250/8250_moxa.c | 157
drivers/tty/serial/8250/8250_pci.c | 14
drivers/tty/serial/8250/Kconfig | 10 +++
drivers/tty/serial/8250/Makefile| 1 +
4 files changed, 182 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_moxa.c
diff --git a/drivers/tty/serial/8250/8250_moxa.c
b/drivers/tty/serial/8250/8250_moxa.c
new file mode 100644
index 000..26eb539
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_moxa.c
@@ -0,0 +1,157 @@
+/*
+ * 8250_moxa.c - MOXA Smartio/Industio MUE multiport serial driver.
+ *
+ * Author: Mathieu OTHACEHE
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+
+#include "8250.h"
+
+#definePCI_DEVICE_ID_MOXA_CP102E 0x1024
+#definePCI_DEVICE_ID_MOXA_CP102EL 0x1025
+#definePCI_DEVICE_ID_MOXA_CP104EL_A0x1045
+#definePCI_DEVICE_ID_MOXA_CP114EL 0x1144
+#definePCI_DEVICE_ID_MOXA_CP116E_A_A 0x1160
+#definePCI_DEVICE_ID_MOXA_CP116E_A_B 0x1161
+#definePCI_DEVICE_ID_MOXA_CP118EL_A0x1182
+#definePCI_DEVICE_ID_MOXA_CP118E_A_I 0x1183
+#definePCI_DEVICE_ID_MOXA_CP132EL 0x1322
+#definePCI_DEVICE_ID_MOXA_CP134EL_A0x1342
+#definePCI_DEVICE_ID_MOXA_CP138E_A 0x1381
+#definePCI_DEVICE_ID_MOXA_CP168EL_A0x1683
+
+#define MOXA_BASE_BAUD 921600
+#define MOXA_UART_OFFSET 0x200
+#define MOXA_BASE_BAR 1
+
+struct moxa8250_board {
+ unsigned int num_ports;
+ int line[0];
+};
+
+enum {
+ moxa8250_2p = 0,
+ moxa8250_4p,
+ moxa8250_8p
+};
+
+static struct moxa8250_board moxa8250_boards[] = {
+ [moxa8250_2p] = { .num_ports = 2},
+ [moxa8250_4p] = { .num_ports = 4},
+ [moxa8250_8p] = { .num_ports = 8},
+};
+
+static int moxa8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct uart_8250_port uart;
+ struct moxa8250_board *brd;
+ void __iomem *ioaddr;
+ resource_size_t baseaddr;
+ unsigned int i, nr_ports;
+ unsigned int offset;
+ int ret;
+
+ brd = &moxa8250_boards[id->driver_data];
+ nr_ports = brd->num_ports;
+
+ ret = pcim_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ brd = devm_kzalloc(&pdev->dev, sizeof(struct moxa8250_board) +
+ sizeof(unsigned int) * nr_ports, GFP_KERNEL);
+ if (!brd)
+ return -ENOMEM;
+
+ memset(&uart, 0, sizeof(struct uart_8250_port));
+
+ uart.port.dev = &pdev->dev;
+ uart.port.irq = pdev->irq;
+ uart.port.uartclk = MOXA_BASE_BAUD * 16;
+ uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
+
+ baseaddr = pci_resource_start(pdev, MOXA_BASE_BAR);
+ ioaddr = pcim_iomap(pdev, MOXA_BASE_BAR, 0);
+ if (!ioaddr)
+ return -ENOMEM;
+
+ for (i = 0; i < nr_ports; i++) {
+
+ /*
+* MOXA Smartio MUE boards with 4 ports have
+* a different offset for port #3
+*/
+ if (nr_ports == 4 && i == 3)
+ offset = 7 * MOXA_UART_OFFSET;
+ else
+ offset = i * MOXA_UART_OFFSET;
+
+ uart.port.iotype = UPIO_MEM;
+ uart.port.iobase = 0;
+ uart.port.mapbase = baseaddr + offset;
+ uart.port.membase = ioaddr + offset;
+ uart.p
Re: 4.5.0-rc6: kernel BUG at ../mm/memory.c:1879
2016-03-07 16:03 GMT+03:00 Russell King - ARM Linux : > On Mon, Mar 07, 2016 at 01:51:40PM +0100, Vlastimil Babka wrote: >> [+CC ARM, module maintainers/lists] >> >> On 03/07/2016 12:14 PM, Matwey V. Kornilov wrote: >> > >> >Hello, >> > >> >I see the following when try to boot 4.5.0-rc6 on ARM TI AM33xx based board. >> > >> > [ 13.907631] [ cut here ] >> > [ 13.912323] kernel BUG at ../mm/memory.c:1879! >> >> That's: >> BUG_ON(addr >= end); >> >> where: >> end = addr + size; >> >> All these variables are unsigned long, so they overflown? >> >> I don't know ARM much, and there's no code for decodecode, but if I get the >> calling convention correctly, and the registers didn't change, both addr is >> r1 and size is r2, i.e. both bf006000. Weird. > > A fix has been recently merged for this. Look out for > "ARM: 8544/1: set_memory_xx fixes" > Many thanks, I'll try again with -rc7. > -- > RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. -- With best regards, Matwey V. Kornilov http://blog.matwey.name xmpp://0x2...@jabber.ru
[PATCH v2 RESEND] tty: serial: Use GFP_ATOMIC instead of GFP_KERNEL in serial8250_em485_init()
serial8250_em485_init() is supposed to be protected with
p->port.lock spinlock.
This may lead to issues when kmalloc sleeps, so it is better to use
GFP_ATOMIC in this spinlocked context.
Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Reported-by: Ильяс Гасанов
Signed-off-by: Matwey V. Kornilov
---
Changes from v1:
- Properly filled Reported-by: tag
drivers/tty/serial/8250/8250_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c908b77..4d6deef 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -586,7 +586,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
if (p->em485 != NULL)
return 0;
- p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+ p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_ATOMIC);
if (p->em485 == NULL)
return -ENOMEM;
--
2.7.0
Re: [PATCH v1] tty: serial: Use GFP_ATOMIC instead of GFP_KERNEL in serial8250_em485_init()
2016-02-19 2:47 GMT+03:00 Greg Kroah-Hartman :
> On Thu, Feb 18, 2016 at 11:29:07PM +0300, Matwey V. Kornilov wrote:
>> 2016-02-18 23:21 GMT+03:00 Andy Shevchenko :
>> > On Thu, Feb 18, 2016 at 8:47 PM, Matwey V. Kornilov
>> > wrote:
>> >> serial8250_em485_init() is supposed to be protected with
>> >> p->port.lock spinlock.
>> >> This may lead to issues when kmalloc sleeps, so it is better to use
>> >> GFP_ATOMIC in this spinlocked context.
>> >>
>> >> Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
>> >> Reported-by:
>> >
>> > Hmm... Can't we put Cyrillic characters to tags as name of a person?
>>
>> I am afraid non-ASCII will break something eventually. I think it
>> would be better to ask Ilias to put his name in proper latin
>> transcription here.
>
> Why, we accept non-ASCII names all the time, why would we not do so?
>
> Please fix up and resend.
Sure, I've just sent v2.
I am used that non-ASCIIs break things, as they did it 20 years ago,
as they do it in 2016. GhostView stole all my Cyrillic characters from
PDF when I tried to embed fonts two days ago .
>
> thansk,
>
> greg k-h
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
[PATCH v2] tty: serial: Use GFP_ATOMIC instead of GFP_KERNEL in serial8250_em485_init()
serial8250_em485_init() is supposed to be protected with
p->port.lock spinlock.
This may lead to issues when kmalloc sleeps, so it is better to use
GFP_ATOMIC in this spinlocked context.
Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Reported-by: Ильяс Гасанов
Signed-off-by: Matwey V. Kornilov
---
Changes from v1:
- Properly filled Reported-by: tag
drivers/tty/serial/8250/8250_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c908b77..4d6deef 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -586,7 +586,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
if (p->em485 != NULL)
return 0;
- p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+ p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_ATOMIC);
if (p->em485 == NULL)
return -ENOMEM;
--
2.7.0
Re: [PATCH v1] tty: serial: Use GFP_ATOMIC instead of GFP_KERNEL in serial8250_em485_init()
2016-02-18 23:21 GMT+03:00 Andy Shevchenko :
> On Thu, Feb 18, 2016 at 8:47 PM, Matwey V. Kornilov wrote:
>> serial8250_em485_init() is supposed to be protected with
>> p->port.lock spinlock.
>> This may lead to issues when kmalloc sleeps, so it is better to use
>> GFP_ATOMIC in this spinlocked context.
>>
>> Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
>> Reported-by:
>
> Hmm... Can't we put Cyrillic characters to tags as name of a person?
I am afraid non-ASCII will break something eventually. I think it
would be better to ask Ilias to put his name in proper latin
transcription here.
> --
> With Best Regards,
> Andy Shevchenko
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
[PATCH v1] tty: serial: Use GFP_ATOMIC instead of GFP_KERNEL in serial8250_em485_init()
serial8250_em485_init() is supposed to be protected with
p->port.lock spinlock.
This may lead to issues when kmalloc sleeps, so it is better to use
GFP_ATOMIC in this spinlocked context.
Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Reported-by:
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c908b77..4d6deef 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -586,7 +586,7 @@ int serial8250_em485_init(struct uart_8250_port *p)
if (p->em485 != NULL)
return 0;
- p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+ p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_ATOMIC);
if (p->em485 == NULL)
return -ENOMEM;
--
2.7.0
[PATCH v1] tty: serial: 8250: Cleanup p->em485 in serial8250_unregister_port
Formally, currently there is no memory leak, but if
serial8250_ports[line] is reused with other 8250 driver, then em485
will be already activated and it will cause issues.
Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Signed-off-by: Matwey V. Kornilov
---
Hi Peter,
There is an issue with my previous patch. Under specific circumstances,
the emulation can be enabled for drivers which don't want (and support) it.
Unfortunately, I can't test this patch because kgdb over 8250_omap console
is the single one option for me on BBB. This patch testing involves
8250_omap module unloading and then inspecting serial8250_ports.
You say that you are preparing some unit-tests. Could you include a test
for this specific issue? And then probably this patch has to be applied.
drivers/tty/serial/8250/8250_core.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index c9720a9..a242881 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1068,6 +1068,15 @@ void serial8250_unregister_port(int line)
struct uart_8250_port *uart = &serial8250_ports[line];
mutex_lock(&serial_mutex);
+
+ if (uart->em485) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&uart->port.lock, flags);
+ serial8250_em485_destroy(uart);
+ spin_unlock_irqrestore(&uart->port.lock, flags);
+ }
+
uart_remove_one_port(&serial8250_reg, &uart->port);
if (serial8250_isa_devs) {
uart->port.flags &= ~UPF_BOOT_AUTOCONF;
--
2.7.0
Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control
2016-02-11 23:19 GMT+03:00 Ильяс Гасанов :
> 2016-02-11 22:08 GMT+03:00 Matwey V. Kornilov :
>> Thanks for pointing out. serial8250_unregister_port should set
>> serial8250_ports[line].em485 to NULL in order to prevent unneeded
>> activation when this struct is reused.
>
> Then the allocated/initialized resources should be freed/released as well.
Sure, I've already sketched the patch, but to test it, I need some
time to build 8250_omap as a module and setup networking console and
kgdb over network.
>
>> You would be able to use serial8250_em485_register in omap8250_probe
>> on &up before serial8250_register_8250_port(&up) if
>> serial8250_register_8250_port could replicate em485 member state.
>> But there are alternatives in implementation.
>
> I'm considering adding the relevant code to the omap8250_startup and
> omap8250_shutdown callback functions. However the serial driver
> documentation claims that these don't have port->lock taken when
> invoked, only port_sem.
>
>> serial8250_register_8250_port may either copy pointer up->em485 to
>> uart->em485 or perform deep copy.
>
> Actually, "up" here in omap8250_probe is not a pointer but a struct
> variable on stack, so copying the pointer to it is out of question.
>
up->em485 is always pointer, however you are right, that
serial8250_em485_init stores pointer to up when the timers are inited.
More complex issue here that serial8250_em485_init need to set RTS to
proper state and probably can't do that before
serial8250_register_8250_port. So omap8250_probe should directly or
indirectly use serial8250_get_port(priv->line) after
serial8250_register_8250_port. As for me, possible solution could be
to add a wrapper:
int serial8250_em485_init_by_line(int line) {
struct uart_8250_port *uart = &serial8250_ports[line];
int ret;
unsigned long flags;
spin_lock_irqsave(&uart->port.lock, flags);
ret = serial8250_em485_init(uart).
spin_unlock_irqrestore(&uart->port.lock, flags);
return ret;
}
>
> Regards,
> Ilyas G.
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH v1] tty: serial: 8250: Fix possible race in serial8250_em485_destroy()
I am sorry, please ignore it. There is no issue actually. The timer
handlers and rs485_config callbacks are protected by the same
spinlock, so they are never run in parallel.
2016-02-11 22:32 GMT+03:00 Matwey V. Kornilov :
> Fix possbile race in serial8250_em485_destroy() when timer handlers can
> dereference p->em485 which is alread destroyed but not yet NULLed.
>
> Signed-off-by: Matwey V. Kornilov
> ---
> I've found that Greg applied initial patchset, so this erratum goes as
> separate patch.
>
> drivers/tty/serial/8250/8250_port.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index c908b77..d962de2 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -617,14 +617,16 @@ EXPORT_SYMBOL_GPL(serial8250_em485_init);
> */
> void serial8250_em485_destroy(struct uart_8250_port *p)
> {
> - if (p->em485 == NULL)
> + struct uart_8250_em485 *em485 = p->em485;
> +
> + if (!em485)
> return;
>
> - del_timer(&p->em485->start_tx_timer);
> - del_timer(&p->em485->stop_tx_timer);
> + del_timer(&em485->start_tx_timer);
> + del_timer(&em485->stop_tx_timer);
>
> - kfree(p->em485);
> p->em485 = NULL;
> + kfree(em485);
> }
> EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
>
> --
> 2.7.0
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
[PATCH v1] tty: serial: 8250: Fix possible race in serial8250_em485_destroy()
Fix possbile race in serial8250_em485_destroy() when timer handlers can
dereference p->em485 which is alread destroyed but not yet NULLed.
Signed-off-by: Matwey V. Kornilov
---
I've found that Greg applied initial patchset, so this erratum goes as separate
patch.
drivers/tty/serial/8250/8250_port.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c908b77..d962de2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -617,14 +617,16 @@ EXPORT_SYMBOL_GPL(serial8250_em485_init);
*/
void serial8250_em485_destroy(struct uart_8250_port *p)
{
- if (p->em485 == NULL)
+ struct uart_8250_em485 *em485 = p->em485;
+
+ if (!em485)
return;
- del_timer(&p->em485->start_tx_timer);
- del_timer(&p->em485->stop_tx_timer);
+ del_timer(&em485->start_tx_timer);
+ del_timer(&em485->stop_tx_timer);
- kfree(p->em485);
p->em485 = NULL;
+ kfree(em485);
}
EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
--
2.7.0
Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control
2016-02-11 19:40 GMT+03:00 Ильяс Гасанов :
> Hello,
>
> 2016-02-01 21:09 GMT+03:00 "Matwey V. Kornilov" :
>> +static int omap_8250_rs485_config(struct uart_port *port,
>> + struct serial_rs485 *rs485)
>> +{
>> + struct uart_8250_port *up = up_to_u8250p(port);
>> +
>> + /* Clamp the delays to [0, 100ms] */
>> + rs485->delay_rts_before_send = min(rs485->delay_rts_before_send,
>> 100U);
>> + rs485->delay_rts_after_send = min(rs485->delay_rts_after_send,
>> 100U);
>> +
>> + port->rs485 = *rs485;
>> +
>> + /*
>> +* Both serial8250_em485_init and serial8250_em485_destroy
>> +* are idempotent
>> +*/
>> + if (rs485->flags & SER_RS485_ENABLED) {
>> + int ret = serial8250_em485_init(up);
>> +
>> + if (ret) {
>> + rs485->flags &= ~SER_RS485_ENABLED;
>> + port->rs485.flags &= ~SER_RS485_ENABLED;
>> + }
>> + return ret;
>> + }
>> +
>> + serial8250_em485_destroy(up);
>> +
>> + return 0;
>> +}
>
> I think that an invocation of serial8250_em485_destroy() should be
> performed inside omap8250_remove() as well. However there is a catch -
> serial8250_get_port() is only reserved for suspend-resume power
> management operations, and I am aware of no other means how to supply
> an uart_8250_port to serial8250_em485_destroy() at that point.
Thanks for pointing out. serial8250_unregister_port should set
serial8250_ports[line].em485 to NULL in order to prevent unneeded
activation when this struct is reused.
>
> Similarly, I got no idea on how to implement handling
> "linux,rs485-enabled-at-boot-time" property without using
> serial8250_get_port() directly (well, honestly I could try invoking
> the corresponding ioctl somehow, but that's gonna be plain ugly at
> least).
You would be able to use serial8250_em485_register in omap8250_probe
on &up before serial8250_register_8250_port(&up) if
serial8250_register_8250_port could replicate em485 member state.
But there are alternatives in implementation.
serial8250_register_8250_port may either copy pointer up->em485 to
uart->em485 or perform deep copy.
>
> Regards,
> Ilyas G.
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: [PATCH v8 0/3] tty: Introduce software RS485 direction control support
2016-02-10 19:05 GMT+03:00 Peter Hurley : > Hi Matwey, > > On 02/01/2016 10:09 AM, Matwey V. Kornilov wrote: >> Changes from v7: >> - rework comments to follow guidelines >> - minor style changes >> Changes from v6: >> - minor style changes >> - timers are not IRQSAFE now >> Changes from v5: >> - rs485_emul variable has been renamed to em485 to follow function names >> convention >> Changes from v4: >> - Add commit message to 1/3 >> Changes from v3: >> - Completely redesigned. >> Changes from v2: >> - Introduced SER_RS485_SOFTWARE to show that software implementation is >> being used >> - serial8250_rs485_config is located as required >> - Timers are used to implement delay_before and delay_after timeouts >> >> Signed-off-by: Matwey V. Kornilov > > The holdup here is that I'd like to unit test this, which is 3rd on my todo > list. > Should be done before the end of the week which will be soon enough > for this series to make linux-next. Hi Peter, Is something required from me? > > Thanks for your work on this. > > Regards, > Peter Hurley > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
[PATCH v8 1/3] tty: Move serial8250_stop_rx() in front of serial8250_start_tx()
Software RS485 emultaion is to be added in the following commit.
serial8250_start_tx() will need to refer serial8250_stop_rx().
Move serial8250_stop_rx() in front of serial8250_start_tx() in order
to avoid function forward declaration.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_port.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 8d262bc..b82fcae 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1304,6 +1304,19 @@ static void autoconfig_irq(struct uart_8250_port *up)
port->irq = (irq > 0) ? irq : 0;
}
+static void serial8250_stop_rx(struct uart_port *port)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ serial8250_rpm_get(up);
+
+ up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+ up->port.read_status_mask &= ~UART_LSR_DR;
+ serial_port_out(port, UART_IER, up->ier);
+
+ serial8250_rpm_put(up);
+}
+
static inline void __stop_tx(struct uart_8250_port *p)
{
if (p->ier & UART_IER_THRI) {
@@ -1371,19 +1384,6 @@ static void serial8250_unthrottle(struct uart_port *port)
port->unthrottle(port);
}
-static void serial8250_stop_rx(struct uart_port *port)
-{
- struct uart_8250_port *up = up_to_u8250p(port);
-
- serial8250_rpm_get(up);
-
- up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- up->port.read_status_mask &= ~UART_LSR_DR;
- serial_port_out(port, UART_IER, up->ier);
-
- serial8250_rpm_put(up);
-}
-
static void serial8250_disable_ms(struct uart_port *port)
{
struct uart_8250_port *up =
--
2.7.0
[PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control
Use software emulated RS485 direction control to provide RS485 API
existed in omap_serial driver. Note that 8250_omap issues interrupt
on shift register empty which is single prerequesite for using software
emulated RS485.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_omap.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index a2c0734..d710985 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -697,6 +697,36 @@ static void omap_8250_throttle(struct uart_port *port)
pm_runtime_put_autosuspend(port->dev);
}
+static int omap_8250_rs485_config(struct uart_port *port,
+ struct serial_rs485 *rs485)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ /* Clamp the delays to [0, 100ms] */
+ rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+ rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
+
+ port->rs485 = *rs485;
+
+ /*
+* Both serial8250_em485_init and serial8250_em485_destroy
+* are idempotent
+*/
+ if (rs485->flags & SER_RS485_ENABLED) {
+ int ret = serial8250_em485_init(up);
+
+ if (ret) {
+ rs485->flags &= ~SER_RS485_ENABLED;
+ port->rs485.flags &= ~SER_RS485_ENABLED;
+ }
+ return ret;
+ }
+
+ serial8250_em485_destroy(up);
+
+ return 0;
+}
+
static void omap_8250_unthrottle(struct uart_port *port)
{
unsigned long flags;
@@ -1146,6 +1176,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.shutdown = omap_8250_shutdown;
up.port.throttle = omap_8250_throttle;
up.port.unthrottle = omap_8250_unthrottle;
+ up.port.rs485_config = omap_8250_rs485_config;
if (pdev->dev.of_node) {
const struct of_device_id *id;
--
2.7.0
[PATCH v8 2/3] tty: Add software emulated RS485 support for 8250
Implementation of software emulation of RS485 direction handling is based
on omap_serial driver.
Before and after transmission RTS is set to the appropriate value.
Note that before calling serial8250_em485_init() the caller has to
ensure that UART will interrupt when shift register empty. Otherwise,
emultaion cannot be used.
Both serial8250_em485_init() and serial8250_em485_destroy() are
idempotent functions.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250.h | 2 +
drivers/tty/serial/8250/8250_port.c | 223 +++-
include/linux/serial_8250.h | 8 ++
3 files changed, 229 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..ce587c0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,8 @@ static inline void serial_dl_write(struct uart_8250_port
*up, int value)
struct uart_8250_port *serial8250_get_port(int line);
void serial8250_rpm_get(struct uart_8250_port *p);
void serial8250_rpm_put(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p);
+void serial8250_em485_destroy(struct uart_8250_port *p);
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index b82fcae..c908b77 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -522,6 +523,20 @@ static void serial8250_clear_fifos(struct uart_8250_port
*p)
}
}
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg);
+static void serial8250_em485_handle_stop_tx(unsigned long arg);
+
void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
{
serial8250_clear_fifos(p);
@@ -546,6 +561,73 @@ void serial8250_rpm_put(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_rpm_put);
+/**
+ * serial8250_em485_init() - put uart_8250_port into rs485 emulating
+ * @p: uart_8250_port port instance
+ *
+ * The function is used to start rs485 software emulating on the
+ * &struct uart_8250_port* @p. Namely, RTS is switched before/after
+ * transmission. The function is idempotent, so it is safe to call it
+ * multiple times.
+ *
+ * The caller MUST enable interrupt on empty shift register before
+ * calling serial8250_em485_init(). This interrupt is not a part of
+ * 8250 standard, but implementation defined.
+ *
+ * The function is supposed to be called from .rs485_config callback
+ * or from any other callback protected with p->port.lock spinlock.
+ *
+ * See also serial8250_em485_destroy()
+ *
+ * Return 0 - success, -errno - otherwise
+ */
+int serial8250_em485_init(struct uart_8250_port *p)
+{
+ if (p->em485 != NULL)
+ return 0;
+
+ p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+ if (p->em485 == NULL)
+ return -ENOMEM;
+
+ setup_timer(&p->em485->stop_tx_timer,
+ serial8250_em485_handle_stop_tx, (unsigned long)p);
+ setup_timer(&p->em485->start_tx_timer,
+ serial8250_em485_handle_start_tx, (unsigned long)p);
+ p->em485->active_timer = NULL;
+
+ serial8250_em485_rts_after_send(p);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+
+/**
+ * serial8250_em485_destroy() - put uart_8250_port into normal state
+ * @p: uart_8250_port port instance
+ *
+ * The function is used to stop rs485 software emulating on the
+ * &struct uart_8250_port* @p. The function is idempotent, so it is safe to
+ * call it multiple times.
+ *
+ * The function is supposed to be called from .rs485_config callback
+ * or from any other callback protected with p->port.lock spinlock.
+ *
+ * See also serial8250_em485_init()
+ */
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->em485 == NULL)
+ return;
+
+ del_timer(&p->em485->start_tx_timer);
+ del_timer(&p->em485->stop_tx_timer);
+
+ kfree(p->em485);
+ p->em485 = NULL;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
+
/*
* These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
* once and disable_runtime_pm_tx() will still disable RPM because the fifo is
@@ -1317,7 +1399,56 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_put(
[PATCH v8 0/3] tty: Introduce software RS485 direction control support
Changes from v7: - rework comments to follow guidelines - minor style changes Changes from v6: - minor style changes - timers are not IRQSAFE now Changes from v5: - rs485_emul variable has been renamed to em485 to follow function names convention Changes from v4: - Add commit message to 1/3 Changes from v3: - Completely redesigned. Changes from v2: - Introduced SER_RS485_SOFTWARE to show that software implementation is being used - serial8250_rs485_config is located as required - Timers are used to implement delay_before and delay_after timeouts Signed-off-by: Matwey V. Kornilov
Re: [PATCH v7 3/3] tty: 8250_omap: Use software emulated RS485 direction control
2016-01-23 18:20 GMT+03:00 Andy Shevchenko :
> On Tue, Jan 19, 2016 at 10:33 PM, Matwey V. Kornilov
> wrote:
>> Use software emulated RS485 direction control to provide RS485 API existed in
>> omap_serial driver. Note that 8250_omap issues interrupt on shift register
>> empty which is single prerequesite for using software emulated RS485.
>
>
>> +static int omap_8250_rs485_config(struct uart_port *port, struct
>> serial_rs485 *rs485)
>> +{
>> + struct uart_8250_port *up = up_to_u8250p(port);
>> +
>> + /* Clamp the delays to [0, 100ms] */
>
> It's not exactly clamping but rather cutting extra as I can see below.
>
I took this code and the comment from
http://marc.info/?l=linux-serial&m=145264055108439&w=2
>> + rs485->delay_rts_before_send = min(rs485->delay_rts_before_send,
>> 100U);
>> + rs485->delay_rts_after_send = min(rs485->delay_rts_after_send,
>> 100U);
>
>> + /* Both serial8250_em485_* functions are idempotent */
>
> Better to decode what functions do you mean here.
>
>> + if (rs485->flags & SER_RS485_ENABLED) {
>> + int ret = serial8250_em485_init(up);
>> + if (ret) {
>> + rs485->flags &= ~SER_RS485_ENABLED;
>> + port->rs485.flags &= ~SER_RS485_ENABLED;
>> + }
>
>> + return ret;
>> + } else
>
> You have to do } else { in multi-line branches, but luckily there is
> redundant else keyword.
>
> And checkpatch.pl doesn't tell you anything?
>
>> + serial8250_em485_destroy(up);
>> +
>> + return 0;
>> +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Re: GPIO-driven RTS on TI hardware with 8250_omap driver
Andy, The half of what is described here are implemented in my patches. But I cannot understand the other half. Each of six AM335x UARTs has RTS/CTS pins which are controlled by pinmux in device tree, no magic required here. 2015-12-27 15:47 GMT+03:00 Andy Shevchenko : > +Peter, Russell, and Matwey. > > I suggest you to ask people I added to the Cc list. > > On Sat, Dec 26, 2015 at 6:17 PM, Ильяс Гасанов wrote: >> Hello. >> >> We are upgrading to the 4.1.x kernel for our smart metering appliance >> project, which is based on TI's Sitara hardware (AM335x SoC), and I >> decided to switch from omap-serial legacy driver to the newer >> 8250-based one. It marginally increases throughput efficiency, CPU >> cycle wise, among other goodies, but I'm looking to implement a rather >> important feature that is present in the legacy driver, but the newer >> one is lacking. >> >> Namely, our project makes use of RS232<->RS485 converters, which in >> turn need to consume RTS signals to switch between Rx and Tx modes at >> the RS485 side, due to the bus variant we use being half-duplex. >> However, the already manufactured hardware is already designed to make >> the use of certain pins to take the RTS signal from, which can only be >> configured as GPIO for that purpose (in other words, no "native" UART >> RTS) - and basically redesigning the h/w configuration now is >> definitely out of question. The omap-serial driver already provides >> FDT options for that, named "rts-gpio", "rs485-rts-active-high" etc. >> >> As far as I could ascertain, the 8250_omap driver (as well as the 8250 >> framework itself) at the moment lacks the means to make use of GPIO >> pins for that purpose. While trying to implement it myself, I noticed >> that the legacy driver has it made in a comparably straightforward >> approach, via dispatching the code to switch the pin in its .start_tx >> and .stop_tx handlers, and some timing adjustments. Unfortunately, the >> situation with 8250-based drivers is different - the aforementioned >> handlers are provided by the 8250_core module and are common for all >> drivers within the framework. >> >> At first, I thought that implementing such feature for the 8250 >> framework itself sounds like a good idea, but after reading this >> particular post: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/271377.html >> I decided to comply with the point of view specified there. However, >> I'm not that familiar with the 8250 framework internals (or serial >> internals at all, for that matter), and my time is quite short, so I >> would appreciate much any useful directions on how to do it >> hardware-specific style, which functions/structs/handlers to use, etc. >> Of particular interest is the following part: >> >>> I don't care whether the drive does it via serial_out magic or a more >>> explicit hook but it doesn't belong here in core code. >> >> Any ideas/clarifications on what might be meant on that part? >> >> Regards, >> Ilyas G. >> -- >> 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/ > > > > -- > With Best Regards, > Andy Shevchenko > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 -- 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 v6 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx
Software RS485 emultaion is to be added in the following commit.
serial8250_start_tx will need to refer serial8250_stop_rx.
Move serial8250_stop_rx in front of serial8250_start_tx in order
to avoid function forward declaration.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_port.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 0bbf340..8ad0b2d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1280,6 +1280,19 @@ static void autoconfig_irq(struct uart_8250_port *up)
port->irq = (irq > 0) ? irq : 0;
}
+static void serial8250_stop_rx(struct uart_port *port)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ serial8250_rpm_get(up);
+
+ up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+ up->port.read_status_mask &= ~UART_LSR_DR;
+ serial_port_out(port, UART_IER, up->ier);
+
+ serial8250_rpm_put(up);
+}
+
static inline void __stop_tx(struct uart_8250_port *p)
{
if (p->ier & UART_IER_THRI) {
@@ -1347,19 +1360,6 @@ static void serial8250_unthrottle(struct uart_port *port)
port->unthrottle(port);
}
-static void serial8250_stop_rx(struct uart_port *port)
-{
- struct uart_8250_port *up = up_to_u8250p(port);
-
- serial8250_rpm_get(up);
-
- up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- up->port.read_status_mask &= ~UART_LSR_DR;
- serial_port_out(port, UART_IER, up->ier);
-
- serial8250_rpm_put(up);
-}
-
static void serial8250_disable_ms(struct uart_port *port)
{
struct uart_8250_port *up =
--
2.6.3
--
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 v6 3/3] tty: 8250_omap: Use software emulated RS485 direction control
Use software emulated RS485 direction control to provide RS485 API existed in
omap_serial driver. Note that 8250_omap issues interrupt on shift register
empty which is single prerequesite for using software emulated RS485.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_omap.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 826c5c4..323c0a4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -698,6 +698,23 @@ static void omap_8250_throttle(struct uart_port *port)
pm_runtime_put_autosuspend(port->dev);
}
+static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485
*rs485)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ if (rs485->flags & SER_RS485_ENABLED && !serial8250_em485_enabled(up)) {
+ port->rs485 = *rs485;
+ return serial8250_em485_init(up);
+ }
+
+ if (serial8250_em485_enabled(up) && !(rs485->flags & SER_RS485_ENABLED))
+ serial8250_em485_destroy(up);
+
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
static void omap_8250_unthrottle(struct uart_port *port)
{
unsigned long flags;
@@ -1144,6 +1161,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.shutdown = omap_8250_shutdown;
up.port.throttle = omap_8250_throttle;
up.port.unthrottle = omap_8250_unthrottle;
+ up.port.rs485_config = omap_8250_rs485_config;
if (pdev->dev.of_node) {
const struct of_device_id *id;
--
2.6.3
--
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 v6 2/3] tty: Add software emulated RS485 support for 8250
Implementation of software emulation of RS485 direction handling is based
on omap_serial driver.
Before and after transmission RTS is set to the appropriate value.
Note that before calling serial8250_em485_init the caller has to
ensure that UART will interrupt when shift register empty. Otherwise,
emultaion cannot be used.
Both serial8250_em485_init and serial8250_em485_destroy are
idempotent functions.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250.h | 6 ++
drivers/tty/serial/8250/8250_port.c | 161 +++-
include/linux/serial_8250.h | 7 ++
3 files changed, 170 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..0189cb3 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,12 @@ static inline void serial_dl_write(struct uart_8250_port
*up, int value)
struct uart_8250_port *serial8250_get_port(int line);
void serial8250_rpm_get(struct uart_8250_port *p);
void serial8250_rpm_put(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p);
+void serial8250_em485_destroy(struct uart_8250_port *p);
+static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
+{
+ return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
+}
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 8ad0b2d..d67a848 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port
*p)
}
}
+static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg);
+static void serial8250_em485_handle_stop_tx(unsigned long arg);
+
void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
{
serial8250_clear_fifos(p);
@@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_rpm_put);
+int serial8250_em485_init(struct uart_8250_port *p)
+{
+ if (p->em485 != NULL)
+ return 0;
+
+ p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL);
+ if (p->em485 == NULL)
+ return -ENOMEM;
+
+ init_timer(&p->em485->stop_tx_timer);
+ p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
+ p->em485->stop_tx_timer.data = (unsigned long)p;
+ p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
+ init_timer(&p->em485->start_tx_timer);
+ p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx;
+ p->em485->start_tx_timer.data = (unsigned long)p;
+ p->em485->start_tx_timer.flags |= TIMER_IRQSAFE;
+
+ serial8250_em485_rts_after_send(p);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->em485 == NULL)
+ return;
+
+ del_timer_sync(&p->em485->start_tx_timer);
+ del_timer_sync(&p->em485->stop_tx_timer);
+
+ kfree(p->em485);
+ p->em485 = NULL;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
+
/*
* These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
* once and disable_runtime_pm_tx() will still disable RPM because the fifo is
@@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_put(up);
}
-static inline void __stop_tx(struct uart_8250_port *p)
+static __u32 __start_tx_rs485(struct uart_8250_port *p)
+{
+ if (!serial8250_em485_enabled(p))
+ return 0;
+
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&p->port);
+
+ del_timer_sync(&p->em485->stop_tx_timer);
+
+ if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p,
UART_MCR) & UART_MCR_RTS)) {
+ serial8250_em485_rts_on_send(p);
+ return p->port.rs485.delay_rts_
[PATCH v6 0/3] tty: Introduce software RS485 direction control support
Changes from v5: - rs485_emul variable has been renamed to em485 to follow function names convention Changes from v4: - Add commit message to 1/3 Changes from v3: - Completely redesigned. Changes from v2: - Introduced SER_RS485_SOFTWARE to show that software implementation is being used - serial8250_rs485_config is located as required - Timers are used to implement delay_before and delay_after timeouts Signed-off-by: Matwey V. Kornilov -- 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 v5 2/3] tty: Add software emulated RS485 support for 8250
2015-12-20 19:41 GMT+03:00 Andy Shevchenko :
> On Sat, Dec 12, 2015 at 1:23 PM, Matwey V. Kornilov wrote:
>> Implementation of software emulation of RS485 direction handling is based
>> on omap_serial driver.
>> Before and after transmission RTS is set to the appropriate value.
>>
>> Note that before calling serial8250_em485_init the caller has to
>> ensure that UART will interrupt when shift register empty. Otherwise,
>> emultaion cannot be used.
>>
>> Both serial8250_em485_init and serial8250_em485_destroy are
>> idempotent functions.
>
> Just nitpick suggesting to rename both struct and field:
>
> struct uart_8250_rs485em *rs485em;
Why not
struct uart_8250_em485 *em485;
to be consistent with function name suffix?
>
> (might be 'sw' suffix as well, which I like more — rs485sw)
>
> and corresponding local variables if any.
>
>>
>> Signed-off-by: Matwey V. Kornilov
>> ---
>> drivers/tty/serial/8250/8250.h | 6 ++
>> drivers/tty/serial/8250/8250_port.c | 161
>> +++-
>> include/linux/serial_8250.h | 7 ++
>> 3 files changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index d54dcd8..a3816c6 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -117,6 +117,12 @@ static inline void serial_dl_write(struct
>> uart_8250_port *up, int value)
>> struct uart_8250_port *serial8250_get_port(int line);
>> void serial8250_rpm_get(struct uart_8250_port *p);
>> void serial8250_rpm_put(struct uart_8250_port *p);
>> +int serial8250_em485_init(struct uart_8250_port *p);
>> +void serial8250_em485_destroy(struct uart_8250_port *p);
>> +static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
>> +{
>> + return p->rs485_emul && (p->port.rs485.flags & SER_RS485_ENABLED);
>> +}
>>
>> #if defined(__alpha__) && !defined(CONFIG_PCI)
>> /*
>> diff --git a/drivers/tty/serial/8250/8250_port.c
>> b/drivers/tty/serial/8250/8250_port.c
>> index 8ad0b2d..394d0d2 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -37,6 +37,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct
>> uart_8250_port *p)
>> }
>> }
>>
>> +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
>> +{
>> + unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> + if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>> + mcr |= UART_MCR_RTS;
>> + else
>> + mcr &= ~UART_MCR_RTS;
>> + serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
>> +{
>> + unsigned char mcr = serial_in(p, UART_MCR);
>> +
>> + if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> + mcr |= UART_MCR_RTS;
>> + else
>> + mcr &= ~UART_MCR_RTS;
>> + serial_out(p, UART_MCR, mcr);
>> +}
>> +
>> +static void serial8250_em485_handle_start_tx(unsigned long arg);
>> +static void serial8250_em485_handle_stop_tx(unsigned long arg);
>> +
>> void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
>> {
>> serial8250_clear_fifos(p);
>> @@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
>> }
>> EXPORT_SYMBOL_GPL(serial8250_rpm_put);
>>
>> +int serial8250_em485_init(struct uart_8250_port *p)
>> +{
>> + if (p->rs485_emul != NULL)
>> + return 0;
>> +
>> + p->rs485_emul = kmalloc(sizeof(struct uart_8250_rs485_emul),
>> GFP_KERNEL);
>> + if (p->rs485_emul == NULL)
>> + return -ENOMEM;
>> +
>> + init_timer(&p->rs485_emul->stop_tx_timer);
>> + p->rs485_emul->stop_tx_timer.function =
>> serial8250_em485_handle_stop_tx;
>> + p->rs485_emul->stop_tx_timer.data = (unsigned long)p;
>> + p->rs485_emul->stop_tx_timer.flags |= TIMER_IRQSAFE;
>> + init_timer(&p->rs485_emul->start_tx_timer);
>> + p->rs485_emul->start_tx_timer.function =
>> serial8250_em485_handle_start_tx;
>> + p->rs485_emul->st
[PATCH v5 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx
Software RS485 emultaion is to be added in the following commit.
serial8250_start_tx will need to refer serial8250_stop_rx.
Move serial8250_stop_rx in front of serial8250_start_tx in order
to avoid function forward declaration.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_port.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 0bbf340..8ad0b2d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1280,6 +1280,19 @@ static void autoconfig_irq(struct uart_8250_port *up)
port->irq = (irq > 0) ? irq : 0;
}
+static void serial8250_stop_rx(struct uart_port *port)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ serial8250_rpm_get(up);
+
+ up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+ up->port.read_status_mask &= ~UART_LSR_DR;
+ serial_port_out(port, UART_IER, up->ier);
+
+ serial8250_rpm_put(up);
+}
+
static inline void __stop_tx(struct uart_8250_port *p)
{
if (p->ier & UART_IER_THRI) {
@@ -1347,19 +1360,6 @@ static void serial8250_unthrottle(struct uart_port *port)
port->unthrottle(port);
}
-static void serial8250_stop_rx(struct uart_port *port)
-{
- struct uart_8250_port *up = up_to_u8250p(port);
-
- serial8250_rpm_get(up);
-
- up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- up->port.read_status_mask &= ~UART_LSR_DR;
- serial_port_out(port, UART_IER, up->ier);
-
- serial8250_rpm_put(up);
-}
-
static void serial8250_disable_ms(struct uart_port *port)
{
struct uart_8250_port *up =
--
2.6.3
--
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 v5 2/3] tty: Add software emulated RS485 support for 8250
Implementation of software emulation of RS485 direction handling is based
on omap_serial driver.
Before and after transmission RTS is set to the appropriate value.
Note that before calling serial8250_em485_init the caller has to
ensure that UART will interrupt when shift register empty. Otherwise,
emultaion cannot be used.
Both serial8250_em485_init and serial8250_em485_destroy are
idempotent functions.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250.h | 6 ++
drivers/tty/serial/8250/8250_port.c | 161 +++-
include/linux/serial_8250.h | 7 ++
3 files changed, 170 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..a3816c6 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,12 @@ static inline void serial_dl_write(struct uart_8250_port
*up, int value)
struct uart_8250_port *serial8250_get_port(int line);
void serial8250_rpm_get(struct uart_8250_port *p);
void serial8250_rpm_put(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p);
+void serial8250_em485_destroy(struct uart_8250_port *p);
+static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
+{
+ return p->rs485_emul && (p->port.rs485.flags & SER_RS485_ENABLED);
+}
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 8ad0b2d..394d0d2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port
*p)
}
}
+static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg);
+static void serial8250_em485_handle_stop_tx(unsigned long arg);
+
void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
{
serial8250_clear_fifos(p);
@@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_rpm_put);
+int serial8250_em485_init(struct uart_8250_port *p)
+{
+ if (p->rs485_emul != NULL)
+ return 0;
+
+ p->rs485_emul = kmalloc(sizeof(struct uart_8250_rs485_emul),
GFP_KERNEL);
+ if (p->rs485_emul == NULL)
+ return -ENOMEM;
+
+ init_timer(&p->rs485_emul->stop_tx_timer);
+ p->rs485_emul->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
+ p->rs485_emul->stop_tx_timer.data = (unsigned long)p;
+ p->rs485_emul->stop_tx_timer.flags |= TIMER_IRQSAFE;
+ init_timer(&p->rs485_emul->start_tx_timer);
+ p->rs485_emul->start_tx_timer.function =
serial8250_em485_handle_start_tx;
+ p->rs485_emul->start_tx_timer.data = (unsigned long)p;
+ p->rs485_emul->start_tx_timer.flags |= TIMER_IRQSAFE;
+
+ serial8250_em485_rts_after_send(p);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->rs485_emul == NULL)
+ return;
+
+ del_timer_sync(&p->rs485_emul->start_tx_timer);
+ del_timer_sync(&p->rs485_emul->stop_tx_timer);
+
+ kfree(p->rs485_emul);
+ p->rs485_emul = NULL;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
+
/*
* These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
* once and disable_runtime_pm_tx() will still disable RPM because the fifo is
@@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_put(up);
}
-static inline void __stop_tx(struct uart_8250_port *p)
+static __u32 __start_tx_rs485(struct uart_8250_port *p)
+{
+ if (!serial8250_em485_enabled(p))
+ return 0;
+
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&p->port);
+
+ del_timer_sync(&p->rs485_emul->stop_tx_timer);
+
+ if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p,
UART_MCR) & UART_MCR_RTS)) {
+
[PATCH v5 3/3] tty: 8250_omap: Use software emulated RS485 direction control
Use software emulated RS485 direction control to provide RS485 API existed in
omap_serial driver. Note that 8250_omap issues interrupt on shift register
empty which is single prerequesite for using software emulated RS485.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_omap.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 826c5c4..323c0a4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -698,6 +698,23 @@ static void omap_8250_throttle(struct uart_port *port)
pm_runtime_put_autosuspend(port->dev);
}
+static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485
*rs485)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ if (rs485->flags & SER_RS485_ENABLED && !serial8250_em485_enabled(up)) {
+ port->rs485 = *rs485;
+ return serial8250_em485_init(up);
+ }
+
+ if (serial8250_em485_enabled(up) && !(rs485->flags & SER_RS485_ENABLED))
+ serial8250_em485_destroy(up);
+
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
static void omap_8250_unthrottle(struct uart_port *port)
{
unsigned long flags;
@@ -1144,6 +1161,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.shutdown = omap_8250_shutdown;
up.port.throttle = omap_8250_throttle;
up.port.unthrottle = omap_8250_unthrottle;
+ up.port.rs485_config = omap_8250_rs485_config;
if (pdev->dev.of_node) {
const struct of_device_id *id;
--
2.6.3
--
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 v5 0/3] tty: Introduce software RS485 direction control support
Changes from v4: - Add commit message to 1/3 Changes from v3: - Completely redesigned. Changes from v2: - Introduced SER_RS485_SOFTWARE to show that software implementation is being used - serial8250_rs485_config is located as required - Timers are used to implement delay_before and delay_after timeouts -- 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 v4 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx
Are there any other issues except empty changelog that I have to fix before v5? 2015-12-05 19:06 GMT+03:00 Greg KH : > On Sat, Dec 05, 2015 at 01:54:49PM +0300, Matwey V. Kornilov wrote: >> Signed-off-by: Matwey V. Kornilov > > Why? > > Please say so in the changelog. > > thanks, > > greg k-h > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 -- 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 v4 3/3] tty: 8250_omap: Use software emulated RS485 direction control
Use software emulated RS485 direction control to provide RS485 API existed in
omap_serial driver. Note that 8250_omap issues interrupt on shift register
empty which is single prerequesite for using software emulated RS485.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_omap.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 826c5c4..323c0a4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -698,6 +698,23 @@ static void omap_8250_throttle(struct uart_port *port)
pm_runtime_put_autosuspend(port->dev);
}
+static int omap_8250_rs485_config(struct uart_port *port, struct serial_rs485
*rs485)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ if (rs485->flags & SER_RS485_ENABLED && !serial8250_em485_enabled(up)) {
+ port->rs485 = *rs485;
+ return serial8250_em485_init(up);
+ }
+
+ if (serial8250_em485_enabled(up) && !(rs485->flags & SER_RS485_ENABLED))
+ serial8250_em485_destroy(up);
+
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
static void omap_8250_unthrottle(struct uart_port *port)
{
unsigned long flags;
@@ -1144,6 +1161,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.shutdown = omap_8250_shutdown;
up.port.throttle = omap_8250_throttle;
up.port.unthrottle = omap_8250_unthrottle;
+ up.port.rs485_config = omap_8250_rs485_config;
if (pdev->dev.of_node) {
const struct of_device_id *id;
--
2.6.3
--
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 v4 1/3] tty: Move serial8250_stop_rx in front of serial8250_start_tx
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250_port.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 0bbf340..8ad0b2d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1280,6 +1280,19 @@ static void autoconfig_irq(struct uart_8250_port *up)
port->irq = (irq > 0) ? irq : 0;
}
+static void serial8250_stop_rx(struct uart_port *port)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+
+ serial8250_rpm_get(up);
+
+ up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+ up->port.read_status_mask &= ~UART_LSR_DR;
+ serial_port_out(port, UART_IER, up->ier);
+
+ serial8250_rpm_put(up);
+}
+
static inline void __stop_tx(struct uart_8250_port *p)
{
if (p->ier & UART_IER_THRI) {
@@ -1347,19 +1360,6 @@ static void serial8250_unthrottle(struct uart_port *port)
port->unthrottle(port);
}
-static void serial8250_stop_rx(struct uart_port *port)
-{
- struct uart_8250_port *up = up_to_u8250p(port);
-
- serial8250_rpm_get(up);
-
- up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
- up->port.read_status_mask &= ~UART_LSR_DR;
- serial_port_out(port, UART_IER, up->ier);
-
- serial8250_rpm_put(up);
-}
-
static void serial8250_disable_ms(struct uart_port *port)
{
struct uart_8250_port *up =
--
2.6.3
--
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 v4 2/3] tty: Add software emulated RS485 support for 8250
Implementation of software emulation of RS485 direction handling is based
on omap_serial driver.
Before and after transmission RTS is set to the appropriate value.
Note that before calling serial8250_em485_init the caller has to
ensure that UART will interrupt when shift register empty. Otherwise,
emultaion cannot be used.
Both serial8250_em485_init and serial8250_em485_destroy are
idempotent functions.
Signed-off-by: Matwey V. Kornilov
---
drivers/tty/serial/8250/8250.h | 6 ++
drivers/tty/serial/8250/8250_port.c | 161 +++-
include/linux/serial_8250.h | 7 ++
3 files changed, 170 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..a3816c6 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -117,6 +117,12 @@ static inline void serial_dl_write(struct uart_8250_port
*up, int value)
struct uart_8250_port *serial8250_get_port(int line);
void serial8250_rpm_get(struct uart_8250_port *p);
void serial8250_rpm_put(struct uart_8250_port *p);
+int serial8250_em485_init(struct uart_8250_port *p);
+void serial8250_em485_destroy(struct uart_8250_port *p);
+static inline bool serial8250_em485_enabled(struct uart_8250_port *p)
+{
+ return p->rs485_emul && (p->port.rs485.flags & SER_RS485_ENABLED);
+}
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index 8ad0b2d..394d0d2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -37,6 +37,7 @@
#include
#include
#include
+#include
#include
#include
@@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port
*p)
}
}
+static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static void serial8250_em485_handle_start_tx(unsigned long arg);
+static void serial8250_em485_handle_stop_tx(unsigned long arg);
+
void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
{
serial8250_clear_fifos(p);
@@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_rpm_put);
+int serial8250_em485_init(struct uart_8250_port *p)
+{
+ if (p->rs485_emul != NULL)
+ return 0;
+
+ p->rs485_emul = kmalloc(sizeof(struct uart_8250_rs485_emul),
GFP_KERNEL);
+ if (p->rs485_emul == NULL)
+ return -ENOMEM;
+
+ init_timer(&p->rs485_emul->stop_tx_timer);
+ p->rs485_emul->stop_tx_timer.function = serial8250_em485_handle_stop_tx;
+ p->rs485_emul->stop_tx_timer.data = (unsigned long)p;
+ p->rs485_emul->stop_tx_timer.flags |= TIMER_IRQSAFE;
+ init_timer(&p->rs485_emul->start_tx_timer);
+ p->rs485_emul->start_tx_timer.function =
serial8250_em485_handle_start_tx;
+ p->rs485_emul->start_tx_timer.data = (unsigned long)p;
+ p->rs485_emul->start_tx_timer.flags |= TIMER_IRQSAFE;
+
+ serial8250_em485_rts_after_send(p);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_init);
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->rs485_emul == NULL)
+ return;
+
+ del_timer_sync(&p->rs485_emul->start_tx_timer);
+ del_timer_sync(&p->rs485_emul->stop_tx_timer);
+
+ kfree(p->rs485_emul);
+ p->rs485_emul = NULL;
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_destroy);
+
/*
* These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
* once and disable_runtime_pm_tx() will still disable RPM because the fifo is
@@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port)
serial8250_rpm_put(up);
}
-static inline void __stop_tx(struct uart_8250_port *p)
+static __u32 __start_tx_rs485(struct uart_8250_port *p)
+{
+ if (!serial8250_em485_enabled(p))
+ return 0;
+
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&p->port);
+
+ del_timer_sync(&p->rs485_emul->stop_tx_timer);
+
+ if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p,
UART_MCR) & UART_MCR_RTS)) {
+
tty: Introduce software RS485 direction control support
Changes from v3: - Completely redesigned. Changes from v2: - Introduced SER_RS485_SOFTWARE to show that software implementation is being used - serial8250_rs485_config is located as required - Timers are used to implement delay_before and delay_after timeouts -- 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 v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485
2015-12-03 22:45 GMT+03:00 Peter Hurley : > On 12/03/2015 12:29 PM, Matwey V. Kornilov wrote: >> 2015-12-03 17:41 GMT+03:00 Peter Hurley : >>> Hi Matwey, >>> >>> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote: >>>> I am working on v4, where I completely redesigned implementation. And >>>> now I think that it is considerably better than v3. >>>> It looks like the following: >>>> https://github.com/matwey/linux/commits/8520_rs485_v4 >>>> But it is not ready yet, there is a bug somewhere. >>>> >>>> In the v4, each subdriver decides separately if it needs rs485 >>>> emulation support. Then it enables it like the following: >>>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0 >>>> Before calling serial8250_rs485_emul_enabled, the driver enables >>>> interrupt on empty shift register (they are always there for omap_). >>> >>> Looks good. >>> >>> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the >>> debug effort? DMA adds a completely different tx path. >> >> Many thanks for the advice. I've just found that the bug is not in my code =) >> Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just >> hangs on open() and the process is in S+ state. > > Hmm, that's odd. So > > $ stty -a < /dev/ttyS5 > > hangs if something like below is running? > > $ cat > /dev/ttyS5 > Nonblocking mode works, blocking mode hands on tty_port_block_til_ready https://bugzilla.kernel.org/show_bug.cgi?id=108851 > >>> Also, before submission, please shorten the identifiers. And Greg hates >>> functions returning bool so just expanded serial8250_rs485_emul_enabled() >>> inline. I would like to keep it as API to hide implementation details. In other words, I don't want to inline it in omap_8250_rs485_config. >> >> Am I allowed to use `re' instead of rs485_emul in names? > > Long names and constructs tend to obscure the execution flow. > Some of the names could be reduced where the meaning is obvious: > > serial8250_rts_on_send > serial8250_rts_after_send > serial8250_handle_start_timer > serial8250_handle_stop_timer > > These two I would inline into their lone call site: > > serial8250_rs485_emul_startup() > serial8250_rs485_emul_shutdown() > > serial8250_rs485_emul_start_tx => __start_tx_rs485 > > rs485_emul => sw485/em485/emul485/soft485 ? > > Or just rs485 (except for the field name and structs so as not to confuse > it with the port->rs485) > > Just my 2¢ > > Regards, > Peter Hurley > > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 -- 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 v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485
2015-12-03 17:41 GMT+03:00 Peter Hurley : > Hi Matwey, > > On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote: >> I am working on v4, where I completely redesigned implementation. And >> now I think that it is considerably better than v3. >> It looks like the following: >> https://github.com/matwey/linux/commits/8520_rs485_v4 >> But it is not ready yet, there is a bug somewhere. >> >> In the v4, each subdriver decides separately if it needs rs485 >> emulation support. Then it enables it like the following: >> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0 >> Before calling serial8250_rs485_emul_enabled, the driver enables >> interrupt on empty shift register (they are always there for omap_). > > Looks good. > > Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the > debug effort? DMA adds a completely different tx path. Many thanks for the advice. I've just found that the bug is not in my code =) Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just hangs on open() and the process is in S+ state. > > Also, before submission, please shorten the identifiers. And Greg hates > functions returning bool so just expanded serial8250_rs485_emul_enabled() > inline. Am I allowed to use `re' instead of rs485_emul in names? > > Regards, > Peter Hurley > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 -- 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 v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485
2015-12-03 2:20 GMT+03:00 Peter Hurley : > On 11/18/2015 02:49 PM, Matwey V. Kornilov wrote: >> 2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov : >>> 2015-11-18 21:33 GMT+03:00 Peter Hurley : >>>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote: >>>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley : >>>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote: > [...] >>>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a >>>>>>> pointless impossible to correctly set flag for all eternity. >>>>>>> >>>>>>> Please explain the correct setting for this flag when a device driver >>>>>>> uses hardware or software or a mix according to what the silicon is >>>>>>> capable of and what values are requested ? How will an application use >>>>>>> the >>>>>>> flag meaningfully. Please explain what will happen if someone discovers >>>>>>> a >>>>>>> silicon bug and in a future 4.x release turns an implementation from >>>>>>> hardware to software - will they have to lie about the flag to avoid >>>>>>> breaking their application code - that strikes me as a bad thing. >>>>>> >>>>>> The existing driver behavior is already significantly variant and needs >>>>>> to be converged, which shouldn't be too difficult. Here's a quick >>>>>> summary: >>>>>> >>>>>> mcfuart ignores delay values, delays unsupported >>>>>> imx clamps delay values to 0, delays unsupported >>>>>> atmel only delay_rts_after_send used; delay_rts_before_send >>>>>> does nothing >>>>>> 8250_fintek clamps delay values to 1, unclear if h/w delay is msecs >>>>>> omap-serial*software emulation (but tx empty polling not reqd) >>>>>> lpc18xx-uartclamps delay_rts_before_send to 0, unsupported >>>>>> clamps delay_rts_after_send to max h/w value >>>>>> max310x returns -ERANGE if either delay value > h/w support (15 >>>>>> msecs) >>>>>> sc16is7xx* returns -EINVAL if delay_rts_after_send is set >>>>>> crisv10*clamps delay_rts_before_send to 1000 msecs >>>>>> ignores delays_rts_after_send (after dma is delayed by 2 >>>>>> * chars) >>>>>> * implements delay(s) in software >>>>>> >>>>>> The omap-serial emulation should not have been merged in its current >>>>>> form. >>>>>> >>>>>> IMO the proper driver behavior should be clamp to h/w limit so an >>>>>> application >>>>>> can determine the maximum delay supported. If a delay is unsupported, it >>>>>> should >>>>>> be clamped to 0. The application should check the RS485 settings >>>>>> returned by >>>>>> TIOCSRS485 to determine how the driver set them. >>>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action >>>>>> ] >>>>> >>>>> But the similar could be true for minimal supported delay. If user >>>>> requires delay which is less than lower bound, the delay is raised to >>>>> the lower bound. If user requires delay which is greater than upper >>>>> bound, the delay is set to the upper bound. Then software >>>>> implementation could use (tx fifo size / baudrate) as lower bound for >>>>> delay_after_send. >>>> >>>> From the application point-of-view (really the only relevant semantics), >>>> delay_dts_after_send refers to the number of milliseconds to delay the >>>> toggle of RTS after the last bit has been _transmitted_. > > Is there consensus then about what the semantics of unsupported RS485 delay > values are? I (or someone else) can trivially add the documentation and > fixes to the existing in-tree drivers. > > >>>> A couple of possibilities for improving the emulation are: >>>> 1) Optionally using an HR timer for sub-jiffy turnaround. >>>> 2) Only supporting 8250-based hardware that can be set to interrupt when >>>>both tx fifo and transmitter shift register are empty. >>> >>> This is to support the RS485 API wit
Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485
2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov : > 2015-11-18 21:33 GMT+03:00 Peter Hurley : >> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote: >>> 2015-11-16 22:18 GMT+03:00 Peter Hurley : >>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote: >>>>>> I specifically asked for it. >>>>>> >>>>>> I can think of 2 reasons that userspace wants to know: >>>>>> 1. Because the characteristics of the software emulation are >>>>>> unacceptable so >>>>>>the application wants to terminate w/error rather than continue. >>>>> >>>>> But that could equally be true of hardware. >>>> >>>> I had this exact same thought, but concluded that it argues for a way >>>> to select the software implementation even when h/w supports RS485. >>>> >>>>> In fact your software >>>>> emulation is going to behave vastly better than many of the hardware ones. >>>>> >>>>>> 2. Because userspace will use different values for h/w vs. s/w. For >>>>>> example, >>>>>>right now, the emulation will raise/lower RTS prematurely when tx >>>>>> ends if >>>>>>the rts-after-send timer is 0. >>>>> >>>>> That's a bug then. It should be fixed as part of the merge or future >>>>> patches - if they are not providing that emulation then they ought to do >>>>> so and at least adjust the timing based on the baud rate so you don't >>>>> have to spin polling the 16x50 uart to check the last bit fell out of the >>>>> register. >>>> >>>> I suppose the timer(s) could be fudged and then TEMT polled (or polled >>>> every >>>> char baud cycles). But I don't see how this will behave better than a h/w >>>> implementation; the granularity of the jiffy clock alone will guarantee >>>> sub-optimal turnaround, even at 9600. >>>> >>>>> I'd have no problem with an API that was about asking what features are >>>>> available : both hardware and software - but the software flag seems to >>>>> make no sense at all. Software doesn't imply anything about quality or >>>>> feature set. If there is something the emulation cannot support then >>>>> there should be a flag indicating that feature is not supported, not a >>>>> flag saying software (which means nothing - as it may be supported in >>>>> future, or may differ by uart etc). >>>> >>>> Fair enough. >>>> >>>>> It's also not "easy to drop". If it ever goes in we are stuck with a >>>>> pointless impossible to correctly set flag for all eternity. >>>>> >>>>> Please explain the correct setting for this flag when a device driver >>>>> uses hardware or software or a mix according to what the silicon is >>>>> capable of and what values are requested ? How will an application use the >>>>> flag meaningfully. Please explain what will happen if someone discovers a >>>>> silicon bug and in a future 4.x release turns an implementation from >>>>> hardware to software - will they have to lie about the flag to avoid >>>>> breaking their application code - that strikes me as a bad thing. >>>> >>>> The existing driver behavior is already significantly variant and needs >>>> to be converged, which shouldn't be too difficult. Here's a quick summary: >>>> >>>> mcfuart ignores delay values, delays unsupported >>>> imx clamps delay values to 0, delays unsupported >>>> atmel only delay_rts_after_send used; delay_rts_before_send does >>>> nothing >>>> 8250_fintek clamps delay values to 1, unclear if h/w delay is msecs >>>> omap-serial*software emulation (but tx empty polling not reqd) >>>> lpc18xx-uartclamps delay_rts_before_send to 0, unsupported >>>> clamps delay_rts_after_send to max h/w value >>>> max310x returns -ERANGE if either delay value > h/w support (15 >>>> msecs) >>>> sc16is7xx* returns -EINVAL if delay_rts_after_send is set >>>> crisv10*clamps delay_rts_before_send to 1000 msecs >>>> ignores delays_rts_after_send (after dma is delayed by 2 * >>>> chars) >>&
Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485
2015-11-18 21:33 GMT+03:00 Peter Hurley : > On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote: >> 2015-11-16 22:18 GMT+03:00 Peter Hurley : >>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote: >>>>> I specifically asked for it. >>>>> >>>>> I can think of 2 reasons that userspace wants to know: >>>>> 1. Because the characteristics of the software emulation are unacceptable >>>>> so >>>>>the application wants to terminate w/error rather than continue. >>>> >>>> But that could equally be true of hardware. >>> >>> I had this exact same thought, but concluded that it argues for a way >>> to select the software implementation even when h/w supports RS485. >>> >>>> In fact your software >>>> emulation is going to behave vastly better than many of the hardware ones. >>>> >>>>> 2. Because userspace will use different values for h/w vs. s/w. For >>>>> example, >>>>>right now, the emulation will raise/lower RTS prematurely when tx ends >>>>> if >>>>>the rts-after-send timer is 0. >>>> >>>> That's a bug then. It should be fixed as part of the merge or future >>>> patches - if they are not providing that emulation then they ought to do >>>> so and at least adjust the timing based on the baud rate so you don't >>>> have to spin polling the 16x50 uart to check the last bit fell out of the >>>> register. >>> >>> I suppose the timer(s) could be fudged and then TEMT polled (or polled every >>> char baud cycles). But I don't see how this will behave better than a h/w >>> implementation; the granularity of the jiffy clock alone will guarantee >>> sub-optimal turnaround, even at 9600. >>> >>>> I'd have no problem with an API that was about asking what features are >>>> available : both hardware and software - but the software flag seems to >>>> make no sense at all. Software doesn't imply anything about quality or >>>> feature set. If there is something the emulation cannot support then >>>> there should be a flag indicating that feature is not supported, not a >>>> flag saying software (which means nothing - as it may be supported in >>>> future, or may differ by uart etc). >>> >>> Fair enough. >>> >>>> It's also not "easy to drop". If it ever goes in we are stuck with a >>>> pointless impossible to correctly set flag for all eternity. >>>> >>>> Please explain the correct setting for this flag when a device driver >>>> uses hardware or software or a mix according to what the silicon is >>>> capable of and what values are requested ? How will an application use the >>>> flag meaningfully. Please explain what will happen if someone discovers a >>>> silicon bug and in a future 4.x release turns an implementation from >>>> hardware to software - will they have to lie about the flag to avoid >>>> breaking their application code - that strikes me as a bad thing. >>> >>> The existing driver behavior is already significantly variant and needs >>> to be converged, which shouldn't be too difficult. Here's a quick summary: >>> >>> mcfuart ignores delay values, delays unsupported >>> imx clamps delay values to 0, delays unsupported >>> atmel only delay_rts_after_send used; delay_rts_before_send does >>> nothing >>> 8250_fintek clamps delay values to 1, unclear if h/w delay is msecs >>> omap-serial*software emulation (but tx empty polling not reqd) >>> lpc18xx-uartclamps delay_rts_before_send to 0, unsupported >>> clamps delay_rts_after_send to max h/w value >>> max310x returns -ERANGE if either delay value > h/w support (15 >>> msecs) >>> sc16is7xx* returns -EINVAL if delay_rts_after_send is set >>> crisv10*clamps delay_rts_before_send to 1000 msecs >>> ignores delays_rts_after_send (after dma is delayed by 2 * >>> chars) >>> * implements delay(s) in software >>> >>> The omap-serial emulation should not have been merged in its current form. >>> >>> IMO the proper driver behavior should be clamp to h/w limit so an >>> application >>> can determine the maximum delay supported. If a delay is unsupported, it >>>
