* Re: [patch v15 3/4] Documentation: jtag: Add bindings for Aspeed SoC 24xx and 25xx families JTAG master driver
From: Joel Stanley @ 2018-01-11 17:50 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: Greg KH, Arnd Bergmann, Linux Kernel Mailing List, Linux ARM,
devicetree, OpenBMC Maillist, Jiří Pírko,
Tobias Klauser, linux-serial, Vadim Pasternak,
system-sw-low-level, Rob Herring, openocd-devel-owner, linux-api,
David S . Miller, mchehab, Jiri Pirko
In-Reply-To: <1514202808-29747-4-git-send-email-oleksandrs@mellanox.com>
On Mon, Dec 25, 2017 at 3:53 AM, Oleksandr Shamray
<oleksandrs@mellanox.com> wrote:
> It has been tested on Mellanox system with BMC equipped with
> Aspeed 2520 SoC for programming CPLD devices.
>
> Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> v14->v15
> v13->v14
> v12->v13
> v11->v12
> v10->v11
> v9->v10
> v8->v9
> v7->v8
> Comments pointed by pointed by Joel Stanley <joel.stan@gmail.com>
> - Change compatible string to ast2400 and ast2000
>
> V6->v7
> Comments pointed by Tobias Klauser <tklauser@distanz.ch>
> - Fix spell "Doccumentation" -> "Documentation"
>
> v5->v6
> Comments pointed by Tobias Klauser <tklauser@distanz.ch>
> - Small nit: s/documentation/Documentation/
>
> v4->v5
>
> V3->v4
> Comments pointed by Rob Herring <robh@kernel.org>
> - delete unnecessary "status" and "reg-shift" descriptions in
> bndings file
>
> v2->v3
> Comments pointed by Rob Herring <robh@kernel.org>
> - split Aspeed jtag driver and binding to sepatrate patches
> - delete unnecessary "status" and "reg-shift" descriptions in
> bndings file
> ---
> .../devicetree/bindings/jtag/aspeed-jtag.txt | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
>
> diff --git a/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt b/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
> new file mode 100644
> index 0000000..8cfc610
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
> @@ -0,0 +1,18 @@
> +Aspeed JTAG driver for ast2400 and ast2500 SoC
> +
> +Required properties:
> +- compatible: Should be one of
> + - "aspeed,ast2400-jtag"
> + - "aspeed,ast2500-jtag"
> +- reg contains the offset and length of the JTAG memory
> + region
> +- clocks root clock of bus, should reference the APB clock
> +- interrupts should contain JTAG controller interrupt
> +
> +Example:
> +jtag: jtag@1e6e4000 {
> + compatible = "aspeed,ast2500-jtag";
> + reg = <0x1e6e4000 0x1c>;
> + clocks = <&clk_apb>;
We've now got a proper clock driver upstream. Can you update the
example to match the newly added bindings?
clocks = <&syscon ASPEED_CLK_APB>;
Cheers,
Joel
^ permalink raw reply
* Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied
From: Ed Blake @ 2018-01-11 17:48 UTC (permalink / raw)
To: Nuno Gonçalves; +Cc: gregkh, linux-kernel, linux-serial
In-Reply-To: <CAEXMXLTYFbBraCNHyfc6JpCFwUMVqPeEanCufbKkgW2At7=d0Q@mail.gmail.com>
On 11/01/18 17:28, Nuno Gonçalves wrote:
> I have to disagree :)
>
> if (rate < i * min_rate) is true to i=a, then
>
> (rate >= i * min_rate && rate <= i * max_rate) will always be false
> for any i=b, where b>a.
No, because 'rate' is assigned from clk_round_rate() each iteration.
The idea of this code is to iterate through integer multiples of baud *
16 until you find an achievable rate that is within the +/- 1.6% range.
Until then, the rate returned from clk_round_rate() could be lower than
i * min_rate or higher than i * max_rate, in which case you keep going.
> If this condition is true, it means the old condition would be always
> false for the remaining of the iteration.
>
> My patch "only" avoids integer overflow and terminates the search as
> soon as possible, since no need to search for bigger dividers if the
> current one would already mean a rate below min_rate (that it, this is
> the closer).
It terminates the search as soon as the rate returned from
clk_round_rate() is lower than i * min_rate, which is too soon.
> So in fact we also break when the closer divider have been found.
>
> Thanks,
> Nuno
>
> On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@sondrel.com> wrote:
>> Hi Nuno,
>>
>> Thanks for reporting this and the patch.
>>
>> On 11/01/18 13:38, Nuno Goncalves wrote:
>>> When target_rate is big enough and not permitted in hardware,
>>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>>> (32b signed).
>>>
>>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>>> means the rate is not permitted.
>> 'rate < i * min_rate' does not mean the rate is not permitted. clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate. This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>>
>>
>>> This avoids arbitraty rates to be applied. Still in my hardware the max
>>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>>> artifact of clk_round_rate which is not understood by me and independent of
>>> this fix. Might or might not be another bug.
>>>
>>> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
>>> ---
>>> drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>>> index 5bb0c42c88dd..a27ea916abbf 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>>
>>> for (i = 1; i <= UART_DIV_MAX; i++) {
>>> rate = clk_round_rate(d->clk, i * target_rate);
>>> - if (rate >= i * min_rate && rate <= i * max_rate)
>>> +
>>> + if (rate < i * min_rate) {
>>> + i = UART_DIV_MAX + 1;
>>> + break;
>>> + }
>>> +
>>> + if (rate <= i * max_rate)
>>> break;
>>> }
>>> if (i <= UART_DIV_MAX) {
>> --
>> Ed
>>
--
Ed
^ permalink raw reply
* Re: [patch v15 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Joel Stanley @ 2018-01-11 17:45 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: Greg KH, Arnd Bergmann, Linux Kernel Mailing List, Linux ARM,
devicetree, OpenBMC Maillist, Jiří Pírko,
Tobias Klauser, linux-serial, Vadim Pasternak,
system-sw-low-level, Rob Herring, openocd-devel-owner, linux-api,
David S . Miller, mchehab, Jiri Pirko
In-Reply-To: <1514202808-29747-3-git-send-email-oleksandrs@mellanox.com>
On Mon, Dec 25, 2017 at 3:53 AM, Oleksandr Shamray
<oleksandrs@mellanox.com> wrote:
> Driver adds support of Aspeed 2500/2400 series SOC JTAG master controller.
>
> Driver implements the following jtag ops:
> - freq_get;
> - freq_set;
> - status_get;
> - idle;
> - xfer;
>
> It has been tested on Mellanox system with BMC equipped with
> Aspeed 2520 SoC for programming CPLD devices.
>
> Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Philippe Ombredanne <pombredanne@nexb.com>
Acked-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
^ permalink raw reply
* Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied
From: Nuno Gonçalves @ 2018-01-11 17:28 UTC (permalink / raw)
To: Ed Blake; +Cc: gregkh, linux-kernel, linux-serial
In-Reply-To: <57ef28bd-41da-73c2-3004-f9b2ecd8b102@sondrel.com>
I have to disagree :)
if (rate < i * min_rate) is true to i=a, then
(rate >= i * min_rate && rate <= i * max_rate) will always be false
for any i=b, where b>a.
If this condition is true, it means the old condition would be always
false for the remaining of the iteration.
My patch "only" avoids integer overflow and terminates the search as
soon as possible, since no need to search for bigger dividers if the
current one would already mean a rate below min_rate (that it, this is
the closer).
So in fact we also break when the closer divider have been found.
Thanks,
Nuno
On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@sondrel.com> wrote:
> Hi Nuno,
>
> Thanks for reporting this and the patch.
>
> On 11/01/18 13:38, Nuno Goncalves wrote:
>> When target_rate is big enough and not permitted in hardware,
>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>> (32b signed).
>>
>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>> means the rate is not permitted.
>
> 'rate < i * min_rate' does not mean the rate is not permitted. clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate. This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>
>
>>
>> This avoids arbitraty rates to be applied. Still in my hardware the max
>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>> artifact of clk_round_rate which is not understood by me and independent of
>> this fix. Might or might not be another bug.
>>
>> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
>> ---
>> drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 5bb0c42c88dd..a27ea916abbf 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>
>> for (i = 1; i <= UART_DIV_MAX; i++) {
>> rate = clk_round_rate(d->clk, i * target_rate);
>> - if (rate >= i * min_rate && rate <= i * max_rate)
>> +
>> + if (rate < i * min_rate) {
>> + i = UART_DIV_MAX + 1;
>> + break;
>> + }
>> +
>> + if (rate <= i * max_rate)
>> break;
>> }
>> if (i <= UART_DIV_MAX) {
>
> --
> Ed
>
^ permalink raw reply
* Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied
From: Ed Blake @ 2018-01-11 17:18 UTC (permalink / raw)
To: Nuno Goncalves, gregkh, linux-kernel, linux-serial
In-Reply-To: <20180111133832.13125-1-nunojpg@gmail.com>
Hi Nuno,
Thanks for reporting this and the patch.
On 11/01/18 13:38, Nuno Goncalves wrote:
> When target_rate is big enough and not permitted in hardware,
> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
> (32b signed).
>
> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
> means the rate is not permitted.
'rate < i * min_rate' does not mean the rate is not permitted. clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate. This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>
> This avoids arbitraty rates to be applied. Still in my hardware the max
> allowed rate (1500000) is aplied when a higher is requested. This seems a
> artifact of clk_round_rate which is not understood by me and independent of
> this fix. Might or might not be another bug.
>
> Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
> ---
> drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 5bb0c42c88dd..a27ea916abbf 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>
> for (i = 1; i <= UART_DIV_MAX; i++) {
> rate = clk_round_rate(d->clk, i * target_rate);
> - if (rate >= i * min_rate && rate <= i * max_rate)
> +
> + if (rate < i * min_rate) {
> + i = UART_DIV_MAX + 1;
> + break;
> + }
> +
> + if (rate <= i * max_rate)
> break;
> }
> if (i <= UART_DIV_MAX) {
--
Ed
^ permalink raw reply
* Re: [PATCH RFC 4/7] i2c: Add device tree bindings for GENI I2C Controller
From: Rob Herring @ 2018-01-11 15:19 UTC (permalink / raw)
To: Karthik Ramasubramanian
Cc: linux-arm-msm, Linux I2C, linux-serial, linux-doc,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, David Brown, Mark Rutland, Jonathan Corbet,
Wolfram Sang, Greg Kroah-Hartman, Jiri Slaby, Sagar Dharia
In-Reply-To: <1d741169-e6e6-1896-d8d4-4f580f5d497d@codeaurora.org>
On Mon, Jan 8, 2018 at 6:33 PM, Karthik Ramasubramanian
<kramasub@codeaurora.org> wrote:
>
>
> On 1/2/2018 8:51 AM, Rob Herring wrote:
>>
>> On Wed, Dec 27, 2017 at 09:27:23AM -0700, Karthikeyan Ramasubramanian
>> wrote:
>>>
>>> Add device tree binding support for I2C Controller in GENI based
>>> QUP Wrapper.
>>>
>>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>>> ---
>>> .../devicetree/bindings/i2c/i2c-qcom-geni.txt | 39
>>> ++++++++++++++++++++++
>>> 1 file changed, 39 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>>> b/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>>> new file mode 100644
>>> index 0000000..d2fa9ce
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>>> @@ -0,0 +1,39 @@
>>> +Qualcomm Technologies Inc. GENI based I2C Controller driver
>>> +
>>> +Required properties:
>>> + - compatible: Should be:
>>> + * "qcom,i2c-geni.
>>
>>
>> Only 1 version?
>
> The Serial Engine used by I2C protocol has the same version as the QUP h/w
> version. The QUP Wrapper driver exposes an interface function to get the h/w
> version so that I2C controller driver can support version-specific
> operations, if any.
>>
>>
>>> + - reg: Should contain QUP register address and length.
>>> + - interrupts: Should contain I2C interrupt.
>>> + - clocks: Serial engine core clock, and AHB clocks needed by the
>>> device.
>>
>>
>> Are there really clocks for a firmware based device or these are just
>> clocks in the parent serial engine?
>
> The clocks are required to derive the protocol clock. The clocks are also
> required by the Serial Engine to access the System Memory during DMA mode of
> operation.
You can get the QUP core (or Serial engine?) node and then get its
clocks if you need to know the frequency. Put the clocks in DT in the
h/w block they belong to. If you don't really have an I2C clock in the
h/w, don't put one in the DT.
Rob
^ permalink raw reply
* Re: [PATCH RFC 6/7] serial: Add device tree bindings for GENI based UART Controller
From: Rob Herring @ 2018-01-11 15:12 UTC (permalink / raw)
To: Karthik Ramasubramanian
Cc: linux-arm-msm, Linux I2C, linux-serial, linux-doc,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, David Brown, Mark Rutland, Jonathan Corbet,
Wolfram Sang, Greg Kroah-Hartman, Jiri Slaby, Girish Mahadevan
In-Reply-To: <068baf4d-963e-f177-311d-d2c76d27a6f9@codeaurora.org>
On Tue, Jan 9, 2018 at 12:36 PM, Karthik Ramasubramanian
<kramasub@codeaurora.org> wrote:
>
>
> On 1/2/2018 8:55 AM, Rob Herring wrote:
>>
>> On Wed, Dec 27, 2017 at 09:27:25AM -0700, Karthikeyan Ramasubramanian
>> wrote:
>>>
>>> Add device tree binding support for GENI based UART Controller in the
>>> QUP Wrapper.
>>>
>>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>>> ---
>>> .../devicetree/bindings/serial/qcom,geni-uart.txt | 31
>>> ++++++++++++++++++++++
>>> 1 file changed, 31 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>>> b/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>>> new file mode 100644
>>> index 0000000..e60ec6a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>>> @@ -0,0 +1,31 @@
>>> +Qualcomm Technologies Inc. GENI based Serial UART Controller driver
>>> +
>>> +This serial UART driver supports console use-cases. This driver is meant
>>> +only for Generic Interface (GENI) based Qualcomm Universal Peripheral
>>> (QUP)
>>> +cores and isn't backwards compatible.
>>> +
>>> +Required properties:
>>> +- compatible: should contain "qcom,geni-uart, qcom,geni-console"
>>
>>
>> Is console different programming model or just how you are using the
>> h/w? for the latter, drop it as we have stdout-path to select a console.
>
> The console programming model is different from a regular UART port and
> hence the compatible field contains console in it.
And "console" is what the h/w reference manual calls it? If so, then
it is fine. If not, sounds like a Linuxism.
Rob
^ permalink raw reply
* [PATCH] 8250_dw: do not int overflow when rate can not be aplied
From: Nuno Goncalves @ 2018-01-11 13:38 UTC (permalink / raw)
To: ed.blake, gregkh, linux-kernel, linux-serial; +Cc: Nuno Goncalves
When target_rate is big enough and not permitted in hardware,
then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
(32b signed).
A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
means the rate is not permitted.
This avoids arbitraty rates to be applied. Still in my hardware the max
allowed rate (1500000) is aplied when a higher is requested. This seems a
artifact of clk_round_rate which is not understood by me and independent of
this fix. Might or might not be another bug.
Signed-off-by: Nuno Goncalves <nunojpg@gmail.com>
---
drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5bb0c42c88dd..a27ea916abbf 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
for (i = 1; i <= UART_DIV_MAX; i++) {
rate = clk_round_rate(d->clk, i * target_rate);
- if (rate >= i * min_rate && rate <= i * max_rate)
+
+ if (rate < i * min_rate) {
+ i = UART_DIV_MAX + 1;
+ break;
+ }
+
+ if (rate <= i * max_rate)
break;
}
if (i <= UART_DIV_MAX) {
--
2.11.0
^ permalink raw reply related
* Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
From: Peter Ujfalusi @ 2018-01-10 14:43 UTC (permalink / raw)
To: Jarkko Nikula, Greg Kroah-Hartman
Cc: linux-fbdev, David Airlie, Joonas Lahtinen, Heiko Carstens,
alsa-devel, dri-devel, Jaroslav Kysela, linux-s390, linux-omap,
James E.J. Bottomley, linux-scsi, Takashi Iwai, Sebastian Ott,
James Smart, Cezary Jackiewicz, linux-serial, Jiri Slaby,
Darren Hart, Zhang Rui, Dick Kennedy, Mathias Nyman,
Bartlomiej Zolnierkiewicz, Peter Oberparleiter, intel-gfx
In-Reply-To: <20171220105404.GA29856@bitmer.com>
On 2017-12-20 12:54, Jarkko Nikula wrote:
> On Wed, Dec 20, 2017 at 10:32:11AM +0100, Greg Kroah-Hartman wrote:
>> On Wed, Dec 20, 2017 at 01:24:44AM -0800, Joe Perches wrote:
>>> On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote:
>>>> On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote:
>>>>> Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
>>> []
>>>>> diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
>>> []
>>>>> @@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev,
>>>>> return size;
>>>>> }
>>>>>
>>>>> -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store);
>>>>> +static DEVICE_ATTR_RW(dma_op_mode);
>>>>>
>>>>
>>>> While I can ack this part here if it helps generic cleanup effort I
>>>> don't understart would it improve code readability in general? Mode 644
>>>> is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go
>>>> through all of these files in order to see what does it mean:
>>
>> Yeah, 644 is "clear", but _RW() is even more clear. Ideally I want to
>> get rid of all of the "non-standard" users that set random modes of
>> sysfs files, as we get it wrong too many times. Using the "defaults" is
>> much better.
>>
> Fair enough. For the sound/soc/omap/ (Acked-by was missing from my
> previous reply):
>
> Acked-by: Jarkko Nikula <jarkko.nikula@bitmer.com>
And from me to the same file (sound/soc/omap/mcbsp.c):
Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH RFC 6/7] serial: Add device tree bindings for GENI based UART Controller
From: Karthik Ramasubramanian @ 2018-01-09 18:36 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm, linux-i2c, linux-serial, linux-doc, devicetree,
andy.gross, david.brown, mark.rutland, corbet, wsa, gregkh,
jslaby, Girish Mahadevan
In-Reply-To: <20180102155517.j4sqhdtamybtgta4@rob-hp-laptop>
On 1/2/2018 8:55 AM, Rob Herring wrote:
> On Wed, Dec 27, 2017 at 09:27:25AM -0700, Karthikeyan Ramasubramanian wrote:
>> Add device tree binding support for GENI based UART Controller in the
>> QUP Wrapper.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> .../devicetree/bindings/serial/qcom,geni-uart.txt | 31 ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt b/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>> new file mode 100644
>> index 0000000..e60ec6a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>> @@ -0,0 +1,31 @@
>> +Qualcomm Technologies Inc. GENI based Serial UART Controller driver
>> +
>> +This serial UART driver supports console use-cases. This driver is meant
>> +only for Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP)
>> +cores and isn't backwards compatible.
>> +
>> +Required properties:
>> +- compatible: should contain "qcom,geni-uart, qcom,geni-console"
>
> Is console different programming model or just how you are using the
> h/w? for the latter, drop it as we have stdout-path to select a console.
The console programming model is different from a regular UART port and
hence the compatible field contains console in it.
>
>> +- reg: Should contain UART register location and length.
>> +- interrupts: Should contain UART core interrupts.
>> +- clocks: clocks needed for UART, includes the core and AHB clock.
>> +- pinctrl-names/pinctrl-0/1: The GPIOs assigned to this core. The names
>> + Should be "active" and "sleep" for the pin confuguration when core is active
>> + or when entering sleep state.
>> +- qcom,wrapper-core: Wrapper QUP core containing this UART controller.
>> +
>> +Example:
>> +qup_uart11: qcom,qup_uart@0xa88000 {
>
> Use generic node names and no '0x':
>
> serial@a88000
I will update as per the recommendation.
>
>> + compatible = "qcom,geni-uart";
>> + reg = <0xa88000 0x7000>;
>> + reg-names = "se_phys";
>> + clock-names = "se-clk", "m-ahb", "s-ahb";
>
> Not documented.
I will add the documentation for the missing element.
>
>> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>,
>> + <&clock_gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
>> + <&clock_gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&qup_1_uart_3_active>;
>> + pinctrl-1 = <&qup_1_uart_3_sleep>;
>> + interrupts = <0 355 0>;
>> + qcom,wrapper-core = <&qup_0>;
>> +};
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH v2 2/2] serdev: only match serdev devices
From: Johan Hovold @ 2018-01-09 16:09 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Hans de Goede, Johan Hovold
In-Reply-To: <20180109160917.30752-1-johan@kernel.org>
Only serdev devices (a.k.a. clients or slaves) are bound to drivers so
bail out early from match() in case the device is not a serdev device
(i.e. if it's a serdev controller).
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serdev/core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c8c43834477b..f710862f5c06 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -63,6 +63,11 @@ static const struct device_type serdev_device_type = {
.release = serdev_device_release,
};
+static bool is_serdev_device(const struct device *dev)
+{
+ return dev->type == &serdev_device_type;
+}
+
static void serdev_ctrl_release(struct device *dev)
{
struct serdev_controller *ctrl = to_serdev_controller(dev);
@@ -76,6 +81,9 @@ static const struct device_type serdev_ctrl_type = {
static int serdev_device_match(struct device *dev, struct device_driver *drv)
{
+ if (!is_serdev_device(dev))
+ return 0;
+
/* TODO: platform matching */
if (acpi_driver_match_device(dev, drv))
return 1;
--
2.15.1
^ permalink raw reply related
* [PATCH v2 1/2] serdev: do not generate modaliases for controllers
From: Johan Hovold @ 2018-01-09 16:09 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Hans de Goede, Johan Hovold
In-Reply-To: <20180109160917.30752-1-johan@kernel.org>
Serdev controllers are not bound to any drivers and it therefore makes
no sense to generate modaliases for them.
This has already been fixed separately for ACPI controllers for which
uevent errors were also being logged during probe due to the missing
ACPI companions (from which ACPI modaliases are generated).
This patch moves the modalias handling from the bus type to the client
device type. Specifically, this means that only serdev devices (a.k.a.
clients or slaves) will have have MODALIAS fields in their uevent
environments and corresponding modalias sysfs attributes.
Also add the missing static keyword for the modalias device attribute
when moving the definition.
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serdev/core.c | 72 ++++++++++++++++++++++-------------------------
1 file changed, 34 insertions(+), 38 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 5dc88f61f506..c8c43834477b 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -19,6 +19,38 @@
static bool is_registered;
static DEFINE_IDA(ctrl_ida);
+static ssize_t modalias_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int len;
+
+ len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
+ if (len != -ENODEV)
+ return len;
+
+ return of_device_modalias(dev, buf, PAGE_SIZE);
+}
+static DEVICE_ATTR_RO(modalias);
+
+static struct attribute *serdev_device_attrs[] = {
+ &dev_attr_modalias.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(serdev_device);
+
+static int serdev_device_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+ int rc;
+
+ /* TODO: platform modalias */
+
+ rc = acpi_device_uevent_modalias(dev, env);
+ if (rc != -ENODEV)
+ return rc;
+
+ return of_device_uevent_modalias(dev, env);
+}
+
static void serdev_device_release(struct device *dev)
{
struct serdev_device *serdev = to_serdev_device(dev);
@@ -26,6 +58,8 @@ static void serdev_device_release(struct device *dev)
}
static const struct device_type serdev_device_type = {
+ .groups = serdev_device_groups,
+ .uevent = serdev_device_uevent,
.release = serdev_device_release,
};
@@ -49,23 +83,6 @@ static int serdev_device_match(struct device *dev, struct device_driver *drv)
return of_driver_match_device(dev, drv);
}
-static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
-{
- int rc;
-
- /* TODO: platform modalias */
-
- /* ACPI enumerated controllers do not have a modalias */
- if (!dev->of_node && dev->type == &serdev_ctrl_type)
- return 0;
-
- rc = acpi_device_uevent_modalias(dev, env);
- if (rc != -ENODEV)
- return rc;
-
- return of_device_uevent_modalias(dev, env);
-}
-
/**
* serdev_device_add() - add a device previously constructed via serdev_device_alloc()
* @serdev: serdev_device to be added
@@ -305,32 +322,11 @@ static int serdev_drv_remove(struct device *dev)
return 0;
}
-static ssize_t modalias_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int len;
-
- len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
- if (len != -ENODEV)
- return len;
-
- return of_device_modalias(dev, buf, PAGE_SIZE);
-}
-DEVICE_ATTR_RO(modalias);
-
-static struct attribute *serdev_device_attrs[] = {
- &dev_attr_modalias.attr,
- NULL,
-};
-ATTRIBUTE_GROUPS(serdev_device);
-
static struct bus_type serdev_bus_type = {
.name = "serial",
.match = serdev_device_match,
.probe = serdev_drv_probe,
.remove = serdev_drv_remove,
- .uevent = serdev_uevent,
- .dev_groups = serdev_device_groups,
};
/**
--
2.15.1
^ permalink raw reply related
* [PATCH v2 0/2] serdev: bus-code clean ups
From: Johan Hovold @ 2018-01-09 16:09 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Hans de Goede, Johan Hovold
As noted by Hans, we currently fail to generate uevents for ACPI serdev
controller as they do not have any ACPI companions from which ACPI
modaliases are constructed.
In fact, we should not have been generating modaliases for controllers
in the first place as controllers are not bound to drivers.
This series applies on top of Hans's minimal fix which suppresses the
uevent errors for ACPI controllers (even though it could replace it
entirely if preferred).
Johan
v2
- add the missing static keyword for the modalias attribute when moving
the definition (noted by Greg)
Johan Hovold (2):
serdev: do not generate modaliases for controllers
serdev: only match serdev devices
drivers/tty/serdev/core.c | 80 +++++++++++++++++++++++++----------------------
1 file changed, 42 insertions(+), 38 deletions(-)
--
2.15.1
^ permalink raw reply
* Re: [PATCH 1/2] serdev: do not generate modaliases for controllers
From: Johan Hovold @ 2018-01-09 15:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Johan Hovold, Rob Herring, Jiri Slaby, linux-serial, linux-kernel,
Frédéric Danis
In-Reply-To: <20180109154803.GA10213@kroah.com>
On Tue, Jan 09, 2018 at 04:48:03PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 08, 2018 at 01:42:32PM +0100, Johan Hovold wrote:
> > Serdev controllers are not bound to any drivers and it therefore makes
> > no sense to generate modaliases for them.
> >
> > This has already been fixed separately for ACPI controllers for which
> > uevent errors were also being logged during probe due to the missing
> > ACPI companions (from which ACPI modaliases are generated).
> >
> > This patch moves the modalias handling from the bus type to the client
> > device type. Specifically, this means that only serdev devices (a.k.a.
> > clients or slaves) will have have MODALIAS fields in their uevent
> > environments and corresponding modalias sysfs attributes.
> >
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> > drivers/tty/serdev/core.c | 72 ++++++++++++++++++++++-------------------------
> > 1 file changed, 34 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index 5dc88f61f506..61c85e49e178 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -19,6 +19,38 @@
> > static bool is_registered;
> > static DEFINE_IDA(ctrl_ida);
> >
> > +static ssize_t modalias_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int len;
> > +
> > + len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> > + if (len != -ENODEV)
> > + return len;
> > +
> > + return of_device_modalias(dev, buf, PAGE_SIZE);
> > +}
> > +DEVICE_ATTR_RO(modalias);
>
> static?
>
> Sorry, minor nit :(
Heh, no worries. I didn't notice that as I was just moving code around
here, but I'll make sure to add that missing static in a v2.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH 0/2] serdev: bus-code clean ups
From: Greg Kroah-Hartman @ 2018-01-09 15:48 UTC (permalink / raw)
To: Johan Hovold
Cc: Rob Herring, Hans de Goede, Jiri Slaby, linux-serial,
linux-kernel, Frédéric Danis
In-Reply-To: <20180108135014.GA27201@localhost>
On Mon, Jan 08, 2018 at 02:50:14PM +0100, Johan Hovold wrote:
> On Mon, Jan 08, 2018 at 01:42:31PM +0100, Johan Hovold wrote:
> > As noted by Hans, we currently fail to generate uevents for ACPI serdev
> > controller as they do not have any ACPI companions from which ACPI
> > modaliases are constructed.
> >
> > In fact, we should not have been generating modaliases for controllers
> > in the first place as controllers are not bound to drivers.
> >
> > This series applies on top of Hans's minimal fix which suppresses the
> > uevent errors for ACPI controllers (even though it could replace it
> > entirely if preferred).
>
> I somehow forgot to CC Hans. Sorry about that. This series can be found
> here:
>
> https://lkml.kernel.org/r/20180108124233.26729-1-johan@kernel.org
Minor issue found with patch 1, can you fix that up and resend and cc:
him this time? :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] serdev: do not generate modaliases for controllers
From: Greg Kroah-Hartman @ 2018-01-09 15:48 UTC (permalink / raw)
To: Johan Hovold
Cc: Rob Herring, Jiri Slaby, linux-serial, linux-kernel,
Frédéric Danis
In-Reply-To: <20180108124233.26729-2-johan@kernel.org>
On Mon, Jan 08, 2018 at 01:42:32PM +0100, Johan Hovold wrote:
> Serdev controllers are not bound to any drivers and it therefore makes
> no sense to generate modaliases for them.
>
> This has already been fixed separately for ACPI controllers for which
> uevent errors were also being logged during probe due to the missing
> ACPI companions (from which ACPI modaliases are generated).
>
> This patch moves the modalias handling from the bus type to the client
> device type. Specifically, this means that only serdev devices (a.k.a.
> clients or slaves) will have have MODALIAS fields in their uevent
> environments and corresponding modalias sysfs attributes.
>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> drivers/tty/serdev/core.c | 72 ++++++++++++++++++++++-------------------------
> 1 file changed, 34 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 5dc88f61f506..61c85e49e178 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -19,6 +19,38 @@
> static bool is_registered;
> static DEFINE_IDA(ctrl_ida);
>
> +static ssize_t modalias_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int len;
> +
> + len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> + if (len != -ENODEV)
> + return len;
> +
> + return of_device_modalias(dev, buf, PAGE_SIZE);
> +}
> +DEVICE_ATTR_RO(modalias);
static?
Sorry, minor nit :(
^ permalink raw reply
* Re: [PATCH] gpio: serial: max310x: Support open-drain configuration for GPIOs
From: Greg Kroah-Hartman @ 2018-01-09 15:37 UTC (permalink / raw)
To: Jan Kundrát
Cc: linux-gpio, linux-serial, Alexander Shiyan, Linus Walleij
In-Reply-To: <18c9e927d2b75b3325ee3eae7e78319129faa779.1513983040.git.jan.kundrat@cesnet.cz>
On Fri, Dec 22, 2017 at 09:29:44PM +0100, Jan Kundrát wrote:
> The push-pull vs. open-drain are the only supported output modes. The
> inputs are always unconditionally equipped with weak pull-downs. That's
> the only mode, so there's probably no point in exporting that. I wonder
> if it's worthwhile to provide a custom dbg_show method to indicate the
> current status of the outputs, though.
>
> This patch and [1] for i2c-gpio together make it possible to bit-bang an
> I2C bus over GPIOs of an UART which is connected via SPI :). Yes, this
> is crazy, but it's fast enough (while on a 26Mhz SPI HW bus with a
> dual-core 1.6GHz CPU) to drive an I2C bus at 200kHz, according to my
> scope.
>
> [1] https://patchwork.ozlabs.org/patch/852591/
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> My suggestion is for this patch to go in via the tty/serial tree; I plan
> to send more patches to this driver (and some of them are alreay in
> tty-next). --jkt
Doesn't apply to my tty-next branch, so I can't apply it, please rebase
and resend.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Marcel Holtmann @ 2018-01-09 15:26 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Carlo Caione, Rob Herring, devicetree,
open list:BLUETOOTH DRIVERS, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Mark Rutland, Gustavo F. Padovan, Johan Hedberg,
Greg Kroah-Hartman, Jiri Slaby, Johan Hovold,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Larry Finger,
Daniel Drake
In-Reply-To: <CAFBinCBasb-WkLngxL2RAk9YMihC437ZgPKSkOd7Fm0RHG_aTw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Martin,
>>>>>>> As Marcel suggested we can assume that the information in the DSDT is
>>>>>>> correct so that we can get rid of the config blob also for x86
>>>>>>> platforms (assuming that the only useful information in the config
>>>>>>> blobs is the UART configuration).
>>>>>> in my tests I tried to send only the firmware without the config to my
>>>>>> RTL8723BS. unfortunately the last firmware chunk (sent to the
>>>>>> controller) times out in that case (even if I set a proper baudrate
>>>>>> before or if I specify no baudrate at all and keep the serdev at
>>>>>> 115200)
>>>>>
>>>>> What's in the config blobs besides UART configuration?
>>>>
>>>> is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?
>>>>
>>>>> It's odd because reading into hciattach_rtk.c it seems that the config
>>>>> file is actually only used by the userspace tools (hciattach) to
>>>>> retrieve the UART configuration and nothing else, whereas in the
>>>>> kernel driver the config blob is appended to the firmware.
>>>>
>>>> Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.
>>>
>>> so I googled for a few config files and this is what this turns into:
>>>
>>> Analyzing rtl8723d_config_1000000_noflow.dms
>>> Signature: 0x8723ab55
>>> Data length: 41
>>> len=1 offset=f4,{ 01 }
>>> len=2 offset=f6,{ 81 00 }
>>> len=2 offset=fa,{ 12 80 }
>>> len=16 offset=0c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b }
>>> len=1 offset=d9,{ 0f }
>>> len=1 offset=e4,{ 08 }
>>> Analyzing rtl8723d_config.dms
>>> Signature: 0x8723ab55
>>> Data length: 41
>>> len=1 offset=f4,{ 01 }
>>> len=2 offset=f6,{ 81 00 }
>>> len=2 offset=fa,{ 12 80 }
>>> len=16 offset=0c,{ 02 80 92 04 50 c5 ea 19 e1 1b fd af 5f 01 a4 0b }
>>> len=1 offset=d9,{ 0f }
>>> len=1 offset=e4,{ 08 }
>>> Analyzing rtl8822b_config.bin
>>> Signature: 0x8723ab55
>>> Data length: 8
>>> len=1 offset=d9,{ 0f }
>>> len=1 offset=e4,{ 08 }
>>>
>>> The first two are some UART based ones and the last one is USB based.
>>>
>>> So the 0x3c offset seems to be the BD_ADDR and 0x0c offset is the UART configuration. It would be good to know which settings the other ones control.
> this matches my findings from the hciattach_rtk tool for rtl8723bs_bt
> and rtl8723ds_bt
>
>>> Also the 16 octet UART config blob seems to be decoded like this:
>>>
>>> uart_config {
>>> le32 baudrate;
>>> u8[8] reserved1;
>>> u8 flowctl;
>>> u8[3] reserved2;
>>> }
>>>
> I could not find this in any of the rtl8723bs_bt or rtl8723ds_bt sources
> so thank you for sharing this (even though this descriptin is missing
> a few bytes)!
>
>>> Actually hciattach_rtk just takes the baud rate and and hardware flow control bit out of this file. That is clearly two things that are better written in plain text in the DT file.
> this matches my findings apart from that hciattach_rtk also appends
> the config blob to the "firmware patch" that is being uploaded to the
> device
> if you are brave you can have a look at [0]
so this makes kinda sense. If you do not load the config file, you end up with the ROM defaults for all their “efuse” settings. Since it is essentially just a memory area where the config file overwrites defaults, this is all fine.
They have to extract the UART settings out of it since they also need to handled special during the firmware loading / boot process. This is also means if there is no config file or not UART settings in the config file, the process should use the default values.
>> so this is actually some funny stuff if you start to understand it.
>>
>> Analyzing rtl8723b_config.dms
>> Signature: 0x8723ab55
>> Data len: 38
>> len=8 offset=00f4,{ 01 00 00 00 05 50 00 00 }
>> len=16 offset=000c,{ 02 80 92 04 50 c5 ea 19 e1 1b f1 af 5f 01 a4 0b },UART_CONFIG
>> len=1 offset=0027,{ 63 }
>> len=1 offset=00fe,{ 01 }
>> Analyzing rtl8723d_config_1000000_noflow.dms
>> Signature: 0x8723ab55
>> Data len: 41
>> len=1 offset=00f4,{ 01 }
>> len=2 offset=00f6,{ 81 00 }
>> len=2 offset=00fa,{ 12 80 }
>> len=16 offset=000c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b },UART_CONFIG
>> len=1 offset=00d9,{ 0f }
>> len=1 offset=00e4,{ 08 }
>>
>> Seems like Realtek really defines memory offsets in this file and they can be defined in various different ways.
> wow, that is interesting - I was wondering why they called it "offset"
> instead of "config_id" (or something similar)
It makes sense, they are just “patching” their configuration space.
>> So 00f4,{ 01 00 00 00 05 50 00 00 } defines the whole PCM settings for interface 1 and 2. And 00f4,{ 01 } + 00f6,{ 81 00 } + 00fa,{ 12 80 } is the same PCM settings, but with only pieces of it defined.
>>
>> This also means a 000c,{ 04 50 00 00 50 } for just setting the baud rate is as valid if flow control defaults to off and all other values are actual defaults. So code inside hciattach_rtk.c is also kind faulty on how it handles the flow control bit. It works if the config files all have offset 000c in it, but if not, then they are going funky.
>>
>> Since these are efuse settings, I really wonder if there is just a HCI vendor command to read out the defaults and we use that to compare. And what I would really like to know is what these settings are suppose to change. Since even for USB, we are actually not even applying them.
> the idea with the vendor command to read out existing memory is interesting
>
> based on the code in "btrtl.c" it seems that we are applying the
> settings from the config blob.
> we are simply appending it at the end of the "firmware patch", see [1]
I am pretty sure it is all the same for USB and UART.
>> Anyway, I am certain that for Realtek UART devices, we just want to specify max-speed DT property like we do with the others. And then maybe a flow-control DT property to control that one (following what nfcmrvl.txt does). We can use the rtlfw tool that I wrote to extract the values from Realtek provided config files. Frankly the PCM settings we have to deal with as well at some point. But that is also missing for Broadcom and others.
> just to make sure I understand you correctly: would you generate the
> config blob in-memory (in a function in btrtl.c) and hardcode all
> unknown bits (the reserved bits in the UART config entry for example)
> or would you mandate that a config blob is present (so
> request_firmware can fetch it) for "high-speed" operation?
>
> for PCM: I cannot test that anyways since the Amlogic platform does
> not have audio support yet
>
>> Also if there is no config file, we should be able to just assume no flow control, 115200 baud rate and H:5 as protocol. That means that almost all chips will just work. They are just slow since we do not deal with the max-speed setting.
> my problem so far was that uploading the firmware patch without
> appending a config blob (see [1]) broke the UART communication (it was
> not 115200 baud as before, and I also tried other speeds such as
> 1500000 and 1000000 baud - neither worked)
> however, extracting the existing values from the efuse using a vendor
> command might give a hint why (maybe they are fallling back to some
> exotic baudrate in that case, etc…)
Have you tried to just create a config file with just the signature 0x8723ab55 and data len set to 0. So essentially an empty config file. I am curious if such a default config is good enough to get things working. Or if they have actually faulty defaults in their ROM that need overwriting to make things fly.
Btw. I am curious why the UART speed is in that file anyway if we have to change it anyway via HCI command. So normally the boot UART speed is what is needed to know, not what final high speed we are using. So this is a bit odd.
I wonder if just loading the config file if present and then use a DT property max-speed to select the speed (even if it is different from the config file).
For me this means that we can have a general config file on disk, but actually allow overwriting the UART speed via DT. Otherwise we might just want to add extra offset entries for the config blob loading from disk and extend it with the DT values. So it would be some sort of overload.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
From: Hans de Goede @ 2018-01-09 8:48 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA, ACPI Devel Maling List,
stable, Leif Liddy, Matthias Kaehlcke, Brian Norris, Daniel Drake,
Kai-Heng Feng
In-Reply-To: <28EBA1CE-4DFF-4828-BF00-99BC2B487484-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Hi,
On 08-01-18 21:46, Marcel Holtmann wrote:
> Hi Hans,
>
>> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
>> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>>
>> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
>> has several issues (see the original commit message). An added advantage
>> of moving over to the USB-core reset-resume handling is that it also
>> disables autosuspend for these devices, which is similarly broken on these.
>>
>> But there are 2 issues with this approach:
>> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
>> devices.
>> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
>> added to usb/core/quirks.c and if we fix the Realtek case the same way
>> we need to add an additional 14 entries. So in essence we need to
>> duplicate a large part of the usb_device_id table in btusb.c in
>> usb/core/quirks.c and manually keep them in sync.
>>
>> This commit instead restores setting a reset-resume quirk for QCA devices
>> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
>>
>> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
>> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
>> usb_device.
>>
>> This commit also moves the BTUSB_REALTEK case over to directly setting the
>> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
>> BTUSB_RESET_RESUME code.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
>> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: Leif Liddy <leif.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> Cc: Kai-Heng Feng <kai.heng.feng-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Note:
>> 1) Once this has been merged, the 2 commits adding QCA device entries to
>> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next.
>> 2) I don't have any of the affected devices, please test
>> ---
>> drivers/bluetooth/btusb.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> patch has been applied to bluetooth-next tree.
I see that you've also dropped the 2 commits adding QCA device entries to
drivers/usb/core/quirks.c, great. Thank you.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH RFC 4/7] i2c: Add device tree bindings for GENI I2C Controller
From: Karthik Ramasubramanian @ 2018-01-09 0:33 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm, linux-i2c, linux-serial, linux-doc, devicetree,
andy.gross, david.brown, mark.rutland, corbet, wsa, gregkh,
jslaby, Sagar Dharia
In-Reply-To: <20180102155149.f2wzjvdhoi6cne4x@rob-hp-laptop>
On 1/2/2018 8:51 AM, Rob Herring wrote:
> On Wed, Dec 27, 2017 at 09:27:23AM -0700, Karthikeyan Ramasubramanian wrote:
>> Add device tree binding support for I2C Controller in GENI based
>> QUP Wrapper.
>>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> ---
>> .../devicetree/bindings/i2c/i2c-qcom-geni.txt | 39 ++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>> new file mode 100644
>> index 0000000..d2fa9ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>> @@ -0,0 +1,39 @@
>> +Qualcomm Technologies Inc. GENI based I2C Controller driver
>> +
>> +Required properties:
>> + - compatible: Should be:
>> + * "qcom,i2c-geni.
>
> Only 1 version?
The Serial Engine used by I2C protocol has the same version as the QUP
h/w version. The QUP Wrapper driver exposes an interface function to get
the h/w version so that I2C controller driver can support
version-specific operations, if any.
>
>> + - reg: Should contain QUP register address and length.
>> + - interrupts: Should contain I2C interrupt.
>> + - clocks: Serial engine core clock, and AHB clocks needed by the device.
>
> Are there really clocks for a firmware based device or these are just
> clocks in the parent serial engine?
The clocks are required to derive the protocol clock. The clocks are
also required by the Serial Engine to access the System Memory during
DMA mode of operation.
>
>> + - pinctrl-names/pinctrl-0/1: The GPIOs assigned to this core. The names
>> + should be "active" and "sleep" for the pin confuguration when core is active
>> + or when entering sleep state.
>> + - #address-cells: Should be <1> Address cells for i2c device address
>> + - #size-cells: Should be <0> as i2c addresses have no size component
>> + - qcom,wrapper-core: Wrapper QUP core containing this I2C controller.
>
> Probably these devices should be child nodes of the QUP core node.
I will update it in the next patch.
>
>> +
>> +Optional property:
>> + - qcom,clk-freq-out : Desired I2C bus clock frequency in Hz.
>> + When missing default to 400000Hz.
>
> There's a standard property for this.
I will update to use the standard property.
>
>> +
>> +Child nodes should conform to i2c bus binding.
>> +
>> +Example:
>> +
>> +i2c@a94000 {
>> + compatible = "qcom,i2c-geni";
>> + reg = <0xa94000 0x4000>;
>> + interrupts = <GIC_SPI 358 0>;
>> + clock-names = "se-clk", "m-ahb", "s-ahb";
>> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S5_CLK>,
>> + <&clock_gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
>> + <&clock_gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&qup_1_i2c_5_active>;
>> + pinctrl-1 = <&qup_1_i2c_5_sleep>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + qcom,wrapper-core = <&qup_0>;
>> + qcom,clk-freq-out = <400000>;
>> +};
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH RFC 2/7] soc: qcom: Add device tree binding for GENI SE
From: Karthik Ramasubramanian @ 2018-01-08 23:59 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
andy.gross-QSEj5FYQhm4dnm+yROfE0A,
david.brown-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
corbet-T1hC0tSOHrs, wsa-z923LK4zBo2bacvFa/9K2g,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k
In-Reply-To: <20180102154734.pbomm5rehyquemp3@rob-hp-laptop>
On 1/2/2018 8:47 AM, Rob Herring wrote:
> On Wed, Dec 27, 2017 at 09:27:21AM -0700, Karthikeyan Ramasubramanian wrote:
>> Add device tree binding support for the QCOM GENI SE driver.
>
> Also, "dt-bindings: ..." is the preferred subject prefix. Same for the
> rest of the series.
I will update the subject-prefix.
>
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC 2/7] soc: qcom: Add device tree binding for GENI SE
From: Karthik Ramasubramanian @ 2018-01-08 23:57 UTC (permalink / raw)
To: Rob Herring
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
andy.gross-QSEj5FYQhm4dnm+yROfE0A,
david.brown-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
corbet-T1hC0tSOHrs, wsa-z923LK4zBo2bacvFa/9K2g,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k
In-Reply-To: <20180102154629.s6rni5r3big2g5sv@rob-hp-laptop>
On 1/2/2018 8:46 AM, Rob Herring wrote:
> On Wed, Dec 27, 2017 at 09:27:21AM -0700, Karthikeyan Ramasubramanian wrote:
>> Add device tree binding support for the QCOM GENI SE driver.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> new file mode 100644
>> index 0000000..5108b62
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -0,0 +1,15 @@
>> +Qualcomm Technologies, Inc. GENI Serial Engine Driver
>> +
>> +GENI Serial Engine Driver manages the GENI firmware based Qualcomm Universal
>> +Peripheral (QUP) Wrapper. GENI SE Driver also manages the common aspects of
>> +individual Serial Engines that composes the QUP Wrapper.
>
> Bindings describe h/w, not drivers.
I will update the bindings to describe just the h/w and remove any
driver description.
>
>> +
>> +Required properties:
>> +- compatible: Must be "qcom,geni-se-qup".
>
> Only one version of the h/w?
There is more than one hardware version. The h/w provides registers to
identify the hardware version. The driver currently uses those registers
to support any version-specific operations. So I am not sure if the
compatible field needs to reflect the h/w version.
>
>> +- reg: Must contain QUP register address and length.
>> +
>> +Example:
>> + qup_0: qcom,geni_se_qup_0@8c0000 {
>
> Don't use '_' in node names.
I will update the bindings to not use '_'.
>
>> + compatible = "qcom,geni-se-qup";
>> + reg = <0x8c0000 0x6000>;
>> + }
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
From: Marcel Holtmann @ 2018-01-08 20:46 UTC (permalink / raw)
To: Hans de Goede
Cc: Gustavo F. Padovan, Johan Hedberg, open list:BLUETOOTH DRIVERS,
linux-serial, ACPI Devel Maling List, stable, Leif Liddy,
Matthias Kaehlcke, Brian Norris, Daniel Drake, Kai-Heng Feng
In-Reply-To: <20180108094416.4789-1-hdegoede@redhat.com>
Hi Hans,
> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>
> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> has several issues (see the original commit message). An added advantage
> of moving over to the USB-core reset-resume handling is that it also
> disables autosuspend for these devices, which is similarly broken on these.
>
> But there are 2 issues with this approach:
> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
> devices.
> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
> added to usb/core/quirks.c and if we fix the Realtek case the same way
> we need to add an additional 14 entries. So in essence we need to
> duplicate a large part of the usb_device_id table in btusb.c in
> usb/core/quirks.c and manually keep them in sync.
>
> This commit instead restores setting a reset-resume quirk for QCA devices
> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
>
> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
> usb_device.
>
> This commit also moves the BTUSB_REALTEK case over to directly setting the
> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
> BTUSB_RESET_RESUME code.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> Cc: stable@vger.kernel.org
> Cc: Leif Liddy <leif.linux@gmail.com>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Daniel Drake <drake@endlessm.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note:
> 1) Once this has been merged, the 2 commits adding QCA device entries to
> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next.
> 2) I don't have any of the affected devices, please test
> ---
> drivers/bluetooth/btusb.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
From: Hans de Goede @ 2018-01-08 17:49 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth, linux-serial, linux-acpi, stable, Leif Liddy,
Matthias Kaehlcke, Brian Norris, Daniel Drake, Kai-Heng Feng
In-Reply-To: <20180108094416.4789-1-hdegoede@redhat.com>
Hi,
On 08-01-18 10:44, Hans de Goede wrote:
> Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices,
> instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c.
>
> This was done because the DIY BTUSB_RESET_RESUME reset-resume handling
> has several issues (see the original commit message). An added advantage
> of moving over to the USB-core reset-resume handling is that it also
> disables autosuspend for these devices, which is similarly broken on these.
>
> But there are 2 issues with this approach:
> 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek
> devices.
> 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been
> added to usb/core/quirks.c and if we fix the Realtek case the same way
> we need to add an additional 14 entries. So in essence we need to
> duplicate a large part of the usb_device_id table in btusb.c in
> usb/core/quirks.c and manually keep them in sync.
>
> This commit instead restores setting a reset-resume quirk for QCA devices
> in the btusb.c code, avoiding the duplicate usb_device_id table problem.
>
> This commit avoids the problems with the original DIY BTUSB_RESET_RESUME
> code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the
> usb_device.
>
> This commit also moves the BTUSB_REALTEK case over to directly setting the
> USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused
> BTUSB_RESET_RESUME code.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836
> Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"")
> Cc: stable@vger.kernel.org
> Cc: Leif Liddy <leif.linux@gmail.com>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Daniel Drake <drake@endlessm.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note:
> 1) Once this has been merged, the 2 commits adding QCA device entries to
> drivers/usb/core/quirks.c should be reverted or dropped from bluetooth-next.
> 2) I don't have any of the affected devices, please test
I've asked the reporter of:
https://bugzilla.redhat.com/show_bug.cgi?id=1514836
To test a kernel with this patch added and he confirms that this fixes
the autosuspend related issues he was seeing, so this is no longer untested.
Regards,
Hans
> ---
> drivers/bluetooth/btusb.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4764100a5888..c4689f03220f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -23,6 +23,7 @@
>
> #include <linux/module.h>
> #include <linux/usb.h>
> +#include <linux/usb/quirks.h>
> #include <linux/firmware.h>
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> @@ -388,9 +389,8 @@ static const struct usb_device_id blacklist_table[] = {
> #define BTUSB_FIRMWARE_LOADED 7
> #define BTUSB_FIRMWARE_FAILED 8
> #define BTUSB_BOOTING 9
> -#define BTUSB_RESET_RESUME 10
> -#define BTUSB_DIAG_RUNNING 11
> -#define BTUSB_OOB_WAKE_ENABLED 12
> +#define BTUSB_DIAG_RUNNING 10
> +#define BTUSB_OOB_WAKE_ENABLED 11
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -3118,6 +3118,12 @@ static int btusb_probe(struct usb_interface *intf,
> if (id->driver_info & BTUSB_QCA_ROME) {
> data->setup_on_usb = btusb_setup_qca;
> hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
> +
> + /* QCA Rome devices lose their updated firmware over suspend,
> + * but the USB hub doesn't notice any status change.
> + * explicitly request a device reset on resume.
> + */
> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> }
>
> #ifdef CONFIG_BT_HCIBTUSB_RTL
> @@ -3128,7 +3134,7 @@ static int btusb_probe(struct usb_interface *intf,
> * but the USB hub doesn't notice any status change.
> * Explicitly request a device reset on resume.
> */
> - set_bit(BTUSB_RESET_RESUME, &data->flags);
> + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> }
> #endif
>
> @@ -3297,14 +3303,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> enable_irq(data->oob_wake_irq);
> }
>
> - /* Optionally request a device reset on resume, but only when
> - * wakeups are disabled. If wakeups are enabled we assume the
> - * device will stay powered up throughout suspend.
> - */
> - if (test_bit(BTUSB_RESET_RESUME, &data->flags) &&
> - !device_may_wakeup(&data->udev->dev))
> - data->udev->reset_resume = 1;
> -
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Arnd Bergmann @ 2018-01-08 16:30 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Greentime Hu, Greentime, Rick Chen, Rick Chen,
Linux Kernel Mailing List, Linus Walleij, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, netdev,
Vincent Chen, DTML, Al Viro, David Howells, Will Deacon,
linux-serial, John Stultz
In-Reply-To: <78dc7813-524d-c108-0e3d-516f8f4dabfe@linaro.org>
On Mon, Jan 8, 2018 at 5:08 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 08/01/2018 16:26, Arnd Bergmann wrote:
>> On Fri, Jan 5, 2018 at 10:31 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>>
>>> etc ...
>>
>> I'd actually prefer to not do it for ARM either: Most other subsystems
>> don't do that, and I don't see a strong reason why clocksource should
>> be special here.
>
> The majority doing the opposite does not mean it is right.
>
> Do you know which clock belongs to which board ? Who will unselect a
> clock ? I'm pretty sure nobody. Everyone relies on the platform Kconfig
> and expect it to select the right drivers.
The point is that there is no platform specific Kconfig that could select
it, as the concept doesn't make a lot of sense here.
If you require the driver to always be selected by the
architecture code, it adds a little bit of bloat on systems
that don't need it. This is possible, but I think it's preferable
to give users a way of tuning a kernel for a particular chip.
> We don't expect the hackers to have a deep knowledge of the hardware and
> the driver dependencies. It is very convenient to not care about that
> and let the platform's Kconfig to select the right drivers.
>
> And that is the behavior I would like to keep.
I'm not worried about the driver bloating the kernel here when it
isn't disabled, this is no different from enabling the USB controller
by default for a board that doesn't have a USB connector, or
enabling ten different pinctrl drivers for all members of a chip
family even though you know which particular chip you are
running on.
>> Selecting 'TIMER_OF' from the individual drivers that need it (as you
>> suggest) makes sense, but I think for ARM we treat SoC families
>> as a bit too special, in the end they are for the most part collections
>> of individual hardware blocks that may or may not be present on
>> some chip.
>>
>> In case of risc-v and nds32, I expect that the separation will be
>> even less visibile in the hardware, as a typical model here is
>> that one company designs SoCs for multiple customers that each
>> have different requirements. Some of them may have one
>> timer and some have another timer or multiple timers, but there
>> is no strict separation between SoC families as I understand.
>> Here we'd be better off not having a per-SoC Kconfig option at
>> all, just a generic defconfig that enables all the drivers that might
>> be used, and integrators can have a defconfig file that only
>> enables the stuff they actually use on a given chip.
>
> Yes, the result is the same, the option is not showed in the menu.
>
> However, I can understand it could be interesting to have the ability to
> unselect a driver for experts.
>
> I'm wondering if we can create a bool_expert which shows up only when
> CONFIG_EXPERT is set.
Having a dependency on CONFIG_EXPERT means that a more
users will end up having to set that for a shipping system. It's
something we can do (even without a special Kconfig keyword),
but it should be used carefully for stuff that 99% of the users want
to enable.
Why not just:
config CLKSRC_ATCPIT100
bool "Clocksource for AE3XX platform"
depends on NDS32 || COMPILE_TEST
depends on HAS_IOMEM
default NDS32
help
This option enables support for the Andestech AE3XX platform timers.
That way, it's simply enabled on NDS32 by default, but just as easy
to disable for systems that don't need it.
Arnd
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox