* 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
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Daniel Lezcano @ 2018-01-08 16:08 UTC (permalink / raw)
To: Arnd Bergmann
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: <CAK8P3a2NVj_M5eHT92fJYNBG3x9juVnHW6uV32p3-ytfqjARxg@mail.gmail.com>
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:
>>>> No. Can't you add in arch/ndis32/Kconfig ?
>>>>
>>>> +select TIMER_ATCPIT100
>>>>
>>>> Like:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50
>>>
>>> IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in
>>> arch/nds32/Kconfig because it is part of SoC instead of CPU.
>>> If we change to another SoC with another timer, we need to select
>>> another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100.
>>> It seems more flexible to be selected in driver layer.
>>>
>>> It seems to be the timer is part of the arch to be selected in arch's Kconfig.
>>> arch/arc/Kconfig: select ARC_TIMERS
>>> arch/arc/Kconfig: select ARC_TIMERS_64BIT
>>> arch/arm/Kconfig: select ARM_ARCH_TIMER
>>> arch/arm64/Kconfig: select ARM_ARCH_TIMER
>>> arch/blackfin/Kconfig: select BFIN_GPTIMERS
>>
>> No, the timer must be selected from the arch/soc's or whatever Kconfig.
>> Not in the clocksource's Kconfig.
>>
>> eg.
>>
>> on ARM:
>>
>> arch/arm/mach-vt8500/Kconfig: select VT8500_TIMER
>> arch/arm/mach-bcm/Kconfig: select BCM_KONA_TIMER
>> arch/arm/mach-actions/Kconfig: select OWL_TIMER
>> arch/arm/mach-digicolor/Kconfig: select DIGICOLOR_TIMER
>>
>> etc ...
>>
>> on ARM64:
>>
>> arch/arm64/Kconfig.platforms: select OWL_TIMER
>> arch/arm64/Kconfig.platforms: select ARM_TIMER_SP804
>> arch/arm64/Kconfig.platforms: select MTK_TIMER
>>
>> 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.
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.
> 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.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Arnd Bergmann @ 2018-01-08 15:26 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
In-Reply-To: <1e75edb3-8f7b-796c-6871-1612b027050e@linaro.org>
On Fri, Jan 5, 2018 at 10:31 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>>> No. Can't you add in arch/ndis32/Kconfig ?
>>>
>>> +select TIMER_ATCPIT100
>>>
>>> Like:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50
>>
>> IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in
>> arch/nds32/Kconfig because it is part of SoC instead of CPU.
>> If we change to another SoC with another timer, we need to select
>> another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100.
>> It seems more flexible to be selected in driver layer.
>>
>> It seems to be the timer is part of the arch to be selected in arch's Kconfig.
>> arch/arc/Kconfig: select ARC_TIMERS
>> arch/arc/Kconfig: select ARC_TIMERS_64BIT
>> arch/arm/Kconfig: select ARM_ARCH_TIMER
>> arch/arm64/Kconfig: select ARM_ARCH_TIMER
>> arch/blackfin/Kconfig: select BFIN_GPTIMERS
>
> No, the timer must be selected from the arch/soc's or whatever Kconfig.
> Not in the clocksource's Kconfig.
>
> eg.
>
> on ARM:
>
> arch/arm/mach-vt8500/Kconfig: select VT8500_TIMER
> arch/arm/mach-bcm/Kconfig: select BCM_KONA_TIMER
> arch/arm/mach-actions/Kconfig: select OWL_TIMER
> arch/arm/mach-digicolor/Kconfig: select DIGICOLOR_TIMER
>
> etc ...
>
> on ARM64:
>
> arch/arm64/Kconfig.platforms: select OWL_TIMER
> arch/arm64/Kconfig.platforms: select ARM_TIMER_SP804
> arch/arm64/Kconfig.platforms: select MTK_TIMER
>
> 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.
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.
Arnd
^ permalink raw reply
* Re: [PATCH 0/2] serdev: bus-code clean ups
From: Johan Hovold @ 2018-01-08 13:50 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman, Hans de Goede
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Johan Hovold
In-Reply-To: <20180108124233.26729-1-johan@kernel.org>
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
> 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(-)
Johan
^ permalink raw reply
* Re: [PATCH v2 1/2] acpi, spcr: Make SPCR avialable to other architectures
From: Prarit Bhargava @ 2018-01-08 12:46 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-acpi, linux-doc, linux-kernel, linux-arm-kernel, linux-pm,
linux-serial, Bhupesh Sharma, Lv Zheng, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86, Jonathan Corbet,
Catalin Marinas, Will Deacon, Rafael J. Wysocki, Timur Tabi,
graeme.gregory, mark.salter
In-Reply-To: <20171213124533.GA32362@red-moon>
[Sorry everyone for the late response, I went away on vacation and pushed this
off until I returned.]
On 12/13/2017 07:45 AM, Lorenzo Pieralisi wrote:
> [+Mark, Graeme]
>
> In $SUBJECT, s/avialable/available
>
> On Mon, Dec 11, 2017 at 10:50:58AM -0500, Prarit Bhargava wrote:
>> Other architectures can use SPCR to setup an early console or console
>> but the current code is ARM64 specific.
>
> I see nothing ARM64 specific in current code (apart from some
> ACPICA macros with an ARM tag in them) please explain to me
> what's preventing you to reuse current code on x86.
Ah, I didn't notice that. I thought the ACPICA macros were ARM specific. I'll
rework this patchset with that in mind.
>
>> Change the name of parse_spcr() to acpi_parse_spcr(). Add a weak
>> function acpi_arch_setup_console() that can be used for arch-specific
>> setup. Move flags into ACPI code. Update the Documention on the use of
>> the SPCR.
>>
>> [v2]: Don't return an error in the baud_rate check of acpi_parse_spcr().
>> Keep ACPI_SPCR_TABLE selected for ARM64. Fix 8-bit port access width
>> mmio value. Move baud rate check earlier.
>
> This does not belong in the commit log.
Yes, but some maintainers like to see what changed between v1 & v2.
>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-pm@vger.kernel.org
>> Cc: linux-acpi@vger.kernel.org
>> Cc: linux-serial@vger.kernel.org
>> Cc: Bhupesh Sharma <bhsharma@redhat.com>
>> Cc: Lv Zheng <lv.zheng@intel.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Timur Tabi <timur@codeaurora.org>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 6 +-
>> arch/arm64/kernel/acpi.c | 128 ++++++++++++++++-
>> drivers/acpi/Kconfig | 7 +-
>> drivers/acpi/spcr.c | 175 ++++++------------------
>> drivers/tty/serial/earlycon.c | 15 +-
>> include/linux/acpi.h | 11 +-
>> include/linux/serial_core.h | 2 -
>> 7 files changed, 184 insertions(+), 160 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 6571fbfdb2a1..0d173289c67e 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -914,9 +914,9 @@
>>
>> earlycon= [KNL] Output early console device and options.
>>
>> - When used with no options, the early console is
>> - determined by the stdout-path property in device
>> - tree's chosen node.
>> + [ARM64] The early console is determined by the
>> + stdout-path property in device tree's chosen node,
>> + or determined by the ACPI SPCR table.
>>
>> cdns,<addr>[,options]
>> Start an early, polled-mode console on a Cadence
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index b3162715ed78..b3e33bbdf3b7 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -25,7 +25,6 @@
>> #include <linux/memblock.h>
>> #include <linux/of_fdt.h>
>> #include <linux/smp.h>
>> -#include <linux/serial_core.h>
>>
>> #include <asm/cputype.h>
>> #include <asm/cpu_ops.h>
>> @@ -177,6 +176,128 @@ static int __init acpi_fadt_sanity_check(void)
>> return ret;
>> }
>>
>> +/*
>> + * Erratum 44 for QDF2432v1 and QDF2400v1 SoCs describes the BUSY bit as
>> + * occasionally getting stuck as 1. To avoid the potential for a hang, check
>> + * TXFE == 0 instead of BUSY == 1. This may not be suitable for all UART
>> + * implementations, so only do so if an affected platform is detected in
>> + * acpi_parse_spcr().
>> + */
>> +bool qdf2400_e44_present;
>> +EXPORT_SYMBOL(qdf2400_e44_present);
>
> My eyes, this is horrible but it is not introduced by this patch. It
> would have been much better if:
>
> drivers/tty/serial/amba-pl011.c
>
> parsed the SPCR table (again) to detect it instead of relying on this
> horrible exported flag.
It looks like Timur & you had a discussion and the current consensus is to keep
the code the same.
>
>> +
>> +/*
>> + * Some Qualcomm Datacenter Technologies SoCs have a defective UART BUSY bit.
>> + * Detect them by examining the OEM fields in the SPCR header, similar to PCI
>> + * quirk detection in pci_mcfg.c.
>> + */
>> +static bool qdf2400_erratum_44_present(struct acpi_table_header *h)
>> +{
>> + if (memcmp(h->oem_id, "QCOM ", ACPI_OEM_ID_SIZE))
>> + return false;
>> +
>> + if (!memcmp(h->oem_table_id, "QDF2432 ", ACPI_OEM_TABLE_ID_SIZE))
>> + return true;
>> +
>> + if (!memcmp(h->oem_table_id, "QDF2400 ", ACPI_OEM_TABLE_ID_SIZE) &&
>> + h->oem_revision == 1)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +/*
>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>> + * register aligned to 32-bit. In addition, the BIOS also encoded the
>> + * access width to be 8 bits. This function detects this errata condition.
>> + */
>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>> +{
>> + bool xgene_8250 = false;
>> +
>> + if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>> + return false;
>> +
>> + if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
>> + memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE))
>> + return false;
>> +
>> + if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>> + xgene_8250 = true;
>> +
>> + if (!memcmp(tb->header.oem_table_id, "ProLiant",
>> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1)
>> + xgene_8250 = true;
>> +
>> + return xgene_8250;
>> +}
>> +
>> +int acpi_arch_setup_console(struct acpi_table_spcr *table,
>> + char *opts, char *uart, char *iotype,
>> + int baud_rate, bool earlycon)
>> +{
>> + if (table->header.revision < 2) {
>> + pr_err("wrong table version\n");
>> + return -ENOENT;
>> + }
>> +
>> + switch (table->interface_type) {
>> + case ACPI_DBG2_ARM_SBSA_32BIT:
>> + snprintf(iotype, ACPI_SPCR_BUF_SIZE, "mmio32");
>> + /* fall through */
>> + case ACPI_DBG2_ARM_PL011:
>> + case ACPI_DBG2_ARM_SBSA_GENERIC:
>> + case ACPI_DBG2_BCM2835:
>> + snprintf(uart, ACPI_SPCR_BUF_SIZE, "pl011");
>> + break;
>> + default:
>> + if (strlen(uart) == 0)
>> + return -ENOENT;
>> + }
>> +
>> + /*
>> + * If the E44 erratum is required, then we need to tell the pl011
>> + * driver to implement the work-around.
>> + *
>> + * The global variable is used by the probe function when it
>> + * creates the UARTs, whether or not they're used as a console.
>> + *
>> + * If the user specifies "traditional" earlycon, the qdf2400_e44
>> + * console name matches the EARLYCON_DECLARE() statement, and
>> + * SPCR is not used. Parameter "earlycon" is false.
>> + *
>> + * If the user specifies "SPCR" earlycon, then we need to update
>> + * the console name so that it also says "qdf2400_e44". Parameter
>> + * "earlycon" is true.
>> + *
>> + * For consistency, if we change the console name, then we do it
>> + * for everyone, not just earlycon.
>> + */
>> + if (qdf2400_erratum_44_present(&table->header)) {
>> + qdf2400_e44_present = true;
>> + if (earlycon)
>> + snprintf(uart, ACPI_SPCR_BUF_SIZE, "qdf2400_e44");
>> + }
>> +
>> + if (xgene_8250_erratum_present(table)) {
>> + snprintf(iotype, ACPI_SPCR_BUF_SIZE, "mmio32");
>> +
>> + /* for xgene v1 and v2 we don't know the clock rate of the
>> + * UART so don't attempt to change to the baud rate state
>> + * in the table because driver cannot calculate the dividers
>> + */
>> + snprintf(opts, ACPI_SPCR_OPTS_SIZE, "%s,%s,0x%llx", uart,
>> + iotype, table->serial_port.address);
>> + } else {
>> + snprintf(opts, ACPI_SPCR_OPTS_SIZE, "%s,%s,0x%llx,%d", uart,
>> + iotype, table->serial_port.address, baud_rate);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(acpi_arch_setup_console);
>
> EXPORT_SYMBOL() ? Why ?
>
> BTW, why do we need an arch hook ? I do not see anything that prevents
> you from using this code on x86 systems - is there anything arch
> specific in the SPCR specification itself ?
See above comment. But also keep in mind I have seen a lot of x86 systems that
do not define the ACPI table version correctly. The table version is 0 or 1,
and the current spec says it must be 2. It looks like ARM requires 2.
>
>> +
>> /*
>> * acpi_boot_table_init() called from setup_arch(), always.
>> * 1. find RSDP and get its address, and then find XSDT
>> @@ -230,10 +351,11 @@ void __init acpi_boot_table_init(void)
>>
>> done:
>> if (acpi_disabled) {
>> - if (earlycon_init_is_deferred)
>> + if (console_acpi_spcr_enable)
>> early_init_dt_scan_chosen_stdout();
>> } else {
>> - parse_spcr(earlycon_init_is_deferred);
>> + /* Always enable the ACPI SPCR console */
>> + acpi_parse_spcr(console_acpi_spcr_enable);
>> if (IS_ENABLED(CONFIG_ACPI_BGRT))
>> acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>> }
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 46505396869e..9ae98eeada76 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -79,7 +79,12 @@ config ACPI_DEBUGGER_USER
>> endif
>>
>> config ACPI_SPCR_TABLE
>> - bool
>> + bool "ACPI Serial Port Console Redirection Support"
>> + default y if ARM64
>
> You need to remove the selection in arch/arm64 then. Also, moving away
> from a non-visible config may have consequences on ARM64, Graeme and
> Mark are more familiar with the SPCR dependencies so please chime in.
Ok, I'll double check this.
>
>> + help
>> + Enable support for Serial Port Console Redirection (SPCR) Table.
>> + This table provides information about the configuration of the
>> + earlycon console.
>>
>> config ACPI_LPIT
>> bool
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>> index 324b35bfe781..f4bb8110e404 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -16,65 +16,18 @@
>> #include <linux/kernel.h>
>> #include <linux/serial_core.h>
>>
>> -/*
>> - * Erratum 44 for QDF2432v1 and QDF2400v1 SoCs describes the BUSY bit as
>> - * occasionally getting stuck as 1. To avoid the potential for a hang, check
>> - * TXFE == 0 instead of BUSY == 1. This may not be suitable for all UART
>> - * implementations, so only do so if an affected platform is detected in
>> - * parse_spcr().
>> - */
>> -bool qdf2400_e44_present;
>> -EXPORT_SYMBOL(qdf2400_e44_present);
>> -
>> -/*
>> - * Some Qualcomm Datacenter Technologies SoCs have a defective UART BUSY bit.
>> - * Detect them by examining the OEM fields in the SPCR header, similiar to PCI
>> - * quirk detection in pci_mcfg.c.
>> - */
>> -static bool qdf2400_erratum_44_present(struct acpi_table_header *h)
>> -{
>> - if (memcmp(h->oem_id, "QCOM ", ACPI_OEM_ID_SIZE))
>> - return false;
>> -
>> - if (!memcmp(h->oem_table_id, "QDF2432 ", ACPI_OEM_TABLE_ID_SIZE))
>> - return true;
>> -
>> - if (!memcmp(h->oem_table_id, "QDF2400 ", ACPI_OEM_TABLE_ID_SIZE) &&
>> - h->oem_revision == 1)
>> - return true;
>> -
>> - return false;
>> -}
>> -
>> -/*
>> - * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>> - * register aligned to 32-bit. In addition, the BIOS also encoded the
>> - * access width to be 8 bits. This function detects this errata condition.
>> - */
>> -static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>> +int __weak acpi_arch_setup_console(struct acpi_table_spcr *table,
>> + char *opts, char *uart, char *iotype,
>> + int baud_rate, bool earlycon)
>> {
>> - bool xgene_8250 = false;
>> -
>> - if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>> - return false;
>> -
>> - if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
>> - memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE))
>> - return false;
>> -
>> - if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>> - ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>> - xgene_8250 = true;
>> -
>> - if (!memcmp(tb->header.oem_table_id, "ProLiant",
>> - ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1)
>> - xgene_8250 = true;
>> -
>> - return xgene_8250;
>> + snprintf(opts, ACPI_SPCR_OPTS_SIZE, "%s,%s,0x%llx,%d", uart, iotype,
>> + table->serial_port.address, baud_rate);
>> + return 0;
>> }
>>
>> +bool console_acpi_spcr_enable __initdata;
>> /**
>> - * parse_spcr() - parse ACPI SPCR table and add preferred console
>> + * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
>> *
>> * @earlycon: set up earlycon for the console specified by the table
>> *
>> @@ -86,13 +39,13 @@ static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>> * from arch initialization code as soon as the DT/ACPI decision is made.
>> *
>> */
>> -int __init parse_spcr(bool earlycon)
>> +int __init acpi_parse_spcr(bool earlycon)
>> {
>> - static char opts[64];
>> + static char opts[ACPI_SPCR_OPTS_SIZE];
>> + static char uart[ACPI_SPCR_BUF_SIZE];
>> + static char iotype[ACPI_SPCR_BUF_SIZE];
>> struct acpi_table_spcr *table;
>> acpi_status status;
>> - char *uart;
>> - char *iotype;
>> int baud_rate;
>> int err;
>>
>> @@ -105,48 +58,6 @@ int __init parse_spcr(bool earlycon)
>> if (ACPI_FAILURE(status))
>> return -ENOENT;
>>
>> - if (table->header.revision < 2) {
>> - err = -ENOENT;
>> - pr_err("wrong table version\n");
>> - goto done;
>> - }
>> -
>> - if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> - switch (ACPI_ACCESS_BIT_WIDTH((
>> - table->serial_port.access_width))) {
>> - default:
>> - pr_err("Unexpected SPCR Access Width. Defaulting to byte size\n");
>> - case 8:
>> - iotype = "mmio";
>> - break;
>> - case 16:
>> - iotype = "mmio16";
>> - break;
>> - case 32:
>> - iotype = "mmio32";
>> - break;
>> - }
>> - } else
>> - iotype = "io";
>> -
>> - switch (table->interface_type) {
>> - case ACPI_DBG2_ARM_SBSA_32BIT:
>> - iotype = "mmio32";
>> - /* fall through */
>> - case ACPI_DBG2_ARM_PL011:
>> - case ACPI_DBG2_ARM_SBSA_GENERIC:
>> - case ACPI_DBG2_BCM2835:
>> - uart = "pl011";
>> - break;
>> - case ACPI_DBG2_16550_COMPATIBLE:
>> - case ACPI_DBG2_16550_SUBSET:
>> - uart = "uart";
>> - break;
>> - default:
>> - err = -ENOENT;
>> - goto done;
>> - }
>> -
>> switch (table->baud_rate) {
>> case 3:
>> baud_rate = 9600;
>> @@ -165,43 +76,36 @@ int __init parse_spcr(bool earlycon)
>> goto done;
>> }
>>
>> - /*
>> - * If the E44 erratum is required, then we need to tell the pl011
>> - * driver to implement the work-around.
>> - *
>> - * The global variable is used by the probe function when it
>> - * creates the UARTs, whether or not they're used as a console.
>> - *
>> - * If the user specifies "traditional" earlycon, the qdf2400_e44
>> - * console name matches the EARLYCON_DECLARE() statement, and
>> - * SPCR is not used. Parameter "earlycon" is false.
>> - *
>> - * If the user specifies "SPCR" earlycon, then we need to update
>> - * the console name so that it also says "qdf2400_e44". Parameter
>> - * "earlycon" is true.
>> - *
>> - * For consistency, if we change the console name, then we do it
>> - * for everyone, not just earlycon.
>> - */
>> - if (qdf2400_erratum_44_present(&table->header)) {
>> - qdf2400_e44_present = true;
>> - if (earlycon)
>> - uart = "qdf2400_e44";
>> + switch (table->interface_type) {
>> + case ACPI_DBG2_16550_COMPATIBLE:
>> + case ACPI_DBG2_16550_SUBSET:
>> + snprintf(uart, ACPI_SPCR_BUF_SIZE, "uart");
>> + break;
>> + default:
>> + break;
>> }
>>
>> - if (xgene_8250_erratum_present(table)) {
>> - iotype = "mmio32";
>> + if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> + u8 width = ACPI_ACCESS_BIT_WIDTH((
>> + table->serial_port.access_width));
>> + switch (width) {
>> + default:
>> + pr_err("Unexpected SPCR Access Width. Defaulting to byte size\n");
>> + case 8:
>> + snprintf(iotype, ACPI_SPCR_BUF_SIZE, "mmio");
>> + break;
>> + case 16:
>> + case 32:
>> + snprintf(iotype, ACPI_SPCR_BUF_SIZE, "mmio%d", width);
>> + break;
>> + }
>> + } else
>> + snprintf(iotype, ACPI_SPCR_BUF_SIZE, "io");
>>
>> - /* for xgene v1 and v2 we don't know the clock rate of the
>> - * UART so don't attempt to change to the baud rate state
>> - * in the table because driver cannot calculate the dividers
>> - */
>> - snprintf(opts, sizeof(opts), "%s,%s,0x%llx", uart, iotype,
>> - table->serial_port.address);
>> - } else {
>> - snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>> - table->serial_port.address, baud_rate);
>> - }
>> + err = acpi_arch_setup_console(table, opts, uart, iotype, baud_rate,
>> + earlycon);
>> + if (err)
>> + goto done;
>>
>> pr_info("console: %s\n", opts);
>>
>> @@ -209,7 +113,6 @@ int __init parse_spcr(bool earlycon)
>> setup_earlycon(opts);
>>
>> err = add_preferred_console(uart, 0, opts + strlen(uart) + 1);
>> -
>
> Unintended change.
>
>> done:
>> acpi_put_table((struct acpi_table_header *)table);
>> return err;
>> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
>> index 4c8b80f1c688..b22afb62c7a3 100644
>> --- a/drivers/tty/serial/earlycon.c
>> +++ b/drivers/tty/serial/earlycon.c
>> @@ -196,26 +196,15 @@ int __init setup_earlycon(char *buf)
>> return -ENOENT;
>> }
>>
>> -/*
>> - * When CONFIG_ACPI_SPCR_TABLE is defined, "earlycon" without parameters in
>> - * command line does not start DT earlycon immediately, instead it defers
>> - * starting it until DT/ACPI decision is made. At that time if ACPI is enabled
>> - * call parse_spcr(), else call early_init_dt_scan_chosen_stdout()
>> - */
>> -bool earlycon_init_is_deferred __initdata;
>> -
>> /* early_param wrapper for setup_earlycon() */
>> static int __init param_setup_earlycon(char *buf)
>> {
>> int err;
>>
>> - /*
>> - * Just 'earlycon' is a valid param for devicetree earlycons;
>> - * don't generate a warning from parse_early_params() in that case
>> - */
>> + /* Just 'earlycon' is a valid param for devicetree and ACPI SPCR. */
>> if (!buf || !buf[0]) {
>> if (IS_ENABLED(CONFIG_ACPI_SPCR_TABLE)) {
>> - earlycon_init_is_deferred = true;
>> + console_acpi_spcr_enable = true;
>
> I am not familiar with this code, I would ask Graeme and Mark to check
> if this change is correct, the logic seems correct to me but I may be
> missing some corner cases.
>
>> return 0;
>> } else if (!buf) {
>> return early_init_dt_scan_chosen_stdout();
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index dc1ebfeeb5ec..875d7327d91c 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1241,10 +1241,17 @@ static inline bool acpi_has_watchdog(void) { return false; }
>> #endif
>>
>> #ifdef CONFIG_ACPI_SPCR_TABLE
>> +#define ACPI_SPCR_OPTS_SIZE 64
>> +#define ACPI_SPCR_BUF_SIZE 32
>> extern bool qdf2400_e44_present;
>> -int parse_spcr(bool earlycon);
>> +extern bool console_acpi_spcr_enable __initdata;
>> +extern int acpi_arch_setup_console(struct acpi_table_spcr *table,
>> + char *opts, char *uart, char *iotype,
>> + int baud_rate, bool earlycon);
>> +int acpi_parse_spcr(bool earlycon);
>> #else
>> -static inline int parse_spcr(bool earlycon) { return 0; }
>> +static const bool console_acpi_spcr_enable;
>
> The assignment in param_setup_earlycon won't compile.
Hmm ... I'm pretty sure it did. But I'll check that before resubmitting.
P.
>
> Lorenzo
>
^ permalink raw reply
* [PATCH 2/2] serdev: only match serdev devices
From: Johan Hovold @ 2018-01-08 12:42 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Johan Hovold
In-Reply-To: <20180108124233.26729-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 61c85e49e178..a30e237eb98c 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 1/2] serdev: do not generate modaliases for controllers
From: Johan Hovold @ 2018-01-08 12:42 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
Johan Hovold
In-Reply-To: <20180108124233.26729-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.
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 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 0/2] serdev: bus-code clean ups
From: Johan Hovold @ 2018-01-08 12:42 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Frédéric Danis,
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
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
* [PATCH] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version
From: Hans de Goede @ 2018-01-08 9:44 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: Hans de Goede, linux-bluetooth, linux-serial, linux-acpi, stable,
Leif Liddy, Matthias Kaehlcke, Brian Norris, Daniel Drake,
Kai-Heng Feng
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(-)
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;
}
--
2.14.3
^ permalink raw reply related
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Martin Blumenstingl @ 2018-01-07 20:07 UTC (permalink / raw)
To: Marcel Holtmann
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: <41669609-3E28-473D-8616-71B9D0EEDCDF-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Hi Marcel,
thank you for digging into this!
On Fri, Jan 5, 2018 at 9:44 PM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Carlo,
>
>>>>>> 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 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)
> 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]
> 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...)
> At the end of the day, this is the same as for Broadcom and ACPI vs DT. This should be no different. The only bit nasty part is that this is H:5 as protocol and we have not really abstracted that one nicely to just have a tiny hci_rtl.c for vendor specific pieces.
yes, you already suggested that I should start making that re-usable
(this is still on my TODO-list)
Regards
Martin
[0] https://github.com/NextThingCo/rtl8723ds_bt/blob/master/rtk_hciattach/hciattach_rtk.c#L2742
[1] https://elixir.free-electrons.com/linux/v4.14.11/source/drivers/bluetooth/btrtl.c#L377
^ permalink raw reply
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Marcel Holtmann @ 2018-01-05 20:44 UTC (permalink / raw)
To: Carlo Caione
Cc: Martin Blumenstingl, 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: <1F394D8B-CFB8-48BA-BC6B-15D1EE51DB08-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Hi Carlo,
>>>>> 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.
>
> 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;
> }
>
> 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.
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.
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.
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.
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.
At the end of the day, this is the same as for Broadcom and ACPI vs DT. This should be no different. The only bit nasty part is that this is H:5 as protocol and we have not really abstracted that one nicely to just have a tiny hci_rtl.c for vendor specific pieces.
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 v2] serial: imx: fix endless loop during suspend
From: Fabio Estevam @ 2018-01-05 17:31 UTC (permalink / raw)
To: Martin Kaiser
Cc: Greg Kroah-Hartman, Sascha Hauer, Jiri Slaby, Philipp Zabel,
Shawn Guo, linux-kernel, linux-serial
In-Reply-To: <1515170803-31662-1-git-send-email-martin@kaiser.cx>
Hi Martin,
On Fri, Jan 5, 2018 at 2:46 PM, Martin Kaiser <martin@kaiser.cx> wrote:
> Hi Fabio and all,
>
> here's a different approach for fixing the awake issue that I see on my
> board. I tried to stick as much as possible to the original order in
> which the operations were done. Could you do a quick check on imx6q?
suspend_stats looks better now:
# cat /sys/kernel/debug/suspend_stats
success: 8
fail: 0
failed_freeze: 0
failed_prepare: 0
failed_suspend: 0
failed_suspend_late: 0
failed_suspend_noirq: 0
failed_resume: 0
failed_resume_early: 0
failed_resume_noirq: 0
failures:
last_failed_dev:
last_failed_errno: 0
0
last_failed_step:
However the board is still automatically returning from resume (I
haven't entered any char in the console)
# echo mem > /sys/power/state
[ 186.723470] PM: suspend entry (deep)
[ 186.727107] PM: Syncing filesystems ... done.
[ 186.735016] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 186.743640] OOM killer disabled.
[ 186.746888] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[ 186.756174] Suspending console(s) (use no_console_suspend to debug)
[ 186.828645] PM: suspend devices took 0.060 seconds
[ 186.837771] Disabling non-boot CPUs ...
[ 187.014044] Enabling non-boot CPUs ...
[ 187.015057] CPU1 is up
[ 187.016003] CPU2 is up
[ 187.016988] CPU3 is up
[ 187.294341] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
[ 187.296228] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[ 187.298111] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[ 187.301490] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[ 187.304873] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[ 187.354759] ata1: SATA link down (SStatus 0 SControl 300)
[ 187.359840] PM: resume devices took 0.340 seconds
[ 187.417332] OOM killer enabled.
[ 187.420487] Restarting tasks ... done.
[ 187.425435] PM: suspend exit
^ permalink raw reply
* [PATCH v2] serial: imx: fix endless loop during suspend
From: Martin Kaiser @ 2018-01-05 16:46 UTC (permalink / raw)
To: Greg Kroah-Hartman, Fabio Estevam, Sascha Hauer, Jiri Slaby,
Philipp Zabel
Cc: Shawn Guo, linux-kernel, linux-serial, Martin Kaiser
In-Reply-To: <1514395632-15390-1-git-send-email-martin@kaiser.cx>
Before we go into suspend mode, we enable the imx uart's interrupt for
the awake bit in the UART Status Register 1. If, for some reason, the
awake bit is already set before we enter suspend mode, we get an
interrupt immediately when we enable interrupts for awake. The uart's
clk_ipg is disabled at this point (unless there's an ongoing transfer).
We end up in the interrupt handler, which usually tries to clear the
awake bit. This doesn't work with the clock disabled. Therefore, we
keep getting interrupts forever, resulting in an endless loop.
Clear the awake bit before setting the awaken bit to signal that we want
an imx interrupt when the awake bit will be set. This ensures that we're
not woken up by events that happened before we started going into
suspend mode.
Change the clock handling so that suspend prepares and enables the clock
and suspend_noirq disables it. Revert these operations in resume_noirq and
resume.
With these preparations in place, we can now modify awake and awaken in
the suspend function when the actual imx interrupt is disabled and the
required clk_ipg is active.
Update the thaw and freeze functions to use the new clock handling since
we share the suspend_noirq function between suspend and hibernate.
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
changes in v2
call serial_imx_enable_wakeup() in suspend, not in suspend_noirq
keep the clock enalbed between suspend and suspend_noirq and
between resume_noirq and resume
Hi Fabio and all,
here's a different approach for fixing the awake issue that I see on my
board. I tried to stick as much as possible to the original order in
which the operations were done. Could you do a quick check on imx6q?
Thanks,
Martin
drivers/tty/serial/imx.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 145ed61..e5d5250 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2221,8 +2221,10 @@ static void serial_imx_enable_wakeup(struct imx_port *sport, bool on)
unsigned int val;
val = readl(sport->port.membase + UCR3);
- if (on)
+ if (on) {
+ writel(USR1_AWAKE, sport->port.membase + USR1);
val |= UCR3_AWAKEN;
+ }
else
val &= ~UCR3_AWAKEN;
writel(val, sport->port.membase + UCR3);
@@ -2239,11 +2241,6 @@ static int imx_serial_port_suspend_noirq(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct imx_port *sport = platform_get_drvdata(pdev);
- int ret;
-
- ret = clk_enable(sport->clk_ipg);
- if (ret)
- return ret;
serial_imx_save_context(sport);
@@ -2264,8 +2261,6 @@ static int imx_serial_port_resume_noirq(struct device *dev)
serial_imx_restore_context(sport);
- clk_disable(sport->clk_ipg);
-
return 0;
}
@@ -2273,15 +2268,19 @@ static int imx_serial_port_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct imx_port *sport = platform_get_drvdata(pdev);
-
- /* enable wakeup from i.MX UART */
- serial_imx_enable_wakeup(sport, true);
+ int ret;
uart_suspend_port(&imx_reg, &sport->port);
disable_irq(sport->port.irq);
- /* Needed to enable clock in suspend_noirq */
- return clk_prepare(sport->clk_ipg);
+ ret = clk_prepare_enable(sport->clk_ipg);
+ if (ret)
+ return ret;
+
+ /* enable wakeup from i.MX UART */
+ serial_imx_enable_wakeup(sport, true);
+
+ return 0;
}
static int imx_serial_port_resume(struct device *dev)
@@ -2295,7 +2294,7 @@ static int imx_serial_port_resume(struct device *dev)
uart_resume_port(&imx_reg, &sport->port);
enable_irq(sport->port.irq);
- clk_unprepare(sport->clk_ipg);
+ clk_disable_unprepare(sport->clk_ipg);
return 0;
}
@@ -2307,8 +2306,7 @@ static int imx_serial_port_freeze(struct device *dev)
uart_suspend_port(&imx_reg, &sport->port);
- /* Needed to enable clock in suspend_noirq */
- return clk_prepare(sport->clk_ipg);
+ return clk_prepare_enable(sport->clk_ipg);
}
static int imx_serial_port_thaw(struct device *dev)
@@ -2318,7 +2316,7 @@ static int imx_serial_port_thaw(struct device *dev)
uart_resume_port(&imx_reg, &sport->port);
- clk_unprepare(sport->clk_ipg);
+ clk_disable_unprepare(sport->clk_ipg);
return 0;
}
--
2.1.4
^ permalink raw reply related
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Marcel Holtmann @ 2018-01-05 16:15 UTC (permalink / raw)
To: Carlo Caione
Cc: Martin Blumenstingl, 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: <EC216531-81EC-4EE8-BAC4-C6954C222092-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Hi Carlo,
>>>> 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.
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;
}
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.
Regards
Marcel
^ permalink raw reply
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Marcel Holtmann @ 2018-01-05 14:57 UTC (permalink / raw)
To: Carlo Caione
Cc: Martin Blumenstingl, 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: <CAL9uMOGkZefjD_JNxjf4yJ0ATFtOG5Cm_XQq1fy0VQDjiMFBmg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Carlo,
>>> 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.
>> have you tested this (= uploading the firmware without the config
>> blob) on your x86 board before?
>
> No, on the cherry-trail I have I'm using hciattach to upload the
> firmware and AFAICT only the firmware is uploaded and (as written
> before) the config blob is only used to get the UART configuration.
>
>> so far the following solutions for the config blob were discussed:
>> 1) put the config blob in userspace (/lib/firmware/...) -> not good
>> because it makes the rootfs board-specific
>> 2) auto-generate the config blob -> didn't work for me, last firmware
>> chunk times out (just as if I had no config at all)
>> 3) putting the config blob in DT -> possible but not very nice to read
>>
>> I had another idea:
>> what if we mix solution 1) and 2)?
>> the idea: load the config blob from userspace (/lib/firmware/...) and
>> update it's settings with the values we got from devicetree-properties
>> / DSDT
>
> If you are shipping already the config blob in userspace I don't see
> why you would do the update since you have already all the info you
> need.
> I would rather check why (2) didn't work fine. Have you any
> documentation how the config blobs are generated? The code in
> hciattach_rtk.c seems a good starting point.
I would almost bet it needs a HCI commands or some HCI command is embedded in the config blob somewhere that essentially tells the firmware it is done and ready. Hence my ask above, we need to know what is in these config file.
Even settings like crystal timing or some RF pieces or even enabled/disabled features are board specific and should be in DT rather than a config blob on the general purpose filesystem.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Daniel Lezcano @ 2018-01-05 9:31 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Rick Chen, Rick Chen, Linux Kernel Mailing List,
Arnd Bergmann, Linus Walleij, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, netdev, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, linux-serial
In-Reply-To: <CAEbi=3frQYZHb0yiMvNk_JUiUCy6ca0tbrsM_pt49LtkMy47mw@mail.gmail.com>
On 05/01/2018 09:45, Greentime Hu wrote:
> Hi, Daniel:
[ ... ]
>>>>>> [ ... ]
>>>>>>
>>>>>>> +config CLKSRC_ATCPIT100
>>>>>>> + bool "Clocksource for AE3XX platform"
>>>>>>> + depends on NDS32 || COMPILE_TEST
>>>>>>> + depends on HAS_IOMEM
>>>>>>> + help
>>>>>>> + This option enables support for the Andestech AE3XX platform timers.
>>>>>>
>>>>>> Hi Rick,
>>>>>>
>>>>>> the general rule for the Kconfig is:
>>>>>>
>>>>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST
>>
>> BTW, select TIMER_OF is missing.
>
> We don't select here because we select TIMER_OF in arch/nds32/Kconfig
> I am not sure if I still need to select TIMER_OF here?
Actually, I want the drivers/clocksource/Kconfig to be consistent across
all entries. As TIMER_OF is needed by the driver and nothing else, it
must be selected in the TIMER entry.
As there are a lot of timers and we do the changes little by little,
there are still entries with different format.
It should be something like that:
config ASM9260_TIMER
bool "ASM9260 timer driver" if COMPILE_TEST
select CLKSRC_MMIO
select TIMER_OF
help
Enables support for the ASM9260 timer.
Move the select TIMER_OF to the timer option entry.
>>>>>> and no deps on the platform.
>>>>>>
>>>>>> It is up to the platform Kconfig to select the option.
>>>>>>
>>>>>> We want here a silent option but make it selectable in case of
>>>>>> compilation test coverage.
>>>>>
>>>>>
>>>>> The way we like to use it is because
>>>>> 1. This timer is a basic component to boot an nds32 CPU and it should
>>>>> be able to select without COMPILE_TEST for nds32 architecture.
>>>>
>>>> Yes, so you don't need it to be selectable, you must select it from the
>>>> platform's Kconfig.
>>>
>>> I am not sure that I get your point or not.
>>> We don't have a CONFIG_PLAT_AE3XX.
>>> Do you mean we should create one and select CLKSRC_ATCPIT100 under
>>> CONFIG_PLAT_AE3XX?
>>
>> No. Can't you add in arch/ndis32/Kconfig ?
>>
>> +select TIMER_ATCPIT100
>>
>> Like:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50
>
> IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in
> arch/nds32/Kconfig because it is part of SoC instead of CPU.
> If we change to another SoC with another timer, we need to select
> another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100.
> It seems more flexible to be selected in driver layer.
>
> It seems to be the timer is part of the arch to be selected in arch's Kconfig.
> arch/arc/Kconfig: select ARC_TIMERS
> arch/arc/Kconfig: select ARC_TIMERS_64BIT
> arch/arm/Kconfig: select ARM_ARCH_TIMER
> arch/arm64/Kconfig: select ARM_ARCH_TIMER
> arch/blackfin/Kconfig: select BFIN_GPTIMERS
No, the timer must be selected from the arch/soc's or whatever Kconfig.
Not in the clocksource's Kconfig.
eg.
on ARM:
arch/arm/mach-vt8500/Kconfig: select VT8500_TIMER
arch/arm/mach-bcm/Kconfig: select BCM_KONA_TIMER
arch/arm/mach-actions/Kconfig: select OWL_TIMER
arch/arm/mach-digicolor/Kconfig: select DIGICOLOR_TIMER
etc ...
on ARM64:
arch/arm64/Kconfig.platforms: select OWL_TIMER
arch/arm64/Kconfig.platforms: select ARM_TIMER_SP804
arch/arm64/Kconfig.platforms: select MTK_TIMER
etc ...
Thanks.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Greentime Hu @ 2018-01-05 8:45 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Greentime, Rick Chen, Rick Chen, Linux Kernel Mailing List,
Arnd Bergmann, Linus Walleij, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, netdev, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, linux-serial
In-Reply-To: <efdb249a-8dc0-e076-a1db-20cecdbf4c13@linaro.org>
Hi, Daniel:
2018-01-05 3:48 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 04/01/2018 15:06, Greentime Hu wrote:
>> Hi, Daniel:
>>
>> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>
>>> Hi,
>>>
>>> sorry I missed your answer. Comments below.
>>>
>>> On 13/12/2017 07:06, Greentime Hu wrote:
>>>> Hi, Daniel:
>>>>
>>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>>>> ATCPIT100 is often used on the Andes architecture,
>>>>>> This timer provide 4 PIT channels. Each PIT channel is a
>>>>>> multi-function timer, can be configured as 32,16,8 bit timers
>>>>>> or PWM as well.
>>>>>>
>>>>>> For system timer it will set channel 1 32-bit timer0 as clock
>>>>>> source and count downwards until underflow and restart again.
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>> +config CLKSRC_ATCPIT100
>>>>>> + bool "Clocksource for AE3XX platform"
>>>>>> + depends on NDS32 || COMPILE_TEST
>>>>>> + depends on HAS_IOMEM
>>>>>> + help
>>>>>> + This option enables support for the Andestech AE3XX platform timers.
>>>>>
>>>>> Hi Rick,
>>>>>
>>>>> the general rule for the Kconfig is:
>>>>>
>>>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST
>
> BTW, select TIMER_OF is missing.
We don't select here because we select TIMER_OF in arch/nds32/Kconfig
I am not sure if I still need to select TIMER_OF here?
>>>>> and no deps on the platform.
>>>>>
>>>>> It is up to the platform Kconfig to select the option.
>>>>>
>>>>> We want here a silent option but make it selectable in case of
>>>>> compilation test coverage.
>>>>
>>>>
>>>> The way we like to use it is because
>>>> 1. This timer is a basic component to boot an nds32 CPU and it should
>>>> be able to select without COMPILE_TEST for nds32 architecture.
>>>
>>> Yes, so you don't need it to be selectable, you must select it from the
>>> platform's Kconfig.
>>
>> I am not sure that I get your point or not.
>> We don't have a CONFIG_PLAT_AE3XX.
>> Do you mean we should create one and select CLKSRC_ATCPIT100 under
>> CONFIG_PLAT_AE3XX?
>
> No. Can't you add in arch/ndis32/Kconfig ?
>
> +select TIMER_ATCPIT100
>
> Like:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50
IMHO, it might be a little bit wierd if we select TIMER_ATCPIT100 in
arch/nds32/Kconfig because it is part of SoC instead of CPU.
If we change to another SoC with another timer, we need to select
another TIMER in arch/nds32/Kconfig and delete TIMER_ATCPIT100.
It seems more flexible to be selected in driver layer.
It seems to be the timer is part of the arch to be selected in arch's Kconfig.
arch/arc/Kconfig: select ARC_TIMERS
arch/arc/Kconfig: select ARC_TIMERS_64BIT
arch/arm/Kconfig: select ARM_ARCH_TIMER
arch/arm64/Kconfig: select ARM_ARCH_TIMER
arch/blackfin/Kconfig: select BFIN_GPTIMERS
>>>> 2. It seems conflict with debug info. I am not sure if there is
>>>> another way to debug kernel(with debug info) with COMPILE_TEST and
>>>> DEBUG_INFO because we need this driver for nds32 architecture.
>>>>
>>>> Symbol: DEBUG_INFO [=n]
>>>> Type : boolean
>>>> Prompt: Compile the kernel with debug info
>>>> Location:
>>>> -> Kernel hacking
>>>> -> Compile-time checks and compiler options
>>>> Defined at lib/Kconfig.debug:140
>>>> Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]
>>>
>>> The COMPILE_TEST option is only there to allow cross-compilation test
>>> coverage, it does not select or unselect the driver in usual way.
>>>
>>> If the COMPILE_TEST is enabled, then the option will appear in the
>>> menuconfig, so that gives the opportunity to select/unselect it.
>>>
>>> Otherwise, the Kconfig's platform selects automatically the driver and
>>> the user *can't* unselect it from the menuconfig as it is a silent
>>> option and that is certainly what you want.
>>>
>>>>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
>>>>> to TIMER_ATCPIT100.
>>>>
>>>> Thanks. We will rename it in the next version patch.
>>>
>>> You just resend an entire series V5 for the architecture. I'm confused,
>>> what is the merging path ?
>>
>> Sorry. I didn't get your point.
>> We sent the timer patch and the architecture patch together because it
>> would be easier for reviewer to check the vdso implementations.
>> What do you mean about the merging path?
>
> I received a [Vx y/3] series and I received a [Vx y/39].
>
> The former from Rick Chen means to me "please pick them through your tree".
>
> The latter from you means to me "can you ack the patches so I can merge
> them through my tree". Note you will have to resend the entire arch
> series for every single review/comment (that could end up upset the
> Cc'ed people).
>
> Which one should I review ? I can not track different patchset
> implementing the same thing. Which one should I comment, review ? Are
> the comments I did on [Vx y/3] taken into account in the arch series ?
> etc ...
>
> Please clarify, it is confusing and impossible to review in this situation.
>
> I suggest we stick to the x/3 series, so I can comment it and you can
> resend a new version without resending the entire arch series. So I can
> merge it through my tree, and you get it via eg. a shared immutable
> branch. The arch series will be reduced by 3 patches.
Thanks for your explanation. :)
I will send these 2 patch set separately next time.
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Daniel Lezcano @ 2018-01-04 19:48 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Rick Chen, Rick Chen, Linux Kernel Mailing List,
Arnd Bergmann, Linus Walleij, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, netdev, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, linux-serial
In-Reply-To: <CAEbi=3c4yAx7Tejz8aPw3-3vj89t8cg-zNEY-vbF7KaMo8OM0Q@mail.gmail.com>
On 04/01/2018 15:06, Greentime Hu wrote:
> Hi, Daniel:
>
> 2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>
>> Hi,
>>
>> sorry I missed your answer. Comments below.
>>
>> On 13/12/2017 07:06, Greentime Hu wrote:
>>> Hi, Daniel:
>>>
>>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>>> ATCPIT100 is often used on the Andes architecture,
>>>>> This timer provide 4 PIT channels. Each PIT channel is a
>>>>> multi-function timer, can be configured as 32,16,8 bit timers
>>>>> or PWM as well.
>>>>>
>>>>> For system timer it will set channel 1 32-bit timer0 as clock
>>>>> source and count downwards until underflow and restart again.
>>>>
>>>> [ ... ]
>>>>
>>>>> +config CLKSRC_ATCPIT100
>>>>> + bool "Clocksource for AE3XX platform"
>>>>> + depends on NDS32 || COMPILE_TEST
>>>>> + depends on HAS_IOMEM
>>>>> + help
>>>>> + This option enables support for the Andestech AE3XX platform timers.
>>>>
>>>> Hi Rick,
>>>>
>>>> the general rule for the Kconfig is:
>>>>
>>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST
BTW, select TIMER_OF is missing.
>>>> and no deps on the platform.
>>>>
>>>> It is up to the platform Kconfig to select the option.
>>>>
>>>> We want here a silent option but make it selectable in case of
>>>> compilation test coverage.
>>>
>>>
>>> The way we like to use it is because
>>> 1. This timer is a basic component to boot an nds32 CPU and it should
>>> be able to select without COMPILE_TEST for nds32 architecture.
>>
>> Yes, so you don't need it to be selectable, you must select it from the
>> platform's Kconfig.
>
> I am not sure that I get your point or not.
> We don't have a CONFIG_PLAT_AE3XX.
> Do you mean we should create one and select CLKSRC_ATCPIT100 under
> CONFIG_PLAT_AE3XX?
No. Can't you add in arch/ndis32/Kconfig ?
+select TIMER_ATCPIT100
Like:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/Kconfig#n50
>>> 2. It seems conflict with debug info. I am not sure if there is
>>> another way to debug kernel(with debug info) with COMPILE_TEST and
>>> DEBUG_INFO because we need this driver for nds32 architecture.
>>>
>>> Symbol: DEBUG_INFO [=n]
>>> Type : boolean
>>> Prompt: Compile the kernel with debug info
>>> Location:
>>> -> Kernel hacking
>>> -> Compile-time checks and compiler options
>>> Defined at lib/Kconfig.debug:140
>>> Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]
>>
>> The COMPILE_TEST option is only there to allow cross-compilation test
>> coverage, it does not select or unselect the driver in usual way.
>>
>> If the COMPILE_TEST is enabled, then the option will appear in the
>> menuconfig, so that gives the opportunity to select/unselect it.
>>
>> Otherwise, the Kconfig's platform selects automatically the driver and
>> the user *can't* unselect it from the menuconfig as it is a silent
>> option and that is certainly what you want.
>>
>>>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
>>>> to TIMER_ATCPIT100.
>>>
>>> Thanks. We will rename it in the next version patch.
>>
>> You just resend an entire series V5 for the architecture. I'm confused,
>> what is the merging path ?
>
> Sorry. I didn't get your point.
> We sent the timer patch and the architecture patch together because it
> would be easier for reviewer to check the vdso implementations.
> What do you mean about the merging path?
I received a [Vx y/3] series and I received a [Vx y/39].
The former from Rick Chen means to me "please pick them through your tree".
The latter from you means to me "can you ack the patches so I can merge
them through my tree". Note you will have to resend the entire arch
series for every single review/comment (that could end up upset the
Cc'ed people).
Which one should I review ? I can not track different patchset
implementing the same thing. Which one should I comment, review ? Are
the comments I did on [Vx y/3] taken into account in the arch series ?
etc ...
Please clarify, it is confusing and impossible to review in this situation.
I suggest we stick to the x/3 series, so I can comment it and you can
resend a new version without resending the entire arch series. So I can
merge it through my tree, and you get it via eg. a shared immutable
branch. The arch series will be reduced by 3 patches.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Greentime Hu @ 2018-01-04 14:06 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Greentime, Rick Chen, Rick Chen, Linux Kernel Mailing List,
Arnd Bergmann, Linus Walleij, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, netdev, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, linux-serial
In-Reply-To: <ff51fff7-e963-f9fd-10d6-9b90a64d7aac@linaro.org>
Hi, Daniel:
2018-01-04 21:50 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>
> Hi,
>
> sorry I missed your answer. Comments below.
>
> On 13/12/2017 07:06, Greentime Hu wrote:
>> Hi, Daniel:
>>
>> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>>> On 12/12/2017 06:46, Rick Chen wrote:
>>>> ATCPIT100 is often used on the Andes architecture,
>>>> This timer provide 4 PIT channels. Each PIT channel is a
>>>> multi-function timer, can be configured as 32,16,8 bit timers
>>>> or PWM as well.
>>>>
>>>> For system timer it will set channel 1 32-bit timer0 as clock
>>>> source and count downwards until underflow and restart again.
>>>
>>> [ ... ]
>>>
>>>> +config CLKSRC_ATCPIT100
>>>> + bool "Clocksource for AE3XX platform"
>>>> + depends on NDS32 || COMPILE_TEST
>>>> + depends on HAS_IOMEM
>>>> + help
>>>> + This option enables support for the Andestech AE3XX platform timers.
>>>
>>> Hi Rick,
>>>
>>> the general rule for the Kconfig is:
>>>
>>> bool "Clocksource for AE3XX platform" if COMPILE_TEST
>>>
>>> and no deps on the platform.
>>>
>>> It is up to the platform Kconfig to select the option.
>>>
>>> We want here a silent option but make it selectable in case of
>>> compilation test coverage.
>>
>>
>> The way we like to use it is because
>> 1. This timer is a basic component to boot an nds32 CPU and it should
>> be able to select without COMPILE_TEST for nds32 architecture.
>
> Yes, so you don't need it to be selectable, you must select it from the
> platform's Kconfig.
I am not sure that I get your point or not.
We don't have a CONFIG_PLAT_AE3XX.
Do you mean we should create one and select CLKSRC_ATCPIT100 under
CONFIG_PLAT_AE3XX?
>> 2. It seems conflict with debug info. I am not sure if there is
>> another way to debug kernel(with debug info) with COMPILE_TEST and
>> DEBUG_INFO because we need this driver for nds32 architecture.
>>
>> Symbol: DEBUG_INFO [=n]
>> Type : boolean
>> Prompt: Compile the kernel with debug info
>> Location:
>> -> Kernel hacking
>> -> Compile-time checks and compiler options
>> Defined at lib/Kconfig.debug:140
>> Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]
>
> The COMPILE_TEST option is only there to allow cross-compilation test
> coverage, it does not select or unselect the driver in usual way.
>
> If the COMPILE_TEST is enabled, then the option will appear in the
> menuconfig, so that gives the opportunity to select/unselect it.
>
> Otherwise, the Kconfig's platform selects automatically the driver and
> the user *can't* unselect it from the menuconfig as it is a silent
> option and that is certainly what you want.
>
>>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
>>> to TIMER_ATCPIT100.
>>
>> Thanks. We will rename it in the next version patch.
>
> You just resend an entire series V5 for the architecture. I'm confused,
> what is the merging path ?
Sorry. I didn't get your point.
We sent the timer patch and the architecture patch together because it
would be easier for reviewer to check the vdso implementations.
What do you mean about the merging path?
Thanks.
^ permalink raw reply
* Re: [PATCH v5 1/3] clocksource/drivers/atcpit100: Add andestech atcpit100 timer
From: Daniel Lezcano @ 2018-01-04 13:50 UTC (permalink / raw)
To: Greentime Hu, Greentime
Cc: Rick Chen, Rick Chen, Linux Kernel Mailing List, Arnd Bergmann,
Linus Walleij, linux-arch, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Rob Herring, netdev, Vincent Chen, DTML, Al Viro,
David Howells, Will Deacon, linux-serial
In-Reply-To: <CAEbi=3cEEtLQRiv-cyUwEmcBNOC2Sz7AhzZkpznQWYb9VUojug@mail.gmail.com>
Hi,
sorry I missed your answer. Comments below.
On 13/12/2017 07:06, Greentime Hu wrote:
> Hi, Daniel:
>
> 2017-12-12 18:05 GMT+08:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 12/12/2017 06:46, Rick Chen wrote:
>>> ATCPIT100 is often used on the Andes architecture,
>>> This timer provide 4 PIT channels. Each PIT channel is a
>>> multi-function timer, can be configured as 32,16,8 bit timers
>>> or PWM as well.
>>>
>>> For system timer it will set channel 1 32-bit timer0 as clock
>>> source and count downwards until underflow and restart again.
>>
>> [ ... ]
>>
>>> +config CLKSRC_ATCPIT100
>>> + bool "Clocksource for AE3XX platform"
>>> + depends on NDS32 || COMPILE_TEST
>>> + depends on HAS_IOMEM
>>> + help
>>> + This option enables support for the Andestech AE3XX platform timers.
>>
>> Hi Rick,
>>
>> the general rule for the Kconfig is:
>>
>> bool "Clocksource for AE3XX platform" if COMPILE_TEST
>>
>> and no deps on the platform.
>>
>> It is up to the platform Kconfig to select the option.
>>
>> We want here a silent option but make it selectable in case of
>> compilation test coverage.
>
>
> The way we like to use it is because
> 1. This timer is a basic component to boot an nds32 CPU and it should
> be able to select without COMPILE_TEST for nds32 architecture.
Yes, so you don't need it to be selectable, you must select it from the
platform's Kconfig.
> 2. It seems conflict with debug info. I am not sure if there is
> another way to debug kernel(with debug info) with COMPILE_TEST and
> DEBUG_INFO because we need this driver for nds32 architecture.
>
> Symbol: DEBUG_INFO [=n]
> Type : boolean
> Prompt: Compile the kernel with debug info
> Location:
> -> Kernel hacking
> -> Compile-time checks and compiler options
> Defined at lib/Kconfig.debug:140
> Depends on: DEBUG_KERNEL [=y] && !COMPILE_TEST [=n]
The COMPILE_TEST option is only there to allow cross-compilation test
coverage, it does not select or unselect the driver in usual way.
If the COMPILE_TEST is enabled, then the option will appear in the
menuconfig, so that gives the opportunity to select/unselect it.
Otherwise, the Kconfig's platform selects automatically the driver and
the user *can't* unselect it from the menuconfig as it is a silent
option and that is certainly what you want.
>> Also, this driver is not a CLKSRC but a TIMER. Rename CLKSRC_ATCPIT100
>> to TIMER_ATCPIT100.
>
> Thanks. We will rename it in the next version patch.
You just resend an entire series V5 for the architecture. I'm confused,
what is the merging path ?
Thanks.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available
From: Carlo Caione @ 2018-01-04 9:46 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Marcel Holtmann, Rob Herring, devicetree,
open list:BLUETOOTH DRIVERS, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Mark Rutland, Gustavo F. Padovan, Johan Hedberg,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k,
johan-DgEjT+Ai2ygdnm+yROfE0A,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ, Daniel Drake
In-Reply-To: <CAFBinCAFsDRpp=4We1hQPiutM1Q0MrqebPLyS3e2_e+JcBoSmg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jan 3, 2018 at 8:50 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> Hi Carlo,
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?
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.
> have you tested this (= uploading the firmware without the config
> blob) on your x86 board before?
No, on the cherry-trail I have I'm using hciattach to upload the
firmware and AFAICT only the firmware is uploaded and (as written
before) the config blob is only used to get the UART configuration.
> so far the following solutions for the config blob were discussed:
> 1) put the config blob in userspace (/lib/firmware/...) -> not good
> because it makes the rootfs board-specific
> 2) auto-generate the config blob -> didn't work for me, last firmware
> chunk times out (just as if I had no config at all)
> 3) putting the config blob in DT -> possible but not very nice to read
>
> I had another idea:
> what if we mix solution 1) and 2)?
> the idea: load the config blob from userspace (/lib/firmware/...) and
> update it's settings with the values we got from devicetree-properties
> / DSDT
If you are shipping already the config blob in userspace I don't see
why you would do the update since you have already all the info you
need.
I would rather check why (2) didn't work fine. Have you any
documentation how the config blobs are generated? The code in
hciattach_rtk.c seems a good starting point.
> I have not tested this yet, but I just want to hear everyone's (at
> least Marcel, Rob and Carlo) opinion on that
> (this would also allow us to fully auto-generate the config blob in
> the future once we figure out how to do that)
>
>> Adding the ACPI support on top of your patches is (hopefully) trivial,
>> just follow what was done for hci_bcm.c, basically adding a new _HID
>> and reading the configuration for GPIOs and UART, all the rest should
>> be transparent for serdev.
> thanks for the reference to hci_bcm.c - I will have a look at this for
> the next version
> one question: "_HID" would be OBDA8723 in our case?
Yes
>> I'll test your patches on the hardware I have.
> great, thank you!
Cheers!
--
Carlo Caione | +44.7384.69.16.04 | Endless
^ permalink raw reply
* Re: [PATCH v5 26/39] nds32: Device tree support
From: Greentime Hu @ 2018-01-04 7:57 UTC (permalink / raw)
To: Rob Herring
Cc: Greentime, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Arnd Bergmann, linux-arch, Thomas Gleixner, Jason Cooper,
Marc Zyngier, netdev, Vincent Chen,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg
In-Reply-To: <CAL_Jsq+CC-3w8BVcUP77__ZR8aYMhxiXDYJ--HZwA=ezHG548g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-04 3:14 GMT+08:00 Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Tue, Jan 2, 2018 at 2:24 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>
>> This patch adds support for device tree.
>>
>> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>> ---
>> arch/nds32/boot/dts/Makefile | 8 +++++
>> arch/nds32/boot/dts/ae3xx.dts | 73 +++++++++++++++++++++++++++++++++++++++++
>> arch/nds32/kernel/devtree.c | 19 +++++++++++
>> 3 files changed, 100 insertions(+)
>> create mode 100644 arch/nds32/boot/dts/Makefile
>> create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>> create mode 100644 arch/nds32/kernel/devtree.c
>>
>> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
>> new file mode 100644
>> index 0000000..d31faa8
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/Makefile
>> @@ -0,0 +1,8 @@
>> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'
>> +BUILTIN_DTB := $(patsubst "%",%,$(CONFIG_NDS32_BUILTIN_DTB)).dtb.o
>> +else
>> +BUILTIN_DTB :=
>> +endif
>> +obj-$(CONFIG_OF) += $(BUILTIN_DTB)
>> +
>> +clean-files := *.dtb *.dtb.S
>> diff --git a/arch/nds32/boot/dts/ae3xx.dts b/arch/nds32/boot/dts/ae3xx.dts
>> new file mode 100644
>> index 0000000..6b23d60
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/ae3xx.dts
>> @@ -0,0 +1,73 @@
>> +/dts-v1/;
>> +/ {
>> + compatible = "andestech,ae3xx";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + interrupt-parent = <&intc>;
>> +
>> + chosen {
>> + stdout-path = &serial0;
>> + };
>> +
>> + memory@0 {
>> + device_type = "memory";
>> + reg = <0x00000000 0x40000000>;
>> + };
>> +
>> + cpus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + cpu@0 {
>> + device_type = "cpu";
>> + compatible = "andestech,n13", "andestech,nds32v3";
>> + reg = <0>;
>> + clock-frequency = <60000000>;
>> + next-level-cache = <&L2>;
>> + };
>> + };
>> +
>> + L2: l2-cache@e0500000 {
>> + compatible = "andestech,atl2c";
>> + reg = <0xe0500000 0x1000>;
>> + cache-unified;
>> + cache-level = <2>;
>> + };
>> +
>> + apb: clk@0 {
>
> unit address without reg is not valid. Drop the "@0".
>
>> + #clock-cells = <0>;
>> + compatible = "fixed-clock";
>> + clock-frequency = <30000000>;
>> + };
>> +
>> +
>> + intc: interrupt-controller {
>> + compatible = "andestech,ativic32";
>> + #interrupt-cells = <1>;
>> + interrupt-controller;
>> + };
>> +
>> + serial0: serial@f0300000 {
>
> All the memory mapped peripherals should be under at least one simple-bus node.
>
>> + compatible = "andestech,uart16550", "ns16550a";
>> + reg = <0xf0300000 0x1000>;
>> + interrupts = <8>;
>> + clock-frequency = <14745600>;
>> + reg-shift = <2>;
>> + reg-offset = <32>;
>> + no-loopback-test = <1>;
>> + };
>> +
>> + timer0: timer@f0400000 {
>> + compatible = "andestech,atcpit100";
>> + reg = <0xf0400000 0x1000>;
>> + interrupts = <2>;
>> + clocks = <&apb>;
>> + clock-names = "PCLK";
>> + };
>> +
>> + mac0: mac@e0100000 {
>
> ethernet@...
>
Hi, Rob:
I'd like to modify it like this in the next version patch.
clock: clk {
#clock-cells = <0>;
compatible = "fixed-clock";
clock-frequency = <30000000>;
};
apb {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges;
serial0: serial@f0300000 {
compatible = "andestech,uart16550", "ns16550a";
reg = <0xf0300000 0x1000>;
interrupts = <8>;
clock-frequency = <14745600>;
reg-shift = <2>;
reg-offset = <32>;
no-loopback-test = <1>;
};
timer0: timer@f0400000 {
compatible = "andestech,atcpit100";
reg = <0xf0400000 0x1000>;
interrupts = <2>;
clocks = <&clock>;
clock-names = "PCLK";
};
};
ahb {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges;
L2: cache-controller@e0500000 {
compatible = "andestech,atl2c";
reg = <0xe0500000 0x1000>;
cache-unified;
cache-level = <2>;
};
mac0: ethernet@e0100000 {
compatible = "andestech,atmac100";
reg = <0xe0100000 0x1000>;
interrupts = <18>;
};
};
--
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
* [PATCH -next] serial: 8250_uniphier: fix error return code in uniphier_uart_probe()
From: Wei Yongjun @ 2018-01-04 7:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Masahiro Yamada
Cc: Wei Yongjun, linux-arm-kernel, linux-serial
Fix to return a negative error code from the port register error
handling case instead of 0, as done elsewhere in this function.
Fixes: 39be40ce066d ("serial: 8250_uniphier: fix serial port index in private data")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/tty/serial/8250/8250_uniphier.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index 45ef506..28d88ccf 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -250,12 +250,13 @@ static int uniphier_uart_probe(struct platform_device *pdev)
up.dl_read = uniphier_serial_dl_read;
up.dl_write = uniphier_serial_dl_write;
- priv->line = serial8250_register_8250_port(&up);
- if (priv->line < 0) {
+ ret = serial8250_register_8250_port(&up);
+ if (ret < 0) {
dev_err(dev, "failed to register 8250 port\n");
clk_disable_unprepare(priv->clk);
return ret;
}
+ priv->line = ret;
platform_set_drvdata(pdev, priv);
^ permalink raw reply related
* Re: [PATCH -next] serial: 8250_uniphier: fix error return code in uniphier_uart_probe()
From: Masahiro Yamada @ 2018-01-04 7:41 UTC (permalink / raw)
To: Wei Yongjun
Cc: Greg Kroah-Hartman, linux-arm-kernel, linux-serial, Jiri Slaby
In-Reply-To: <1515051735-59992-1-git-send-email-weiyongjun1@huawei.com>
2018-01-04 16:42 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
> Fix to return a negative error code from the port register error
> handling case instead of 0, as done elsewhere in this function.
>
> Fixes: 39be40ce066d ("serial: 8250_uniphier: fix serial port index in private data")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Thanks!
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: fix endless loop during suspend
From: Martin Kaiser @ 2018-01-03 21:43 UTC (permalink / raw)
To: Fabio Estevam
Cc: Greg Kroah-Hartman, Jiri Slaby, Sascha Hauer, Philipp Zabel,
Shawn Guo, linux-serial, linux-kernel, stable
In-Reply-To: <CAOMZO5BVQE45W6cSaPVLkDWAvTsaR54FPWD20krKCpgdGLX3CQ@mail.gmail.com>
Hi Fabio,
Thus wrote Fabio Estevam (festevam@gmail.com):
> I am not able to reproduce this problem on a imx25 pdk running 4.14.11
> or linux-next.
this is no surprise. The problem shows up only if the AWAKE bit in UART
Status Register 1 is set before we go into suspend. My understanding is
that this bit will be set when the signal on the rx pin goes from high
to low.
> Is it 100% reproducible on your board?
On my board, I have one uart port that's connected to an external chip.
A power cycle of this external chip creates the falling edge on rx and
makes the imx uart set the AWAKE bit. The problem can then be reproduced
100%.
It can be argued that this is an obscure scenario, but nevertheless, it
should not put the kernel into an endless loop.
This is my understanding of what happens:
- AWAKE is set in the USR1 register
- there's no uart transfer running, the clocks are disabled
(As I write this I'm not sure why this is the case, clk_ipg should
still be enabled but it seems that it's not. If I enable it
manually, the behaviour is different.)
- we enter suspend
- AWAKEN (UART control register 3) is set so that AWAKE creates an interrupt
- we get an interrupt immediately
(the imx interrupt is not yet disabled at this point. it'll be
disabled later and then reenabled if the uart port acts as a wakeup
source)
-> we get into the interrupt handler with the clocks disabled
- the interrupt handler tries to clear the AWAKE bit, this does not work
since the clocks are off, we exit and AWAKE is still set
- we get another interrupt immediately
-> endless loop
What I'm trying to do with my patch is to clear the AWAKE bit before we
set AWAKEEN. We don't want to be woken up by events that happened before
we started going into suspend.
To do this, I have to find a suitable place where the clocks are
enabled. That's why I tried to move clearing AWAKE and setting AWAKEEN
into suspend_noirq, where the clocks are already enabled to save all
registers before we finally suspend. While this works ok on my board, it
cuases problems on imx6q.
I'm not sure what makes my patch fail for you. I could imagine it is
because now, the imx interrupt is enabled (as a wakeup source) before
AWAKEN is set. The current code sets AWAKEN first and then enables the
interrupt for the wakeup source.
I'll try a different approach that keeps this order.
> Care to share its dts? Do you use multiple UART ports? Do any of them
> use RTS/CTS?
There's nothing special regarding the uarts, There's a couple of them,
none of which are using rts/cts.
It all depends on the awake bit.
Best regards,
Martin
^ 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