Re: [PATCH v4] arm64: dts: imx8mq: Init rates and parents configs for clocks
On 2019-10-23 9:29 AM, Viorel Suman wrote:
> On Mi, 2019-08-21 at 20:39 +0000, Leonard Crestez wrote:
>> The audio PLLs should run below 650 mHz so please use 393216000 and
>> 361267200 instead of 786432000 and 722534400. For the 8mm equivalent see
>> commit 053a4ffe2988 ("clk: imx: imx8mm: fix audio pll setting").
>
> Hi Leonard,
>
> Audio PLL IP on 8mm and 8mn is different than the Audio PLL IP on 8mq,
> so the requirement to run below 650 MHZ may not apply to 8mq.
This "max 650mHz" limit is from internal ADD and is also mentioned for
imx8mq.
Peng: you made the change in our internal tree, can you confirm this
requirement also applies to 8mq?
Viorel: Is there any impact from 393216000 vs 786432000 on PLL on audio?
As far as I can know this rate goes through various dividers anyway.
--
Regards,
Leonard
Re: [PATCH v2] scripts/gdb: fix lx-dmesg when CONFIG_PRINTK_CALLER is set
On 11.10.2019 16:02, Joel Colledge wrote: > On Fri, Oct 11, 2019 at 2:47 PM Leonard Crestez > wrote: >> This struct printk_log is quite small, I wonder if it's possible to do a >> single read for each log entry? This might make lx-dmesg faster because >> of fewer roundtrips to gdbserver and jtag (or whatever backend you're >> using). > > I think this is already covered. utils.read_memoryview uses > inferior.read_memory and I think that reads the entire log buffer at > once (at most 2 reads, one for each half). You're right, sorry
Re: [PATCH v2] scripts/gdb: fix lx-dmesg when CONFIG_PRINTK_CALLER is set
On 11.10.2019 15:25, Joel Colledge wrote:
> When CONFIG_PRINTK_CALLER is set, struct printk_log contains an
> additional member caller_id. This affects the offset of the log text.
> Account for this by using the type information from gdb to determine all
> the offsets instead of using hardcoded values.
>
> This fixes following error:
>
>(gdb) lx-dmesg
>Python Exception embedded null character:
>Error occurred in Python command: embedded null character
>
> Signed-off-by: Joel Colledge
> ---
> Changes in v2:
> - use type information from gdb instead of hardcoded offsets
>
> Thanks for the idea about using the struct layout info from gdb, Leonard. I
> can't see any reason we shouldn't use that here, since most of the other
> commands use it. LxDmesg has used hardcoded offsets since scripts/gdb was
> introduced, so I assume it just ended up like that during the initial
> development of the tool. Here is a version of the fix using offsets from gdb.
This struct printk_log is quite small, I wonder if it's possible to do a
single read for each log entry? This might make lx-dmesg faster because
of fewer roundtrips to gdbserver and jtag (or whatever backend you're
using).
>
> scripts/gdb/linux/dmesg.py | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
> index 6d2e09a2ad2f..8f5d899029b7 100644
> --- a/scripts/gdb/linux/dmesg.py
> +++ b/scripts/gdb/linux/dmesg.py
> @@ -16,6 +16,8 @@ import sys
>
> from linux import utils
>
> +printk_log_type = utils.CachedType("struct printk_log")
> +
>
> class LxDmesg(gdb.Command):
> """Print Linux kernel log buffer."""
> @@ -42,9 +44,14 @@ class LxDmesg(gdb.Command):
> b = utils.read_memoryview(inf, log_buf_addr, log_next_idx)
> log_buf = a.tobytes() + b.tobytes()
>
> +length_offset = printk_log_type.get_type()['len'].bitpos // 8
> +text_len_offset = printk_log_type.get_type()['text_len'].bitpos // 8
> +time_stamp_offset = printk_log_type.get_type()['ts_nsec'].bitpos // 8
> +text_offset = printk_log_type.get_type().sizeof
> +
> pos = 0
> while pos < log_buf.__len__():
> -length = utils.read_u16(log_buf[pos + 8:pos + 10])
> +length = utils.read_u16(log_buf[pos + length_offset:pos +
> length_offset + 2])
> if length == 0:
> if log_buf_2nd_half == -1:
> gdb.write("Corrupted log buffer!\n")
> @@ -52,10 +59,11 @@ class LxDmesg(gdb.Command):
> pos = log_buf_2nd_half
> continue
>
> -text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
> -text = log_buf[pos + 16:pos + 16 + text_len].decode(
> +text_len = utils.read_u16(log_buf[pos + text_len_offset:pos +
> text_len_offset + 2])
> +text = log_buf[pos + text_offset:pos + text_offset +
> text_len].decode(
> encoding='utf8', errors='replace')
> -time_stamp = utils.read_u64(log_buf[pos:pos + 8])
> +time_stamp = utils.read_u64(
> +log_buf[pos + time_stamp_offset:pos + time_stamp_offset + 8])
>
> for line in text.splitlines():
> msg = u"[{time:12.6f}] {line}\n".format(
>
Re: [PATCH] scripts/gdb: fix lx-dmesg when CONFIG_PRINTK_CALLER is set
On 10.10.2019 18:14, Jan Kiszka wrote: > On 25.09.19 17:03, Joel Colledge wrote: >> When CONFIG_PRINTK_CALLER is set, struct printk_log contains an >> additional member caller_id. As a result, the offset of the log text is >> different. >> >> This fixes the following error: >> >>(gdb) lx-dmesg >>Python Exception embedded null character: >>Error occurred in Python command: embedded null character >> >> Signed-off-by: Joel Colledge >> --- >> scripts/gdb/linux/constants.py.in | 1 + >> scripts/gdb/linux/dmesg.py| 4 +++- >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/gdb/linux/constants.py.in >> b/scripts/gdb/linux/constants.py.in >> index 2efbec6b6b8d..3c9794a0bf55 100644 >> --- a/scripts/gdb/linux/constants.py.in >> +++ b/scripts/gdb/linux/constants.py.in >> @@ -74,4 +74,5 @@ LX_CONFIG(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) >> LX_CONFIG(CONFIG_HIGH_RES_TIMERS) >> LX_CONFIG(CONFIG_NR_CPUS) >> LX_CONFIG(CONFIG_OF) >> +LX_CONFIG(CONFIG_PRINTK_CALLER) >> LX_CONFIG(CONFIG_TICK_ONESHOT) >> diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py >> index 6d2e09a2ad2f..1352680ef731 100644 >> --- a/scripts/gdb/linux/dmesg.py >> +++ b/scripts/gdb/linux/dmesg.py >> @@ -14,6 +14,7 @@ >> import gdb >> import sys >> >> +from linux import constants >> from linux import utils >> >> >> @@ -53,7 +54,8 @@ class LxDmesg(gdb.Command): >> continue >> >> text_len = utils.read_u16(log_buf[pos + 10:pos + 12]) >> -text = log_buf[pos + 16:pos + 16 + text_len].decode( >> +text_start = pos + (20 if constants.LX_CONFIG_PRINTK_CALLER >> else 16) >> +text = log_buf[text_start:text_start + text_len].decode( >> encoding='utf8', errors='replace') >> time_stamp = utils.read_u64(log_buf[pos:pos + 8]) > > Sorry for the delay: > > Reviewed-by: Jan Kiszka > > I suspect we will see more in nearer future with upcoming printk rework... The patch looks correct but I'm curious: is there a reason this code doesn't use struct printk_log? GDB already knows about struct offsets so there should be no need to handle ifdefs with arithmetic. Is it realistic to use gdb without struct layout info? -- Regards, Leonard
Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
On 2019-09-27 4:20 AM, Anson Huang wrote:
>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>> On 19-09-26 08:03, Anson Huang wrote:
> On 19-09-25 18:07, Anson Huang wrote:
>> The SCU firmware does NOT always have return value stored in
>> message header's function element even the API has response data,
>> those special APIs are defined as void function in SCU firmware, so
>> they should be treated as return success always.
>>
>> +static const struct imx_sc_rpc_msg whitelist[] = {
>> +{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> IMX_SC_MISC_FUNC_UNIQUE_ID },
>> +{ .svc = IMX_SC_RPC_SVC_MISC, .func =
>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>
> Is this going to be extended in the near future? I see some upcoming
> problems here if someone uses a different scu-fw<->kernel
> combination as nxp would suggest.
Could be, but I checked the current APIs, ONLY these 2 will be used
in Linux kernel, so I ONLY add these 2 APIs for now.
>>>
>>> Okay.
>>>
However, after rethink, maybe we should add another imx_sc_rpc API
for those special APIs? To avoid checking it for all the APIs called which
>> may impact some performance.
Still under discussion, if you have better idea, please advise, thanks!
>>
>> My suggestion is to refactor the code and add a new API for the this "no
>> error value" convention. Internally they can call a common function with
>> flags.
>
> If I understand your point correctly, that means the loop check of whether
> the API
> is with "no error value" for every API still NOT be skipped, it is just
> refactoring the code,
> right?
There would be no "loop" anywhere: the responsibility would fall on the
call to call the right RPC function. In the current layering scheme
(drivers -> RPC -> mailbox) the RPC layer treats all calls the same and
it's up the the caller to provide information about calling convention.
An example implementation:
* Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
* Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp to
a flag
* Make get button status call __imx_sc_rpc_call_flags with the
_IMX_SC_RPC_NOERROR flag
Hope this makes my suggestion clearer? Pushing this to the caller is a
bit ugly but I think it's worth preserving the fact that the imx rpc
core treats services in an uniform way.
>>> Adding a special api shouldn't be the right fix. Imagine if someone
>>> (not a nxp-developer) wants to add a new driver. How could he be
>>> expected to know which api he should use. The better abbroach would be
>>> to fix the scu-fw instead of adding quirks..
>
> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner,
> the SCU
> FW released has been finalized, so the API implementation can NOT be changed,
> but
> they will pay attention to this issue for new added APIs later. That means
> the number
> of APIs having this issue a very limited.
>
>>
>> Right now developers who want to make SCFW calls in upstream need to
>> define the message struct in their driver based on protocol documentation.
>> This includes:
>>
>> * Binary layout of the message (a packed struct)
>> * If the message has a response (already a bool flag)
>> * If an error code is returned (this patch adds support for it)
>>
>> Since callers are already exposed to the binary protocol exposing them to
>> minor quirks of the calling convention also seems reasonable. Having the
>> low-level IPC code peek at message IDs seems like a hack; this belong at a
>> slightly higher level.
>
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message
> IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from
> imx_scu_call_rpc()
> function, am I right?
No, I am saying that the caller (button driver) should be responsible
for calling with a special flag which states that the RPC call.
In internal tree this is handled inside the machine-generated function
calls, right? These are mostly skipped in upstream but maybe for these
particular calls we can manually add wrappers inside
"drivers/firmware/imx/misc.c".
And even if the functions "return void" from a SCFW perspective it still
makes sense to return general kernel errors, for example from mailbox.
--
Regards,
Leonard
Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
On 27.09.2019 12:06, Marco Felsch wrote:
> Hi Anson, Leonard,
>
> On 19-09-27 01:20, Anson Huang wrote:
>> Hi, Leonard
>>
>>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
On 19-09-26 08:03, Anson Huang wrote:
>> On 19-09-25 18:07, Anson Huang wrote:
>>> The SCU firmware does NOT always have return value stored in
>>> message header's function element even the API has response data,
>>> those special APIs are defined as void function in SCU firmware, so
>>> they should be treated as return success always.
>>>
>>> +static const struct imx_sc_rpc_msg whitelist[] = {
>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>> IMX_SC_MISC_FUNC_UNIQUE_ID },
>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>>
>> Is this going to be extended in the near future? I see some upcoming
>> problems here if someone uses a different scu-fw<->kernel
>> combination as nxp would suggest.
>
> Could be, but I checked the current APIs, ONLY these 2 will be used
> in Linux kernel, so I ONLY add these 2 APIs for now.
Okay.
> However, after rethink, maybe we should add another imx_sc_rpc API
> for those special APIs? To avoid checking it for all the APIs called which
>>> may impact some performance.
> Still under discussion, if you have better idea, please advise, thanks!
>>>
>>> My suggestion is to refactor the code and add a new API for the this "no
>>> error value" convention. Internally they can call a common function with
>>> flags.
>>
Adding a special api shouldn't be the right fix. Imagine if someone
(not a nxp-developer) wants to add a new driver. How could he be
expected to know which api he should use. The better abbroach would be
to fix the scu-fw instead of adding quirks..
>>
>> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner,
>> the SCU
>> FW released has been finalized, so the API implementation can NOT be
>> changed, but
>> they will pay attention to this issue for new added APIs later. That means
>> the number
>> of APIs having this issue a very limited.
>
> This means those APIs which already have this bug will not be fixed?
> IMHO this sounds a bit weird since this is a changeable peace of code ;)
It's not a bug, it's a documented feature ;)
>>> Right now developers who want to make SCFW calls in upstream need to
>>> define the message struct in their driver based on protocol documentation.
>>> This includes:
>>>
>>> * Binary layout of the message (a packed struct)
>>> * If the message has a response (already a bool flag)
>>> * If an error code is returned (this patch adds support for it)
>
> Why should I specify if a error code is returned?
Because you're already defining the message struct and you're already
specifying if a response is required.
The assumption is that anyone adding a SCFW call to a driver is already
looking at SCFW documentation which describes the binary message format.
--
Regards,
Leonard
Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
On 27.09.2019 04:20, Anson Huang wrote:
>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>> On 19-09-26 08:03, Anson Huang wrote:
> On 19-09-25 18:07, Anson Huang wrote:
>> The SCU firmware does NOT always have return value stored in
>> message header's function element even the API has response data,
>> those special APIs are defined as void function in SCU firmware, so
>> they should be treated as return success always.
>>
>> +static const struct imx_sc_rpc_msg whitelist[] = {
>> +{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> IMX_SC_MISC_FUNC_UNIQUE_ID },
>> +{ .svc = IMX_SC_RPC_SVC_MISC, .func =
>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>
> Is this going to be extended in the near future? I see some upcoming
> problems here if someone uses a different scu-fw<->kernel
> combination as nxp would suggest.
Could be, but I checked the current APIs, ONLY these 2 will be used
in Linux kernel, so I ONLY add these 2 APIs for now.
>>>
>>> Okay.
>>>
However, after rethink, maybe we should add another imx_sc_rpc API
for those special APIs? To avoid checking it for all the APIs called which
>> may impact some performance.
Still under discussion, if you have better idea, please advise, thanks!
>>
>> My suggestion is to refactor the code and add a new API for the this "no
>> error value" convention. Internally they can call a common function with
>> flags.
>
> If I understand your point correctly, that means the loop check of whether
> the API
> is with "no error value" for every API still NOT be skipped, it is just
> refactoring the code,
> right?
>> Right now developers who want to make SCFW calls in upstream need to
>> define the message struct in their driver based on protocol documentation.
>> This includes:
>>
>> * Binary layout of the message (a packed struct)
>> * If the message has a response (already a bool flag)
>> * If an error code is returned (this patch adds support for it)
>>
>> Since callers are already exposed to the binary protocol exposing them to
>> minor quirks of the calling convention also seems reasonable. Having the
>> low-level IPC code peek at message IDs seems like a hack; this belong at a
>> slightly higher level.
>
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message
> IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from
> imx_scu_call_rpc()
> function, am I right?
No, I mean there should be no loop enumerating svc/func ids: *the caller
should know* that it's calling a func which doesn't return an error code
and call a different variant of imx_scu_call_rpc
Maybe add an internal __imx_scu_call_rpc_flags and turn the current
imx_scu_call_rpc into a wrapper.
--
Regards,
Leonard
Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
On 2019-09-26 1:06 PM, Marco Felsch wrote:
> On 19-09-26 08:03, Anson Huang wrote:
>>> On 19-09-25 18:07, Anson Huang wrote:
The SCU firmware does NOT always have return value stored in message
header's function element even the API has response data, those
special APIs are defined as void function in SCU firmware, so they
should be treated as return success always.
+static const struct imx_sc_rpc_msg whitelist[] = {
+ { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
+ { .svc = IMX_SC_RPC_SVC_MISC, .func =
+IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>>>
>>> Is this going to be extended in the near future? I see some upcoming
>>> problems here if someone uses a different scu-fw<->kernel combination as
>>> nxp would suggest.
>>
>> Could be, but I checked the current APIs, ONLY these 2 will be used in Linux
>> kernel, so
>> I ONLY add these 2 APIs for now.
>
> Okay.
>
>> However, after rethink, maybe we should add another imx_sc_rpc API for those
>> special
>> APIs? To avoid checking it for all the APIs called which may impact some
>> performance.
>> Still under discussion, if you have better idea, please advise, thanks!
My suggestion is to refactor the code and add a new API for the this "no
error value" convention. Internally they can call a common function with
flags.
> Adding a special api shouldn't be the right fix. Imagine if someone (not
> a nxp-developer) wants to add a new driver. How could he be expected to
> know which api he should use. The better abbroach would be to fix the
> scu-fw instead of adding quirks..
Right now developers who want to make SCFW calls in upstream need to
define the message struct in their driver based on protocol
documentation. This includes:
* Binary layout of the message (a packed struct)
* If the message has a response (already a bool flag)
* If an error code is returned (this patch adds support for it)
Since callers are already exposed to the binary protocol exposing them
to minor quirks of the calling convention also seems reasonable. Having
the low-level IPC code peek at message IDs seems like a hack; this
belong at a slightly higher level.
In older internal trees we use a very large amount of generated wrapper
functions. This hides calling convention details from callers but is
extremely ugly and verbose.
--
Regards,
Leonard
Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
On 25.09.2019 13:09, Anson Huang wrote:
> The SCU firmware does NOT always have return value stored in message
> header's function element even the API has response data, those special
> APIs are defined as void function in SCU firmware, so they should be
> treated as return success always.
>
> Signed-off-by: Anson Huang
> ---
> - This patch is based on the patch of
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11129553%2Fdata=02%7C01%7Cleonard.crestez%40nxp.com%7Cc0ced6cd07f04023977008d741a07367%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637050029712216472sdata=Ccq%2Fb2RJdMqmnL7VXrl8YhOlUwC7bWiUG%2BNmiw4OsSM%3Dreserved=0
> ---
> drivers/firmware/imx/imx-scu.c | 34 --
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 869be7a..ced5b12 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -78,6 +78,11 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] = {
> -EIO,/* IMX_SC_ERR_FAIL */
> };
>
> +static const struct imx_sc_rpc_msg whitelist[] = {
> + { .svc = IMX_SC_RPC_SVC_MISC, .func = IMX_SC_MISC_FUNC_UNIQUE_ID },
> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
> IMX_SC_MISC_FUNC_GET_BUTTON_STATUS },
> +};
Until now this low level IPC code didn't treat any svc/func specially
and this seems good.
The imx_scu_call_rpc function already has an have_resp argument and
callers are responsible to fill it. Can't we deal with this by adding an
additional err_ret flag passed by the caller?
We can add wrapper functions to avoid tree-wide changes for all callers.
> +
> static struct imx_sc_ipc *imx_sc_ipc_handle;
>
> static inline int imx_sc_to_linux_errno(int errno)
> @@ -157,11 +162,24 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc,
> void *msg)
> return 0;
> }
>
> +static bool imx_scu_call_skip_return_value_check(uint8_t svc, uint8_t func)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(whitelist); i++)
> + if (svc == whitelist[i].svc &&
> + func == whitelist[i].func)
> + return true;
> +
> + return false;
> +}
> +
> /*
>* RPC command/response
>*/
> int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
> {
> + uint8_t saved_svc, saved_func;
> struct imx_sc_rpc_msg *hdr;
> int ret;
>
> @@ -171,8 +189,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> *msg, bool have_resp)
> mutex_lock(_ipc->lock);
> reinit_completion(_ipc->done);
>
> - if (have_resp)
> + if (have_resp) {
> sc_ipc->msg = msg;
> + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
> + saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
> + }
> sc_ipc->count = 0;
> ret = imx_scu_ipc_write(sc_ipc, msg);
> if (ret < 0) {
> @@ -190,7 +211,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> *msg, bool have_resp)
>
> /* response status is stored in hdr->func field */
> hdr = msg;
> - ret = hdr->func;
> + /*
> + * Some special SCU firmware APIs do NOT have return value
> + * in hdr->func, but they do have response data, those special
> + * APIs are defined as void function in SCU firmware, so they
> + * should be treated as return success always.
> + */
> + if (!imx_scu_call_skip_return_value_check(saved_svc,
> saved_func))
> + ret = hdr->func;
> + else
> + ret = 0;
> }
>
> out:
>
Re: [PATCH AUTOSEL 4.19 075/128] PM / devfreq: passive: Use non-devm notifiers
On 22.09.2019 21:56, Sasha Levin wrote:
> From: Leonard Crestez
>
> [ Upstream commit 0ef7c7cce43f6ecc2b96d447e69b2900a9655f7c ]
This will introduce an "unused variable warning" unless you also
cherry-pick commit 0465814831a9 ("PM / devfreq: passive: fix compiler
warning").
> The devfreq passive governor registers and unregisters devfreq
> transition notifiers on DEVFREQ_GOV_START/GOV_STOP using devm wrappers.
>
> If devfreq itself is registered with devm then a warning is triggered on
> rmmod from devm_devfreq_unregister_notifier. Call stack looks like this:
>
> devm_devfreq_unregister_notifier+0x30/0x40
> devfreq_passive_event_handler+0x4c/0x88
> devfreq_remove_device.part.8+0x6c/0x9c
> devm_devfreq_dev_release+0x18/0x20
> release_nodes+0x1b0/0x220
> devres_release_all+0x78/0x84
> device_release_driver_internal+0x100/0x1c0
> driver_detach+0x4c/0x90
> bus_remove_driver+0x7c/0xd0
> driver_unregister+0x2c/0x58
> platform_driver_unregister+0x10/0x18
> imx_devfreq_platdrv_exit+0x14/0xd40 [imx_devfreq]
>
> This happens because devres_release_all will first remove all the nodes
> into a separate todo list so the nested devres_release from
> devm_devfreq_unregister_notifier won't find anything.
>
> Fix the warning by calling the non-devm APIS for frequency notification.
> Using devm wrappers is not actually useful for a governor anyway: it
> relies on the devfreq core to correctly match the GOV_START/GOV_STOP
> notifications.
>
> Fixes: 996133119f57 ("PM / devfreq: Add new passive governor")
> Signed-off-by: Leonard Crestez
> Acked-by: Chanwoo Choi
> Signed-off-by: MyungJoo Ham
> Signed-off-by: Sasha Levin
> ---
> drivers/devfreq/governor_passive.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/devfreq/governor_passive.c
> b/drivers/devfreq/governor_passive.c
> index 3bc29acbd54e8..22fd41b4c1098 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -168,12 +168,12 @@ static int devfreq_passive_event_handler(struct devfreq
> *devfreq,
> p_data->this = devfreq;
>
> nb->notifier_call = devfreq_passive_notifier_call;
> - ret = devm_devfreq_register_notifier(dev, parent, nb,
> + ret = devfreq_register_notifier(parent, nb,
> DEVFREQ_TRANSITION_NOTIFIER);
> break;
> case DEVFREQ_GOV_STOP:
> - devm_devfreq_unregister_notifier(dev, parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + WARN_ON(devfreq_unregister_notifier(parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER));
> break;
> default:
> break;
>
Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
On 04.09.2019 05:35, Andrey Smirnov wrote:
> Returning -EBUSY from platform device's .remove() callback won't stop
> the removal process, so the code in caam_jr_remove() is not going to
> have the desired effect of preventing JR from being removed.
>
> In order to be able to deal with removal of the JR device, change the
> code as follows:
>
>1. To make sure that underlying struct device remains valid for as
> long as we have a reference to it, add appropriate device
> refcount management to caam_jr_alloc() and caam_jr_free()
>
>2. To make sure that device removal doesn't happen in parallel to
>use using the device in caam_jr_enqueue() augment the latter to
>acquire/release device lock for the duration of the subroutine
>
>3. In order to handle the case when caam_jr_enqueue() is executed
> right after corresponding caam_jr_remove(), add code to check
> that driver data has not been set to NULL.
What happens if a driver is unbound while a transform is executing
asynchronously?
Doing get_device and put_device in caam_jr_alloc and caam_jr_free
doesn't protect you against an explicit unbind of caam_jr, that path
doesn't care about device reference counts. For example on imx6qp:
# echo 2102000.jr1 > /sys/bus/platform/drivers/caam_jr/unbind
CPU: 2 PID: 561 Comm: bash Not tainted 5.3.0-rc7-next-20190904
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x20/0x24)
[] (show_stack) from [] (dump_stack+0xdc/0x114)
[] (dump_stack) from [] (caam_jr_remove+0x24/0xf8)
[] (caam_jr_remove) from []
(platform_drv_remove+0x34/0x4c)
[] (platform_drv_remove) from []
(device_release_driver_internal+0xf8/0x1b0)
[] (device_release_driver_internal) from []
(device_driver_detach+0x20/0x24)
[] (device_driver_detach) from []
(unbind_store+0x6c/0xe0)
[] (unbind_store) from [] (drv_attr_store+0x30/0x3c)
[] (drv_attr_store) from [] (sysfs_kf_write+0x50/0x5c)
[] (sysfs_kf_write) from []
(kernfs_fop_write+0x10c/0x1f8)
[] (kernfs_fop_write) from [] (__vfs_write+0x44/0x1d0)
[] (__vfs_write) from [] (vfs_write+0xb0/0x194)
[] (vfs_write) from [] (ksys_write+0x64/0xd8)
[] (ksys_write) from [] (sys_write+0x18/0x1c)
[] (sys_write) from [] (ret_fast_syscall+0x0/0x28)
I think you need to do some form of slow wait loop in jrpriv until
jrpriv->tfm_count reaches zero. Preventing new operations from starting
would help but isn't strictly necessary for correctness.
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 47b389cb1c62..8a30bbd7f2aa 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -124,14 +124,6 @@ static int caam_jr_remove(struct platform_device *pdev)
> jrdev = >dev;
> jrpriv = dev_get_drvdata(jrdev);
>
> - /*
> - * Return EBUSY if job ring already allocated.
> - */
> - if (atomic_read(>tfm_count)) {
> - dev_err(jrdev, "Device is busy\n");
> - return -EBUSY;
> - }
> -
> /* Unregister JR-based RNG & crypto algorithms */
> unregister_algs();
>
> @@ -300,7 +292,7 @@ struct device *caam_jr_alloc(void)
>
> if (min_jrpriv) {
> atomic_inc(_jrpriv->tfm_count);
> - dev = min_jrpriv->dev;
> + dev = get_device(min_jrpriv->dev);
> }
> spin_unlock(_data.jr_alloc_lock);
>
> @@ -318,13 +310,16 @@ void caam_jr_free(struct device *rdev)
> struct caam_drv_private_jr *jrpriv = dev_get_drvdata(rdev);
>
> atomic_dec(>tfm_count);
> + put_device(rdev);
> }
> EXPORT_SYMBOL(caam_jr_free);
>
> /**
>* caam_jr_enqueue() - Enqueue a job descriptor head. Returns 0 if OK,
>* -EBUSY if the queue is full, -EIO if it cannot map the caller's
> - * descriptor.
> + * descriptor, -ENODEV if given device was removed and is no longer
> + * valid
> + *
>* @dev: device of the job ring to be used. This device should have
>*been assigned prior by caam_jr_register().
>* @desc: points to a job descriptor that execute our request. All
> @@ -354,15 +349,32 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
> u32 status, void *areq),
> void *areq)
> {
> - struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
> + struct caam_drv_private_jr *jrp;
> struct caam_jrentry_info *head_entry;
> int head, tail, desc_size;
> dma_addr_t desc_dma;
>
> + /*
> + * Lock the device to prevent it from being removed while we
> + * are using it
> + */
> + device_lock(dev);
> +
> + /*
> + * If driver data is NULL, it is very likely that this device
> + * was removed already. Nothing we can do here but bail out.
> + */
> + jrp = dev_get_drvdata(dev);
> + if (!jrp) {
> + device_unlock(dev);
> + return -ENODEV;
> + }
> +
> desc_size = (caam32_to_cpu(*desc) &
Re: [PATCH 2/2] ARM: dts: imx7d: Add opp-suspend property
On 2019-09-12 5:57 AM, Anson Huang wrote: > Add "opp-suspend" property for i.MX7D to make sure system > suspend with max available opp. > > Signed-off-by: Anson Huang Reviewed-by: Leonard Crestez
Re: [PATCH 1/2] ARM: dts: imx7d: Correct speed grading fuse settings
On 2019-09-12 5:57 AM, Anson Huang wrote: > The 800MHz opp speed grading fuse mask should be 0xd instead > of 0xf according to fuse map definition: > > SPEED_GRADING[1:0]MHz > 00 800 > 01 500 > 10 1000 > 11 1200 > > Signed-off-by: Anson Huang Reviewed-by: Leonard Crestez Are you going to add the 500mhz OPP as well?
Re: [PATCH V2 1/2] clk: imx8mm: Move 1443X/1416X PLL clock structure to common place
On 06.09.2019 04:35, Anson Huang wrote: > Many i.MX8M SoCs use same 1443X/1416X PLL, such as i.MX8MM, > i.MX8MN and later i.MX8M SoCs, moving these PLL definitions > to pll14xx driver can save a lot of duplicated code on each > platform. > > Meanwhile, no need to define PLL clock structure for every > module which uses same type of PLL, e.g., audio/video/dram use > 1443X PLL, arm/gpu/vpu/sys use 1416X PLL, define 2 PLL clock > structure for each group is enough. > > Signed-off-by: Anson Huang For both: Reviewed-by: Leonard Crestez
Re: [PATCH 1/2] clk: imx8mm: Move 1443X/1416X PLL clock structure to common place
On 05.09.2019 12:59, Anson Huang wrote:
> Many i.MX8M SoCs use same 1443X/1416X PLL, such as i.MX8MM,
> i.MX8MN and later i.MX8M SoCs, moving these PLL definitions
> to common place can save a lot of duplicated code on each
> platform.
There are lots of similarities between imx8m clocks, do you plan to do
combine them further?
> Meanwhile, no need to define PLL clock structure for every
> module which uses same type of PLL, e.g., audio/video/dram use
> 1443X PLL, arm/gpu/vpu/sys use 1416X PLL, define 2 PLL clock
> structure for each group is enough.
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> +const struct imx_pll14xx_rate_table imx_pll1416x_tbl[] = {
> + PLL_1416X_RATE(18U, 225, 3, 0),
> + PLL_1416X_RATE(16U, 200, 3, 0),
> + PLL_1416X_RATE(12U, 300, 3, 1),
> + PLL_1416X_RATE(10U, 250, 3, 1),
> + PLL_1416X_RATE(8U, 200, 3, 1),
> + PLL_1416X_RATE(75000U, 250, 2, 2),
> + PLL_1416X_RATE(7U, 350, 3, 2),
> + PLL_1416X_RATE(6U, 300, 3, 2),
> +};
> +
> +const struct imx_pll14xx_rate_table imx_pll1443x_tbl[] = {
> + PLL_1443X_RATE(65000U, 325, 3, 2, 0),
> + PLL_1443X_RATE(59400U, 198, 2, 2, 0),
> + PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
> + PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),
> +};
> +
> +struct imx_pll14xx_clk imx_1443x_pll = {
> + .type = PLL_1443X,
> + .rate_table = imx_pll1443x_tbl,
> + .rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
> +};
> +
> +struct imx_pll14xx_clk imx_1416x_pll = {
> + .type = PLL_1416X,
> + .rate_table = imx_pll1416x_tbl,
> + .rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
> +};
Perhaps these consts should be in clk-pll14xx.c? That way they won't be
compiled for imx6 as well.
--
Regards,
Leonard
Re: [PATCH] soc: imx: imx-scu: Getting UID from SCU should have response
On 2019-09-04 10:14 AM, Anson Huang wrote:
> The SCU firmware API for getting UID should have response,
> otherwise, the message stored in function stack could be
> released and then the response data received from SCU will be
> stored into that released stack and cause kernel NULL pointer
> dump.
This fix looks good, but looking at imx-scu code it seems that passing
the incorrect "have_resp" argument to imx_scu_call_rpc or just receiving
an unexpected message from SCFW will always result in kernel stack
corruption!
This is worth handling inside imx-scu itself: unless a response was
explicitly requested it should ignore and print a warning on rx, for
example by setting imx_sc_ipc to NULL when not waiting for a response.
Holding on to arbitrary stack pointers is bad.
--
Regards,
Leonard
> diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c
> index 50831eb..c68882e 100644
> --- a/drivers/soc/imx/soc-imx-scu.c
> +++ b/drivers/soc/imx/soc-imx-scu.c
> @@ -46,7 +46,7 @@ static ssize_t soc_uid_show(struct device *dev,
> hdr->func = IMX_SC_MISC_FUNC_UNIQUE_ID;
> hdr->size = 1;
>
> - ret = imx_scu_call_rpc(soc_ipc_handle, , false);
> + ret = imx_scu_call_rpc(soc_ipc_handle, , true);
> if (ret) {
> pr_err("%s: get soc uid failed, ret %d\n", __func__, ret);
> return ret;
Re: [PATCH V3 1/5] thermal: qoriq: Add clock operations
On 27.08.2019 04:51, Anson Huang wrote:
>> In an earlier series the CLK_IS_CRITICAL flags was removed from the TMU
>> clock so if the thermal driver doesn't explicitly enable it the system will
>> hang
>> on probe. This is what happens in linux-next right now!
>
> The thermal driver should be built with module, so default kernel should can
> boot
> up, do you modify the thermal driver as built-in?
>
>> Unless this patches is merged soon we'll end up with a 5.4-rc1 that doesn't
>> boot on imx8mq. An easy fix would be to drop/revert commit
>> 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
>> IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
>
> If the thermal driver is built as module, I think no need to revert the
> commit, but
> if by default thermal driver is built-in or mod probed, then yes, it should
> NOT break
> kernel boot up.
The qoriq_thermal driver is built as a module in defconfig and when
modules are properly installed in rootfs they will be automatically be
probed on boot and cause a hang.
I usually run nfsroot with modules:
make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
--
Regards,
Leonard
Re: [PATCH V3 1/5] thermal: qoriq: Add clock operations
On 7/30/2019 5:31 AM, anson.hu...@nxp.com wrote:
> From: Anson Huang
>
> Some platforms like i.MX8MQ has clock control for this module,
> need to add clock operations to make sure the driver is working
> properly.
>
> Signed-off-by: Anson Huang
> Reviewed-by: Guido Günther
This series looks good, do you think it can be merged in time for v5.4?
Today was v5.3-rc6.
In an earlier series the CLK_IS_CRITICAL flags was removed from the TMU
clock so if the thermal driver doesn't explicitly enable it the system
will hang on probe. This is what happens in linux-next right now!
Unless this patches is merged soon we'll end up with a 5.4-rc1 that
doesn't boot on imx8mq. An easy fix would be to drop/revert commit
951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
Merging patches out-of-order when they have hard (boot-breaking)
dependencies also breaks bisect.
--
Regards,
Leonard
Re: linux-next: build warning after merge of the devfreq tree
On 26.08.2019 14:50, Stephen Rothwell wrote:
> Hi all,
>
> After merging the devfreq tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> drivers/devfreq/governor_passive.c: In function
> 'devfreq_passive_event_handler':
> drivers/devfreq/governor_passive.c:152:17: warning: unused variable 'dev'
> [-Wunused-variable]
>struct device *dev = devfreq->dev.parent;
> ^~~
>
> Introduced by commit
>
>0ef7c7cce43f ("PM / devfreq: passive: Use non-devm notifiers")
>
An older version was merged somehow, this was fixed in v3:
https://patchwork.kernel.org/patch/11091793/
--
Regards,
Leonard
Re: [PATCH] ARM: imx: Drop imx_anatop_init()
On 22.08.2019 23:06, Andrey Smirnov wrote:
>> On 31.07.2019 21:01, Andrey Smirnov wrote:
>>> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
>>> detect in mxs_phy_hw_init()") in tree all of the necessary charger
>>> setup is done by the USB PHY driver which covers all of the affected
>>> i.MX6 SoCs.
>>>
>>> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
>>> datasheet it appears to have a different USB PHY IP block, so
>>> executing i.MX6 charger disable configuration seems unnecessary.
>>>
>>> -void __init imx_anatop_init(void)
>>> -{
>>> - anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
>>> - if (IS_ERR(anatop)) {
>>> - pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
>>> - return;
>>> - }
>>
>> This patch breaks suspend on imx6 in linux-next because the "anatop"
>> regmap is no longer initialized. This was found via bisect but
>> no_console_suspend prints a helpful stack anyway:
>>
>> (regmap_read) from [] (imx_anatop_enable_weak2p5+0x28/0x70)
>> (imx_anatop_enable_weak2p5) from []
>> (imx_anatop_pre_suspend+0x18/0x64)
>> (imx_anatop_pre_suspend) from [] (imx6q_pm_enter+0x60/0x16c)
>> (imx6q_pm_enter) from [] (suspend_devices_and_enter+0x7d4/0xcbc)
>> (suspend_devices_and_enter) from [] (pm_suspend+0x7b8/0x904)
>> (pm_suspend) from [] (state_store+0x68/0xc8)
>>
>
> My bad, completely missed that fact that anatop was a global variable
> in imx_anatop_init(). Sorry about that.
>
>> Minimal fix looks like this:
>>
>> --- arch/arm/mach-imx/anatop.c
>> +++ arch/arm/mach-imx/anatop.c
>> @@ -111,6 +111,12 @@ void __init imx_init_revision_from_anatop(void)
>>digprog = readl_relaxed(anatop_base + offset);
>>iounmap(anatop_base);
>>
>> + anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
>> + if (IS_ERR(anatop)) {
>> + pr_err("failed to find imx6q-anatop regmap!\n");
>> + return;
>> + }
>>
>> Since all SOCs that called imx_anatop_init also call
>> imx_init_revision_from_anatop this might be an acceptable solution,
>> unless there is some limitation preventing early regmap lookup.
>>
>
> Would making every function that uses anatop explicitly request it via
> syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop") be too much of
> a code duplication? This way we won't need to worry if
> imx_init_revision_from_anatop() was called before any of them are
> used.
It's only used from pre_suspend and post_suspend, everything else in
arch/arm/mach-imx/anatop.c is static. Doing a lookup every time would be
silly, it's fine to let this be global.
I was wondering if maybe imx_init_revision could somehow end up getting
called before syscon/regmap core init.
--
Regards,
Leonard
Re: [PATCH] ARM: imx: Drop imx_anatop_init()
On 31.07.2019 21:01, Andrey Smirnov wrote:
> With commit b5bbe2235361 ("usb: phy: mxs: Disable external charger
> detect in mxs_phy_hw_init()") in tree all of the necessary charger
> setup is done by the USB PHY driver which covers all of the affected
> i.MX6 SoCs.
>
> NOTE: Imx_anatop_init() was also called for i.MX7D, but looking at its
> datasheet it appears to have a different USB PHY IP block, so
> executing i.MX6 charger disable configuration seems unnecessary.
>
> -void __init imx_anatop_init(void)
> -{
> - anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
> - if (IS_ERR(anatop)) {
> - pr_err("%s: failed to find imx6q-anatop regmap!\n", __func__);
> - return;
> - }
This patch breaks suspend on imx6 in linux-next because the "anatop"
regmap is no longer initialized. This was found via bisect but
no_console_suspend prints a helpful stack anyway:
(regmap_read) from [] (imx_anatop_enable_weak2p5+0x28/0x70)
(imx_anatop_enable_weak2p5) from []
(imx_anatop_pre_suspend+0x18/0x64)
(imx_anatop_pre_suspend) from [] (imx6q_pm_enter+0x60/0x16c)
(imx6q_pm_enter) from [] (suspend_devices_and_enter+0x7d4/0xcbc)
(suspend_devices_and_enter) from [] (pm_suspend+0x7b8/0x904)
(pm_suspend) from [] (state_store+0x68/0xc8)
Minimal fix looks like this:
--- arch/arm/mach-imx/anatop.c
+++ arch/arm/mach-imx/anatop.c
@@ -111,6 +111,12 @@ void __init imx_init_revision_from_anatop(void)
digprog = readl_relaxed(anatop_base + offset);
iounmap(anatop_base);
+ anatop = syscon_regmap_lookup_by_compatible("fsl,imx6q-anatop");
+ if (IS_ERR(anatop)) {
+ pr_err("failed to find imx6q-anatop regmap!\n");
+ return;
+ }
Since all SOCs that called imx_anatop_init also call
imx_init_revision_from_anatop this might be an acceptable solution,
unless there is some limitation preventing early regmap lookup.
--
Regards,
Leonard
Re: [PATCH] clk: imx: pll14xx: avoid glitch when set rate
On 22.08.2019 12:18, Peng Fan wrote: >> Subject: Re: [PATCH] clk: imx: pll14xx: avoid glitch when set rate >> >> On 20.08.2019 05:17, Peng Fan wrote: >>> According to PLL1443XA and PLL1416X spec, "When BYPASS is 0 and RESETB >>> is changed from 0 to 1, FOUT starts to output unstable clock until >>> lock time passes. PLL1416X/PLL1443XA may generate a glitch at FOUT." >>> >>> So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch. >>> In the end of set rate, BYPASS will be cleared. >>> >>> @@ -191,6 +191,10 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, >> unsigned long drate, >>> tmp &= ~RST_MASK; >>> writel_relaxed(tmp, pll->base); >>> >>> + /* Enable BYPASS */ >>> + tmp |= BYPASS_MASK; >>> + writel(tmp, pll->base); >>> + >> >> Shouldn't BYPASS be set before reset? > > No. the glitch happens when RESET changes from 0 to 1, not from 1 to 0. You're right, sorry. >> Also, isn't a similar bypass/unbypass dance also needed in >> clk_pll14xx_prepare? As far as I understand that could also output glitches >> until the PLL is locked. It could be a separate patch. > > Yes, that might also output glitch. Fix in v2. > >> >> It's strange that this BYPASS bit is also handled by muxes like >> audio_pll1_bypass in clk-imx8mm.c but that's a separate issue not strictly >> related to the glitches you're trying to fix here. > > Yes, need use EXT_BYPASS for the mux usage. Might make sense to post as a series.
Re: [PATCH] clk: imx: pll14xx: avoid glitch when set rate
On 20.08.2019 05:17, Peng Fan wrote: > According to PLL1443XA and PLL1416X spec, > "When BYPASS is 0 and RESETB is changed from 0 to 1, FOUT starts to > output unstable clock until lock time passes. PLL1416X/PLL1443XA may > generate a glitch at FOUT." > > So set BYPASS when RESETB is changed from 0 to 1 to avoid glitch. > In the end of set rate, BYPASS will be cleared. > > @@ -191,6 +191,10 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, > unsigned long drate, > tmp &= ~RST_MASK; > writel_relaxed(tmp, pll->base); > > + /* Enable BYPASS */ > + tmp |= BYPASS_MASK; > + writel(tmp, pll->base); > + Shouldn't BYPASS be set before reset? Also, isn't a similar bypass/unbypass dance also needed in clk_pll14xx_prepare? As far as I understand that could also output glitches until the PLL is locked. It could be a separate patch. It's strange that this BYPASS bit is also handled by muxes like audio_pll1_bypass in clk-imx8mm.c but that's a separate issue not strictly related to the glitches you're trying to fix here. -- Regards, Leonard
Re: [PATCH v4] arm64: dts: imx8mq: Init rates and parents configs for clocks
On 28.07.2019 18:20, Daniel Baluta wrote:
> From: Abel Vesa
>
> Add the initial configuration for clocks that need default parent and rate
> setting. This is based on the vendor tree clock provider parents and rates
> configuration except this is doing the setup in dts rather then using clock
> consumer API in a clock provider driver.
>
> Note that by adding the initial rate setting for audio_pll1/audio_pll
> setting we need to remove it from imx8mq-librem5-devkit.dts
Setting default rates for audio_pll1 and audio_pll2 in soc dtsi makes a
lot of sense to me; the intention is for one to run at a multiple of
44.1k and another at a multiple of 48k.
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 02fbd0625318..a55d72ba2e05 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -494,6 +494,25 @@
> clock-names = "ckil", "osc_25m", "osc_27m",
> "clk_ext1", "clk_ext2",
> "clk_ext3", "clk_ext4";
> + assigned-clocks = < IMX8MQ_VIDEO_PLL1>,
> + < IMX8MQ_AUDIO_PLL1>,
> + < IMX8MQ_AUDIO_PLL2>,
> + < IMX8MQ_CLK_AHB>,
> + < IMX8MQ_CLK_NAND_USDHC_BUS>,
> + < IMX8MQ_CLK_AUDIO_AHB>,
> + < IMX8MQ_VIDEO_PLL1_REF_SEL>,
> + < IMX8MQ_CLK_NOC>;
> + assigned-clock-parents = <0>,
> + <0>,
> + <0> > +
> < IMX8MQ_SYS1_PLL_133M>,
> + < IMX8MQ_SYS1_PLL_266M>,
> + < IMX8MQ_SYS2_PLL_500M>,
> + < IMX8MQ_CLK_27M>,
> + < IMX8MQ_SYS1_PLL_800M>;
> + assigned-clock-rates = <59399>,
> + <786432000>,
> + <722534400>;
The audio PLLs should run below 650 mHz so please use 393216000 and
361267200 instead of 786432000 and 722534400. For the 8mm equivalent see
commit 053a4ffe2988 ("clk: imx: imx8mm: fix audio pll setting").
You should also move the unbypassing of AUDIO_PLL1 and AUDIO_PLL2 here
just add two more assigned-clocks and assigned-clock-parents.
--
Regards,
Leonard
Re: [PATCH 1/6] arm64: dts: imx8mn-ddr4-evk: Add i2c1 support
On 15.08.2019 14:18, anson.hu...@nxp.com wrote: > From: Anson Huang > > Enable i2c1 on i.MX8MN DDR4 EVK board. > > Signed-off-by: Anson Huang Didn't see a cover letter but all 6 patches look good: Reviewed-by: Leonard Crestez
[tip:perf/urgent] perf/core: Fix creating kernel counters for PMUs that override event->cpu
Commit-ID: 4ce54af8b33d3e21ca935fc1b89b58cbba956051 Gitweb: https://git.kernel.org/tip/4ce54af8b33d3e21ca935fc1b89b58cbba956051 Author: Leonard Crestez AuthorDate: Wed, 24 Jul 2019 15:53:24 +0300 Committer: Ingo Molnar CommitDate: Thu, 25 Jul 2019 15:41:31 +0200 perf/core: Fix creating kernel counters for PMUs that override event->cpu Some hardware PMU drivers will override perf_event.cpu inside their event_init callback. This causes a lockdep splat when initialized through the kernel API: WARNING: CPU: 0 PID: 250 at kernel/events/core.c:2917 ctx_sched_out+0x78/0x208 pc : ctx_sched_out+0x78/0x208 Call trace: ctx_sched_out+0x78/0x208 __perf_install_in_context+0x160/0x248 remote_function+0x58/0x68 generic_exec_single+0x100/0x180 smp_call_function_single+0x174/0x1b8 perf_install_in_context+0x178/0x188 perf_event_create_kernel_counter+0x118/0x160 Fix this by calling perf_install_in_context with event->cpu, just like perf_event_open Signed-off-by: Leonard Crestez Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Mark Rutland Cc: Alexander Shishkin Cc: Arnaldo Carvalho de Melo Cc: Frank Li Cc: Jiri Olsa Cc: Linus Torvalds Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: Will Deacon Link: https://lkml.kernel.org/r/c4ebe0503623066896d7046def4d6b1e06e0eb2e.1563972056.git.leonard.cres...@nxp.com Signed-off-by: Ingo Molnar --- kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index 026a14541a38..0463c1151bae 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -11274,7 +11274,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu, goto err_unlock; } - perf_install_in_context(ctx, event, cpu); + perf_install_in_context(ctx, event, event->cpu); perf_unpin_context(ctx); mutex_unlock(>mutex);
[PATCH] perf/core: Fix creating kernel counters for PMUs that override event->cpu
Some hardware PMU drivers will override perf_event.cpu inside their
event_init callback. This causes a lockdep splat when initialized through
the kernel API:
WARNING: CPU: 0 PID: 250 at kernel/events/core.c:2917 ctx_sched_out+0x78/0x208
CPU: 0 PID: 250 Comm: systemd-udevd Not tainted
5.3.0-rc1-next-20190723-00024-g94e04593c88a #80
Hardware name: FSL i.MX8MM EVK board (DT)
pstate: 4085 (nZcv daIf -PAN -UAO)
pc : ctx_sched_out+0x78/0x208
lr : ctx_sched_out+0x78/0x208
sp : 127a3750
x29: 127a3750 x28:
x27: 1162bf20 x26: 08cf3310
x25: 127a3de0 x24: 115ff808
x23: 7dffbff851b8 x22: 0004
x21: 7dffbff851b0 x20:
x19: 7dffbffc51b0 x18: 0010
x17: 0001 x16: 0007
x15: 2e8ba2e8ba2e8ba3 x14: 5114
x13: 117d5e30 x12: 11898378
x11: x10: 117d5000
x9 : 0045 x8 :
x7 : 10168194 x6 : 117d59d0
x5 : 0001 x4 : 80007db56128
x3 : 80007db56128 x2 : 0d9c118347a77600
x1 : x0 : 0024
Call trace:
ctx_sched_out+0x78/0x208
__perf_install_in_context+0x160/0x248
remote_function+0x58/0x68
generic_exec_single+0x100/0x180
smp_call_function_single+0x174/0x1b8
perf_install_in_context+0x178/0x188
perf_event_create_kernel_counter+0x118/0x160
Fix by calling perf_install_in_context with event->cpu, just like
perf_event_open
Signed-off-by: Leonard Crestez
---
I don't understand why PMUs outside the core are bound to a CPU anyway,
all this patch does is attempt to satisfy the assumptions made by
__perf_install_in_context and ctx_sched_out at init time so that lockdep
no longer complains.
ctx_sched_out asserts ctx->lock which seems to be taken by
__perf_install_in_context:
struct perf_event_context *ctx = event->ctx;
struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
...
raw_spin_lock(>ctx.lock);
The lockdep warning happens when ctx != >ctx which can happen if
__perf_install_in_context is called on a cpu other than event->cpu.
Found while working on this patch:
https://patchwork.kernel.org/patch/11056785/
kernel/events/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 026a14541a38..0463c1151bae 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11272,11 +11272,11 @@ perf_event_create_kernel_counter(struct
perf_event_attr *attr, int cpu,
if (!exclusive_event_installable(event, ctx)) {
err = -EBUSY;
goto err_unlock;
}
- perf_install_in_context(ctx, event, cpu);
+ perf_install_in_context(ctx, event, event->cpu);
perf_unpin_context(ctx);
mutex_unlock(>mutex);
return event;
--
2.17.1
[RFCv3 2/3] PM / devfreq: Add imx driver
Add initial support for frequency switching on pieces of the imx
interconnect fabric.
Uses clk_set_min_rate so that other subsytems can also impose minimum
rate requests.
Signed-off-by: Leonard Crestez
---
drivers/devfreq/Kconfig | 10 +++
drivers/devfreq/Makefile | 1 +
drivers/devfreq/imx-devfreq.c | 143 ++
3 files changed, 154 insertions(+)
create mode 100644 drivers/devfreq/imx-devfreq.c
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index defe1d438710..dc3311ead538 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -90,10 +90,20 @@ config ARM_EXYNOS_BUS_DEVFREQ
Each memory bus group could contain many memoby bus block. It reads
PPMU counters of memory controllers by using DEVFREQ-event device
and adjusts the operating frequencies and voltages with OPP support.
This does not yet operate with optimal voltages.
+config ARM_IMX_DEVFREQ
+ tristate "i.MX DEVFREQ Driver"
+ depends on ARCH_MXC || COMPILE_TEST
+ select DEVFREQ_GOV_USERSPACE
+ select PM_OPP
+ help
+ This adds the DEVFREQ driver for the i.MX family of SoCs.
+ It allows adjusting frequencies for DDRC (DDR Controller) and various
+ NICs and NOCs which form the SOC interconnect fabric
+
config ARM_TEGRA_DEVFREQ
tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
ARCH_TEGRA_210_SOC || \
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 338ae8440db6..c2463ed4c934 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)+= governor_userspace.o
obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
# DEVFREQ Drivers
obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ) += imx-devfreq.o
obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
obj-$(CONFIG_ARM_TEGRA_DEVFREQ)+= tegra30-devfreq.o
obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o
# DEVFREQ Event Drivers
diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
new file mode 100644
index ..3ee2d37883c6
--- /dev/null
+++ b/drivers/devfreq/imx-devfreq.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 NXP
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct imx_devfreq {
+ struct devfreq_dev_profile profile;
+ struct devfreq *devfreq;
+ struct clk *clk;
+};
+
+static int imx_devfreq_target(struct device *dev, unsigned long *freq, u32
flags)
+{
+ struct imx_devfreq *priv = dev_get_drvdata(dev);
+ struct dev_pm_opp *new_opp;
+ unsigned long new_freq;
+ int ret;
+
+ new_opp = devfreq_recommended_opp(dev, freq, flags);
+ if (IS_ERR(new_opp)) {
+ ret = PTR_ERR(new_opp);
+ dev_err(dev, "failed to get recommended opp: %d\n", ret);
+ return ret;
+ }
+ new_freq = dev_pm_opp_get_freq(new_opp);
+ dev_pm_opp_put(new_opp);
+
+ ret = clk_set_min_rate(priv->clk, new_freq);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(priv->clk, 0);
+ if (ret) {
+ clk_set_min_rate(priv->clk, priv->devfreq->previous_freq);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+ struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+ *freq = clk_get_rate(priv->clk);
+
+ return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+ struct devfreq_dev_status *stat)
+{
+ struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+ stat->busy_time = 0;
+ stat->total_time = 0;
+ stat->current_frequency = clk_get_rate(priv->clk);
+
+ return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+ return dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+ struct device *dev = >dev;
+ struct imx_devfreq *priv;
+ const char *gov = DEVFREQ_GOV_USERSPACE;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->devfreq)) {
+ ret = PTR_ERR(priv->devfreq);
+ dev_err(dev, "failed to fetch clk: %d\n", ret);
+ return ret;
+ }
+ platform_set_drvdata(pdev, priv);
+
+ ret = dev_pm_
[RFCv3 3/3] PM / devfreq: Add imx perf event support
The imx8m ddrc has a performance monitoring block attached which can be
used to measure bandwidth usage automatically adjust frequency.
There is already a perf driver for that block so instead of implementing
a devfreq events driver use the in-kernel perf API to fetch read/write
bandwidth values and sum them.
Signed-off-by: Leonard Crestez
---
drivers/devfreq/imx-devfreq.c | 135 ++
1 file changed, 135 insertions(+)
diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
index 3ee2d37883c6..fd4c8ffb8b4a 100644
--- a/drivers/devfreq/imx-devfreq.c
+++ b/drivers/devfreq/imx-devfreq.c
@@ -11,14 +11,28 @@
#include
#include
#include
#include
+#include
+#include
+
struct imx_devfreq {
struct devfreq_dev_profile profile;
struct devfreq *devfreq;
struct clk *clk;
+
+ struct platform_device* pmu_pdev;
+ struct pmu *pmu;
+
+ struct perf_event_attr rd_event_attr;
+ struct perf_event_attr wr_event_attr;
+ struct perf_event *rd_event;
+ struct perf_event *wr_event;
+
+ u64 last_rd_val, last_rd_ena, last_rd_run;
+ u64 last_wr_val, last_wr_ena, last_wr_run;
};
static int imx_devfreq_target(struct device *dev, unsigned long *freq, u32
flags)
{
struct imx_devfreq *priv = dev_get_drvdata(dev);
@@ -64,22 +78,131 @@ static int imx_devfreq_get_dev_status(struct device *dev,
stat->busy_time = 0;
stat->total_time = 0;
stat->current_frequency = clk_get_rate(priv->clk);
+ if (priv->rd_event && priv->wr_event) {
+ u64 rd_delta, rd_val, rd_ena, rd_run;
+ u64 wr_delta, wr_val, wr_ena, wr_run;
+
+ rd_val = perf_event_read_value(priv->rd_event, _ena,
_run);
+ wr_val = perf_event_read_value(priv->wr_event, _ena,
_run);
+
+ rd_delta = (rd_val - priv->last_rd_val) * (rd_ena -
priv->last_rd_ena) / (rd_run - priv->last_rd_run);
+ priv->last_rd_val = rd_val;
+ priv->last_rd_ena = rd_ena;
+ priv->last_rd_run = rd_run;
+ wr_delta = (wr_val - priv->last_wr_val) * (wr_ena -
priv->last_wr_ena) / (wr_run - priv->last_wr_run);
+ priv->last_wr_val = wr_val;
+ priv->last_wr_ena = wr_ena;
+ priv->last_wr_run = wr_run;
+
+ /* magic numbers, possibly wrong */
+ stat->busy_time = 4 * (rd_delta + wr_delta);
+ stat->total_time = stat->current_frequency;
+
+ dev_dbg(dev, "perf load %02lu%% read=%lu write=%lu freq=%lu\n",
+ 100 * stat->busy_time / stat->total_time,
+ rd_delta, wr_delta, stat->current_frequency);
+ }
+
+ return 0;
+}
+
+static int imx_devfreq_perf_disable(struct imx_devfreq *priv)
+{
+ /* release and set to NULL */
+ if (!IS_ERR_OR_NULL(priv->rd_event))
+ perf_event_release_kernel(priv->rd_event);
+ if (!IS_ERR_OR_NULL(priv->wr_event))
+ perf_event_release_kernel(priv->wr_event);
+ priv->rd_event = NULL;
+ priv->wr_event = NULL;
+
+ return 0;
+}
+
+static int imx_devfreq_perf_enable(struct imx_devfreq *priv)
+{
+ int ret;
+
+ priv->rd_event_attr.size = sizeof(priv->rd_event_attr);
+ priv->rd_event_attr.type = priv->pmu->type;
+ priv->rd_event_attr.config = 0x2a;
+
+ priv->rd_event = perf_event_create_kernel_counter(
+ >rd_event_attr, 0, NULL, NULL, NULL);
+ if (IS_ERR(priv->rd_event)) {
+ ret = PTR_ERR(priv->rd_event);
+ goto err;
+ }
+
+ priv->wr_event_attr.size = sizeof(priv->wr_event_attr);
+ priv->wr_event_attr.type = priv->pmu->type;
+ priv->wr_event_attr.config = 0x2b;
+
+ priv->wr_event = perf_event_create_kernel_counter(
+ >wr_event_attr, 0, NULL, NULL, NULL);
+ if (IS_ERR(priv->wr_event)) {
+ ret = PTR_ERR(priv->wr_event);
+ goto err;
+ }
+
return 0;
+
+err:
+ imx_devfreq_perf_disable(priv);
+ return ret;
+}
+
+static int imx_devfreq_init_events(struct device *dev,
+ struct device_node* events_node)
+{
+ struct imx_devfreq *priv = dev_get_drvdata(dev);
+ struct device_driver *driver;
+
+ /*
+* We need pmu->type for perf_event_attr but there is no API for
+* mapping device_node to pmu. Fetch private data for imx-ddr-pmu and
+* cast that to a struct pmu instead.
+*/
+ priv->pmu_pdev = of_find_device_by_node(events_node);
+ if (!priv->pmu_pdev)
+ return -ENOENT;
+ driver = priv->pmu_pdev->dev.driver
[RFCv3 1/3] dt-bindings: devfreq: Add initial bindings for i.MX
Add initial dt bindings for the interconnects inside i.MX chips.
Multiple external IPs are involved but SOC integration means the
software controllable interfaces are very similar.
This is initially only for imx8mm but add an "fsl,imx-bus" fallback
similar to exynos-bus.
Signed-off-by: Leonard Crestez
---
.../devicetree/bindings/devfreq/imx.yaml | 59 +++
1 file changed, 59 insertions(+)
create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml
diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml
b/Documentation/devicetree/bindings/devfreq/imx.yaml
new file mode 100644
index ..87f90cddfd29
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/imx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+ - Leonard Crestez
+
+description: |
+ The i.MX SoC family has multiple buses for which clock frequency (and
sometimes
+ voltage) can be adjusted.
+
+ Some of those buses expose register areas mentioned in the memory maps as GPV
+ ("Global Programmers View") but not all. Access to this area might be denied
for
+ normal world.
+
+ The buses are based on externally licensed IPs such as ARM NIC-301 and
Arteris
+ FlexNOC but DT bindings are specific to the integration of these bus
+ interconnect IPs into imx SOCs.
+
+properties:
+ reg:
+maxItems: 1
+description: GPV area
+
+ compatible:
+contains:
+ enum:
+ - fsl,imx8m-noc
+ - fsl,imx8m-nic
+ - fsl,imx8m-ddrc
+
+ clocks:
+maxItems: 1
+
+required:
+ - compatible
+ - clocks
+
+examples:
+ - |
+#include
+ddrc: dram-controller@3d40 {
+compatible = "fsl,imx8mm-ddrc";
+reg = <0x3d40 0x40>;
+clocks = < IMX8MM_CLK_DRAM>;
+operating-points-v2 = <_opp_table>;
+};
+
+ - |
+noc: noc@3270 {
+compatible = "fsl,imx8mm-noc";
+reg = <0x3270 0x10>;
+clocks = < IMX8MM_CLK_NOC>;
+operating-points-v2 = <_opp_table>;
+};
--
2.17.1
[RFCv2 0/3] PM / devfreq: Add imx driver
This series attempts to add devfreq support for imx8mm, covering dynamic scaling of internal buses and dram. Actual scaling is performed through the clk framework: The NOC and main NICs are driven by composite clks and a new 'imx8m-dram' clk is used for scaling dram using firmware calls. Frequency target is set via "clk_set_min_rate", this allows an unrelated subsystem (for example interconnect) to also request minimum rates as a form for proactive scaling. The dram controller (DDRC) has a performance monitoring block attached for which a perf driver already exists. Instead of reimplementing that as devfreq-events the perf in-kernel API is used. Changes since v2: * Solve review comments * Add yaml binding doc * Add perf event support Link to v2: https://patchwork.kernel.org/patch/11021571/ DRAM frequency switching through clk framework is here: * https://patchwork.kernel.org/patch/11049429/ That part might not be accepted in clk and it might have to be moved to devfreq also. Leonard Crestez (3): dt-bindings: devfreq: Add initial bindings for i.MX PM / devfreq: Add imx driver PM / devfreq: Add imx perf event support .../devicetree/bindings/devfreq/imx.yaml | 59 drivers/devfreq/Kconfig | 10 + drivers/devfreq/Makefile | 1 + drivers/devfreq/imx-devfreq.c | 278 ++ 4 files changed, 348 insertions(+) create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml create mode 100644 drivers/devfreq/imx-devfreq.c -- 2.17.1
Re: [PATCH 3/3] dt-bindings: dsp: fsl: Add DSP core binding support
On 18.07.2019 21:24, Daniel Baluta wrote: > On Thu, Jul 18, 2019 at 7:41 PM Rob Herring wrote: >> >> On Thu, Jul 18, 2019 at 9:13 AM Daniel Baluta wrote: >>> >>> This describes the DSP device tree node. >>> >>> Signed-off-by: Daniel Baluta >>> + power-domains: >>> +description: >>> + List of phandle and PM domain specifier as documented in >>> + Documentation/devicetree/bindings/power/power_domain.txt >> >> How many? 4? > > Yes, 4 for i.MX8QXP. Also, the same number is for i.MX8QM. Anyhow, I didn't > added added a limit here because I really don't know how many will be > in upcoming i.MX platforms. Which 4? It might help to use power-domain-names explicitly just like it's done for clocks and mboxes. This is very common for phandle lists. -- Regards, Leonard
Re: [PATCH] cpufreq: imx-cpufreq-dt: Assign max supported frequency as suspend frequency
On 7/8/2019 10:55 AM, anson.hu...@nxp.com wrote:
> To reduce the suspend/resume latency, CPU's max supported frequency
> should be used during low level suspend/resume phase, "opp-suspend"
> property is NOT feasible since OPP defined in DT could be NOT supported
> according to speed garding and market segment fuse settings. So we
> can assign the cpufreq policy's suspend_freq with max available
> frequency provided by cpufreq driver.
>
> Signed-off-by: Anson Huang
> diff --git a/drivers/cpufreq/imx-cpufreq-dt.c
> b/drivers/cpufreq/imx-cpufreq-dt.c
> +static int __init imx_cpufreq_dt_setup_suspend_opp(void)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get(0);
> +
> + policy->suspend_freq = cpufreq_quick_get_max(0);
> +
> + return 0;
> +}
> +late_initcall(imx_cpufreq_dt_setup_suspend_opp);
The imx-cpufreq-dt driver is built as a module by default and this patch
produces an error:
In file included from ../drivers/cpufreq/imx-cpufreq-dt.c:11:
../include/linux/module.h:131:42: error: redefinition of ‘__inittest’
static inline initcall_t __maybe_unused __inittest(void) \
^~
../include/linux/device.h:1656:1: note: in expansion of macro ‘module_init’
module_init(__driver##_init); \
^~~
As far as I can tell late_initcall is not supported for modules.
Viresh: "max freq as suspend freq" is something that could be useful for
other SOC families. The hardware can suspend at any freq; it's just that
the highest one makes sense because it makes suspend/resume slightly faster.
Could this behavior be pushed to cpufreq-dt as a bool flag inside struct
cpufreq_dt_platform_data?
Only a few other platforms use this, most others pass NULL like imx. But
passing custom SOC-specific flags to cpufreq-dt makes a lot of sense
--
Regards,
Leonard
Re: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp
On 7/4/2019 9:23 AM, anson.hu...@nxp.com wrote: > From: Anson Huang > > Assign highest OPP as suspend OPP to reduce suspend/resume > latency on i.MX8MM. > > Signed-off-by: Anson Huang > --- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index b11fc5e..3a62407 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -136,6 +136,7 @@ > opp-microvolt = <100>; > opp-supported-hw = <0x8>, <0x3>; > clock-latency-ns = <15>; > + opp-suspend; > }; > }; What if the highest OPP is unavailable due to speed grading? Ideally we should find a way to suspend at the highest *supported* OPP. Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt driver code? -- Regards, Leonard
Re: [PATCH V2 1/2] arm64: dts: imx8mm: Correct OPP table according to latest datasheet
On 6/29/2019 1:31 PM, anson.hu...@nxp.com wrote: > From: Anson Huang > > According to latest datasheet (Rev.0.2, 04/2019) from below links, > 1.8GHz is ONLY available for consumer part, so the market segment > bits for 1.8GHz opp should ONLY available for consumer part accordingly. > > Fixes: f403a26c865b (arm64: dts: imx8mm: Add cpu speed grading and all OPPs) > Signed-off-by: Anson Huang For both: Reviewed-by: Leonard Crestez The vendor tree goes through a lot of testing so switching to the exact speed grading interpretation from there does make sense.
Re: [PATCH 1/2] arm64: dts: imx8mq: Correct OPP table according to latest datasheet
On 6/28/2019 9:16 AM, Anson Huang wrote:
>> From: Leonard Crestez
>>> From: Anson Huang
>>>
>>> According to latest datasheet (Rev.1, 10/2018) from below links, in
>>> the consumer datasheet, 1.5GHz is mentioned as highest opp but depends
>>> on speed grading fuse, and in the industrial datasheet, 1.3GHz is
>>> mentioned as highest opp but depends on speed grading fuse. 1.5GHz and
>>> 1.3GHz opp use same voltage, so no need for consumer part to support
>>> 1.3GHz opp, with same voltage, CPU should run at highest frequency in
>>> order to go into idle as quick as possible, this can save power.
>>
>> I looked at the same datasheets and it's not clear to me that 1.3 Ghz should
>> be disallowed for consumer parts. Power consumption increases with both
>> voltage and frequency so having two OPPs with same voltage does make
>> sense.
>
> The consumer part datasheet does NOT mention 1.3GHz at all, so consumer part
> ONLY
> support 1GHz/1.5GHz, and industrial part ONLY support 800MHz/1.3GHz, this is
> what
> we did in our internal tree and NPI release, so better to make them aligned,
> otherwise,
> we have to change it when kernel upgrade.
Datasheet seems ambiguous: it mentions "max freq for volt" so my
understanding is that any lower freqs should also work at that voltage.
This also seems to be the understanding behind commit 8cfd813c7307
("arm64: dts: imx8mq: fix higher CPU operating point") by Lucas.
On datasheet page 7 it mentions that product code can have "JZ" or "HZ"
for 1.3Ghz or 1.5Ghz. Are you saying that only industrial parts can be "JZ"?
>>> opp-hz = /bits/ 64 <15>;
>>> opp-microvolt = <100>;
>>> /* Consumer only but rely on speed grading */
>>> - opp-supported-hw = <0x8>, <0x7>;
>>> + opp-supported-hw = <0x8>, <0x3>;
>>
>> If you don't want to rely on the fact that only consumer parts should be
>> fused for 1.5 Ghz then please delete the comment.
>
> Don't quite understand, 1.5GHz is indeed consumer ONLY, but if the consumer
> part is fused to 1GHz, then 1.5GHz is also NOT available, so it also rely on
> speed
> grading. So keep the comment there is OK?
What I meant with that comment is that 1.5Ghz is only mentioned for
consumer parts but instead of explicitly banning it on industrial parts
we rely on MFG never fusing industrial parts for 1.5Ghz.
Now you're explicitly banning it on industrial parts.
This comment is indeed confusing so better to just remove all instances.
Re: [PATCH 2/2] arm64: dts: imx8mm: Correct OPP table according to latest datasheet
On 28.06.2019 06:37, anson.hu...@nxp.com wrote: > According to latest datasheet (Rev.0.2, 04/2019) from below links, > 1.8GHz is ONLY available for consumer part, so the market segment > bits for 1.8GHz opp should ONLY available for consumer part accordingly. > > opp-hz = /bits/ 64 <18>; > opp-microvolt = <100>; > /* Consumer only but rely on speed grading */ > - opp-supported-hw = <0x8>, <0x7>; > + opp-supported-hw = <0x8>, <0x3>; Only consumer parts should be fused for this highest OPP. If you don't want to rely on this then maybe also delete the comment above? -- Regards, leonard
Re: [PATCH 1/2] arm64: dts: imx8mq: Correct OPP table according to latest datasheet
On 28.06.2019 06:37, anson.hu...@nxp.com wrote: > From: Anson Huang > > According to latest datasheet (Rev.1, 10/2018) from below links, > in the consumer datasheet, 1.5GHz is mentioned as highest opp but > depends on speed grading fuse, and in the industrial datasheet, > 1.3GHz is mentioned as highest opp but depends on speed grading > fuse. 1.5GHz and 1.3GHz opp use same voltage, so no need for > consumer part to support 1.3GHz opp, with same voltage, CPU should > run at highest frequency in order to go into idle as quick as > possible, this can save power. I looked at the same datasheets and it's not clear to me that 1.3 Ghz should be disallowed for consumer parts. Power consumption increases with both voltage and frequency so having two OPPs with same voltage does make sense. > opp-hz = /bits/ 64 <13>; > opp-microvolt = <100>; > - opp-supported-hw = <0xc>, <0x7>; > + /* Industrial only but rely on speed grading */ > + opp-supported-hw = <0xc>, <0x4>; Comment is false, you're explicitly excluding consumer parts via the second element. > opp-hz = /bits/ 64 <15>; > opp-microvolt = <100>; > /* Consumer only but rely on speed grading */ > - opp-supported-hw = <0x8>, <0x7>; > + opp-supported-hw = <0x8>, <0x3>; If you don't want to rely on the fact that only consumer parts should be fused for 1.5 Ghz then please delete the comment.
[PATCH 1/2] scripts/gdb: add lx-genpd-summary command
This is like /sys/kernel/debug/pm/pm_genpd_summary except it's
accessible through a debugger.
This can be useful if the target crashes or hangs because power domains
were not properly enabled.
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/genpd.py | 83 ++
scripts/gdb/vmlinux-gdb.py | 1 +
2 files changed, 84 insertions(+)
create mode 100644 scripts/gdb/linux/genpd.py
diff --git a/scripts/gdb/linux/genpd.py b/scripts/gdb/linux/genpd.py
new file mode 100644
index ..6ca93bd2949e
--- /dev/null
+++ b/scripts/gdb/linux/genpd.py
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) NXP 2019
+
+import gdb
+import sys
+
+from linux.utils import CachedType
+from linux.lists import list_for_each_entry
+
+generic_pm_domain_type = CachedType('struct generic_pm_domain')
+pm_domain_data_type = CachedType('struct pm_domain_data')
+device_link_type = CachedType('struct device_link')
+
+
+def kobject_get_path(kobj):
+path = kobj['name'].string()
+parent = kobj['parent']
+if parent:
+path = kobject_get_path(parent) + '/' + path
+return path
+
+
+def rtpm_status_str(dev):
+if dev['power']['runtime_error']:
+return 'error'
+if dev['power']['disable_depth']:
+return 'unsupported'
+_RPM_STATUS_LOOKUP = [
+"active",
+"resuming",
+"suspended",
+"suspending"
+]
+return _RPM_STATUS_LOOKUP[dev['power']['runtime_status']]
+
+
+class LxGenPDSummary(gdb.Command):
+'''Print genpd summary
+
+Output is similar to /sys/kernel/debug/pm_genpd/pm_genpd_summary'''
+
+def __init__(self):
+super(LxGenPDSummary, self).__init__('lx-genpd-summary',
gdb.COMMAND_DATA)
+
+def summary_one(self, genpd):
+if genpd['status'] == 0:
+status_string = 'on'
+else:
+status_string = 'off-{}'.format(genpd['state_idx'])
+
+slave_names = []
+for link in list_for_each_entry(
+genpd['master_links'],
+device_link_type.get_type().pointer(),
+'master_node'):
+slave_names.apend(link['slave']['name'])
+
+gdb.write('%-30s %-15s %s\n' % (
+genpd['name'].string(),
+status_string,
+', '.join(slave_names)))
+
+# Print devices in domain
+for pm_data in list_for_each_entry(genpd['dev_list'],
+pm_domain_data_type.get_type().pointer(),
+'list_node'):
+dev = pm_data['dev']
+kobj_path = kobject_get_path(dev['kobj'])
+gdb.write('%-50s %s\n' % (kobj_path, rtpm_status_str(dev)))
+
+def invoke(self, arg, from_tty):
+gdb.write('domain status slaves\n');
+gdb.write('/device
runtime status\n');
+
gdb.write('--\n');
+for genpd in list_for_each_entry(
+gdb.parse_and_eval('_list'),
+generic_pm_domain_type.get_type().pointer(),
+'gpd_list_node'):
+self.summary_one(genpd)
+
+
+LxGenPDSummary()
diff --git a/scripts/gdb/vmlinux-gdb.py b/scripts/gdb/vmlinux-gdb.py
index eff5a48ac026..a504f511e752 100644
--- a/scripts/gdb/vmlinux-gdb.py
+++ b/scripts/gdb/vmlinux-gdb.py
@@ -33,5 +33,6 @@ else:
import linux.rbtree
import linux.proc
import linux.constants
import linux.timerlist
import linux.clk
+import linux.genpd
--
2.17.1
[PATCH 2/2] scripts/gdb: add helpers to find and list devices
Add helper commands and functions for finding pointers to struct device
by enumerating linux device bus/class infrastructure. This can be used
to fetch subsystem and driver-specific structs:
(gdb) p *$container_of($lx_device_find_by_class_name("net", "eth0"), "struct
net_device", "dev")
(gdb) p *$container_of($lx_device_find_by_bus_name("i2c", "0-004b"), "struct
i2c_client", "dev")
(gdb) p *(struct imx_port*)$lx_device_find_by_class_name("tty",
"ttymxc1")->parent->driver_data
Several generic "lx-device-list" functions are included to enumerate
devices by bus and class:
(gdb) lx-device-list-bus usb
(gdb) lx-device-list-class
(gdb) lx-device-list-tree _bus
Similar information is available in /sys but pointer values are
deliberately hidden.
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/device.py | 182
scripts/gdb/vmlinux-gdb.py | 1 +
2 files changed, 183 insertions(+)
create mode 100644 scripts/gdb/linux/device.py
diff --git a/scripts/gdb/linux/device.py b/scripts/gdb/linux/device.py
new file mode 100644
index ..16376c5cfec6
--- /dev/null
+++ b/scripts/gdb/linux/device.py
@@ -0,0 +1,182 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) NXP 2019
+
+import gdb
+
+from linux.utils import CachedType
+from linux.utils import container_of
+from linux.lists import list_for_each_entry
+
+
+device_private_type = CachedType('struct device_private')
+device_type = CachedType('struct device')
+
+subsys_private_type = CachedType('struct subsys_private')
+kobject_type = CachedType('struct kobject')
+kset_type = CachedType('struct kset')
+
+bus_type = CachedType('struct bus_type')
+class_type = CachedType('struct class')
+
+
+def dev_name(dev):
+dev_init_name = dev['init_name']
+if dev_init_name:
+return dev_init_name.string()
+return dev['kobj']['name'].string()
+
+
+def kset_for_each_object(kset):
+return list_for_each_entry(kset['list'],
+kobject_type.get_type().pointer(), "entry")
+
+
+def for_each_bus():
+for kobj in kset_for_each_object(gdb.parse_and_eval('bus_kset')):
+subsys = container_of(kobj, kset_type.get_type().pointer(), 'kobj')
+subsys_priv = container_of(subsys,
subsys_private_type.get_type().pointer(), 'subsys')
+yield subsys_priv['bus']
+
+
+def for_each_class():
+for kobj in kset_for_each_object(gdb.parse_and_eval('class_kset')):
+subsys = container_of(kobj, kset_type.get_type().pointer(), 'kobj')
+subsys_priv = container_of(subsys,
subsys_private_type.get_type().pointer(), 'subsys')
+yield subsys_priv['class']
+
+
+def get_bus_by_name(name):
+for item in for_each_bus():
+if item['name'].string() == name:
+return item
+raise gdb.GdbError("Can't find bus type {!r}".format(name))
+
+
+def get_class_by_name(name):
+for item in for_each_class():
+if item['name'].string() == name:
+return item
+raise gdb.GdbError("Can't find device class {!r}".format(name))
+
+
+klist_type = CachedType('struct klist')
+klist_node_type = CachedType('struct klist_node')
+
+
+def klist_for_each(klist):
+return list_for_each_entry(klist['k_list'],
+klist_node_type.get_type().pointer(), 'n_node')
+
+
+def bus_for_each_device(bus):
+for kn in klist_for_each(bus['p']['klist_devices']):
+dp = container_of(kn, device_private_type.get_type().pointer(),
'knode_bus')
+yield dp['device']
+
+
+def class_for_each_device(cls):
+for kn in klist_for_each(cls['p']['klist_devices']):
+dp = container_of(kn, device_private_type.get_type().pointer(),
'knode_class')
+yield dp['device']
+
+
+def device_for_each_child(dev):
+for kn in klist_for_each(dev['p']['klist_children']):
+dp = container_of(kn, device_private_type.get_type().pointer(),
'knode_parent')
+yield dp['device']
+
+
+def _show_device(dev, level=0, recursive=False):
+gdb.write('{}dev {}:\t{}\n'.format('\t' * level, dev_name(dev), dev))
+if recursive:
+for child in device_for_each_child(dev):
+_show_device(child, level + 1, recursive)
+
+
+class LxDeviceListBus(gdb.Command):
+'''Print devices on a bus (or all buses if not specified)'''
+
+def __init__(self):
+super(LxDeviceListBus, self).__init__('lx-device-list-bus',
gdb.COMMAND_DATA)
+
+def invoke(self, arg, from_tty):
+if not arg:
+for bus in for_each_bus():
+gdb.write('bus {}:\t{}\n'.format(bus['name'].string(), bus))
+for dev in bus_for_each_device(bus):
+_show_device(dev, level=1)
+else:
+bus = get_bus_by_name(arg)
+if not bus:
+raise gdb.GdbError("Can't find bus {!r}".format(arg))
+for dev
Re: [PATCH] arm64: dts: imx8mq: Init rates and parents configs for clocks
On 25.06.2019 15:51, Abel Vesa wrote: > Add the initial configuration for clocks that need default parent and rate > setting. This is based on the vendor tree clock provider parents and rates > configuration except this is doing the setup in dts rather then using clock > consumer API in a clock provider driver. > > Signed-off-by: Abel Vesa > --- > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 34 > +++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index d09b808..e0abe02 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -489,6 +489,40 @@ > + < IMX8MQ_CLK_PCIE1_CTRL>, > + < IMX8MQ_CLK_PCIE1_PHY>, > + < IMX8MQ_CLK_PCIE2_CTRL>, > + < IMX8MQ_CLK_PCIE2_PHY>, > + < IMX8MQ_CLK_CSI1_CORE>, > + < IMX8MQ_CLK_CSI1_PHY_REF>, > + < IMX8MQ_CLK_CSI1_ESC>, > + < IMX8MQ_CLK_CSI2_CORE>, > + < IMX8MQ_CLK_CSI2_PHY_REF>, > + < IMX8MQ_CLK_CSI2_ESC>; This stuff (and NAND) looks like it would belong to device nodes instead. The rest seem fine though I'm not sure why exactly those clks are adjusted in vendor tree. -- Regards, Leonard
Re: [PATCH v3 4/5] crypto: caam - simplfy clock initialization
On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> Simplify clock initialization code by converting it to use clk-bulk,
> devres and soc_device_match() match table. No functional change
> intended.
Subject is misspelled.
> +struct clk_bulk_caam {
> + const struct clk_bulk_data *clks;
> + int num_clks;
> +};
clks could be an array[0] at the end to avoid an additional allocation.
> +static void disable_clocks(void *private)
> +{
> + struct clk_bulk_caam *context = private;
> +
> + clk_bulk_disable_unprepare(context->num_clks,
> +(struct clk_bulk_data *)context->clks);
> +}
Not sure using devm for this is worthwhile. Maybe someday CAAM clks will
be enabled dynamically?
It would be make sense to reference "clk" instead of "clocks".
> +static int init_clocks(struct device *dev,
> +const struct clk_bulk_caam *data)
> +{
> + struct clk_bulk_data *clks;
> + struct clk_bulk_caam *context;
> + int num_clks;
> + int ret;
> +
> + num_clks = data->num_clks;
> + clks = devm_kmemdup(dev, data->clks,
> + data->num_clks * sizeof(data->clks[0]),
> + GFP_KERNEL);
> + if (!clks)
> + return -ENOMEM;
> +
> + ret = devm_clk_bulk_get(dev, num_clks, clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to request all necessary clocks\n");
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(num_clks, clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable all necessary clocks\n");
> + return ret;
> + }
> +
> + context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> + if (!context)
> + return -ENOMEM;
Aren't clks left enabled if this fails? Can move this allocation higher.
> + context->num_clks = num_clks;
> + context->clks = clks;
> +
> + ret = devm_add_action_or_reset(dev, disable_clocks, context);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> static int caam_probe(struct platform_device *pdev)
> {
> int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> u64 caam_id;
> - static const struct soc_device_attribute imx_soc[] = {
> - {.family = "Freescale i.MX"},
> - {},
> - };
> + const struct soc_device_attribute *soc_attr;
This "soc_attr" is difficult to understand, maybe rename to something
like "imx_soc_match"?
Re: [RFC PATCH 1/3] drivers: interconnect: Add a driver for i.MX SoC
On 3/13/2019 9:36 PM, Alexandre Bailon wrote:
>
> This adds support of i.MX SoC to interconnect framework.
> This is based on busfreq, from NXP's tree.
> This is is generic enough to be used to add support of interconnect
> framework to any i.MX SoC, and, even, this could used for some other
> SoC.
> Thanks to interconnect framework, devices' driver request for
> bandwidth which is use by busfreq to determine a performance level,
> and then scale the frequency.
This part is difficult for me to understand:
> +static int busfreq_opp_bw_gt(struct busfreq_opp_node *opp_node,
> + u32 avg_bw, u32 peak_bw)
> +{
> + if (!opp_node)
> + return 0;
> + if (opp_node->min_avg_bw == BUSFREQ_UNDEFINED_BW) {
> + if (avg_bw)
> + return 2;
> + else
> + return 1;
> + }
> + if (opp_node->min_peak_bw == BUSFREQ_UNDEFINED_BW) {
> + if (peak_bw)
> + return 2;
> + else
> + return 1;
> + }
> + if (avg_bw >= opp_node->min_avg_bw)
> + return 1;
> + if (peak_bw >= opp_node->min_peak_bw)
> + return 1;
> + return 0;
> +}
> +
> +static struct busfreq_opp *busfreq_cmp_bw_opp(struct busfreq_icc_desc *desc,
> + struct busfreq_opp *opp1,
> + struct busfreq_opp *opp2)
> +{
> + int i;
> + int opp1_valid;
> + int opp2_valid;
> + int opp1_count = 0;
> + int opp2_count = 0;
> +
> + if (!opp1 && !opp2)
> + return desc->current_opp;
> +
> + if (!opp1)
> + return opp2;
> +
> + if (!opp2)
> + return opp1;
> +
> + if (opp1 == opp2)
> + return opp1;
> +
> + for (i = 0; i < opp1->nodes_count; i++) {
> + struct busfreq_opp_node *opp_node1, *opp_node2;
> + struct icc_node *icc_node;
> + u32 avg_bw;
> + u32 peak_bw;
> +
> + opp_node1 = >nodes[i];
> + opp_node2 = >nodes[i];
> + icc_node = opp_node1->icc_node;
> + avg_bw = icc_node->avg_bw;
> + peak_bw = icc_node->peak_bw;
> +
> + opp1_valid = busfreq_opp_bw_gt(opp_node1, avg_bw, peak_bw);
> + opp2_valid = busfreq_opp_bw_gt(opp_node2, avg_bw, peak_bw);
> +
> + if (opp1_valid == opp2_valid && opp1_valid == 1) {
> + if (opp_node1->min_avg_bw > opp_node2->min_avg_bw &&
> + opp_node1->min_avg_bw != BUSFREQ_UNDEFINED_BW)
> + opp1_valid++;
> + if (opp_node1->min_avg_bw < opp_node2->min_avg_bw &&
> + opp_node2->min_avg_bw != BUSFREQ_UNDEFINED_BW)
> + opp2_valid++;
> + }
> +
> + opp1_count += opp1_valid;
> + opp2_count += opp2_valid;
> + }
> +
> + if (opp1_count > opp2_count)
> + return opp1;
> + if (opp1_count < opp2_count)
> + return opp2;
> + return opp1->perf_level >= opp2->perf_level ? opp2 : opp1;
> +}
> +
> +static int busfreq_set_best_opp(struct busfreq_icc_desc *desc)
> +{
> + struct busfreq_opp *opp, *best_opp = desc->current_opp;
> +
> + list_for_each_entry(opp, >opps, node)
> + best_opp = busfreq_cmp_bw_opp(desc, opp, best_opp);
> + return busfreq_set_opp(desc, best_opp);
> +}
The goal seems to be to find the smaller platform-level "busfreq_opp"
which can meet all aggregated requests.
But why implement this by comparing opps against each other? It would be
easier to just iterate OPPs from low to high and stop on the first one
which meets all requirements.
The sdm845 provider seems to take a different approach: there are BCMs
("Bus Clock Managers") which are attached to some of the icc nodes and
those are individually scaled in order to meet BW requirements. On imx8m
we can scale buses via clk so we could just attach clks to some of the
nodes (NOC, DRAM, few PL301).
--
Regards,
Leonard
Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
On 6/10/2019 5:08 PM, Marc Zyngier wrote: > On 10/06/2019 14:55, Abel Vesa wrote: >> On 19-06-10 14:39:02, Marc Zyngier wrote: >>> On 10/06/2019 14:29, Abel Vesa wrote: On 19-06-10 14:19:21, Mark Rutland wrote: > On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote: >> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call >> handler and registers instead a wrapper which calls in the 'hijacked' >> handler, after that calling into EL3 which will take care of the actual >> wake up. This time, instead of expanding the PSCI ABI, we use a new >> vendor SIP. > > IIUC from last time [1,2], this erratum affects all interrupts > targetting teh idle CPU, not just IPIs, so even if the bodge is more > self-contained, it doesn't really solve the issue, and there are still > cases where a CPU will not be woken from idle when it should be (e.g. > upon receipt of an LPI). Wrong, this erratum does not affect any other type of interrupts, other than IPIs. That is because all the other interrupts go through GPC, which means the cores will wake up on any other type (again, other than IPI). >>> >>> Huh... Are you saying that LPIs and PPIs are going through the GPC, and >>> will trigger the wake-up of the core? That's not the conclusion we >>> reached last time. >> >> Hmm, I don't think that was the conclusion. Yes, Lucas was saying (IIRC) >> that if you terminate the IRQs at GIC then all the other interrupts will be >> in the same situation. But the performance improvement given by terminating >> them at GIC might not be worth it when compared to the cpuidle support. > > PPIs are broken, > relying on some other terrible hack for the timer (and only the timer, > leaving other PPIs dead as a nail). It also implies that LPIs have never > been looked into, and given that they aren't routed through the GPC, the > conclusion is pretty easy to draw. > > Nobody is talking about performance here. It is strictly about > correctness, and what I read about this system is that it cannot > reliably use cpuidle. My argument was that it's fine if PPIs and LPIs are broken as long as they're not used: * PPIs are only used for local timer which is not used for wakeup. * LPIs on imx are not currently implemented. This workaround is only targeted at a very specific SOC with specific usecases and in that context it behaves correctly, as far as I can tell. As mentioned in another thread the HW issue was already solved in newer chips of the same family (like imx8mm). If there is a need for PPIs and LPIs on imx8mq in the future then maybe we can detect that scenario and disable cpuidle? -- Regards, Leonard
Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
On 6/10/2019 3:15 PM, Abel Vesa wrote:
> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
> breaks cpuidle support due to inability to wake target cores on IPIs.
>
> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
> only, that is, not waking up all the cores every time, we can unmask/mask the
> IRQ 32 in the first GPC IMR register.
>
> Since EL3 is the one that deals with powering down/up the cores, and since the
> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
> the masking to be done on the power-up sequence by the core itself.
Manipulating same IMR registers in TF-A and Linux is racy so all IMR
manipulation (set_wake etc) needs to be done through SIP calls with
locking inside TF-A.
It would make sense to have an entirely separate SIP-based
irq-imx8mq-gpc.c driver based on what is used in NXP tree.
> + iomux_gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> + if (!IS_ERR(iomux_gpr))
> + regmap_update_bits(iomux_gpr, IOMUXC_GPR1, IMX6Q_GPR1_GINT,
> + IMX6Q_GPR1_GINT);
Doesn't this initialization belong in TF-A? On boot enable the irq and
keep it masked until somebody calls "wake".
--
Regards,
Leonard
Re: [RFC PATCH 0/3] Add support of busfreq
On 6/4/2019 11:44 AM, Anson Huang wrote: > As exemple, this series implements busfreq for i.MX8MM whose > upstreaming is in progress. Because this relies on ATF to do the > frequency scaling, it won't be hard make it work. > > I have similar question as previous reviewer, is there any branch that we can > test > this series? I've been looking at this and pushed a fixed-up functional variant to my personal github: https://github.com/cdleonard/linux/commits/next_imx8mm_busfreq It builds and probes and switches DRAM freq between low and high based on whether ethernet is down or up (for testing purposes). The pile of out-of-tree patches required to get this work is quite small. The DRAM freq switch is done via a clk wrapper previously sent as RFC: https://patchwork.kernel.org/patch/10968303/ That part needs more work but it could serve as a neat encapsulation similar to imx_cpu clk used for cpufreq-dt. > And, from the patch, it has multiple levels description of fabric arch, while > we ONLY > intend to scale "bus" frequency per devices' request, here "bus" includes > DRAM, NOC and > AHB, AXI, should we make it more flatter, such as just a virtual fabric as a > single provider, and then > all other devices as nodes under this provider? The imx8mm interconnect bindings describe many bus endpoints but all requests are aggregated to a single platform-level OPP which is equivalent to "low/audio/high mode" from NXP tree. It might be better to associate clks to several ICC nodes and this way scale NOC and DRAM separately? As far as I understand an interconnect provider is free to decide on granularity. As a wilder idea it might even be possible to use a stanard "devfreq-with-perfmon" for DDRC and have interconnect request a min freq to that instead of doing clk_set_rate on dram directly. That could bring features from both worlds, scaling both proactively and reactively. -- Regards, Leonard
Re: [PATCH V2 2/3] clk: imx: Add support for i.MX8MN clock driver
On 6/3/2019 4:33 AM, anson.hu...@nxp.com wrote:
> From: Anson Huang
>
> This patch adds i.MX8MN clock driver support.
> +#include "clk.h"
> +
> +#define PLL_1416X_RATE(_rate, _m, _p, _s)\
> + { \
> + .rate = (_rate),\
> + .mdiv = (_m), \
> + .pdiv = (_p), \
> + .sdiv = (_s), \
> + }
> +
> +#define PLL_1443X_RATE(_rate, _m, _p, _s, _k)\
> + { \
> + .rate = (_rate),\
> + .mdiv = (_m), \
> + .pdiv = (_p), \
> + .sdiv = (_s), \
> + .kdiv = (_k), \
> + }
These macros are shared with clk-imx8mm (and perhaps some future chips)
so they should be moved to driver/clk/imx/clk.h
Re: [PATCH] ARM: dts: imx7d-sdb: Make SW2's voltage fixed
On 29.05.2019 09:49, anson.hu...@nxp.com wrote:
> From: Anson Huang
>
> On i.MX7D SDB board, SW2 supplies a lot of peripheral devices,
> its voltage should be fixed at 1.8V. The commit 43967d9b5a7c
> ("ARM: dts: imx7d-sdb: Assign corresponding power supply for LDOs")
> assigns SW2 as the supplier of vdd1p0d, and when its comsumers
> pcie-phy/mipi-phy try to set the vdd1p0d to 1.0V, regulator core
> will also set SW2 to its best(min) voltage to 1.5V, and it will
> lead to board reset.
>
> This patch makes SW2's voltage fixed at 1.8V to avoid this issue.
>
> Fixes: 43967d9b5a7c ("ARM: dts: imx7d-sdb: Assign corresponding power supply
> for LDOs")
> Reported-by: Leonard Crestez
> Signed-off-by: Anson Huang
Reviewed-by: Leonard Crestez
Other boards don't seem to be affected by the original series.
Re: [PATCH RESEND 2/5] ARM: dts: imx7d-sdb: Assign corresponding power supply for LDOs
On 12.05.2019 12:57, Anson Huang wrote:
> On i.MX7D SDB board, sw2 supplies 1p0d/1p2 LDO, this patch assigns
> corresponding power supply for 1p0d/1p2 LDO to avoid confusion by
> below log:
>
> vdd1p0d: supplied by regulator-dummy
> vdd1p2: supplied by regulator-dummy
>
> With this patch, the power supply is more accurate:
>
> vdd1p0d: supplied by SW2
> vdd1p2: supplied by SW2
>
> diff --git a/arch/arm/boot/dts/imx7d-sdb.dts b/arch/arm/boot/dts/imx7d-sdb.dts
>
> +_1p0d {
> + vin-supply = <_reg>;
> +};
> +
> +_1p2 {
> + vin-supply = <_reg>;
> +};
It's not clear why but this patch breaks imx7d-sdb boot. Checked two
boards: in a board farm and on my desk.
--
Regards,
Leonard
Re: [RFC PATCH] soc: imx: Try harder to get imq8mq SoC revisions
On 22.05.2019 16:40, Lucas Stach wrote: > Am Mittwoch, den 22.05.2019, 13:30 + schrieb Leonard Crestez: >> On 22.05.2019 16:13, Guido Günther wrote: >>> Subject: Re: [RFC PATCH] soc: imx: Try harder to get imq8mq SoC revisions >>> On Wed, May 08, 2019 at 02:40:18PM +0200, Guido Günther wrote: >>>> Thanks for your comments. Let's try s.th. different then: identify by >>>> bootrom, ocotop and anatop and fall back to ATF afterwards (I'll split >>>> out the DT part and add binding docs if this makes sense). I'm also >>>> happy to drop the whole ATF logic until mailine ATF catched up: >>>> >>>> The mainline ATF doesn't currently support the FSL_SIP_GET_SOC_INFO call >>>> nor does it have the code to identify different imx8mq SOC revisions so >>>> mimic what NXPs ATF does here. >>> >>> Does this makes sense? If so I'll send this out as a series. >> >> Mainline ATF has recently caught up: >> >>>> As a fallback use ATF so we can identify new revisions once it gains >>>> support or when using NXPs ATF. >>> >>> I'm also fine with dropping the ATF part if we don't want to depend on >>> it in mainline. >> >> Linux arm64 depends on ATF to implement power management via PSCI: >> hotplug cpuidle and suspend. >> >> It is not clear why Linux would avoid other services and insist on >> reimplementing hardware workarounds. > > I fully agree. We should not duplicate functionality between ATF and > Linux kernel. Excellent, will remember this when debating who should manipulate GPC. Guido: Are you going to resend a variant of your V1? You mentioned that you need this for erratas, how exactly are you going to fetch soc revision from a driver? For 32bit imx there is a global imx_get_soc_revision(), maybe the definition could be moved from arch/arm/mach-imx/cpu.c to drivers/soc/imx/revision.c so that it's available everywhere? -- Regards, Leonard
Re: [PATCH] scripts/gdb: Fix invocation when CONFIG_COMMON_CLK is not set
On 5/24/2019 1:46 AM, Stephen Boyd wrote: > Quoting Fabiano Rosas (2019-05-23 12:53:11) >> diff --git a/scripts/gdb/linux/constants.py.in >> b/scripts/gdb/linux/constants.py.in >> index 1d73083da6cb..2efbec6b6b8d 100644 >> --- a/scripts/gdb/linux/constants.py.in >> +++ b/scripts/gdb/linux/constants.py.in >> @@ -40,7 +40,8 @@ >> import gdb >> >> /* linux/clk-provider.h */ >> -LX_GDBPARSED(CLK_GET_RATE_NOCACHE) >> +if IS_BUILTIN(CONFIG_COMMON_CLK): >> +LX_GDBPARSED(CLK_GET_RATE_NOCACHE) >> > > Why is this LX_GDBPARSED() instead of LX_VALUE()? From what I can tell > it doesn't need to be runtime evaluated, just assigned to something that > is macro expanded by CPP. Because CLK_GET_RATE_NOCACHE expands to BIT() which expands to a constant with an UL suffix which python doesn't understand. Alternatively we could redefine the BIT macros inside constants.py.in but using gdb features seemed better. We could even try to strip integer literal suffixes with sed. Mentioned before: https://lkml.org/lkml/2019/5/3/341 -- Regards, Leonard
Re: [RFC PATCH] soc: imx: Try harder to get imq8mq SoC revisions
On 22.05.2019 16:13, Guido Günther wrote: > Subject: Re: [RFC PATCH] soc: imx: Try harder to get imq8mq SoC revisions Fixed subject > On Wed, May 08, 2019 at 02:40:18PM +0200, Guido Günther wrote: >> Thanks for your comments. Let's try s.th. different then: identify by >> bootrom, ocotop and anatop and fall back to ATF afterwards (I'll split >> out the DT part and add binding docs if this makes sense). I'm also >> happy to drop the whole ATF logic until mailine ATF catched up: >> >> The mainline ATF doesn't currently support the FSL_SIP_GET_SOC_INFO call >> nor does it have the code to identify different imx8mq SOC revisions so >> mimic what NXPs ATF does here. > > Does this makes sense? If so I'll send this out as a series. Mainline ATF has recently caught up: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/imx/imx8m/imx8mq/imx8mq_bl31_setup.c#n52 >> As a fallback use ATF so we can identify new revisions once it gains >> support or when using NXPs ATF. > > I'm also fine with dropping the ATF part if we don't want to depend on > it in mainline. Linux arm64 depends on ATF to implement power management via PSCI: hotplug cpuidle and suspend. It is not clear why Linux would avoid other services and insist on reimplementing hardware workarounds. -- Regards, Leonard
Re: [PATCH V4 1/2] soc: imx: Add SCU SoC info driver support
On 5/17/2019 8:49 AM, Anson Huang wrote:
> + root = of_find_node_by_path("/");
> +
> + np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
It's possibly not very important for root or FW communication nodes but
you should probably of_node_put those back.
Re: [PATCH 1/2] soc: imx: soc-imx8: Avoid unnecessary of_node_put() in error handling
On 5/21/2019 12:18 PM, Anson Huang wrote: > of_node_put() is called after of_match_node() successfully called, > then in the following error handling, of_node_put() is called again > which is unnecessary, this patch adjusts the location of of_node_put() > to avoid such scenario. > > Signed-off-by: Anson Huang For both: Reviewed-by: Leonard Crestez I was thinking that maybe you could of_node_put as soon as you were done with it but the model is read straight into soc_dev_attr so just freeing everything at the end makes more sense.
Re: [RFC 1/2] dt-bindings: imx-ocotp: Add fusable-node property
On 20.05.2019 06:06, Peng Fan wrote: > Introduce fusable-node property for i.MX OCOTP driver. > The property will only be used by Firmware(eg. U-Boot) to > runtime disable the nodes. > > Take i.MX6ULL for example, there are several parts that only > have limited modules enabled controlled by OCOTP fuse. It is > not flexible to provide several dts for the serval parts, instead > we could provide one device tree and let Firmware to runtime disable > the device tree nodes for those modules that are disable(fused). > > Signed-off-by: Peng Fan > --- > > Currently NXP vendor use U-Boot to set status to disabled for devices > that could not function, > https://source.codeaurora.org/external/imx/uboot-imx/tree/arch/arm/mach-imx/mx6/module_fuse.c?h=imx_v2018.03_4.14.98_2.0.0_ga#n149 > But this approach is will not work if kernel dts node path changed. > > There are two approaches to resolve: > > 1. This patch is to add a fusable-node property, and Firmware will parse > the property and read fuse to decide whether to disable or keeep enable > the nodes. > > 2. There is another approach is that add nvmem-cells for all nodes that > could be disabled(fused). Then in each linux driver to use nvmem > api to detect fused or not, or in linux driver common code to check > device functionable or not with nvmem API. > > > To make it easy to work, we choose [1] here. Please advise whether > it is acceptable, because the property is not used by linux driver in > approach [1]. Or you prefer [2] or please advise if any better solution. Couldn't firmware parse nvmem-cells? Even without a full nvmem subsystem it would be possible for imx-specific code to walk the entire device tree, parse nvmem-cells and disable nodes which are disabled by fuse. -- Regards, Leonard
Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
On 16.05.2019 06:24, Anson Huang wrote:
> Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce
> driver dependency into Kconfig as CONFIG_IMX_SCU must be
> selected to support i.MX SCU SoC driver, also need to use
> platform driver model to make sure IMX_SCU driver is probed
> before i.MX SCU SoC driver.
> +#define imx_scu_revision(soc_rev) \
> + soc_rev ? \
> + kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & 0xf) : \
> + "unknown"
> + id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
> + data = id->data;
> + if (data) {
> + soc_dev_attr->soc_id = data->name;
> + if (data->soc_revision)
> + soc_rev = data->soc_revision();
> + }
> +
> + soc_dev_attr->revision = imx_scu_revision(soc_rev);
> + if (!soc_dev_attr->revision)
> + return -ENODEV;
The imx_scu_revision macro returns either kasprintf or "unknown", never
NULL. So it's not clear what this return -ENODEV does exactly.
It makes more sense to return -ENODEV if get_soc_revision fails, so
maybe check "soc_rev != 0" instead?
If you really want to check the kasprintf result then you should return
-ENOMEM for it. It would be clearer if you dropped the imx_scu_revision
revision macro and open-coded instead.
--
Regards,
Leonard
Re: [PATCH 1/3] dt-bindings: clock: imx8mq: Add SNVS clock
On 15.05.2019 04:09, Anson Huang wrote: > Add macro for the SNVS clock of the i.MX8MQ. > > Signed-off-by: Anson Huang For series (couldn't find a cover letter): Reviewed-by: Leonard Crestez
Re: [PATCH 1/3] dt-bindings: clock: imx8mm: Add SNVS clock
On 15.05.2019 04:29, Anson Huang wrote: > Add macro for the SNVS clock of the i.MX8MM. > > Signed-off-by: Anson Huang > --- > This patch is based on patch: https://patchwork.kernel.org/patch/10939997/ Numbering also conflicts with one of my patches: https://patchwork.kernel.org/patch/10940303/ The conflict is easy to resolve but I don't mind resending if your patches get accepted first. If should probably resend anyway to also add gic clk to 8mq. For series: Reviewed-by: Leonard Crestez
Re: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support
On 15.05.2019 11:32, Anson Huang wrote:
> Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce
> driver dependency into Kconfig as CONFIG_IMX_SCU must be
> selected to support i.MX SCU SoC driver, also need to use
> platform driver model to make sure IMX_SCU driver is probed
> before i.MX SCU SoC driver.
>
> With this patch, SoC info can be read from sysfs:
> + id = of_match_node(imx_scu_soc_match, root);
> + if (!id) {
> + of_node_put(root);
> + return -ENODEV;
> + }
Perhaps this check should be moved from imx_scu_soc_probe to
imx_scu_soc_init? As far as I can tell this "probe" function will be
attempted on all SOCs (even non-imx). Better to check if we're on a
SCU-based soc early and avoid temporary allocations.
> +module_init(imx_scu_soc_init);
> +module_exit(imx_scu_soc_exit);
Please don't make this a module
--
Regards,
Leonard
Re: [RFC PATCH 0/3] Add support of busfreq
On 15.03.2019 18:55, Alexandre Bailon wrote: >> On Wed, Mar 13, 2019 at 12:33 PM Alexandre Bailon >> wrote: >>> As exemple, this series implements busfreq for i.MX8MM whose >>> upstreaming is in progress. Because this relies on ATF to >>> do the frequency scaling, it won't be hard make it work. >> >> It's not clear to me whether this series actual scales the dram >> frequency based on what you said above. Is it just theoretical or do >> you have it working with a pile of out-of-tree patches? Would be good >> to include that pile of patches in your integration branch that I >> suggested above. > The current series only introduce busfreq generic driver, and the > busfreq driver for the imx8mm. > As is, the imx8mm driver will just be loaded, but do nothing because > none of the drivers have been updated to request bandwidth using the > interconnect framework. > > My intent was to sent a first draft o busfreq, to get some feedback, > before to send a more complete, and fully functional series. It's been a while since this was first posted and imx8mm now boots fine in linux-next. Is there a more up-to-date WIP branch somewhere? Otherwise I can try to hack this series into a bootable form. > In addition, the current clock driver of imx8mm doesn't allow dram > frequency scaling, so if busfreq driver tries, it will fail (should be > harmless because any other clocks should restored to their previous > rate). I'm confused about this. In NXP tree the actual DRAM switch is done inside ATF via SIP calls and involves corralling all CPUs. Do you want an "dram" clk which wraps the SIP calls required to changing dram frequency and root switching etc? I've been looking at the busfreq implementation in the NXP tree and refactoring just the "dram freq switch" behind a clk might work nicely. This would be similar to the imx_cpu clk used for cpufreq-dt and it might even be possible to upstream this separately from the rest of busfreq logic dealing with device requests. I haven't done a very careful review but I noticed you're not using the OPP framework and instead redefined everything? It's not clear why. -- Regards, Leonard
Re: [PATCH net-next v2 0/4] of_get_mac_address ERR_PTR fixes
On 07.05.2019 00:25, Petr Štetiar wrote: > Hi, > > this patch series is an attempt to fix the mess, I've somehow managed to > introduce. > > First patch in this series is defacto v5 of the previous 05/10 patch in the > series, but since the v4 of this 05/10 patch wasn't picked up by the > patchwork for some unknown reason, this patch wasn't applied with the other > 9 patches in the series, so I'm resending it as a separate patch of this > fixup series again. > > Second patch is a result of this rebase against net-next tree, where I was > checking again all current users of of_get_mac_address and found out, that > there's new one in DSA, so I've converted this user to the new ERR_PTR > encoded error value as well. > > Third patch which was sent as v5 wasn't considered for merge, but I still > think, that we need to check for possible NULL value, thus current IS_ERR > check isn't sufficient and we need to use IS_ERR_OR_NULL instead. > > Fourth patch fixes warning reported by kbuild test robot. > > Cheers, > > Petr > > Petr Štetiar (4): >net: ethernet: support of_get_mac_address new ERR_PTR error >net: dsa: support of_get_mac_address new ERR_PTR error >staging: octeon-ethernet: Fix of_get_mac_address ERR_PTR check >net: usb: smsc: fix warning reported by kbuild test robot > drivers/net/ethernet/freescale/fec_main.c | 2 +- This fixes netboot on imx (probably all of them). Tested-by: Leonard Crestez But shouldn't "support of_get_mac_address new ERR_PTR error" somehow be reordered so that it's done before allowing non-null errors from of_get_mac_address? Otherwise it will break bisect for many people. -- Regards, Leonard
Re: [PATCH] soc: imx: Get iMX8MQ revision for B0 from ATF
On 03.05.2019 16:53, Guido Günther wrote: > This is similar to what the BSP does and needed to e.g. determine > necessary quirks for MIPI DSI. > > Signed-off-by: Guido Günther > > From the list discussion and changelog it's not clear to me why a > different method was chosen for the B1 silicon so I left that in place > as is and only trigger on the B0 silicon I have here. Fetching revision without an ATF call was done for the sake of avoiding depending on ATF as much as vendor tree does. I'm not sure avoiding ATF dependencies is a good approach. The imx8mq reference manual claims that 0x3036006c is should be soc revision but that incorrectly reports 0x00824010 meaning "A0" on all chips. So some nasty hacks are done in ATF instead by poking at ROM and OCOTP. There were multiple discussions also for GPCv2 and 8mm about how much to rely on firmware. I personally think that duplicating ATF workarounds just makes supporting imx8m harder in Linux. Don't we want firmware to help us with silicon erratas? > +#define IMX8MQ_ATF_GET_SOC_INFO 0xc206 Any reason not to use original FSL_SIP_GET_SOC_INFO constant name? Since ATF can fetch revision for B1 as well it makes no sense to keep the old code if we switch to using a SIP call, just call ATF always. ATF upstream currently has 8mq support but no SIP call for GET_SOC_INFO, that could be added easily. -- Regards, Leonard
[PATCH 2/2] scripts/gdb: Print cached rate in lx-clk-summary
The clk rate is always stored in clk_core but might be out of date and
require calls to update from hardware.
Deal with that case by printing a (c) suffix.
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/clk.py | 21 ++---
scripts/gdb/linux/constants.py.in | 4
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/scripts/gdb/linux/clk.py b/scripts/gdb/linux/clk.py
index 6bf71976b55d..061aecfa294e 100644
--- a/scripts/gdb/linux/clk.py
+++ b/scripts/gdb/linux/clk.py
@@ -3,42 +3,49 @@
# Copyright (c) NXP 2019
import gdb
import sys
-from linux import utils, lists
+from linux import utils, lists, constants
clk_core_type = utils.CachedType("struct clk_core")
def clk_core_for_each_child(hlist_head):
return lists.hlist_for_each_entry(hlist_head,
clk_core_type.get_type().pointer(), "child_node")
class LxClkSummary(gdb.Command):
-"""Print Linux kernel log buffer."""
+"""Print clk tree summary
+
+Output is a subset of /sys/kernel/debug/clk/clk_summary
+
+No calls are made during printing, instead a (c) if printed after values which
+are cached and potentially out of date"""
def __init__(self):
super(LxClkSummary, self).__init__("lx-clk-summary", gdb.COMMAND_DATA)
def show_subtree(self, clk, level):
-gdb.write("%*s%-*s %7d %8d %8d\n" % (
+gdb.write("%*s%-*s %7d %8d %8d %11lu%s\n" % (
level * 3 + 1, "",
30 - level * 3,
clk['name'].string(),
clk['enable_count'],
clk['prepare_count'],
-clk['protect_count']))
+clk['protect_count'],
+clk['rate'],
+'(c)' if clk['flags'] & constants.LX_CLK_GET_RATE_NOCACHE else
' '))
for child in clk_core_for_each_child(clk['children']):
self.show_subtree(child, level + 1)
def invoke(self, arg, from_tty):
-gdb.write(" enable prepare
protect\n")
-gdb.write(" clock countcount
count\n")
-
gdb.write("-\n")
+gdb.write(" enable prepare protect
\n")
+gdb.write(" clock countcountcount
rate \n")
+
gdb.write("\n")
for clk in
clk_core_for_each_child(gdb.parse_and_eval("clk_root_list")):
self.show_subtree(clk, 0)
for clk in
clk_core_for_each_child(gdb.parse_and_eval("clk_orphan_list")):
self.show_subtree(clk, 0)
diff --git a/scripts/gdb/linux/constants.py.in
b/scripts/gdb/linux/constants.py.in
index 76f46f427b96..1d73083da6cb 100644
--- a/scripts/gdb/linux/constants.py.in
+++ b/scripts/gdb/linux/constants.py.in
@@ -10,10 +10,11 @@
*
* This work is licensed under the terms of the GNU GPL version 2.
*
*/
+#include
#include
#include
#include
#include
#include
@@ -36,10 +37,13 @@
/* The build system will take care of deleting everything above this marker */
import gdb
+/* linux/clk-provider.h */
+LX_GDBPARSED(CLK_GET_RATE_NOCACHE)
+
/* linux/fs.h */
LX_VALUE(SB_RDONLY)
LX_VALUE(SB_SYNCHRONOUS)
LX_VALUE(SB_MANDLOCK)
LX_VALUE(SB_DIRSYNC)
--
2.17.1
[PATCH 1/2] scripts/gdb: Cleanup error handling in list helpers
An incorrect argument to list_for_each is an internal error in gdb
scripts so a TypeError should be raised. The gdb.GdbError exception type
is intended for user errors such as incorrect invocation.
Drop the type assertion in list_for_each_entry because list_for_each isn't
going to suddenly yield something else.
Applies to both list and hlist
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/lists.py | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py
index 55356b66f8ea..c487ddf09d38 100644
--- a/scripts/gdb/linux/lists.py
+++ b/scripts/gdb/linux/lists.py
@@ -22,45 +22,39 @@ hlist_node = utils.CachedType("struct hlist_node")
def list_for_each(head):
if head.type == list_head.get_type().pointer():
head = head.dereference()
elif head.type != list_head.get_type():
-raise gdb.GdbError("Must be struct list_head not {}"
+raise TypeError("Must be struct list_head not {}"
.format(head.type))
node = head['next'].dereference()
while node.address != head.address:
yield node.address
node = node['next'].dereference()
def list_for_each_entry(head, gdbtype, member):
for node in list_for_each(head):
-if node.type != list_head.get_type().pointer():
-raise TypeError("Type {} found. Expected struct list_head *."
-.format(node.type))
yield utils.container_of(node, gdbtype, member)
def hlist_for_each(head):
if head.type == hlist_head.get_type().pointer():
head = head.dereference()
elif head.type != hlist_head.get_type():
-raise gdb.GdbError("Must be struct hlist_head not {}"
+raise TypeError("Must be struct hlist_head not {}"
.format(head.type))
node = head['first'].dereference()
while node.address:
yield node.address
node = node['next'].dereference()
def hlist_for_each_entry(head, gdbtype, member):
for node in hlist_for_each(head):
-if node.type != hlist_node.get_type().pointer():
-raise TypeError("Type {} found. Expected struct hlist_head *."
-.format(node.type))
yield utils.container_of(node, gdbtype, member)
def list_check(head):
nb = 0
--
2.17.1
[PATCH 0/2] gdb/scripts: Improve lx-clk-summary
The earlier series adding clk support to gdb/scripts was quickly accepted but some concerns were raised by Stephen Boyd so this series attempts to address them. Link to previous series: https://lkml.org/lkml/2019/4/22/55 This is not a v2 and squashing is not expected. Fields other than clk rate not covered because they're much more rarely used and cache logic can get more complicated and brittle. LX_GDBPARSED is used in constants.py.in because python does not understand C integer literal suffixes like the "1UL" from the definition of BIT() used by CLK_GET_RATE_NOCACHE. Alternative workarounds would be hacking away UL suffixes with sed or redefining BIT but relying on gdb evaluation is easier and much more flexible. Leonard Crestez (2): scripts/gdb: Cleanup error handling in list helpers scripts/gdb: Print cached rate in lx-clk-summary scripts/gdb/linux/clk.py | 21 ++--- scripts/gdb/linux/constants.py.in | 4 scripts/gdb/linux/lists.py| 10 ++ 3 files changed, 20 insertions(+), 15 deletions(-) -- 2.17.1
Re: [PATCH v2] soc: imx: Fix build error without CONFIG_SOC_BUS
On 24.04.2019 12:16, Yue Haibing wrote:
> From: YueHaibing
>
> During randconfig builds, I occasionally run into an invalid configuration
>
> drivers/soc/imx/soc-imx8.o: In function `imx8_soc_init':
> soc-imx8.c:(.init.text+0x144): undefined reference to `soc_device_register'
>
> while CONFIG_SOC_BUS is not set, the building failed like this. This patch
> selects SOC_BUS to fix it.
>
> Reported-by: Hulk Robot
> Fixes: a7e26f356ca1 ("soc: imx: Add generic i.MX8 SoC driver")
> Suggested-by: Leonard Crestez
> Signed-off-by: YueHaibing
Reviewed-by: Leonard Crestez
Re: [PATCH] soc: imx: Fix build error without CONFIG_SOC_BUS
On 4/24/2019 11:00 AM, Yue Haibing wrote: > From: YueHaibing > > During randconfig builds, I occasionally run into an invalid configuration > > drivers/soc/imx/soc-imx8.o: In function `imx8_soc_init': > soc-imx8.c:(.init.text+0x144): undefined reference to `soc_device_register' > > while CONFIG_SOC_BUS is not set, the building failed like this. This patch > selects SOC_BUS to fix it. > > diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig > @@ -5,6 +5,7 @@ config IMX_GPCV2_PM_DOMAINS > depends on ARCH_MXC || (COMPILE_TEST && OF) > depends on PM > select PM_GENERIC_DOMAINS > + select SOC_BUS > default y if SOC_IMX7D You should "select SOC_BUS" from "config ARCH_MXC" in arm64 Kconfig.platforms, not from a power domain implementation. Your patch works because arm64 ARCH_MXC currently does "select IMX_GPCV2_PM_DOMAINS" but it's silly to use this intermediary. -- Regards, Leonard
Re: [PATCH 3/3] scripts/gdb: Add $lx_clk_core_lookup function
On 4/22/2019 11:18 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-04-22 01:26:57)
>> diff --git a/scripts/gdb/linux/clk.py b/scripts/gdb/linux/clk.py
>> +class LxClkCoreLookup(gdb.Function):
>> +"""Find struct clk_core by name"""
>> +
>> +def __init__(self):
>> +super(LxClkCoreLookup, self).__init__("lx_clk_core_lookup")
>> +
>> +def lookup_hlist(self, hlist_head, name):
>> +for child in clk_core_for_each_child(hlist_head):
>> +if child['name'].string() == name:
>
> Do you need to do the .string() for comparison? Or does it work just as
> well to compare a gdb.Value object to a python string? It would be nice
> if the gdb.Value object could figure out that they're not both gdb.Value
> objects so it can do a string comparison itself.
The gdb manual is not clear on how comparisons work on gdb.Value types.
Converting to a python string and comparing in python work well, using
== on string gdb.Values results in this:
gdb.error: evaluation of this expression requires the program to
have a function "malloc"
My guess is gdb attempts to convert both arguments to gdb.Value and do
the comparison via a call on the target? This is very undesirable here.
I get the same error if "name" is a gdb.Value instead of being converted
to a string in invoke().
--
Regards,
Leonard
Re: [PATCH 1/3] scripts/gdb: Add hlist utilities
On 4/22/2019 11:41 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-04-22 01:26:56)
>> This allows easily examining kernel hlists in python.
>>
>> Signed-off-by: Leonard Crestez
>
>> +def hlist_for_each_entry(head, gdbtype, member):
>> +for node in hlist_for_each(head):
>> +if node.type != hlist_node.get_type().pointer():
>> +raise TypeError("Type {} found. Expected struct hlist_head *."
>
> Maybe drop the full-stop? It looks weird to see struct list_head *.
Makes sense, it was copy/pasted from list_for_each_entry. Since this
iterates over items from hlist_for_each the check is only useful as an
assertion anyway so I'll just drop it.
The hlist_for_each/list_for_each raise gdb.GdbError on type mismatch; as
far as I understand the difference is that GdbError won't print a python
backtrace. However these are internal helpers so TypeError makes more sense.
Re: [PATCH 1/2] soc: imx-sc: add i.MX system controller soc driver support
On 4/22/2019 9:46 AM, Anson Huang wrote: >> -Original Message- >> From: Anson Huang >>> From: Shawn Guo [mailto:shawn...@kernel.org] >>> On Sun, Apr 21, 2019 at 03:40:00PM +0800, Shawn Guo wrote: On Thu, Apr 11, 2019 at 06:49:12AM +, Anson Huang wrote: > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller > inside, the system controller is in charge of controlling power, > clock and fuse etc.. > > This patch adds i.MX system controller soc driver support, Linux > kernel has to communicate with system controller via MU (message > unit) IPC to get soc revision, uid etc.. > > With this patch, soc info can be read from sysfs: > > drivers/soc/imx/Kconfig | 7 ++ > drivers/soc/imx/Makefile | 1 + > drivers/soc/imx/soc-imx-sc.c | 220 > +++ > 3 files changed, 228 insertions(+) create mode 100644 > drivers/soc/imx/soc-imx-sc.c Rather than creating a new driver, please take a look at Abel's generic i.MX8 SoC driver, and see if it can be extended to cover i.MX8QXP. >> >> Got it, I didn't notice that this patch bas been accepted, I will redo the >> patch >> based on it, thanks. > > I have sent the new patch set to support i.MX8QXP SoC revision based on > generic i.MX8 > SoC driver, however, the Kconfig modification is NOT good, it may break > i.MX8MQ if IMX_SCU > is NOT enabled, although we can add some warp function for SCU firmware API > call to fix it, > but after further thought and discussion with Dong Aisheng, I think we may > need to roll back to > use this patch series to create a new SoC driver dedicated for i.MX8 SoCs > with system controller inside, such as i.MX8QXP, i.MX8QM etc., the reason are > as below: > > For i.MX8MQ/i.MX8MM: > 1. SoC driver does NOT depends on i.MX SCU firmware, so no need to use > platform driver >probe model, just device_init phase call is good enough; > 2. The SoC driver no need to depends on IMX_SCU, so it can be always > built in, no need to >check IMX_SCU config; > 3. The fuse check for CPU speed grading, HDCP status, NoC settings etc. > could be added to this driver, > but they are ONLY for i.MX8MQ/i.MX8MM etc.. > For i.MX8QXP/i.MX8QM: > 1. SoC driver MUST depends on IMX_SCU; > 2. MUST use platform model to support defer probe; > 3. No fuse check for CPU speed grading. > > So, I guess the reused code for i.MX8MQ and i.MX8QXP is ONLY those part of > creating SoC id device node (less than > 30% I think), all other functions are implemented in total different ways, > that is why I created the imx_sc_soc driver > in this patch series, so do you think we can add new SoC driver for i.MX8 SoC > with SCU inside? Putting 2 different architecture > SoCs' driver into 1 file looks like NOT making enough sense. +1 for separate SOC driver. The 8mq/8mm and 8qm/8qxp families are very different, they just happen to share the imx8 prefix. It makes sense to allow people to compile one without the other and this is easier with distinct SOC drivers. -- Regards, Leonard
[PATCH 3/3] scripts/gdb: Add $lx_clk_core_lookup function
Finding an individual clk_core requires walking the tree which can be
quite complicated so add a helper for easy access.
(gdb) print *(struct clk_scu*)$lx_clk_core_lookup("uart0_clk")->hw
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/clk.py | 23 +++
1 file changed, 23 insertions(+)
diff --git a/scripts/gdb/linux/clk.py b/scripts/gdb/linux/clk.py
index a9129d865c5d..6bf71976b55d 100644
--- a/scripts/gdb/linux/clk.py
+++ b/scripts/gdb/linux/clk.py
@@ -42,5 +42,28 @@ class LxClkSummary(gdb.Command):
for clk in
clk_core_for_each_child(gdb.parse_and_eval("clk_orphan_list")):
self.show_subtree(clk, 0)
LxClkSummary()
+
+
+class LxClkCoreLookup(gdb.Function):
+"""Find struct clk_core by name"""
+
+def __init__(self):
+super(LxClkCoreLookup, self).__init__("lx_clk_core_lookup")
+
+def lookup_hlist(self, hlist_head, name):
+for child in clk_core_for_each_child(hlist_head):
+if child['name'].string() == name:
+return child
+result = self.lookup_hlist(child['children'], name)
+if result:
+return result
+
+def invoke(self, name):
+name = name.string()
+return (self.lookup_hlist(gdb.parse_and_eval("clk_root_list"), name) or
+self.lookup_hlist(gdb.parse_and_eval("clk_orphan_list"), name))
+
+
+LxClkCoreLookup()
--
2.17.1
[PATCH 2/3] scripts/gdb: Initial clk support: lx-clk-summary
Add an lx-clk-summary command which prints a subset of
/sys/kernel/debug/clk/clk_summary.
This can be used to examine hangs caused by clk not being enabled.
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/clk.py | 46 ++
scripts/gdb/vmlinux-gdb.py | 1 +
2 files changed, 47 insertions(+)
create mode 100644 scripts/gdb/linux/clk.py
diff --git a/scripts/gdb/linux/clk.py b/scripts/gdb/linux/clk.py
new file mode 100644
index ..a9129d865c5d
--- /dev/null
+++ b/scripts/gdb/linux/clk.py
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) NXP 2019
+
+import gdb
+import sys
+
+from linux import utils, lists
+
+clk_core_type = utils.CachedType("struct clk_core")
+
+
+def clk_core_for_each_child(hlist_head):
+return lists.hlist_for_each_entry(hlist_head,
+clk_core_type.get_type().pointer(), "child_node")
+
+
+class LxClkSummary(gdb.Command):
+"""Print Linux kernel log buffer."""
+
+def __init__(self):
+super(LxClkSummary, self).__init__("lx-clk-summary", gdb.COMMAND_DATA)
+
+def show_subtree(self, clk, level):
+gdb.write("%*s%-*s %7d %8d %8d\n" % (
+level * 3 + 1, "",
+30 - level * 3,
+clk['name'].string(),
+clk['enable_count'],
+clk['prepare_count'],
+clk['protect_count']))
+
+for child in clk_core_for_each_child(clk['children']):
+self.show_subtree(child, level + 1)
+
+def invoke(self, arg, from_tty):
+gdb.write(" enable prepare
protect\n")
+gdb.write(" clock countcount
count\n")
+
gdb.write("-\n")
+for clk in
clk_core_for_each_child(gdb.parse_and_eval("clk_root_list")):
+self.show_subtree(clk, 0)
+for clk in
clk_core_for_each_child(gdb.parse_and_eval("clk_orphan_list")):
+self.show_subtree(clk, 0)
+
+
+LxClkSummary()
diff --git a/scripts/gdb/vmlinux-gdb.py b/scripts/gdb/vmlinux-gdb.py
index 033578cc4cd7..eff5a48ac026 100644
--- a/scripts/gdb/vmlinux-gdb.py
+++ b/scripts/gdb/vmlinux-gdb.py
@@ -32,5 +32,6 @@ else:
import linux.lists
import linux.rbtree
import linux.proc
import linux.constants
import linux.timerlist
+import linux.clk
--
2.17.1
[PATCH 1/3] scripts/gdb: Add hlist utilities
This allows easily examining kernel hlists in python.
Signed-off-by: Leonard Crestez
---
scripts/gdb/linux/lists.py | 23 +++
1 file changed, 23 insertions(+)
diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py
index 1987d756b36b..55356b66f8ea 100644
--- a/scripts/gdb/linux/lists.py
+++ b/scripts/gdb/linux/lists.py
@@ -14,10 +14,12 @@
import gdb
from linux import utils
list_head = utils.CachedType("struct list_head")
+hlist_head = utils.CachedType("struct hlist_head")
+hlist_node = utils.CachedType("struct hlist_node")
def list_for_each(head):
if head.type == list_head.get_type().pointer():
head = head.dereference()
@@ -37,10 +39,31 @@ def list_for_each_entry(head, gdbtype, member):
raise TypeError("Type {} found. Expected struct list_head *."
.format(node.type))
yield utils.container_of(node, gdbtype, member)
+def hlist_for_each(head):
+if head.type == hlist_head.get_type().pointer():
+head = head.dereference()
+elif head.type != hlist_head.get_type():
+raise gdb.GdbError("Must be struct hlist_head not {}"
+ .format(head.type))
+
+node = head['first'].dereference()
+while node.address:
+yield node.address
+node = node['next'].dereference()
+
+
+def hlist_for_each_entry(head, gdbtype, member):
+for node in hlist_for_each(head):
+if node.type != hlist_node.get_type().pointer():
+raise TypeError("Type {} found. Expected struct hlist_head *."
+.format(node.type))
+yield utils.container_of(node, gdbtype, member)
+
+
def list_check(head):
nb = 0
if (head.type == list_head.get_type().pointer()):
head = head.dereference()
elif (head.type != list_head.get_type()):
--
2.17.1
[PATCH 0/3] scripts/gdb: Initial clk support
Tested with openocd on imx but should be useful everywhere. It is common for clk issues to lockup the system and make /sys inaccesible. Not sure where to send this; there doesn't appear to be any list more specific than LKML. Leonard Crestez (3): scripts/gdb: Add hlist utilities scripts/gdb: Initial clk support: lx-clk-summary scripts/gdb: Add $lx_clk_core_lookup function scripts/gdb/linux/clk.py | 69 ++ scripts/gdb/linux/lists.py | 23 + scripts/gdb/vmlinux-gdb.py | 1 + 3 files changed, 93 insertions(+) create mode 100644 scripts/gdb/linux/clk.py -- 2.17.1
Re: [RESEND v2] soc: imx: Add generic i.MX8 SoC driver
On 3/28/2019 6:43 PM, Leonard Crestez wrote: > On Fri, 2019-03-22 at 16:49 +, Abel Vesa wrote: >> Add generic i.MX8 SoC driver along with the i.MX8MQ SoC specific code. >> For now, only i.MX8MQ revision B1 is supported. For any other, i.MX8MQ >> revision it will print 'unknown'. > >> +#define REV_B1 0x21 >> + >> +#define IMX8MQ_SW_INFO_B1 0x40 >> +#define IMX8MQ_SW_MAGIC_B1 0xff0055aa >> + >> +np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");+ >> +ocotp_base = of_iomap(np, 0); >> + >> +magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); >> +if (magic == IMX8MQ_SW_MAGIC_B1) >> +rev = REV_B1; > > This is based on ATF code in vendor tree, but shouldn't we have some > sort of explanation for this "magic"? > > Looking at the OCOTP driver reg 0x40 is IMX_OCOTP_ADDR_DATA2 and it's > used as part of fuse writes. According to the driver code 8mq is > compatible with 7d and this write path is enabled for imx8mq-ocotp. After further digging in NXP manuals and uboot sources it seems that imx8mq ocotp is like imx6 rather than imx7. Posted fix for nvmem driver: https://patchwork.kernel.org/patch/10908081/ Reviewed-by: Leonard Crestez It might still be nice to find a way to identify imx8mq B0.
Re: [PATCH v2 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit
On Fri, 2019-04-12 at 07:04 -0600, Angus Ainslie (Purism) wrote:
> + {
> + clock-frequency = <40>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_i2c1>;
> + status = "okay";
> +
> + pmic: pmic@4b {
> + reg = <0x4b>;
> + compatible = "rohm,bd71837";
> + pinctrl-names = "default";
> + pinctrl-0 = <_pmic>;
> + clocks = <_osc>;
> + clock-names = "osc";
> + clock-output-names = "pmic_clk";
> + interrupt-parent = <>;
> + interrupts = <3 GPIO_ACTIVE_LOW>;
> + interrupt-names = "irq";
> + rohm,reset-snvs-powered;
> +
> + gpo {
> + /* 0b_1100 all gpos with cmos output mode
> */
> + rohm,drv = <0x0C>;
> + };
After searching for this gpo/rohm,drv property it does not seem to be
documented or handled anywhere?
Should probably just drop.
> + regulators {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + buck1_reg: BUCK1@0 {
> + reg = <0>;
> + regulator-name = "buck1";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <130>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-ramp-delay = <1250>;
> + rohm,dvs-run-voltage = <90>;
> + rohm,dvs-idle-voltage = <85>;
> + rohm,dvs-suspend-voltage = <80>;
> + };
> +
> + buck2_reg: BUCK2@1 {
> + reg = <1>;
> + regulator-name = "buck2";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <130>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-ramp-delay = <1250>;
> + rohm,dvs-run-voltage = <100>;
> + rohm,dvs-idle-voltage = <90>;
> + };
> +
> + buck3_reg: BUCK3@2 {
> + reg = <2>;
> + regulator-name = "buck3";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <130>;
> + rohm,dvs-run-voltage = <100>;
> + };
> +
> + buck4_reg: BUCK4@3 {
> + reg = <3>;
> + regulator-name = "buck4";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <130>;
> + rohm,dvs-run-voltage = <100>;
> + };
> +
> + buck5_reg: BUCK5@4 {
> + reg = <4>;
> + regulator-name = "buck5";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <135>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck6_reg: BUCK6@5 {
> + reg = <5>;
> + regulator-name = "buck6";
> + regulator-min-microvolt = <300>;
> + regulator-max-microvolt = <330>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck7_reg: BUCK7@6 {
> + reg = <6>;
> + regulator-name = "buck7";
> + regulator-min-microvolt = <1605000>;
> + regulator-max-microvolt = <1995000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck8_reg: BUCK8@7 {
> + reg = <7>;
> + regulator-name = "buck8";
> + regulator-min-microvolt = <80>;
> + regulator-max-microvolt = <140>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
Re: [RESEND v2] soc: imx: Add generic i.MX8 SoC driver
On Fri, 2019-03-22 at 16:49 +, Abel Vesa wrote: > Add generic i.MX8 SoC driver along with the i.MX8MQ SoC specific code. > For now, only i.MX8MQ revision B1 is supported. For any other, i.MX8MQ > revision it will print 'unknown'. > +#define REV_B1 0x21 > + > +#define IMX8MQ_SW_INFO_B10x40 > +#define IMX8MQ_SW_MAGIC_B1 0xff0055aa > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");+ > + ocotp_base = of_iomap(np, 0); > + > + magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); > + if (magic == IMX8MQ_SW_MAGIC_B1) > + rev = REV_B1; This is based on ATF code in vendor tree, but shouldn't we have some sort of explanation for this "magic"? Looking at the OCOTP driver reg 0x40 is IMX_OCOTP_ADDR_DATA2 and it's used as part of fuse writes. According to the driver code 8mq is compatible with 7d and this write path is enabled for imx8mq-ocotp. Looking at the OCOTP manual reg 0x40 is OCOTP_HW_OCOTP_READ_FUSE_DATA and it's meant to be used together with IMX_OCOTP_ADDR_CTRL to read info. Maybe my manual (rev0 2018-01) is incorrect? Looking at the manual this will return the value of the fuse last requested via IMX_OCOTP_ADDR_CTRL but no such request is made in this driver. So reading from OCOTP 0x40 might return an unrelated value?! The manual does document that fuse 0x440[3:0] is a "silicon revision number"; maybe we should read that? -- Regards, Leonard
Re: [RFC 0/7] cpuidle: Add poking mechanism to support non-IPI wakeup
On Wed, 2019-03-27 at 17:45 +, Marc Zyngier wrote: > On 27/03/2019 16:06, Lucas Stach wrote: > > Am Mittwoch, den 27.03.2019, 15:57 + schrieb Marc Zyngier: > > > On 27/03/2019 15:44, Lucas Stach wrote: > > > > Am Mittwoch, den 27.03.2019, 13:21 + schrieb Abel Vesa: > > > > > This work is a workaround I'm looking into (more as a background task) > > > > > in order to add support for cpuidle on i.MX8MQ based platforms. > > > > > > > > > > The main idea here is getting around the missing GIC wake_request > > > > > signal > > > > > (due to integration design issue) by waking up a each individual core > > > > > through > > > > > some dedicated SW power-up bits inside the power controller (GPC) > > > > > right before > > > > > every IPI is requested for that each individual core. > > > > > > > > Just a general comment, without going into the details of this series: > > > > this issue is not only affecting IPIs, but also MSIs terminated at the > > > > GIC. Currently MSIs are terminated at the PCIe core, but terminating > > > > them at the GIC is clearly preferable, as this allows assigning CPU > > > > affinity to individual MSIs and lowers IRQ service overhead. > > > > > > > > I'm not sure what the consequences are for upstream Linux support yet, > > > > but we should keep in mind that having a workaround for IPIs is only > > > > solving part of the issue. > > > > > > If this erratum is affecting more than just IPIs, then indeed I don't > > > see how this patch series solves anything. > > > > > > But the erratum documentation seems to imply that only SGIs are > > > affected, and goes as far as suggesting to use an external interrupt > > > would solve it. How comes this is not the case? Or is it that anything > > > directly routed to a redistributor is also affected? This would break > > > LPIs (and thus MSIs) and PPIs (the CPU timer, among others). > > > > Anything that isn't visible to the GPC and requires the GIC > > wake_request signal to behave as specified is broken by this erratum. > > I really wonder how a timer interrupt (a PPI, hence not routed through > the GPC) can wake up the CPU in this case. It really feels like > something like "program CNTV_CVAL_EL0 to expire at some later point; > WFI" could result in the CPU going to a deep sleep state, and not > wake-up at all. This is already a common issue for cpuidle implementions handled by the "local-timer-stop" property. imx has other timer blocks in the SOC, they generate SPIs which are connected to GPC. > This would indicate that not only cpuidle is broken with this, but > absolutely every interrupt that is not routed through the GPC. Yes, cpuidle is broken for irqs not routed through GPC. However: * All SPIs are connected to GPC in a 1:1 mapping * This series deals with SGIs * The timer PPIs are not required; covered by local-timer-stop * LPIs are currently unused (I understand imx-pci uses SPI by default from Lucas) Anything missing? My understanding is that this wake request feature via GIC is new in v3 and this is maybe why HW team missed it during integration. Older imx6/7 has GICv2 and has deep idle states which always rely on GPC to wakeup so the approach can work. -- Regards, Leonard
Re: [RFC 0/7] cpuidle: Add poking mechanism to support non-IPI wakeup
On Wed, 2019-03-27 at 17:06 +0100, Lucas Stach wrote: > Am Mittwoch, den 27.03.2019, 15:57 + schrieb Marc Zyngier: > > On 27/03/2019 15:44, Lucas Stach wrote: > > > Am Mittwoch, den 27.03.2019, 13:21 + schrieb Abel Vesa: > > > > This work is a workaround I'm looking into (more as a background task) > > > > in order to add support for cpuidle on i.MX8MQ based platforms. > > > > > > > > The main idea here is getting around the missing GIC wake_request signal > > > > (due to integration design issue) by waking up a each individual core > > > > through > > > > some dedicated SW power-up bits inside the power controller (GPC) right > > > > before > > > > every IPI is requested for that each individual core. > > > > > > Just a general comment, without going into the details of this series: > > > this issue is not only affecting IPIs, but also MSIs terminated at the > > > GIC. Currently MSIs are terminated at the PCIe core, but terminating > > > them at the GIC is clearly preferable, as this allows assigning CPU > > > affinity to individual MSIs and lowers IRQ service overhead. > > > > > > I'm not sure what the consequences are for upstream Linux support yet, > > > but we should keep in mind that having a workaround for IPIs is only > > > solving part of the issue. > > > > If this erratum is affecting more than just IPIs, then indeed I don't > > see how this patch series solves anything. > > > > But the erratum documentation seems to imply that only SGIs are > > affected, and goes as far as suggesting to use an external interrupt > > would solve it. How comes this is not the case? Or is it that anything > > directly routed to a redistributor is also affected? This would break > > LPIs (and thus MSIs) and PPIs (the CPU timer, among others). > > > > What is the *exact* status of this thing? I have the ugly feeling that > > the true workaround is just to disable cpuidle. > > As far as I understand the erratum, the basic issue is that the GIC > wake_request signals are not connected to the GPC (the CPU/peripheral > power sequencer). The SPIs are routed through the GPC and thus are > visible as wakeup sources, which is why the workaround of using an > external SPI as wakeup trigger for the IPI works. We had a kernel workaround for IPIs in our internal tree for a long time and I don't think we do anything special for PCI. Does PCI MSI really bypass the GPC on 8mq? Adding Richard/Jacky, they might know about this. This seems like something of a corner case to me, don't many imx boards ship without PCI; especially for low-power scenarios? If required it might be reasonable to add an additional workaround to disable all cpuidle if pci msis are used. -- Regards, Leonard
Re: [RFC PATCH 0/3] Add support of busfreq
On 3/15/19 11:31 AM, Alexandre Bailon wrote: >>> This series is sent as RFC mostly because the current support of i.MX SoC >>> won't >>> benefit of busfreq framework, because the clocks' driver don't support >>> interconnect / dram frequency scaling. >>> As exemple, this series implements busfreq for i.MX8MM whose upstreaming >>> is in progress. Because this relies on ATF to do the frequency scaling, it >>> won't >>> be hard make it work. >> How can I test this patch series? >> Any additional patches you can share with us? >> Or what else we need to do to test it? We can help with it. > Many other patches will be required to test the series. > There are a couple of patches that updates i.MX device drivers to > request for bandwidth (does similar thing as bus_freq_request and > bus_freq_release). The interconnect framework asks for bandwidth in bytes/second but in NXP tree most requests are of the form "request_bus_freq(BUS_FREQ_HIGH);". In many cases (ethernet) it doesn't seem you can calculate a specific bandwidth usefully. Instead of asking for "infinite bandwidth zero latency" to force everything to "high" it would be nicer to "request an opp". Power-domain bindings mentioned that consumer-devices can specify a "required-opps" property but I've found zero users in tree. Maybe some helpers could be written to parse that property and automatically request ICC opp on device suspend/resume via device-links? I know that stuff was written for genpd but it looks like a very good fit to me. > In addition, a patch to that allow to scale the DRAM > frequency using CCF is required. I'm still working on this patch. I'm not sure what you mean here, do you want a clk set_rate to change the DRAM freq? It makes sense for DDRC to be a node in the interconnect framework and switch OPP inside icc implementation via an ATF call. -- Regards, Leonard
Re: [PATCH v2] soc: imx: Add generic i.MX8 SoC driver
On Wed, 2019-02-27 at 08:41 +, Abel Vesa wrote: > On 19-02-26 13:34:52, Leonard Crestez wrote: > > On Tue, 2019-02-26 at 10:53 +, Abel Vesa wrote: > > > Add generic i.MX8 SoC driver along with the i.MX8MQ SoC specific code. > > > For now, only i.MX8MQ revision B1 is supported. For any other, i.MX8MQ > > > revision it will print 'unknown'. > > > > > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); > > > + if (!np) > > > + goto out; > > > + > > > + ocotp_base = of_iomap(np, 0); > > > + WARN_ON(!ocotp_base); > > > + > > > + magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); > > > + if (magic == IMX8MQ_SW_MAGIC_B1) > > > + rev = REV_B1; > > > > Turns out that imx8mq version determination is uniquely messy. I think > > we should try to print the revision number even for older chips so that > > we know how old they are, but this code can be enhanced in later > > patches. > > Fair enough. I believe we should stick to B1 only for now though. Yes, people who want their old SOC revisions to be be detected can submit later patches.
Re: [PATCH v2] soc: imx: Add generic i.MX8 SoC driver
On Tue, 2019-02-26 at 10:53 +, Abel Vesa wrote: > Add generic i.MX8 SoC driver along with the i.MX8MQ SoC specific code. > For now, only i.MX8MQ revision B1 is supported. For any other, i.MX8MQ > revision it will print 'unknown'. > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp"); > + if (!np) > + goto out; > + > + ocotp_base = of_iomap(np, 0); > + WARN_ON(!ocotp_base); > + > + magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); > + if (magic == IMX8MQ_SW_MAGIC_B1) > + rev = REV_B1; Turns out that imx8mq version determination is uniquely messy. I think we should try to print the revision number even for older chips so that we know how old they are, but this code can be enhanced in later patches. In the vendor tree we handle this with a SIP call to ATF, it's not clear why we shouldn't just upstream that (in both ATF and Linux). Also, there are some imx soc revision declarations in include/soc/imx/revision.h. Those are implemented in arch/arm/mach-imx for older chips, would it make sense for soc-imx8 to define imx_get_soc_revision? -- Regards, Leonard
Re: [PATCH v8] PCI: imx6: limit DBI register length
On Tue, 2019-02-26 at 14:06 +0100, Stefan Agner wrote:
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers
> beyond that point, avoiding the following abort on a i.MX 6Quad:
>
> +static void imx6_pcie_quirk(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pcie_port *pp = bus->sysdata;
> +
> + /* Bus parent is the PCI bridge, its parent is this platform driver */
> + if (!bus->dev.parent || !bus->dev.parent->parent)
> + return;
> +
> + /* Make sure we only quirk devices associated with this driver */
> + if (bus->dev.parent->parent->driver != _pcie_driver.driver)
> + return;
This looks like it would be enough to prevent interfering with other
dwc-based pci drivers.
Reviewed-by: Leonard Crestez
Re: [PATCH v7] PCI: imx6: limit DBI register length
On Mon, 2019-02-25 at 17:02 +0100, Stefan Agner wrote:
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers
> beyond that point, avoiding the following abort on a i.MX 6Quad:
>
> +static void imx6_pcie_quirk(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pcie_port *pp = bus->sysdata;
> +
> + if (bus->number == pp->root_bus_nr) {
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> + /*
> + * Limit config length to avoid the kernel reading beyond
> + * the register set and causing an abort on i.MX 6Quad
> + */
> + if (imx6_pcie->drvdata->dbi_length)
> + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> + }
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd, imx6_pcie_quirk);
This looks like a default from SYNOPSYS so it likely run on other SOCs
using the DesignWare PCI IP and crash because of those unchecked casts.
There might also be some way to configure pci ids before boot (not
sure, didn't check).
Maybe you should check the device driver name in the quirk function?
--
Regards,
Leonard
Re: [PATCH v6] PCI: imx6: limit DBI register length
On Mon, 2019-02-25 at 15:25 +0100, Stefan Agner wrote:
> Define the length of the DBI registers and limit config space to its
> length. This makes sure that the kernel does not access registers
> beyond that point, avoiding the following abort on a i.MX 6Quad:
> # cat /sys/devices/soc0/soc/1ffc000.pcie/pci\:00/\:00\:00.0/config
> [ 100.021433] Unhandled fault: imprecise external abort (0x1406) at
> 0xb6ea7000
> ...
> [ 100.056423] PC is at dw_pcie_read+0x50/0x84
> [ 100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
>
> +static void imx6_pcie_quirk(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct pcie_port *pp = bus->sysdata;
> +
> + if (bus->number == pp->root_bus_nr) {
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci);
> +
> + /*
> + * Limit config length to avoid the kernel reading beyond
> + * the register set and causing an abort on i.MX 6Quad
> + */
> + dev->cfg_size = imx6_pcie->drvdata->dbi_length;
> + }
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, imx6_pcie_quirk);
Are you sure that this quirk is only applied to imx6-pcie?
Superficially it looks like it would apply to all PCI roots and this
could be a problem on configs which enable all drivers, like arm64.
Maybe the linker magic inside DECLARE_PCI_FIXUP deals with that, in
that case I apologize.
--
Regards,
Leonard
[RFC] pinctrl/amd: Clear interrupt enable bits on probe
My Acer Nitro 5 AN515-42 laptop with a Ryzen 2700U hangs on boot because
of spurious interrupts from pinctrl-amd.
This seems to happen because the touchpad interrupt is enabled on boot
in "level" mode and there is no way to clear it until a touchpad driver
probes.
Fix by disabling all gpio interrupts at probe time until they are
explicitly requested by drivers.
Signed-off-by: Leonard Crestez
---
It's strange that nobody else has run into this problem, AMD hardware is
relatively common. Maybe firmware generally disables GPIO interrupts
itself?
This patch fixes boot but this same laptop has other issues:
* Suspend is broken
* Ethernet is broken (only sometimes)
* The CPU freq gets stuck at 400 Mhz (sometimes)
Those issues happen on maybe 80% of boots without a clear pattern. It
seems that inserting/removing the ethernet jack during boot helps
cpufreq? It's possible that these problems are also caused by pin
misconfiguration so this fix might be incomplete.
When the cpufreq issue happens `rdmsr 0xc0010061 -a` shows 0x22 for all
cpus; maybe this is some broken thermal throttling?
Also, perhaps amd_gpio_irq_disable_all should enumerate in a nicer less
verbose way?
Previously: https://lore.kernel.org/patchwork/patch/1028047/
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 2a7d638978d8..3cb7ea46f32c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -592,10 +592,53 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void
*dev_id)
raw_spin_unlock_irqrestore(_dev->lock, flags);
return ret;
}
+static void amd_gpio_irq_disable_all(struct amd_gpio *gpio_dev)
+{
+ unsigned int bank, i, pin_num;
+ u32 regval;
+
+ for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
+ switch (bank) {
+ case 0:
+ i = 0;
+ pin_num = AMD_GPIO_PINS_BANK0;
+ break;
+ case 1:
+ i = 64;
+ pin_num = AMD_GPIO_PINS_BANK1 + i;
+ break;
+ case 2:
+ i = 128;
+ pin_num = AMD_GPIO_PINS_BANK2 + i;
+ break;
+ case 3:
+ i = 192;
+ pin_num = AMD_GPIO_PINS_BANK3 + i;
+ break;
+ default:
+ /* Illegal bank number, ignore */
+ continue;
+ }
+
+ for (; i < pin_num; i++) {
+ unsigned long flags;
+ raw_spin_lock_irqsave(_dev->lock, flags);
+ regval = readl(gpio_dev->base + i * 4);
+ if (regval & BIT(INTERRUPT_ENABLE_OFF)) {
+ dev_info(_dev->pdev->dev,
+ "Pin %d interrupt enabled on
boot: disable\n", i);
+ regval &= ~BIT(INTERRUPT_ENABLE_OFF);
+ writel(regval, gpio_dev->base + i * 4);
+ }
+ raw_spin_unlock_irqrestore(_dev->lock, flags);
+ }
+ }
+}
+
static int amd_get_groups_count(struct pinctrl_dev *pctldev)
{
struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
return gpio_dev->ngroups;
@@ -910,10 +953,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
ret = gpiochip_add_data(_dev->gc, gpio_dev);
if (ret)
return ret;
+ amd_gpio_irq_disable_all(gpio_dev);
+
ret = gpiochip_add_pin_range(_dev->gc, dev_name(>dev),
0, 0, gpio_dev->gc.ngpio);
if (ret) {
dev_err(>dev, "Failed to add pin range\n");
goto out2;
--
2.20.1
Re: [PATCH V2 2/2] clk: imx: scu: add cpu frequency scaling support
On Wed, 2019-02-13 at 13:32 +, Anson Huang wrote:
> On NXP's i.MX SoCs with system controller inside, CPU frequency
> scaling can ONLY be done by system controller firmware, and it
> can ONLY be requested from secure mode, so Linux kernel has to
> call ARM SMC to trap to ARM-Trusted-Firmware to request system
> controller firmware to do CPU frequency scaling.
> +#ifdef CONFIG_CPUFREQ_DT
> +#define IMX_SIP_CPUFREQ 0xC201
> +#define IMX_SIP_SET_CPUFREQ 0x00
> +
> static struct imx_sc_ipc *ccm_ipc_handle;
Without CONFIG_CPUFREQ_DT the ccm_ipc_handle won't be defined and build
will break.
But is there a good reason for ifdef? In general clock function are
compiled even if no driver is calling them.
> +#ifdef CONFIG_CPUFREQ_DT
> + /* CPU clock can ONLY be done by TF-A */
> + if (clk->clk_type == IMX_SC_PM_CLK_CPU) {
> + struct arm_smccc_res res;
> + unsigned int cluster_id;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(imx_sc_cpufreq_data); i++) {
> + if (!strcmp(clk_hw_get_name(hw),
> + imx_sc_cpufreq_data[i].clk_name)) {
> + cluster_id = imx_sc_cpufreq_data[i].cluster_id;
> + break;
> + }
> + }
> +
> + /*
> + * As some other clock types have same value as
> + * IMX_SC_PM_CLK_CPU, so we need to double check
> + * the clock being scaled is indeed CPU clock which
> + * matches the table we define.
> + */
> + if (i < ARRAY_SIZE(imx_sc_cpufreq_data)) {
> + arm_smccc_smc(IMX_SIP_CPUFREQ, IMX_SIP_SET_CPUFREQ,
> + cluster_id, rate, 0, 0, 0, 0, );
> + return 0;
> + }
> + }
> +#endif
The code inside the ifdef would look better in a separate
imx_clk_atf_set_rate function. Maybe even separate clk_ops?
Re: [PATCH 0/3] kbuild: Add wilddt function instead of listing dtbs
On Fri, 2019-01-25 at 01:03 +0900, Masahiro Yamada wrote: > On Tue, Jan 22, 2019 at 1:17 AM Leonard Crestez > wrote: > > > > On 1/7/2019 9:31 PM, Leonard Crestez wrote: > > > The dts makefiles go through a lot of pointless churn when boards are > > > added. Many SOCs (such as imx) have very simple naming conventions for > > > all boards using a certain chip and board listings can be easily > > > collapsed using wildcards. > > > > > > Add a "wilddt" function and use it for imx6/7/8 and layerscape. This can > > > be applied to many other soc families later. > > > > Any feedback? This should be reviewed by kbuild and arm-soc > > Personally, I prefer explicit listing > because I can browse the Makefile to get a quick idea > of which boards are compiled. Or you can just ls soc-*.dts? > I like this to be consistent. > - Exploit wildcard for all platforms > - Do not do this at all > > But, I am pessimistic about the former > when I look at AT91 platform. I can try to resend in a form that uses a wildcard for most platforms but skips those with very few SOCs or confusing conventions (like at91). -- Regards, Leonard
Re: [PATCH 0/3] kbuild: Add wilddt function instead of listing dtbs
On 1/7/2019 9:31 PM, Leonard Crestez wrote: > The dts makefiles go through a lot of pointless churn when boards are > added. Many SOCs (such as imx) have very simple naming conventions for > all boards using a certain chip and board listings can be easily > collapsed using wildcards. > > Add a "wilddt" function and use it for imx6/7/8 and layerscape. This can > be applied to many other soc families later. > > Previously sent as RFC: https://lore.kernel.org/patchwork/patch/1022737/ Any feedback? This should be reviewed by kbuild and arm-soc I think this is a worthwhile cleanup, are there any corner cases or more exotic setup I should try in order to validate this series? -- Regards, Leonard
Re: [PATCH V2] ARM: imx: add i.MX7ULP cpuidle support
On 1/14/19 2:56 AM, Shawn Guo wrote: > On Fri, Jan 11, 2019 at 05:57:30AM +, Anson Huang wrote: > > This patch adds cpuidle support for i.MX7ULP, 3 cpuidle > > states supported as below: > > > > 1. WFI, just ARM wfi; > > 2. WAIT mode, mapped to SoC's partial stop mode #3; > > 3. STOP mode, mapped to SoC's partial stop mode #1. > > > > In WAIT mode, system clock and bus clock will be enabled; > > In STOP mode, system clock and bus clock will be disabled. > > > > Signed-off-by: Anson Huang > > Applied, thanks. > Hello, I'm not sure if anyone else already noticed but it seems the arch/arm/mach-imx/cpuidle-imx7ulp.c file was dropped? It's not in your tree: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/soc=cf2d2d392906f5603cf0d0f3630f6386babf7e4b This causes build failures in next-20190115 with imx_v6_v7_defconfig. -- Regards, Leonard
[PATCH 3/3] arm64: dts: freescale: Use wilddt function
Use the wilddt function instead of listing dts files. This shrinks the makefile and avoids needing future changes when boards are added. Signed-off-by: Leonard Crestez --- arch/arm64/boot/dts/freescale/Makefile | 25 - 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile index f9be2426f83c..ab28e485179d 100644 --- a/arch/arm64/boot/dts/freescale/Makefile +++ b/arch/arm64/boot/dts/freescale/Makefile @@ -1,22 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-frdm.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-frwy.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1012a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1028a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1043a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1046a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1088a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls1088a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2080a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2080a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2080a-simu.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2088a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2088a-rdb.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-qds.dtb -dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb - -dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb +dtb-$(CONFIG_ARCH_LAYERSCAPE) += \ + $(call wilddt,fsl-ls*) \ + $(call wilddt,fsl-lx*) +dtb-$(CONFIG_ARCH_MXC) += $(call wilddt,imx*) -- 2.17.1
[PATCH 0/3] kbuild: Add wilddt function instead of listing dtbs
The dts makefiles go through a lot of pointless churn when boards are added. Many SOCs (such as imx) have very simple naming conventions for all boards using a certain chip and board listings can be easily collapsed using wildcards. Add a "wilddt" function and use it for imx6/7/8 and layerscape. This can be applied to many other soc families later. Previously sent as RFC: https://lore.kernel.org/patchwork/patch/1022737/ Changes since RFC: * Split into 3-part series * Move the wilddt to Kbuild.include so that it's available everywhere * Use $(srctree)/$(src) instead of $(dtstree) * Also use wilddt in arm64/boot/dts/freescale Series is against next-20190107, conflicts are to be expected as board list keeps changing. Leonard Crestez (3): kbuild: Add wilddt function ARM: dts: imx: Use wilddt function arm64: dts: freescale: Use wilddt function arch/arm/boot/dts/Makefile | 201 ++--- arch/arm64/boot/dts/freescale/Makefile | 25 +-- scripts/Kbuild.include | 6 + 3 files changed, 21 insertions(+), 211 deletions(-) -- 2.17.1
[PATCH 2/3] ARM: dts: imx: Use wilddt function
Use the wilddt function instead of listing imx6/7 dts files. This shrinks the makefile and avoids needing future changes when boards are added. Signed-off-by: Leonard Crestez --- arch/arm/boot/dts/Makefile | 201 ++--- 1 file changed, 11 insertions(+), 190 deletions(-) diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index bd40148a15b2..b71c8af4cd57 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -385,206 +385,27 @@ dtb-$(CONFIG_SOC_IMX53) += \ imx53-tx53-x03x.dtb \ imx53-tx53-x13x.dtb \ imx53-usbarmory.dtb \ imx53-voipac-bsb.dtb dtb-$(CONFIG_SOC_IMX6Q) += \ - imx6dl-apf6dev.dtb \ - imx6dl-aristainetos_4.dtb \ - imx6dl-aristainetos_7.dtb \ - imx6dl-aristainetos2_4.dtb \ - imx6dl-aristainetos2_7.dtb \ - imx6dl-colibri-eval-v3.dtb \ - imx6dl-cubox-i.dtb \ - imx6dl-cubox-i-emmc-som-v15.dtb \ - imx6dl-cubox-i-som-v15.dtb \ - imx6dl-dfi-fs700-m60.dtb \ - imx6dl-emcon-avari.dtb \ - imx6dl-gw51xx.dtb \ - imx6dl-gw52xx.dtb \ - imx6dl-gw53xx.dtb \ - imx6dl-gw54xx.dtb \ - imx6dl-gw551x.dtb \ - imx6dl-gw552x.dtb \ - imx6dl-gw553x.dtb \ - imx6dl-gw560x.dtb \ - imx6dl-gw5903.dtb \ - imx6dl-gw5904.dtb \ - imx6dl-hummingboard.dtb \ - imx6dl-hummingboard-emmc-som-v15.dtb \ - imx6dl-hummingboard-som-v15.dtb \ - imx6dl-hummingboard2.dtb \ - imx6dl-hummingboard2-emmc-som-v15.dtb \ - imx6dl-hummingboard2-som-v15.dtb \ - imx6dl-icore.dtb \ - imx6dl-icore-mipi.dtb \ - imx6dl-icore-rqs.dtb \ - imx6dl-mamoj.dtb \ - imx6dl-nit6xlite.dtb \ - imx6dl-nitrogen6x.dtb \ - imx6dl-phytec-mira-rdk-nand.dtb \ - imx6dl-phytec-pbab01.dtb \ - imx6dl-rex-basic.dtb \ - imx6dl-riotboard.dtb \ - imx6dl-sabreauto.dtb \ - imx6dl-sabrelite.dtb \ - imx6dl-sabresd.dtb \ - imx6dl-savageboard.dtb \ - imx6dl-ts4900.dtb \ - imx6dl-ts7970.dtb \ - imx6dl-tx6dl-comtft.dtb \ - imx6dl-tx6s-8034.dtb \ - imx6dl-tx6s-8034-mb7.dtb \ - imx6dl-tx6s-8035.dtb \ - imx6dl-tx6s-8035-mb7.dtb \ - imx6dl-tx6u-801x.dtb \ - imx6dl-tx6u-80xx-mb7.dtb \ - imx6dl-tx6u-8033.dtb \ - imx6dl-tx6u-8033-mb7.dtb \ - imx6dl-tx6u-811x.dtb \ - imx6dl-tx6u-81xx-mb7.dtb \ - imx6dl-udoo.dtb \ - imx6dl-wandboard.dtb \ - imx6dl-wandboard-revb1.dtb \ - imx6dl-wandboard-revd1.dtb \ - imx6q-apalis-eval.dtb \ - imx6q-apalis-ixora.dtb \ - imx6q-apalis-ixora-v1.1.dtb \ - imx6q-apf6dev.dtb \ - imx6q-arm2.dtb \ - imx6q-b450v3.dtb \ - imx6q-b650v3.dtb \ - imx6q-b850v3.dtb \ - imx6q-cm-fx6.dtb \ - imx6q-cubox-i.dtb \ - imx6q-cubox-i-emmc-som-v15.dtb \ - imx6q-cubox-i-som-v15.dtb \ - imx6q-dfi-fs700-m60.dtb \ - imx6q-dhcom-pdk2.dtb \ - imx6q-display5-tianma-tm070-1280x768.dtb \ - imx6q-dmo-edmqmx6.dtb \ - imx6q-dms-ba16.dtb \ - imx6q-emcon-avari.dtb \ - imx6q-evi.dtb \ - imx6q-gk802.dtb \ - imx6q-gw51xx.dtb \ - imx6q-gw52xx.dtb \ - imx6q-gw53xx.dtb \ - imx6q-gw5400-a.dtb \ - imx6q-gw54xx.dtb \ - imx6q-gw551x.dtb \ - imx6q-gw552x.dtb \ - imx6q-gw553x.dtb \ - imx6q-gw560x.dtb \ - imx6q-gw5903.dtb \ - imx6q-gw5904.dtb \ - imx6q-h100.dtb \ - imx6q-hummingboard.dtb \ - imx6q-hummingboard-emmc-som-v15.dtb \ - imx6q-hummingboard-som-v15.dtb \ - imx6q-hummingboard2.dtb \ - imx6q-hummingboard2-emmc-som-v15.dtb \ - imx6q-hummingboard2-som-v15.dtb \ - imx6q-icore.dtb \ - imx6q-icore-mipi.dtb \ - imx6q-icore-ofcap10.dtb \ - imx6q-icore-ofcap12.dtb \ - imx6q-icore-rqs.dtb \ - imx6q-kp-tpc.dtb \ - imx6q-marsboard.dtb \ - imx6q-mccmon6.dtb \ - imx6q-nitrogen6x.dtb \ - imx6q-nitrogen6_max.dtb \ - imx6q-nitrogen6_som2.dtb \ - imx6q-novena.dtb \ - imx6q-phytec-mira-rdk-emmc.dtb \ - imx6q-phytec-mira-rdk-nand.dtb \ - imx6q-phytec-pbab01.dtb \ - imx6q-pistachio.dtb \ - imx6q-rex-pro.dtb \ - imx6q-sabreauto.dtb \ - imx6q-sabrelite.dtb \ - imx6q-sabresd.dtb \ - imx6q-savageboard.dtb \ - imx6q-sbc6x.dtb \ - imx6q-tbs2910.dtb \ - imx6q-ts4900.dtb \ - imx6q-ts7970.dtb \ - imx6q-tx6q-1010.dtb \ - imx6q-tx6q-1010-comtft.dtb \ - imx6q-tx6q-1020.dtb \ - imx6q-tx6q-1020-comtft.dtb \ - imx6q-tx6q-1036.dtb \ - imx6q-tx6q-1036-mb7.dtb \ - imx6q-tx6q-10x0-mb7.dtb \ - imx6q-tx6q-1110.dtb \ - imx6q-tx6q-11x0-mb7.dtb \ - imx6q-udoo.dtb \ - imx6q-utilite-pro.dtb \ - imx6q-var-dt6customboard.dtb \ - imx6q-wandboard.dtb \ - imx6q
[PATCH 1/3] kbuild: Add wilddt function
Many SOCs have very simple naming conventions for all boards using a certain chip and long dtb lists can be collapsed using wildcards. Since this applied to many architectures add a wilddt function to Kbuild.include. Example usage: dtb-$CONFIG_SOC_ABC123 += $(call wilddt,abc123-*) Signed-off-by: Leonard Crestez --- scripts/Kbuild.include | 6 ++ 1 file changed, 6 insertions(+) diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 46bf1a073f5d..6b8c0cca07c1 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -197,10 +197,16 @@ clean := -f $(srctree)/scripts/Makefile.clean obj # Shorthand for $(Q)$(MAKE) -f scripts/Makefile.headersinst obj= # Usage: # $(Q)$(MAKE) $(hdr-inst)=dir hdr-inst := -f $(srctree)/scripts/Makefile.headersinst obj +### +# Return list of .dtb for all .dts matching $1 in current directory +# +# Example usage: dtb-$CONFIG_SOC_ABC123 += $(call wilddt,abc123-*) +wilddt = $(patsubst $(srctree)/$(src)/%.dts,%.dtb, $(wildcard $(srctree)/$(src)/$(1).dts)) + # Prefix -I with $(srctree) if it is not an absolute path. # skip if -I has no parameter addtree = $(if $(patsubst -I%,%,$(1)), \ $(if $(filter-out -I/% -I./% -I../%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)),$(1)) -- 2.17.1
[PATCH RESEND] ARM: dts: imx6sx: Add DISPLAY power domain support
This was implemented in the driver but not actually defined and
referenced in dts. This makes it always on.
>From reference manual in section "10.4.1.4.1 Power Distribution":
"Display domain - The DISPLAY domain contains GIS, CSI, PXP, LCDIF,
PCIe, DCIC, and LDB. It is supplied by internal regulator."
The current pd_pcie is actually only for PCIE_PHY, the PCIE ip block is
actually inside the DISPLAY domain. Handle this by adding the pcie node
in both power domains.
Signed-off-by: Leonard Crestez
---
arch/arm/boot/dts/imx6sx.dtsi | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
This is the last part of a series which was previously accepted, it was
delayed because it depends on PCI multi-pd support. All driver
dependencies have landed in 5.0-rc1, resending for 5.1 as discussed:
https://lore.kernel.org/patchwork/patch/996812/#1190746
Only change is a minor conflict with removing a pxp clk.
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 272ff6133ec1..ecf3f3e5c0a0 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -783,10 +783,22 @@
#power-domain-cells = <0>;
power-supply = <_soc>;
clocks = < IMX6SX_CLK_GPU>;
};
+ pd_disp: power-domain@2 {
+ reg = <2>;
+ #power-domain-cells = <0>;
+ clocks = <
IMX6SX_CLK_PXP_AXI>,
+<
IMX6SX_CLK_DISPLAY_AXI>,
+<
IMX6SX_CLK_LCDIF1_PIX>,
+<
IMX6SX_CLK_LCDIF_APB>,
+<
IMX6SX_CLK_LCDIF2_PIX>,
+< IMX6SX_CLK_CSI>,
+<
IMX6SX_CLK_VADC>;
+ };
+
pd_pci: power-domain@3 {
reg = <3>;
#power-domain-cells = <0>;
power-supply = <_pcie>;
};
@@ -1203,10 +1215,11 @@
compatible = "fsl,imx6sx-pxp",
"fsl,imx6ull-pxp";
reg = <0x02218000 0x4000>;
interrupts = ;
clocks = < IMX6SX_CLK_PXP_AXI>;
clock-names = "axi";
+ power-domains = <_disp>;
status = "disabled";
};
csi2: csi@221c000 {
reg = <0x0221c000 0x4000>;
@@ -1224,10 +1237,11 @@
interrupts = ;
clocks = < IMX6SX_CLK_LCDIF1_PIX>,
< IMX6SX_CLK_LCDIF_APB>,
< IMX6SX_CLK_DISPLAY_AXI>;
clock-names = "pix", "axi", "disp_axi";
+ power-domains = <_disp>;
status = "disabled";
};
lcdif2: lcdif@2224000 {
compatible = "fsl,imx6sx-lcdif",
"fsl,imx28-lcdif";
@@ -1235,19 +1249,21 @@
interrupts = ;
clocks = < IMX6SX_CLK_LCDIF2_PIX>,
< IMX6SX_CLK_LCDIF_APB>,
< IMX6SX_CLK_DISPLAY_AXI>;
clock-names = "pix", "axi", "disp_axi";
+ power-domains = <_disp>;
status = "disabled";
};
vadc: vadc@2228000 {
reg = <0x02228000 0x4000>, <0x0222c000
0x4000>;
reg-names = "vadc-vafe&qu
Interrupt storm from pinctrl-amd on Acer AN515-42
Hello,
My Acer Nitro 5 AN515-42 laptop with a Ryzen 2700U hangs on boot with
recent kernel on an interrupt storm from pinctrl-amd.
Older kernels work but this seems to be because this module was disabled
by default. I tried to copy over old driver from 4.9 but it still
experiences same issue.
Digging a little deeper it seems the touchpad interrupt is active on
boot and since it's configured as "level" and no touchpad driver is
available yet there does not seem to be any way to clear it.
I don't know how this should be handled, booting with an active enabled but
unclearable interrupt seems like a platform bug to me. There is even an
option to set touchpad to "basic" which does some sort of ps2 emulation
but the IRQ issue still happens!
One workaround is to explicitly disable the interrupt from the handler
if no mapping is found; this will keep it disabled until
amd_gpio_irq_set_type is called later.
--- drivers/pinctrl/pinctrl-amd.c
+++ drivers/pinctrl/pinctrl-amd.c
@@ -567,22 +567,27 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void
*dev_id)
regval = readl(regs + i);
if (!(regval & PIN_IRQ_PENDING) ||
!(regval & BIT(INTERRUPT_MASK_OFF)))
continue;
irq = irq_find_mapping(gc->irq.domain, irqnr + i);
- generic_handle_irq(irq);
+ if (irq) {
+ generic_handle_irq(irq);
+ ret = IRQ_HANDLED;
+ }
/* Clear interrupt.
* We must read the pin register again, in case the
* value was changed while executing
* generic_handle_irq() above.
*/
raw_spin_lock_irqsave(_dev->lock, flags);
regval = readl(regs + i);
+ /* Disable if pending but unmapped */
+ if (!irq && (regval & PIN_IRQ_PENDING))
+ regval &= ~BIT(INTERRUPT_ENABLE_OFF);
writel(regval, regs + i);
raw_spin_unlock_irqrestore(_dev->lock, flags);
- ret = IRQ_HANDLED;
}
}
/* Signal EOI to the GPIO unit */
When in "i2c mode" the touchpad has an ACPI hid "ELAN0504", there are
many similar compatibe hids in elan_i2c driver and if I add this one it
probes successfully and handles irqs but fails to report input (i2c
read data is invalid).
Same laptop experiences some severe p-state throttling issues so there
are many things wrong here. Let me know if you want more version info
or ACPI dumps.
--
Regards,
Leonard
Re: [PATCH v3 3/3] PCI: imx6: Add support for i.MX8MQ
On 12/20/2018 3:22 AM, Trent Piepho wrote: > On Wed, 2018-12-19 at 16:47 -0800, Andrey Smirnov wrote: This series initially added explicit offsets but I suggested a single "controller-id" because: * There are multiple bit and byte offsets * Other imx8 SOCs also have 2x pcie with other bit/byte offsets Hiding this behind a compatible string and single "controller-id" seem preferable to elaborating register maps in dt bindings. It also makes upgrades simpler: if features are added which use other bits there is no need to describe them in DT and deal with compatibility headaches. >>> >>> You already have an id for the controllers: the address. Use that if >>> you don't want to put the register offsets in DT. >> >> Lucas, are you on board with this? > > Does address here mean the address from the controller's reg property? > > How do you map that address to the controller's index? I guess you could have a constant for the address of the first controller and then substract. But hardcoding any sort of physical address feels wrong with DT. > The situation here is that some registers for these controllers are > interleaved, right? I.e., there's one register somewhere where bit 0 > means enable controller 0 and bit 1 means enable controller 1 and so > on. > > Isn't cell-index already the standard device tree property for this > kind of setup? Look at how this cell-index property is documented in other bindings it seems to be an excellent fit: just rename controller-id to cell-index.
Re: [PATCH v3 3/3] PCI: imx6: Add support for i.MX8MQ
On 12/18/2018 5:15 PM, Rob Herring wrote: > On Mon, Dec 17, 2018 at 08:07:02PM -0800, Andrey Smirnov wrote: >> Add code needed to support i.MX8MQ variant. >> >> Signed-off-by: Andrey Smirnov >> Reviewed-by: Lucas Stach >> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt >> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt >> >> +Additional required properties for imx8mq-pcie: >> +- fsl,controller-id: Logical ID of a given PCIE controller. PCIE1 is 0, >> PCIE2 is 1; >> + > > Remove this. > > If GPR register offset is what you need, then put that into DT. > Typically, we'd have a property with iomuxc phandle and offset. This series initially added explicit offsets but I suggested a single "controller-id" because: * There are multiple bit and byte offsets * Other imx8 SOCs also have 2x pcie with other bit/byte offsets Hiding this behind a compatible string and single "controller-id" seem preferable to elaborating register maps in dt bindings. It also makes upgrades simpler: if features are added which use other bits there is no need to describe them in DT and deal with compatibility headaches. Link to older thread: https://lkml.org/lkml/2018/11/29/888 It's possible my suggestion was misguided.
Re: [PATCH v3 3/3] PCI: imx6: Add support for i.MX8MQ
On Mon, 2018-12-17 at 20:07 -0800, Andrey Smirnov wrote:
> Add code needed to support i.MX8MQ variant.
> static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> {
> +
> +
Remove empty lines?
> + unsigned int mask, val, offset;
> +
> + mask = IMX6Q_GPR12_DEVICE_TYPE;
> + val = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT);
... snip ...
> - regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> - IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, mask, val);
Maybe setting port type should be a separate function from init_phy?
Re: [PATCH 2/3] PCI: imx6: Invert checks in imx6_pcie_reset_phy() and imx6_setup_phy_mpll()
On 12/17/2018 1:09 AM, Andrey Smirnov wrote:
> In order to avoid having potentially ever growing list of variants
> that don't support methods implemented in imx6_pcie_reset_phy() and
> imx6_setup_phy_mpll(), change logical checks in the to check for SoC's
> that _do_ support what they implement. While at it, share the code via
> a small helper function.
>
> +static bool imx6_pcie_has_imx6_phy(struct imx6_pcie *imx6_pcie)
> +{
> + return imx6_pcie->variant == IMX6Q ||
> +imx6_pcie->variant == IMX6SX ||
> +imx6_pcie->variant == IMX6QP;
> +}
This would be an ideal match for adding a field inside drvdata, sadly
that was part of a series which stalled:
https://patchwork.kernel.org/patch/10712261/
