* Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Jae Hyun Yoo @ 2018-03-09 23:47 UTC (permalink / raw)
To: Milton Miller II, Pavel Machek
Cc: linux-hwmon, andrew, jdelvare, arnd, linux-doc, andrew, gregkh,
openbmc, linux-kernel, devicetree, linux, linux-arm-kernel
In-Reply-To: <OF08D8261E.8F72E5DC-ON0025824B.008218BB-0025824B.008218C6@notes.na.collabserv.com>
Hi Milton,
Thanks for sharing your time to review this patch. Please see my answer
inline.
Jae
On 3/9/2018 3:41 PM, Milton Miller II wrote:
> About 03/07/2018 04:12PM in some time zone, Pavel Machek wrote:
>> Subject: Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings:
>> Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
>>
>> Hi!
>>
>>>> Are these SoCs x86-based?
>>>
>>> Yes, these are ARM SoCs. Please see Andrew's answer as well.
>>
>> Understood, thanks.
>>
>>>>> + Read sampling point selection. The whole period of a bit time
>> will be
>>>>> + divided into 16 time frames. This value will determine which
>> time frame
>>>>> + this controller will sample PECI signal for data read back.
>> Usually in
>>>>> + the middle of a bit time is the best.
>>>>
>>>> English? "This value will determine when this controller"?
>>>>
>>>
>>> Could I change it like below?:
>>>
>>> "This value will determine in which time frame this controller
>> samples PECI
>>> signal for data read back"
>>
>> I guess... I'm not native speaker, I guess this could be improved
>> some
>> more.
>>
>
> I agree this wording is still confusing.
>
> The problem is that the key subject, the time of the sampling, is in the descriptive clause "in which time frame".
>
> "This value will determine the time frame in which the controller will sample"
>
> or perhaps phrase it as saving a specific sample from the over-clock, or a phase of the clock.
>
Yes, that looks more better. I'll change the wording as you suggested.
Thanks a lot!
Jae
>> Best regards,
>> Pavel
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
>
> milton
> --
> Speaking for myself not IBM.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Milton Miller II @ 2018-03-09 23:41 UTC (permalink / raw)
To: Pavel Machek
Cc: Jae Hyun Yoo, linux-hwmon, andrew, jdelvare, arnd, linux-doc,
andrew, gregkh, openbmc, linux-kernel, devicetree, linux,
linux-arm-kernel
In-Reply-To: <20180307221124.GD10438@amd>
About 03/07/2018 04:12PM in some time zone, Pavel Machek wrote:
>Subject: Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings:
>Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
>
>Hi!
>
>> >Are these SoCs x86-based?
>>
>> Yes, these are ARM SoCs. Please see Andrew's answer as well.
>
>Understood, thanks.
>
>> >>+ Read sampling point selection. The whole period of a bit time
>will be
>> >>+ divided into 16 time frames. This value will determine which
>time frame
>> >>+ this controller will sample PECI signal for data read back.
>Usually in
>> >>+ the middle of a bit time is the best.
>> >
>> >English? "This value will determine when this controller"?
>> >
>>
>> Could I change it like below?:
>>
>> "This value will determine in which time frame this controller
>samples PECI
>> signal for data read back"
>
>I guess... I'm not native speaker, I guess this could be improved
>some
>more.
>
I agree this wording is still confusing.
The problem is that the key subject, the time of the sampling, is in the descriptive clause "in which time frame".
"This value will determine the time frame in which the controller will sample"
or perhaps phrase it as saving a specific sample from the over-clock, or a phase of the clock.
>Best regards,
> Pavel
>
>--
>(english) http://www.livejournal.com/~pavelmachek
>(cesky, pictures)
>http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>
milton
--
Speaking for myself not IBM.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-09 23:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mike Galbraith, Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <20180309221736.GB5926@hirez.programming.kicks-ass.net>
On 03/09/2018 05:17 PM, Peter Zijlstra wrote:
> On Fri, Mar 09, 2018 at 03:43:34PM -0500, Waiman Long wrote:
>> The isolcpus= parameter just reduce the cpus available to the rests of
>> the system. The cpuset controller does look at that value and make
>> adjustment accordingly, but it has no dependence on exclusive cpu/mem
>> features of cpuset.
> The isolcpus= boot param is donkey shit and needs to die. cpuset _used_
> to be able to fully replace it, but with the advent of cgroup 'feature'
> this got lost.
>
> And instead of fixing it, you're making it _far_ worse. You completely
> removed all the bits that allow repartitioning the scheduler domains.
>
> Mike is completely right, full NAK on any such approach.
So you are talking about sched_relax_domain_level and
sched_load_balance. I have not removed any bits. I just haven't exposed
them yet. It does seem like these 2 control knobs are useful from the
scheduling perspective. Do we also need cpu_exclusive or just the two
sched control knobs are enough?
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Peter Zijlstra @ 2018-03-09 22:17 UTC (permalink / raw)
To: Waiman Long
Cc: Mike Galbraith, Tejun Heo, Li Zefan, Johannes Weiner, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <53de9683-01b7-bac4-8b70-dc1f93ede600@redhat.com>
On Fri, Mar 09, 2018 at 03:43:34PM -0500, Waiman Long wrote:
> The isolcpus= parameter just reduce the cpus available to the rests of
> the system. The cpuset controller does look at that value and make
> adjustment accordingly, but it has no dependence on exclusive cpu/mem
> features of cpuset.
The isolcpus= boot param is donkey shit and needs to die. cpuset _used_
to be able to fully replace it, but with the advent of cgroup 'feature'
this got lost.
And instead of fixing it, you're making it _far_ worse. You completely
removed all the bits that allow repartitioning the scheduler domains.
Mike is completely right, full NAK on any such approach.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] [v2] docs: clarify security-bugs disclosure policy
From: Linus Torvalds @ 2018-03-09 21:15 UTC (permalink / raw)
To: Alan Cox
Cc: Dave Hansen, Linux Kernel Mailing List, Dan Williams,
Thomas Gleixner, Greg Kroah-Hartman, Andrea Arcangeli,
Andrew Lutomirski, Kees Cook, Tim Chen, Al Viro, Andrew Morton,
open list:DOCUMENTATION, Jonathan Corbet, Mark Rutland
In-Reply-To: <20180309204526.56301f43@alans-desktop>
On Fri, Mar 9, 2018 at 12:45 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>
> If you want to be taken seriously then I think minimum you also need to
> - Give a GPG key for messages to the list
Oh, I don't want to be taken seriously by people who use gpg encrypted email.
It's garbage and should be shunned as such.
I keep quoting this:
https://motherboard.vice.com/en_us/article/vvbw9a/even-the-inventor-of-pgp-doesnt-use-pgp
and anybody who thinks pgp encrypted email is fine is a clown.
> - State what security is in place (encryption etc) to protect the list
> itself
That could be stated, but it's worth noting the other rules.
If you have some long corrupt vendor disclosure period and are worried
about any good guys finding out (the bad guys probably already have
it), we're not the list for you anyway.
Keep your "we'll keep security problems under wraps so that they can
be exploited for a long time" emails to yourself, or send them to
/dev/null.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] [v2] docs: clarify security-bugs disclosure policy
From: Alan Cox @ 2018-03-09 20:45 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, dan.j.williams, tglx, gregkh, torvalds, aarcange,
luto, keescook, tim.c.chen, viro, akpm, linux-doc, corbet,
mark.rutland
In-Reply-To: <20180307214624.D4361772@viggo.jf.intel.com>
On Wed, 07 Mar 2018 13:46:24 -0800
Dave Hansen <dave.hansen@linux.intel.com> wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> I think we need to soften the language a bit. It might scare folks
> off, especially the:
>
> We prefer to fully disclose the bug as soon as possible.
>
> which is not really the case. Linus says:
>
> It's not full disclosure, it's not coordinated disclosure,
> and it's not "no disclosure". It's more like just "timely
> open fixes".
>
> I changed a bit of the wording in here, but mostly to remove the word
> "disclosure" since it seems to mean very specific things to people
> that we do not mean here.
>
If you want to be taken seriously then I think minimum you also need to
- Give a GPG key for messages to the list
- State what security is in place (encryption etc) to protect the list
itself
There are probably a lot more things people would ask but given the
policy now clear that it's basically just an 'early tip off'/'make sure
Linus doesn't miss this' list for very short notification periods doesn't
matter so much.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-09 20:43 UTC (permalink / raw)
To: Mike Galbraith, Tejun Heo, Li Zefan, Johannes Weiner,
Peter Zijlstra, Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1520624424.27998.76.camel@gmx.de>
On 03/09/2018 02:40 PM, Mike Galbraith wrote:
>>>
>>> If v2 is to ever supersede v1, as is the normal way of things, core
>>> functionality really should be on the v2 boat when it sails. What you
>>> left standing on the dock is critical core cpuset functionality.
>>>
>>> -Mike
>> From your perspective, what are core functionality that should be
>> included in cpuset v2 other than the ability to restrict cpus and memory
>> nodes.
> Exclusive sets are essential, no? How else can you manage set wide
> properties such as topology (and hopefully soonish nohz). You clearly
> can't have overlapping sets, one having scheduler topology, the other
> having none. Whatever the form, something as core as the capability to
> dynamically partition and isolate should IMO be firmly aboard the v2
> boat before it sails.
>
> -Mike
The isolcpus= parameter just reduce the cpus available to the rests of
the system. The cpuset controller does look at that value and make
adjustment accordingly, but it has no dependence on exclusive cpu/mem
features of cpuset.
-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Mike Galbraith @ 2018-03-09 19:40 UTC (permalink / raw)
To: Waiman Long, Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra,
Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <55809fe4-98ba-5566-86ed-457acfef0e1c@redhat.com>
On Fri, 2018-03-09 at 13:20 -0500, Waiman Long wrote:
> On 03/09/2018 01:17 PM, Mike Galbraith wrote:
> > On Fri, 2018-03-09 at 12:45 -0500, Waiman Long wrote:
> >> On 03/09/2018 11:34 AM, Mike Galbraith wrote:
> >>> On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
> >>>> Given the fact that thread mode had been merged into 4.14, it is now
> >>>> time to enable cpuset to be used in the default hierarchy (cgroup v2)
> >>>> as it is clearly threaded.
> >>>>
> >>>> The cpuset controller had experienced feature creep since its
> >>>> introduction more than a decade ago. Besides the core cpus and mems
> >>>> control files to limit cpus and memory nodes, there are a bunch of
> >>>> additional features that can be controlled from the userspace. Some of
> >>>> the features are of doubtful usefulness and may not be actively used.
> >>> One rather important features is the ability to dynamically partition a
> >>> box and isolate critical loads. How does one do that with v2?
> >>>
> >>> In v1, you create two or more exclusive sets, one for generic
> >>> housekeeping, and one or more for critical load(s), RT in my case,
> >>> turning off load balancing in the critical set(s) for obvious reasons.
> >> This patch just serves as a foundation for cpuset support in v2. I am
> >> not excluding the fact that more v1 features will be added in future
> >> patches. We want to start with a clean slate and add on it after careful
> >> consideration. There are some v1 cpuset features that are not used or
> >> rarely used. We certainly want to get rid of them, if possible.
> > If v2 is to ever supersede v1, as is the normal way of things, core
> > functionality really should be on the v2 boat when it sails. What you
> > left standing on the dock is critical core cpuset functionality.
> >
> > -Mike
>
> From your perspective, what are core functionality that should be
> included in cpuset v2 other than the ability to restrict cpus and memory
> nodes.
Exclusive sets are essential, no? How else can you manage set wide
properties such as topology (and hopefully soonish nohz). You clearly
can't have overlapping sets, one having scheduler topology, the other
having none. Whatever the form, something as core as the capability to
dynamically partition and isolate should IMO be firmly aboard the v2
boat before it sails.
-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-09 18:20 UTC (permalink / raw)
To: Mike Galbraith, Tejun Heo, Li Zefan, Johannes Weiner,
Peter Zijlstra, Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1520619426.27998.18.camel@gmx.de>
On 03/09/2018 01:17 PM, Mike Galbraith wrote:
> On Fri, 2018-03-09 at 12:45 -0500, Waiman Long wrote:
>> On 03/09/2018 11:34 AM, Mike Galbraith wrote:
>>> On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
>>>> Given the fact that thread mode had been merged into 4.14, it is now
>>>> time to enable cpuset to be used in the default hierarchy (cgroup v2)
>>>> as it is clearly threaded.
>>>>
>>>> The cpuset controller had experienced feature creep since its
>>>> introduction more than a decade ago. Besides the core cpus and mems
>>>> control files to limit cpus and memory nodes, there are a bunch of
>>>> additional features that can be controlled from the userspace. Some of
>>>> the features are of doubtful usefulness and may not be actively used.
>>> One rather important features is the ability to dynamically partition a
>>> box and isolate critical loads. How does one do that with v2?
>>>
>>> In v1, you create two or more exclusive sets, one for generic
>>> housekeeping, and one or more for critical load(s), RT in my case,
>>> turning off load balancing in the critical set(s) for obvious reasons.
>> This patch just serves as a foundation for cpuset support in v2. I am
>> not excluding the fact that more v1 features will be added in future
>> patches. We want to start with a clean slate and add on it after careful
>> consideration. There are some v1 cpuset features that are not used or
>> rarely used. We certainly want to get rid of them, if possible.
> If v2 is to ever supersede v1, as is the normal way of things, core
> functionality really should be on the v2 boat when it sails. What you
> left standing on the dock is critical core cpuset functionality.
>
> -Mike
From your perspective, what are core functionality that should be
included in cpuset v2 other than the ability to restrict cpus and memory
nodes.
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver
From: Karthik Ramasubramanian @ 2018-03-09 18:18 UTC (permalink / raw)
To: Stephen Boyd, Stephen Boyd, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <152037339742.218381.11498404122038956963@swboyd.mtv.corp.google.com>
On 3/6/2018 2:56 PM, Stephen Boyd wrote:
> Quoting Karthik Ramasubramanian (2018-03-02 16:58:23)
>>
>>
>> On 3/2/2018 1:41 PM, Stephen Boyd wrote:
>>> Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
>>>> +/**
>>>> + * geni_se_read_proto() - Read the protocol configured for a Serial Engine
>>>> + * @se: Pointer to the concerned Serial Engine.
>>>> + *
>>>> + * Return: Protocol value as configured in the serial engine.
>>>> + */
>>>> +u32 geni_se_read_proto(struct geni_se *se)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
>>>> +
>>>> + return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
>>>> +}
>>>> +EXPORT_SYMBOL(geni_se_read_proto);
>>>
>>> Is this API really needed outside of this file? It would seem like the
>>> drivers that implement the protocol, which are child devices, would only
>>> use this API to confirm that the protocol chosen is for their particular
>>> protocol.
>> No, this API is meant for the protocol drivers to confirm that the
>> serial engine is programmed with the firmware for the concerned protocol
>> before using the serial engine. If the check fails, the protocol drivers
>> stop using the serial engine.
>
> Ok maybe we don't really need it then?
Without this function the protocol drivers may not be able to verify if
the serial engine is programmed with the right protocol. Operating on a
serial engine that is not programmed with the right protocol leads to
totally undefined behavior.
> >>>> +
>>>> + ret = geni_se_clks_on(se);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = pinctrl_pm_select_default_state(se->dev);
>>>> + if (ret)
>>>> + geni_se_clks_off(se);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(geni_se_resources_on);
>>>
>>> IS there a reason why we can't use runtime PM or normal linux PM
>>> infrastructure to power on the wrapper and keep it powered while the
>>> protocol driver is active?
>> Besides turning on the clocks & pinctrl settings, wrapper also has to do
>> the bus scaling votes. The bus scaling votes depend on the individual
>> serial interface bandwidth requirements. The bus scaling votes is not
>> present currently. But once the support comes in, this function enables
>> adding it.
>
> Ok, but that would basically be some code consolidation around picking a
> bandwidth and enabling/disabling? It sounds like it could go into either
> the serial interface drivers or into the runtime PM path of the wrapper.
Not really. SPI slaves, for example, can operate on different
frequencies and therefore within a serial engine the bandwidth
requirements can vary based on the slave. UART & I2C serial interfaces
have different bandwidth requirements than SPI. So each serial interface
driver has to specify their bandwidth requirements depending on their
use-case. This function also allows for aggregation of the votes from
the wrapper perspective, instead of constant RPMh communication
>
>>>
>>>> +
>>>> +/**
>>>> + * geni_se_clk_tbl_get() - Get the clock table to program DFS
>>>> + * @se: Pointer to the concerned Serial Engine.
>>>> + * @tbl: Table in which the output is returned.
>>>> + *
>>>> + * This function is called by the protocol drivers to determine the different
>>>> + * clock frequencies supported by Serial Engine Core Clock. The protocol
>>>> + * drivers use the output to determine the clock frequency index to be
>>>> + * programmed into DFS.
>>>> + *
>>>> + * Return: number of valid performance levels in the table on success,
>>>> + * standard Linux error codes on failure.
>>>> + */
>>>> +int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
>>>> +{
>>>> + struct geni_wrapper *wrapper = se->wrapper;
>>>> + unsigned long freq = 0;
>>>> + int i;
>>>> + int ret = 0;
>>>> +
>>>> + mutex_lock(&wrapper->lock);
>>>> + if (wrapper->clk_perf_tbl) {
>>>> + *tbl = wrapper->clk_perf_tbl;
>>>> + ret = wrapper->num_clk_levels;
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + wrapper->clk_perf_tbl = kcalloc(MAX_CLK_PERF_LEVEL,
>>>> + sizeof(*wrapper->clk_perf_tbl),
>>>> + GFP_KERNEL);
>>>> + if (!wrapper->clk_perf_tbl) {
>>>> + ret = -ENOMEM;
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
>>>> + freq = clk_round_rate(se->clk, freq + 1);
>>>> + if (!freq || freq == wrapper->clk_perf_tbl[i - 1])
>>>> + break;
>>>> + wrapper->clk_perf_tbl[i] = freq;
>>>> + }
>>>> + wrapper->num_clk_levels = i;
>>>> + *tbl = wrapper->clk_perf_tbl;
>>>> + ret = wrapper->num_clk_levels;
>>>> +out_unlock:
>>>> + mutex_unlock(&wrapper->lock);
>>>
>>> Is this lock actually protecting anything? I mean to say, is any more
>>> than one geni protocol driver calling this function at a time? Or is
>>> the same geni protocol driver calling this from multiple threads at the
>>> same time? The lock looks almost useless.
>> Yes, there is a possibility of multiple I2C instances within the same
>> wrapper trying to get this table simultaneously.
>>
>> As Evan mentioned in the other thread, Bjorn had the comment to move it
>> to the probe and remove the lock. I looked into the possibility of it.
>> From the hardware perspective, this table belongs to the wrapper and is
>> shared by all the serial engines within the wrapper. But due to software
>> implementation reasons, clk_round_rate can be be performed only on the
>> clocks that are tagged as DFS compatible and only the serial engine
>> clocks are tagged so. At least this was the understanding based on our
>> earlier discussion with the concerned folks. We will revisit it and
>> check if anything has changed recently.
>
> Hmm sounds like the round rate should happen on the parent of the
> se_clk, and this wrapper DT binding should get the clk for the parent of
> the se->clk to run round_rate() on. Then it could all be done in probe,
> which sounds good.
The parent of the se->clk is also specific to the serial engine itself.
So putting that into the wrapper's DT binding does not look like a right
location. For now, I will move the table to the individual serial engine
themselves. Hence the lock can be removed.
>
>>>> +
>>>> +/**
>>>> + * struct geni_se - GENI Serial Engine
>>>> + * @base: Base Address of the Serial Engine's register block.
>>>> + * @dev: Pointer to the Serial Engine device.
>>>> + * @wrapper: Pointer to the parent QUP Wrapper core.
>>>> + * @clk: Handle to the core serial engine clock.
>>>> + */
>>>> +struct geni_se {
>>>> + void __iomem *base;
>>>> + struct device *dev;
>>>> + void *wrapper;
>>>
>>> Can this get the geni_wrapper type? It could be opaque if you like.
>> I am not sure if it is ok to have the children know the details of the
>> parent. That is why it is kept as opaque.
>
> That's fine, but I mean to have struct geni_wrapper *wrapper, and then
> struct geni_wrapper; in this file. Children won't know details and we
> get slightly more type safety.
Ok.
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Mike Galbraith @ 2018-03-09 18:17 UTC (permalink / raw)
To: Waiman Long, Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra,
Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1c3fe7b0-2600-c46d-1527-d3aaf024bb91@redhat.com>
On Fri, 2018-03-09 at 12:45 -0500, Waiman Long wrote:
> On 03/09/2018 11:34 AM, Mike Galbraith wrote:
> > On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
> >> Given the fact that thread mode had been merged into 4.14, it is now
> >> time to enable cpuset to be used in the default hierarchy (cgroup v2)
> >> as it is clearly threaded.
> >>
> >> The cpuset controller had experienced feature creep since its
> >> introduction more than a decade ago. Besides the core cpus and mems
> >> control files to limit cpus and memory nodes, there are a bunch of
> >> additional features that can be controlled from the userspace. Some of
> >> the features are of doubtful usefulness and may not be actively used.
> > One rather important features is the ability to dynamically partition a
> > box and isolate critical loads. How does one do that with v2?
> >
> > In v1, you create two or more exclusive sets, one for generic
> > housekeeping, and one or more for critical load(s), RT in my case,
> > turning off load balancing in the critical set(s) for obvious reasons.
>
> This patch just serves as a foundation for cpuset support in v2. I am
> not excluding the fact that more v1 features will be added in future
> patches. We want to start with a clean slate and add on it after careful
> consideration. There are some v1 cpuset features that are not used or
> rarely used. We certainly want to get rid of them, if possible.
If v2 is to ever supersede v1, as is the normal way of things, core
functionality really should be on the v2 boat when it sails. What you
left standing on the dock is critical core cpuset functionality.
-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PULL] Documentation build fix
From: Jonathan Corbet @ 2018-03-09 17:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LKML, linux-doc
The following changes since commit
e67548254b86a4a6e1b493f041bb7fe28ee74249:
Merge tag 'mips_fixes_4.16_4' of git://git.kernel.org/pub/scm/linux/kernel/git/jhogan/mips (2018-03-08 10:03:12 -0800)
are available in the Git repository at:
git://git.lwn.net/linux.git tags/docs-4.16-fix
for you to fetch changes up to ff690eeed804f112242f9a0614eafdf559f9276a:
Documentation/sphinx: Fix Directive import error (2018-03-09 10:46:14 -0700)
----------------------------------------------------------------
The Sphinx 1.7 release broke the build process for reasons that are mostly
our fault. This is a single fix, cherry-picked from docs-next (where it
has been for a few days now), that restores docs buildability for all
supported Sphinx versions.
----------------------------------------------------------------
Matthew Wilcox (1):
Documentation/sphinx: Fix Directive import error
Documentation/sphinx/kerneldoc.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-09 17:45 UTC (permalink / raw)
To: Mike Galbraith, Tejun Heo, Li Zefan, Johannes Weiner,
Peter Zijlstra, Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1520613285.12489.36.camel@gmx.de>
On 03/09/2018 11:34 AM, Mike Galbraith wrote:
> On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
>> Given the fact that thread mode had been merged into 4.14, it is now
>> time to enable cpuset to be used in the default hierarchy (cgroup v2)
>> as it is clearly threaded.
>>
>> The cpuset controller had experienced feature creep since its
>> introduction more than a decade ago. Besides the core cpus and mems
>> control files to limit cpus and memory nodes, there are a bunch of
>> additional features that can be controlled from the userspace. Some of
>> the features are of doubtful usefulness and may not be actively used.
> One rather important features is the ability to dynamically partition a
> box and isolate critical loads. How does one do that with v2?
>
> In v1, you create two or more exclusive sets, one for generic
> housekeeping, and one or more for critical load(s), RT in my case,
> turning off load balancing in the critical set(s) for obvious reasons.
This patch just serves as a foundation for cpuset support in v2. I am
not excluding the fact that more v1 features will be added in future
patches. We want to start with a clean slate and add on it after careful
consideration. There are some v1 cpuset features that are not used or
rarely used. We certainly want to get rid of them, if possible.
Now for the exclusive cpuset, it is certainly something that can be done
in userspace. If there is a valid use case that requires exclusive
cpuset support in the kernel, we can certainly consider putting it into
v2 as well.
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST
From: Jonathan Corbet @ 2018-03-09 17:41 UTC (permalink / raw)
To: Jonathan Neuschäfer
Cc: linux-gpio, linux-kernel, linux-doc, Linus Walleij
In-Reply-To: <20180308234024.24145-1-j.neuschaefer@gmx.net>
On Fri, 9 Mar 2018 00:40:16 +0100
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> The aim of this patchset is to move the GPIO subsystem's documentation
> under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> and compiled into HTML. I moved everything except for sysfs.txt, because
> this file describes the legacy sysfs ABI, and doesn't seem to serve much
> (non-historical) purpose anymore.
>
> There are still some rough edges:
>
> * I think the API documentation (kernel-doc) should be moved out of
> index.rst into more appropriate files.
> * The headings are arguably wrong, because driver.rst, consumer.rst,
> etc. use the "document title" style, even though they are part of the
> GPIO chapter. But the resulting TOC tree is consistent, and I did not
> want to change almost all headings.
> * Some of the files could use more :c:func:`...` references and general
> ReST polish.
>
> But I don't want to make this patchset too large.
[For future reference, if you're going to CC me on most of a patch series,
I'd really rather get the whole thing so I don't have to go looking for
it.]
From a quick look, it seems like a reasonable RST conversion to me. I do
wonder if sysfs.txt should just be removed, since it documents something
we don't want people to use at this point? Historical purposes are well
served by the repository history.
I'd say changing the headings is worthwhile if it produces a better
result. OTOH I'd be wary of adding too much "polish"; we really want to
retain the readability of the plain-text files.
Anyway, I'm happy to take these through the docs tree or see them go via
GPIO; Linus, what's your preference?
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Mike Galbraith @ 2018-03-09 17:23 UTC (permalink / raw)
To: Waiman Long, Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra,
Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1520613285.12489.36.camel@gmx.de>
On Fri, 2018-03-09 at 17:34 +0100, Mike Galbraith wrote:
> On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
> > Given the fact that thread mode had been merged into 4.14, it is now
> > time to enable cpuset to be used in the default hierarchy (cgroup v2)
> > as it is clearly threaded.
> >
> > The cpuset controller had experienced feature creep since its
> > introduction more than a decade ago. Besides the core cpus and mems
> > control files to limit cpus and memory nodes, there are a bunch of
> > additional features that can be controlled from the userspace. Some of
> > the features are of doubtful usefulness and may not be actively used.
>
> One rather important features is the ability to dynamically partition a
> box and isolate critical loads. How does one do that with v2?
This still very much in use stuff that started below, not to mention
the nohz_full stuff that Frederic Weisbecker is working on integrating
so it can blossom into a proper dynamic set property.
Author: Dinakar Guniguntala <dino@in.ibm.com> 2005-06-25 23:57:33
Committer: Linus Torvalds <torvalds@ppc970.osdl.org> 2005-06-26 01:24:45
Parent: 37e4ab3f0cba13adf3535d373fd98e5ee47b5410 ([PATCH] Changing RT priority without CAP_SYS_NICE)
Child: 85d7b94981e2e919697bc235aad7367b33c3864b ([PATCH] Dynamic sched domains: cpuset changes)
Branches: master, remotes/origin/master and many more (82)
Follows: v2.6.12
Precedes: v2.6.13-rc1
[PATCH] Dynamic sched domains: sched changes
The following patches add dynamic sched domains functionality that was
extensively discussed on lkml and lse-tech. I would like to see this added to
-mm
o The main advantage with this feature is that it ensures that the scheduler
load balacing code only balances against the cpus that are in the sched
domain as defined by an exclusive cpuset and not all of the cpus in the
system. This removes any overhead due to load balancing code trying to
pull tasks outside of the cpu exclusive cpuset only to be prevented by
the tasks' cpus_allowed mask.
o cpu exclusive cpusets are useful for servers running orthogonal
workloads such as RT applications requiring low latency and HPC
applications that are throughput sensitive
o It provides a new API partition_sched_domains in sched.c
that makes dynamic sched domains possible.
o cpu_exclusive cpusets sets are now associated with a sched domain.
Which means that the users can dynamically modify the sched domains
through the cpuset file system interface
o ia64 sched domain code has been updated to support this feature as well
o Currently, this does not support hotplug. (However some of my tests
indicate hotplug+preempt is currently broken)
o I have tested it extensively on x86.
o This should have very minimal impact on performance as none of
the fast paths are affected
Signed-off-by: Dinakar Guniguntala <dino@in.ibm.com>
Acked-by: Paul Jackson <pj@sgi.com>
Acked-by: Nick Piggin <nickpiggin@yahoo.com.au>
Acked-by: Matthew Dobson <colpatch@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Mike Galbraith @ 2018-03-09 16:34 UTC (permalink / raw)
To: Waiman Long, Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra,
Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
torvalds, Roman Gushchin
In-Reply-To: <1520609707-16582-1-git-send-email-longman@redhat.com>
On Fri, 2018-03-09 at 10:35 -0500, Waiman Long wrote:
> Given the fact that thread mode had been merged into 4.14, it is now
> time to enable cpuset to be used in the default hierarchy (cgroup v2)
> as it is clearly threaded.
>
> The cpuset controller had experienced feature creep since its
> introduction more than a decade ago. Besides the core cpus and mems
> control files to limit cpus and memory nodes, there are a bunch of
> additional features that can be controlled from the userspace. Some of
> the features are of doubtful usefulness and may not be actively used.
One rather important features is the ability to dynamically partition a
box and isolate critical loads. How does one do that with v2?
In v1, you create two or more exclusive sets, one for generic
housekeeping, and one or more for critical load(s), RT in my case,
turning off load balancing in the critical set(s) for obvious reasons.
> This patch enables cpuset controller in the default hierarchy with
> a minimal set of features, namely just the cpus and mems and their
> effective_* counterparts. We can certainly add more features to the
> default hierarchy in the future if there is a real user need for them
> later on.
>
> Alternatively, with the unified hiearachy, it may make more sense
> to move some of those additional cpuset features, if desired, to
> memory controller or may be to the cpu controller instead of staying
> with cpuset.
>
> v4:
> - Further minimize the feature set by removing the flags control knob.
>
> v3:
> - Further trim the additional features down to just memory_migrate.
> - Update Documentation/cgroup-v2.txt.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> Documentation/cgroup-v2.txt | 96 ++++++++++++++++++++++++++++++++++++++++-----
> kernel/cgroup/cpuset.c | 44 +++++++++++++++++++--
> 2 files changed, 127 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
> index 74cdeae..8d7300f 100644
> --- a/Documentation/cgroup-v2.txt
> +++ b/Documentation/cgroup-v2.txt
> @@ -48,16 +48,18 @@ v1 is available under Documentation/cgroup-v1/.
> 5-2-1. Memory Interface Files
> 5-2-2. Usage Guidelines
> 5-2-3. Memory Ownership
> - 5-3. IO
> - 5-3-1. IO Interface Files
> - 5-3-2. Writeback
> - 5-4. PID
> - 5-4-1. PID Interface Files
> - 5-5. Device
> - 5-6. RDMA
> - 5-6-1. RDMA Interface Files
> - 5-7. Misc
> - 5-7-1. perf_event
> + 5-3. Cpuset
> + 5.3-1. Cpuset Interface Files
> + 5-4. IO
> + 5-4-1. IO Interface Files
> + 5-4-2. Writeback
> + 5-5. PID
> + 5-5-1. PID Interface Files
> + 5-6. Device
> + 5-7. RDMA
> + 5-7-1. RDMA Interface Files
> + 5-8. Misc
> + 5-8-1. perf_event
> 5-N. Non-normative information
> 5-N-1. CPU controller root cgroup process behaviour
> 5-N-2. IO controller root cgroup process behaviour
> @@ -1243,6 +1245,80 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
> belonging to the affected files to ensure correct memory ownership.
>
>
> +Cpuset
> +------
> +
> +The "cpuset" controller provides a mechanism for constraining
> +the CPU and memory node placement of tasks to only the resources
> +specified in the cpuset interface files in a task's current cgroup.
> +This is especially valuable on large NUMA systems where placing jobs
> +on properly sized subsets of the systems with careful processor and
> +memory placement to reduce cross-node memory access and contention
> +can improve overall system performance.
> +
> +The "cpuset" controller is hierarchical. That means the controller
> +cannot use CPUs or memory nodes not allowed in its parent.
> +
> +
> +Cpuset Interface Files
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> + cpuset.cpus
> + A read-write multiple values file which exists on non-root
> + cgroups.
> +
> + It lists the CPUs allowed to be used by tasks within this
> + cgroup. The CPU numbers are comma-separated numbers or
> + ranges. For example:
> +
> + # cat cpuset.cpus
> + 0-4,6,8-10
> +
> + An empty value indicates that the cgroup is using the same
> + setting as the nearest cgroup ancestor with a non-empty
> + "cpuset.cpus" or all the available CPUs if none is found.
> +
> + The value of "cpuset.cpus" stays constant until the next update
> + and won't be affected by any CPU hotplug events.
> +
> + cpuset.effective_cpus
> + A read-only multiple values file which exists on non-root
> + cgroups.
> +
> + It lists the onlined CPUs that are actually allowed to be
> + used by tasks within the current cgroup. It is a subset of
> + "cpuset.cpus". Its value will be affected by CPU hotplug
> + events.
> +
> + cpuset.mems
> + A read-write multiple values file which exists on non-root
> + cgroups.
> +
> + It lists the memory nodes allowed to be used by tasks within
> + this cgroup. The memory node numbers are comma-separated
> + numbers or ranges. For example:
> +
> + # cat cpuset.mems
> + 0-1,3
> +
> + An empty value indicates that the cgroup is using the same
> + setting as the nearest cgroup ancestor with a non-empty
> + "cpuset.mems" or all the available memory nodes if none
> + is found.
> +
> + The value of "cpuset.mems" stays constant until the next update
> + and won't be affected by any memory nodes hotplug events.
> +
> + cpuset.effective_mems
> + A read-only multiple values file which exists on non-root
> + cgroups.
> +
> + It lists the onlined memory nodes that are actually allowed
> + to be used by tasks within the current cgroup. It is a subset
> + of "cpuset.mems". Its value will be affected by memory nodes
> + hotplug events.
> +
> +
> IO
> --
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index b42037e..7837d1f 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1823,12 +1823,11 @@ static s64 cpuset_read_s64(struct cgroup_subsys_state *css, struct cftype *cft)
> return 0;
> }
>
> -
> /*
> * for the common functions, 'private' gives the type of file
> */
>
> -static struct cftype files[] = {
> +static struct cftype legacy_files[] = {
> {
> .name = "cpus",
> .seq_show = cpuset_common_seq_show,
> @@ -1931,6 +1930,43 @@ static s64 cpuset_read_s64(struct cgroup_subsys_state *css, struct cftype *cft)
> };
>
> /*
> + * This is currently a minimal set for the default hierarchy. It can be
> + * expanded later on by migrating more features and control files from v1.
> + */
> +static struct cftype dfl_files[] = {
> + {
> + .name = "cpus",
> + .seq_show = cpuset_common_seq_show,
> + .write = cpuset_write_resmask,
> + .max_write_len = (100U + 6 * NR_CPUS),
> + .private = FILE_CPULIST,
> + },
> +
> + {
> + .name = "mems",
> + .seq_show = cpuset_common_seq_show,
> + .write = cpuset_write_resmask,
> + .max_write_len = (100U + 6 * MAX_NUMNODES),
> + .private = FILE_MEMLIST,
> + },
> +
> + {
> + .name = "effective_cpus",
> + .seq_show = cpuset_common_seq_show,
> + .private = FILE_EFFECTIVE_CPULIST,
> + },
> +
> + {
> + .name = "effective_mems",
> + .seq_show = cpuset_common_seq_show,
> + .private = FILE_EFFECTIVE_MEMLIST,
> + },
> +
> + { } /* terminate */
> +};
> +
> +
> +/*
> * cpuset_css_alloc - allocate a cpuset css
> * cgrp: control group that the new cpuset will be part of
> */
> @@ -2104,8 +2140,10 @@ struct cgroup_subsys cpuset_cgrp_subsys = {
> .post_attach = cpuset_post_attach,
> .bind = cpuset_bind,
> .fork = cpuset_fork,
> - .legacy_cftypes = files,
> + .legacy_cftypes = legacy_files,
> + .dfl_cftypes = dfl_files,
> .early_init = true,
> + .threaded = true,
> };
>
> /**
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-09 15:35 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto, efault,
torvalds, Roman Gushchin, Waiman Long
Given the fact that thread mode had been merged into 4.14, it is now
time to enable cpuset to be used in the default hierarchy (cgroup v2)
as it is clearly threaded.
The cpuset controller had experienced feature creep since its
introduction more than a decade ago. Besides the core cpus and mems
control files to limit cpus and memory nodes, there are a bunch of
additional features that can be controlled from the userspace. Some of
the features are of doubtful usefulness and may not be actively used.
This patch enables cpuset controller in the default hierarchy with
a minimal set of features, namely just the cpus and mems and their
effective_* counterparts. We can certainly add more features to the
default hierarchy in the future if there is a real user need for them
later on.
Alternatively, with the unified hiearachy, it may make more sense
to move some of those additional cpuset features, if desired, to
memory controller or may be to the cpu controller instead of staying
with cpuset.
v4:
- Further minimize the feature set by removing the flags control knob.
v3:
- Further trim the additional features down to just memory_migrate.
- Update Documentation/cgroup-v2.txt.
Signed-off-by: Waiman Long <longman@redhat.com>
---
Documentation/cgroup-v2.txt | 96 ++++++++++++++++++++++++++++++++++++++++-----
kernel/cgroup/cpuset.c | 44 +++++++++++++++++++--
2 files changed, 127 insertions(+), 13 deletions(-)
diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
index 74cdeae..8d7300f 100644
--- a/Documentation/cgroup-v2.txt
+++ b/Documentation/cgroup-v2.txt
@@ -48,16 +48,18 @@ v1 is available under Documentation/cgroup-v1/.
5-2-1. Memory Interface Files
5-2-2. Usage Guidelines
5-2-3. Memory Ownership
- 5-3. IO
- 5-3-1. IO Interface Files
- 5-3-2. Writeback
- 5-4. PID
- 5-4-1. PID Interface Files
- 5-5. Device
- 5-6. RDMA
- 5-6-1. RDMA Interface Files
- 5-7. Misc
- 5-7-1. perf_event
+ 5-3. Cpuset
+ 5.3-1. Cpuset Interface Files
+ 5-4. IO
+ 5-4-1. IO Interface Files
+ 5-4-2. Writeback
+ 5-5. PID
+ 5-5-1. PID Interface Files
+ 5-6. Device
+ 5-7. RDMA
+ 5-7-1. RDMA Interface Files
+ 5-8. Misc
+ 5-8-1. perf_event
5-N. Non-normative information
5-N-1. CPU controller root cgroup process behaviour
5-N-2. IO controller root cgroup process behaviour
@@ -1243,6 +1245,80 @@ POSIX_FADV_DONTNEED to relinquish the ownership of memory areas
belonging to the affected files to ensure correct memory ownership.
+Cpuset
+------
+
+The "cpuset" controller provides a mechanism for constraining
+the CPU and memory node placement of tasks to only the resources
+specified in the cpuset interface files in a task's current cgroup.
+This is especially valuable on large NUMA systems where placing jobs
+on properly sized subsets of the systems with careful processor and
+memory placement to reduce cross-node memory access and contention
+can improve overall system performance.
+
+The "cpuset" controller is hierarchical. That means the controller
+cannot use CPUs or memory nodes not allowed in its parent.
+
+
+Cpuset Interface Files
+~~~~~~~~~~~~~~~~~~~~~~
+
+ cpuset.cpus
+ A read-write multiple values file which exists on non-root
+ cgroups.
+
+ It lists the CPUs allowed to be used by tasks within this
+ cgroup. The CPU numbers are comma-separated numbers or
+ ranges. For example:
+
+ # cat cpuset.cpus
+ 0-4,6,8-10
+
+ An empty value indicates that the cgroup is using the same
+ setting as the nearest cgroup ancestor with a non-empty
+ "cpuset.cpus" or all the available CPUs if none is found.
+
+ The value of "cpuset.cpus" stays constant until the next update
+ and won't be affected by any CPU hotplug events.
+
+ cpuset.effective_cpus
+ A read-only multiple values file which exists on non-root
+ cgroups.
+
+ It lists the onlined CPUs that are actually allowed to be
+ used by tasks within the current cgroup. It is a subset of
+ "cpuset.cpus". Its value will be affected by CPU hotplug
+ events.
+
+ cpuset.mems
+ A read-write multiple values file which exists on non-root
+ cgroups.
+
+ It lists the memory nodes allowed to be used by tasks within
+ this cgroup. The memory node numbers are comma-separated
+ numbers or ranges. For example:
+
+ # cat cpuset.mems
+ 0-1,3
+
+ An empty value indicates that the cgroup is using the same
+ setting as the nearest cgroup ancestor with a non-empty
+ "cpuset.mems" or all the available memory nodes if none
+ is found.
+
+ The value of "cpuset.mems" stays constant until the next update
+ and won't be affected by any memory nodes hotplug events.
+
+ cpuset.effective_mems
+ A read-only multiple values file which exists on non-root
+ cgroups.
+
+ It lists the onlined memory nodes that are actually allowed
+ to be used by tasks within the current cgroup. It is a subset
+ of "cpuset.mems". Its value will be affected by memory nodes
+ hotplug events.
+
+
IO
--
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index b42037e..7837d1f 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1823,12 +1823,11 @@ static s64 cpuset_read_s64(struct cgroup_subsys_state *css, struct cftype *cft)
return 0;
}
-
/*
* for the common functions, 'private' gives the type of file
*/
-static struct cftype files[] = {
+static struct cftype legacy_files[] = {
{
.name = "cpus",
.seq_show = cpuset_common_seq_show,
@@ -1931,6 +1930,43 @@ static s64 cpuset_read_s64(struct cgroup_subsys_state *css, struct cftype *cft)
};
/*
+ * This is currently a minimal set for the default hierarchy. It can be
+ * expanded later on by migrating more features and control files from v1.
+ */
+static struct cftype dfl_files[] = {
+ {
+ .name = "cpus",
+ .seq_show = cpuset_common_seq_show,
+ .write = cpuset_write_resmask,
+ .max_write_len = (100U + 6 * NR_CPUS),
+ .private = FILE_CPULIST,
+ },
+
+ {
+ .name = "mems",
+ .seq_show = cpuset_common_seq_show,
+ .write = cpuset_write_resmask,
+ .max_write_len = (100U + 6 * MAX_NUMNODES),
+ .private = FILE_MEMLIST,
+ },
+
+ {
+ .name = "effective_cpus",
+ .seq_show = cpuset_common_seq_show,
+ .private = FILE_EFFECTIVE_CPULIST,
+ },
+
+ {
+ .name = "effective_mems",
+ .seq_show = cpuset_common_seq_show,
+ .private = FILE_EFFECTIVE_MEMLIST,
+ },
+
+ { } /* terminate */
+};
+
+
+/*
* cpuset_css_alloc - allocate a cpuset css
* cgrp: control group that the new cpuset will be part of
*/
@@ -2104,8 +2140,10 @@ struct cgroup_subsys cpuset_cgrp_subsys = {
.post_attach = cpuset_post_attach,
.bind = cpuset_bind,
.fork = cpuset_fork,
- .legacy_cftypes = files,
+ .legacy_cftypes = legacy_files,
+ .dfl_cftypes = dfl_files,
.early_init = true,
+ .threaded = true,
};
/**
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Karthik Ramasubramanian @ 2018-03-09 6:43 UTC (permalink / raw)
To: Doug Anderson, Jonathan Corbet, Andy Gross, David Brown,
Rob Herring, Mark Rutland, Wolfram Sang, Greg Kroah-Hartman
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
Jiri Slaby, evgreen, acourbot, Sagar Dharia, Girish Mahadevan,
swboyd
In-Reply-To: <CAD=FV=WbeP8xpVi7cji9hsNyxEam2D5D+_21ur_MuUO8FK42OQ@mail.gmail.com>
On 3/7/2018 2:16 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Feb 27, 2018 at 5:38 PM, Karthikeyan Ramasubramanian
> <kramasub@codeaurora.org> wrote:
>> This bus driver supports the GENI based i2c hardware controller in the
>> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
>> module supporting a wide range of serial interfaces including I2C. The
>> driver supports FIFO mode and DMA mode of transfer and switches modes
>> dynamically depending on the size of the transfer.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> drivers/i2c/busses/Kconfig | 11 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-qcom-geni.c | 626 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 638 insertions(+)
>
> I'm not an expert on geni (and, to be honest, I haven't read the main
> geni patch yet). ...but I figured I could at least add my $0.02 since
> I've stared at i2c bus drivers a lot in the past. Feel free to tell
> me if I'm full or crap...
>
>
>> create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index e2954fb..1ddf5cd 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -848,6 +848,17 @@ config I2C_PXA_SLAVE
>> is necessary for systems where the PXA may be a target on the
>> I2C bus.
>>
>> +config I2C_QCOM_GENI
>> + tristate "Qualcomm Technologies Inc.'s GENI based I2C controller"
>> + depends on ARCH_QCOM
>> + depends on QCOM_GENI_SE
>> + help
>> + If you say yes to this option, support will be included for the
>> + built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs.
>
> Kind of a generic description and this driver is only for new SoCs,
> right? Maybe make it a little more specific?
Ok.
>
>
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called i2c-qcom-geni.
>> +
>> config I2C_QUP
>> tristate "Qualcomm QUP based I2C controller"
>> depends on ARCH_QCOM
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 2ce8576..201fce1 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
>> obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
>> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
>> obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
>> +obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o
>> obj-$(CONFIG_I2C_QUP) += i2c-qup.o
>> obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
>> obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> new file mode 100644
>> index 0000000..e1e4268
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -0,0 +1,626 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +
>> +#include <linux/clk.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/qcom-geni-se.h>
>> +
>> +#define SE_I2C_TX_TRANS_LEN 0x26c
>> +#define SE_I2C_RX_TRANS_LEN 0x270
>> +#define SE_I2C_SCL_COUNTERS 0x278
>> +
>> +#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\
>> + M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN)
>> +#define SE_I2C_ABORT BIT(1)
>> +
>> +/* M_CMD OP codes for I2C */
>> +#define I2C_WRITE 0x1
>> +#define I2C_READ 0x2
>> +#define I2C_WRITE_READ 0x3
>> +#define I2C_ADDR_ONLY 0x4
>> +#define I2C_BUS_CLEAR 0x6
>> +#define I2C_STOP_ON_BUS 0x7
>> +/* M_CMD params for I2C */
>> +#define PRE_CMD_DELAY BIT(0)
>> +#define TIMESTAMP_BEFORE BIT(1)
>> +#define STOP_STRETCH BIT(2)
>> +#define TIMESTAMP_AFTER BIT(3)
>> +#define POST_COMMAND_DELAY BIT(4)
>> +#define IGNORE_ADD_NACK BIT(6)
>> +#define READ_FINISHED_WITH_ACK BIT(7)
>> +#define BYPASS_ADDR_PHASE BIT(8)
>> +#define SLV_ADDR_MSK GENMASK(15, 9)
>> +#define SLV_ADDR_SHFT 9
>> +/* I2C SCL COUNTER fields */
>> +#define HIGH_COUNTER_MSK GENMASK(29, 20)
>> +#define HIGH_COUNTER_SHFT 20
>> +#define LOW_COUNTER_MSK GENMASK(19, 10)
>> +#define LOW_COUNTER_SHFT 10
>> +#define CYCLE_COUNTER_MSK GENMASK(9, 0)
>> +
>> +#define GP_IRQ0 0
>> +#define GP_IRQ1 1
>> +#define GP_IRQ2 2
>> +#define GP_IRQ3 3
>> +#define GP_IRQ4 4
>> +#define GP_IRQ5 5
>> +#define GENI_OVERRUN 6
>> +#define GENI_ILLEGAL_CMD 7
>> +#define GENI_ABORT_DONE 8
>> +#define GENI_TIMEOUT 9
>
> Above should be an enum; then use the enum type as the parameter to
> geni_i2c_err() so it's obvious that "err" is not a normal linux error
> code.
>
>
>> +#define I2C_NACK GP_IRQ1
>> +#define I2C_BUS_PROTO GP_IRQ3
>> +#define I2C_ARB_LOST GP_IRQ4
>
> Get rid of definition of GP_IRQ1, 3, and 4 and just define I2C_NACK,
> I2C_BUS_PROTO, and I2C_ARB_LOST directly.
>
>
>> +#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \
>> + << 5)
>
> Should these really be using "GP_IRQ1", "GP_IRQ3", and "GP_IRQ4".
> Does this use of those numbers have anything to do with the other use
> of them? Seems like this should just be BIT(1) | BIT(3) | BIT(4).
>
> Said another way: does bit 1 in this field coorespond to NACK, bit 3
> correspond to BUS_PROTO, and bit 4 correspond to ARB_LOST? If not
> then I see no reason to try to tie them together. If they do
> correspond then use BIT(I2C_NACK), etc...
>
The programming manual identifies the bits of the IRQ_STATUS register as
GP_IRQ* and when the concerned serial engines are used as I2C
controllers, those bit fields mean NACK, BUS_PROTO, ARB_LOST, etc. That
is why it was mentioned that way. I will update as you point out.
>
>> +
>> +#define I2C_AUTO_SUSPEND_DELAY 250
>
> Why 250 ms? That seems like an eternity. Is it really that expensive
> to turn resources off and on? I would sorta just expect clocks and
> stuff to get turned off right after a transaction finished unless
> another one was pending right behind it...
>
>
>> +#define KHz(freq) (1000 * freq)
>
> I probably wouldn't define KHz macro and just used numbers like 100000
> like all the other i2c drivers, but I guess it's OK. Should be all
> caps, though?
I will change to all caps.
>
>
>> +#define PACKING_BYTES_PW 4
>> +
>> +struct geni_i2c_dev {
>> + struct geni_se se;
>> + u32 tx_wm;
>> + int irq;
>> + int err;
>> + struct i2c_adapter adap;
>> + struct completion done;
>> + struct i2c_msg *cur;
>> + int cur_wr;
>> + int cur_rd;
>> + u32 clk_freq_out;
>> + const struct geni_i2c_clk_fld *clk_fld;
>> +};
>> +
>> +struct geni_i2c_err_log {
>> + int err;
>> + const char *msg;
>> +};
>> +
>> +static struct geni_i2c_err_log gi2c_log[] = {
>
> static const?
Ok.
>
>
>> + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
>> + [I2C_NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its power/reset-ln"},
>
> Longer than 80 characters; don't split the string, but you could still
> wrap better.
In the v2 patch, there was a comment to break the 80 character limit to
improve the readability.
>
>
>> + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
>> + [I2C_BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"},
>> + [I2C_ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"},
>> + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
>> + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
>> + [GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state machine"},
>> + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
>> + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
>> +};
>> +
>> +struct geni_i2c_clk_fld {
>> + u32 clk_freq_out;
>> + u8 clk_div;
>> + u8 t_high;
>> + u8 t_low;
>> + u8 t_cycle;
>> +};
>> +
>> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
>> + {KHz(100), 7, 10, 11, 26},
>> + {KHz(400), 2, 5, 12, 24},
>> + {KHz(1000), 1, 3, 9, 18},
>
> So I guess this is all relying on an input serial clock of 19.2MHz?
> Maybe document that?
>
> Assuming I'm understanding the math here, is it really OK for your
> 100kHz and 1MHz mode to be running slightly fast?
>
> 19200. / 2 / 24
>>>> 400.0
>
> 19200. / 7 / 26
>>>> 105.49450549450549
>
> 19200. / 1 / 18
>>>> 1066.6666666666667
>
> It seems like you'd want the fastest clock that you can make that's
> _less than_ the spec.
>
>
> It would also be interesting to know if it's expected that boards
> might need to tweak the t_high / t_low depending on their electrical
> characteristics. In the past I've had lots of requests from board
> makers to tweak things because they've got a long trace, or a stronger
> or weaker pull, or ... If so we might later need to add some dts
> properties like "i2c-scl-rising-time-ns" and make the math more
> dynamic here, unless your hardware somehow automatically adjusts for
> this type of thing...
>
>
>> +};
>> +
>> +static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>> +{
>> + int i;
>> + const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map;
>> +
>> + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) {
>> + if (itr->clk_freq_out == gi2c->clk_freq_out) {
>> + gi2c->clk_fld = geni_i2c_clk_map + i;
>
> Isn't "geni_i2c_clk_map + i" just "itr"?
Yes right.
>
>
>> + return 0;
>> + }
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>> +{
>> + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>> + u32 val;
>> +
>> + writel_relaxed(0, gi2c->se.base + SE_GENI_CLK_SEL);
>> +
>> + val = (itr->clk_div << CLK_DIV_SHFT) | SER_CLK_EN;
>> + writel_relaxed(val, gi2c->se.base + GENI_SER_M_CLK_CFG);
>> +
>> + val = itr->t_high << HIGH_COUNTER_SHFT;
>> + val |= itr->t_low << LOW_COUNTER_SHFT;
>> + val |= itr->t_cycle;
>> + writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>> + /*
>> + * Ensure later writes/reads to serial engine register block is
>> + * not reordered before this point.
>> + */
>> + mb();
>
> This mb() is to make sure that later writes to "gi2c->se.base" are not
> reordered to be above the ones in this function? You don't need a
> mb(). writel_relaxed() already enforces this.
Let me check if there are no readl_relaxed after this. If not, I will
remove the barrier.
>
>
>> +}
>> +
>> +static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 m_cmd = readl_relaxed(gi2c->se.base + SE_GENI_M_CMD0);
>> + u32 m_stat = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
>> + u32 geni_s = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
>> + u32 geni_ios = readl_relaxed(gi2c->se.base + SE_GENI_IOS);
>> + u32 dma = readl_relaxed(gi2c->se.base + SE_GENI_DMA_MODE_EN);
>> + u32 rx_st, tx_st;
>> +
>> + if (dma) {
>> + rx_st = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT);
>> + tx_st = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT);
>> + } else {
>> + rx_st = readl_relaxed(gi2c->se.base + SE_GENI_RX_FIFO_STATUS);
>> + tx_st = readl_relaxed(gi2c->se.base + SE_GENI_TX_FIFO_STATUS);
>> + }
>> + dev_dbg(gi2c->se.dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n",
>> + dma, tx_st, rx_st, m_stat);
>> + dev_dbg(gi2c->se.dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n",
>> + m_cmd, geni_s, geni_ios);
>> +}
>> +
>> +static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
>> +{
>> + gi2c->err = gi2c_log[err].err;
>
> You should only set gi2c->err if it was 0 to start with. You want
> "err" to contain the first error, not the last one. This is
> especially important due to the comment elsewhere in this patch "if
> this is err with done-bit not set, handle that through timeout". You
> don't want the timeout to clobber the true error.
True.
>
>
> On a separate note: I wonder if it makes sense to couch the rest of
> this function in something that will compile to a no-op if DEBUG and
> DYNAMIC_DEBUG aren't defined? Then you can avoid including code for
> all these readl calls.
Given that these are not common scenarios, it may be a premature
optimization.
>
>> + if (gi2c->cur)
>> + dev_dbg(gi2c->se.dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
>> + gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
>> + dev_dbg(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
>> +
>> + if (err != I2C_NACK && err != GENI_ABORT_DONE)
>> + geni_i2c_err_misc(gi2c);
>> +}
>> +
>> +static irqreturn_t geni_i2c_irq(int irq, void *dev)
>> +{
>> + struct geni_i2c_dev *gi2c = dev;
>> + int j;
>> + u32 m_stat = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
>> + u32 rx_st = readl_relaxed(gi2c->se.base + SE_GENI_RX_FIFO_STATUS);
>> + u32 dm_tx_st = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT);
>> + u32 dm_rx_st = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT);
>> + u32 dma = readl_relaxed(gi2c->se.base + SE_GENI_DMA_MODE_EN);
>> + struct i2c_msg *cur = gi2c->cur;
>> +
>> + if (!cur ||
>> + m_stat & (M_CMD_FAILURE_EN | M_CMD_ABORT_EN) ||
>> + dm_rx_st & (DM_I2C_CB_ERR)) {
>> + if (m_stat & M_GP_IRQ_1_EN)
>> + geni_i2c_err(gi2c, I2C_NACK);
>> + if (m_stat & M_GP_IRQ_3_EN)
>> + geni_i2c_err(gi2c, I2C_BUS_PROTO);
>> + if (m_stat & M_GP_IRQ_4_EN)
>> + geni_i2c_err(gi2c, I2C_ARB_LOST);
>> + if (m_stat & M_CMD_OVERRUN_EN)
>> + geni_i2c_err(gi2c, GENI_OVERRUN);
>> + if (m_stat & M_ILLEGAL_CMD_EN)
>> + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD);
>> + if (m_stat & M_CMD_ABORT_EN)
>> + geni_i2c_err(gi2c, GENI_ABORT_DONE);
>> + if (m_stat & M_GP_IRQ_0_EN)
>> + geni_i2c_err(gi2c, GP_IRQ0);
>> +
>> + /* Disable the TX Watermark interrupt to stop TX */
>> + if (!dma)
>> + writel_relaxed(0, gi2c->se.base +
>> + SE_GENI_TX_WATERMARK_REG);
>> + goto irqret;
>> + }
>> +
>> + if (dma) {
>> + dev_dbg(gi2c->se.dev, "i2c dma tx:0x%x, dma rx:0x%x\n",
>> + dm_tx_st, dm_rx_st);
>> + goto irqret;
>> + }
>> +
>> + if (cur->flags & I2C_M_RD &&
>> + m_stat & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) {
>> + u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
>> +
>> + for (j = 0; j < rxcnt; j++) {
>> + u32 val;
>> + int p = 0;
>> +
>> + val = readl_relaxed(gi2c->se.base + SE_GENI_RX_FIFOn);
>> + while (gi2c->cur_rd < cur->len && p < sizeof(val)) {
>> + cur->buf[gi2c->cur_rd++] = val & 0xff;
>> + val >>= 8;
>> + p++;
>> + }
>> + if (gi2c->cur_rd == cur->len)
>> + break;
>> + }
>> + } else if (!(cur->flags & I2C_M_RD) &&
>> + m_stat & M_TX_FIFO_WATERMARK_EN) {
>> + for (j = 0; j < gi2c->tx_wm; j++) {
>> + u32 temp;
>> + u32 val = 0;
>> + int p = 0;
>> +
>> + while (gi2c->cur_wr < cur->len && p < sizeof(val)) {
>> + temp = (u32)cur->buf[gi2c->cur_wr++];
>
> What is the (u32) cast doing here?
The intention is to cast a char to u32. I will revisit its purpose and
also check if any warnings were observed.
>
>
>> + val |= (temp << (p * 8));
>
> Get rid of extra parenthesis.
Ok.
>
>
>> + p++;
>> + }
>> + writel_relaxed(val, gi2c->se.base + SE_GENI_TX_FIFOn);
>> + /* TX Complete, Disable the TX Watermark interrupt */
>> + if (gi2c->cur_wr == cur->len) {
>> + writel_relaxed(0, gi2c->se.base +
>> + SE_GENI_TX_WATERMARK_REG);
>> + break;
>> + }
>> + }
>> + }
>> +irqret:
>> + if (m_stat)
>> + writel_relaxed(m_stat, gi2c->se.base + SE_GENI_M_IRQ_CLEAR);
>> +
>> + if (dma) {
>> + if (dm_tx_st)
>> + writel_relaxed(dm_tx_st, gi2c->se.base +
>> + SE_DMA_TX_IRQ_CLR);
>> + if (dm_rx_st)
>> + writel_relaxed(dm_rx_st, gi2c->se.base +
>> + SE_DMA_RX_IRQ_CLR);
>> + }
>> + /* if this is err with done-bit not set, handle that through timeout. */
>> + if (m_stat & M_CMD_DONE_EN)
>> + complete(&gi2c->done);
>> + else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE))
>> + complete(&gi2c->done);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + unsigned long timeout = HZ;
>
> Rename to time_left? ...and maybe use a #define for the init value?
Ok.
>
>
>> +
>> + geni_i2c_err(gi2c, GENI_TIMEOUT);
>> + gi2c->cur = NULL;
>
> Don't you need a spinlock or something? In most of the other cases
> you could get away with no locking because the irq isn't happening at
> the same time as other code that's mucking with stuff, but in the
> timeout case we may be mucking with stuff at the same time as the irq.
Except for aborting the current command, there is no yanking away of
buffer used by IRQ. But I will check the programming manual regarding
what will happen to the contents of the RX FIFO & access to the TX FIFO
when the command gets aborted.
>
>
>> + geni_se_abort_m_cmd(&gi2c->se);
>> + do {
>> + timeout = wait_for_completion_timeout(&gi2c->done, timeout);
>> + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
>> + } while (!(val & M_CMD_ABORT_EN) && timeout);
>
> Print an error if there was a timeout aborting?
Ok.
>
>
>> +}
>> +
>> +static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> + u32 m_param)
>> +{
>> + dma_addr_t rx_dma;
>> + enum geni_se_xfer_mode mode;
>> + unsigned long timeout;
>> +
>> + gi2c->cur = msg;
>> + mode = msg->len > 32 ? GENI_SE_DMA : GENI_SE_FIFO;
>
> DMA is hard and i2c transfers > 32 bytes are rare. Do we really gain
> a lot by transferring i2c commands over DMA compared to a FIFO?
> Enough to justify the code complexity and the set of bugs that will
> show up? I'm sure it will be a controversial assertion given that the
> code's already written, but personally I'd be supportive of ripping
> DMA mode out to simplify the driver. I'd be curious if anyone else
> agrees. To me it seems like premature optimization.
>
>
>> + geni_se_select_mode(&gi2c->se, mode);
>> + writel_relaxed(msg->len, gi2c->se.base + SE_I2C_RX_TRANS_LEN);
>> + geni_se_setup_m_cmd(&gi2c->se, I2C_READ, m_param);
>> + if (mode == GENI_SE_DMA) {
>> + rx_dma = geni_se_rx_dma_prep(&gi2c->se, msg->buf, msg->len);
>
> Randomly I noticed a flag called "I2C_M_DMA_SAFE". Do we need to
> check this flag before using msg->buf for DMA? ...or use
> i2c_get_dma_safe_msg_buf()?
>
> ...btw: the relative lack of people doing this in the kernel is
> further evidence of DMA not really being worth it for i2c busses.
>
>
>> + if (!rx_dma) {
>> + mode = GENI_SE_FIFO;
>> + geni_se_select_mode(&gi2c->se, mode);
>> + }
>> + }
>> +
>> + timeout = wait_for_completion_timeout(&gi2c->done, HZ);
>
> Perhaps make a #define for the timeout instead of just hardcoding HZ (1 second).
>
>
>> + if (!timeout)
>
> Can you rename "timeout" to "time_left"? Otherwise this read like "if
> there wasn't a timeout then abort".
>
>
>> + geni_i2c_abort_xfer(gi2c);
>> +
>> + gi2c->cur_rd = 0;
>> + if (mode == GENI_SE_DMA) {
>> + if (gi2c->err) {
>> + writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST);
>> + wait_for_completion_timeout(&gi2c->done, HZ);
>
> Worth printing an error if this one times out? Seems like we'd be in
> bad shape...
Ok.
>
> ...also: to be paranoid do you need a re_init_completion before you
> reset things? In theory one could conceive of the concept that the
> earlier completion timed out and then the DMA interrupt came right
> after. Now there will be a completion already on the books so your
> wait will return instantly even though the reset hasn't been done.
>
reinit_completion can help only if the prior DMA interrupt came before
reinit_completion. If it comes after reinit_completion, then it will end
up signaling the wait prematurely.
Rather the better idea is to wait and check if indeed the reset is done.
If it is done, then all is fine. Else go back to wait again. Same logic
like in the case of abort.
>
>> + }
>> + geni_se_rx_dma_unprep(&gi2c->se, rx_dma, msg->len);
>> + }
>> + if (gi2c->err)
>> + dev_err(gi2c->se.dev, "i2c error :%d\n", gi2c->err);
>
> OK, so I'm a bit baffled. You've got all these tables in this driver
> that give you nice/informative error messages. Then those nice error
> messages are just calling dev_dbg() and here you print out an arcane
> linux error?
Agree, I will try to log a human-readable error while avoiding the log spew.
>
> Also: seems like you wouldn't want to print errors for NACKs, right?
> Otherwise i2cdetect is going to be spewing isn't it?
>
>
>> + return gi2c->err;
>> +}
>> +
>> +static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> + u32 m_param)
>> +{
>> + dma_addr_t tx_dma;
>> + enum geni_se_xfer_mode mode;
>> + unsigned long timeout;
>> +
>> + gi2c->cur = msg;
>> + mode = msg->len > 32 ? GENI_SE_DMA : GENI_SE_FIFO;
>> + geni_se_select_mode(&gi2c->se, mode);
>> + writel_relaxed(msg->len, gi2c->se.base + SE_I2C_TX_TRANS_LEN);
>> + geni_se_setup_m_cmd(&gi2c->se, I2C_WRITE, m_param);
>> + if (mode == GENI_SE_DMA) {
>> + tx_dma = geni_se_tx_dma_prep(&gi2c->se, msg->buf, msg->len);
>> + if (!tx_dma) {
>> + mode = GENI_SE_FIFO;
>> + geni_se_select_mode(&gi2c->se, mode);
>> + }
>> + }
>> +
>> + if (mode == GENI_SE_FIFO) /* Get FIFO IRQ */
>> + writel_relaxed(1, gi2c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +
>> + timeout = wait_for_completion_timeout(&gi2c->done, HZ);
>> + if (!timeout)
>> + geni_i2c_abort_xfer(gi2c);
>> +
>> + gi2c->cur_wr = 0;
>> + if (mode == GENI_SE_DMA) {
>> + if (gi2c->err) {
>> + writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST);
>> + wait_for_completion_timeout(&gi2c->done, HZ);
>> + }
>> + geni_se_tx_dma_unprep(&gi2c->se, tx_dma, msg->len);
>> + }
>> + if (gi2c->err)
>> + dev_err(gi2c->se.dev, "i2c error :%d\n", gi2c->err);
>> + return gi2c->err;
>> +}
>> +
>> +static int geni_i2c_xfer(struct i2c_adapter *adap,
>> + struct i2c_msg msgs[],
>> + int num)
>> +{
>> + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
>> + int i, ret;
>> +
>> + gi2c->err = 0;
>> + reinit_completion(&gi2c->done);
>> + ret = pm_runtime_get_sync(gi2c->se.dev);
>> + if (ret < 0) {
>> + dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret);
>> + pm_runtime_put_noidle(gi2c->se.dev);
>> + /* Set device in suspended since resume failed */
>> + pm_runtime_set_suspended(gi2c->se.dev);
>> + return ret;
>
> Wow, that's a cluster of arcane calls to handle a call that probably
> will never fail (it just enables clocks and sets pinctrl). Sigh.
> ...but as far as I can tell the whole sequence is right. You
> definitely need a "put" after a failed get and it looks like
> pm_runtime_set_suspended() has a special exception where it can be
> called if you got a runtime error...
>
>
>> + }
>> +
>> + qcom_geni_i2c_conf(gi2c);
>> + for (i = 0; i < num; i++) {
>> + u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
>> +
>> + m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK);
>> +
>> + if (msgs[i].flags & I2C_M_RD)
>> + ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param);
>> + else
>> + ret = geni_i2c_tx_one_msg(gi2c, &msgs[i], m_param);
>> +
>> + if (ret) {
>> + dev_err(gi2c->se.dev, "i2c error %d @ %d\n", ret, i);
>> + break;
>> + }
>> + }
>> + if (ret == 0)
>> + ret = num;
>> +
>> + pm_runtime_mark_last_busy(gi2c->se.dev);
>> + pm_runtime_put_autosuspend(gi2c->se.dev);
>> + gi2c->cur = NULL;
>> + gi2c->err = 0;
>> + return ret;
>> +}
>> +
>> +static u32 geni_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
>> +}
>> +
>> +static const struct i2c_algorithm geni_i2c_algo = {
>> + .master_xfer = geni_i2c_xfer,
>> + .functionality = geni_i2c_func,
>> +};
>> +
>> +static int geni_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct geni_i2c_dev *gi2c;
>> + struct resource *res;
>> + u32 proto, tx_depth;
>> + int ret;
>> +
>> + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL);
>> + if (!gi2c)
>> + return -ENOMEM;
>> +
>> + gi2c->se.dev = &pdev->dev;
>> + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(gi2c->se.base)) {
>> + ret = PTR_ERR(gi2c->se.base);
>> + dev_err(&pdev->dev, "Err IO Mapping register block %d\n", ret);
>
> No need for error message with devm_ioremap_resource().
Ok.
>
>
>> + return ret;
>> + }
>> +
>> + gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
>> + if (IS_ERR(gi2c->se.clk)) {
>> + ret = PTR_ERR(gi2c->se.clk);
>> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>> + &gi2c->clk_freq_out);
>> + if (ret) {
>> + dev_info(&pdev->dev,
>> + "Bus frequency not specified, default to 400KHz.\n");
>> + gi2c->clk_freq_out = KHz(400);
>> + }
>
> I feel like it should default to 100KHz. i2c_parse_fw_timings()
> defaults to this and to me the wording "New drivers almost always
> should use the defaults" makes me feel this should be the defaults.
>
>> +
>> + gi2c->irq = platform_get_irq(pdev, 0);
>> + if (gi2c->irq < 0) {
>> + dev_err(&pdev->dev, "IRQ error for i2c-geni\n");
>> + return gi2c->irq;
>> + }
>> +
>> + ret = geni_i2c_clk_map_idx(gi2c);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Invalid clk frequency %d KHz: %d\n",
>> + gi2c->clk_freq_out, ret);
>
> Need a divide by 1000 since your printout includes "KHz". Also note
> that the proper Si units is kHz not KHz, isn't it?
I will remove the KHz and just log as Hz.
>
>
>> + return ret;
>> + }
>> +
>> + gi2c->adap.algo = &geni_i2c_algo;
>> + init_completion(&gi2c->done);
>> + platform_set_drvdata(pdev, gi2c);
>> + ret = devm_request_irq(&pdev->dev, gi2c->irq, geni_i2c_irq,
>> + IRQF_TRIGGER_HIGH, "i2c_geni", gi2c);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Request_irq failed:%d: err:%d\n",
>> + gi2c->irq, ret);
>> + return ret;
>> + }
>> + disable_irq(gi2c->irq);
>
> Can you explain the goal of the disable_irq() here. Is it actually
> needed for something or does it somehow save power? From drivers I've
> reviewed in the past this doesn't seem like a common thing to do, so
> I'm curious what it's supposed to gain for you. I'd be inclined to
> just delete the whole disable/enable of the irq from this driver.
>
>
>> + i2c_set_adapdata(&gi2c->adap, gi2c);
>> + gi2c->adap.dev.parent = &pdev->dev;
>> + gi2c->adap.dev.of_node = pdev->dev.of_node;
>> + strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
>> +
>> + ret = geni_se_resources_on(&gi2c->se);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
>> + return ret;
>> + }
>> + proto = geni_se_read_proto(&gi2c->se);
>> + tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>> + if (unlikely(proto != GENI_SE_I2C)) {
>
> Avoid compiler hints like unlikely() unless you're really truly
> optimizing a tight inner loop. Otherwise let the compiler do its job.
Ok.
>
>
>> + dev_err(&pdev->dev, "Invalid proto %d\n", proto);
>> + geni_se_resources_off(&gi2c->se);
>> + return -ENXIO;
>> + }
>> + gi2c->tx_wm = tx_depth - 1;
>> + geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
>> + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, PACKING_BYTES_PW,
>> + true, true, true);
>> + geni_se_resources_off(&gi2c->se);
>> + dev_dbg(&pdev->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>> +
>> + pm_runtime_set_suspended(gi2c->se.dev);
>> + pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
>> + pm_runtime_use_autosuspend(gi2c->se.dev);
>> + pm_runtime_enable(gi2c->se.dev);
>> + i2c_add_adapter(&gi2c->adap);
>> +
>> + dev_dbg(&pdev->dev, "I2C probed\n");
>
> Is this really a useful dev_dbg()? Just turn on initcall debugging...
Can be removed.
>
>
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_remove(struct platform_device *pdev)
>> +{
>> + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_disable(gi2c->se.dev);
>
> Is this right? You don't want to disable PM Runtime transitions but
> you actually want to force the adapter into suspended state, don't
> you? I don't see anything that does that...
>
>
>> + i2c_del_adapter(&gi2c->adap);
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_resume_noirq(struct device *device)
>> +{
>> + return 0;
>> +}
>
> No need for a dummy function; just stick NULL in the structure, no?
>
>> +
>> +#ifdef CONFIG_PM
>> +static int geni_i2c_runtime_suspend(struct device *dev)
>> +{
>> + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>> +
>> + disable_irq(gi2c->irq);
>> + geni_se_resources_off(&gi2c->se);
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_runtime_resume(struct device *dev)
>> +{
>> + int ret;
>> + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>> +
>> + ret = geni_se_resources_on(&gi2c->se);
>> + if (ret)
>> + return ret;
>> +
>> + enable_irq(gi2c->irq);
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_suspend_noirq(struct device *device)
>> +{
>> + struct geni_i2c_dev *gi2c = dev_get_drvdata(device);
>> + int ret;
>> +
>> + /* Make sure no transactions are pending */
>> + ret = i2c_trylock_bus(&gi2c->adap, I2C_LOCK_SEGMENT);
>> + if (!ret) {
>> + dev_err(gi2c->se.dev, "late I2C transaction request\n");
>> + return -EBUSY;
>> + }
>
> Does this happen? How?
>
> Nothing about this code looks special for your hardware. If this is
> really needed, why is it not part of the i2c core since there's
> nothing specific about your driver here?
>
>
>> + if (!pm_runtime_status_suspended(device)) {
>> + geni_i2c_runtime_suspend(device);
>> + pm_runtime_disable(device);
>> + pm_runtime_set_suspended(device);
>> + pm_runtime_enable(device);
>> + }
>
> Similar question. Why do you need this special case code? Are there
> cases where we're all the way at suspend_noirq and yet we still
> haven't runtime suspended? Can you please document how we get into
> this state?
>
>
>> + i2c_unlock_bus(&gi2c->adap, I2C_LOCK_SEGMENT);
>> + return 0;
>> +}
>> +#else
>> +static int geni_i2c_runtime_suspend(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_runtime_resume(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int geni_i2c_suspend_noirq(struct device *device)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops geni_i2c_pm_ops = {
>> + .suspend_noirq = geni_i2c_suspend_noirq,
>> + .resume_noirq = geni_i2c_resume_noirq,
>> + .runtime_suspend = geni_i2c_runtime_suspend,
>> + .runtime_resume = geni_i2c_runtime_resume,
>
> Please use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS and SET_RUNTIME_PM_OPS. Then
> you can get rid of all the dummy functions. AKA something like:
>
> static const struct dev_pm_ops geni_i2c_pm_ops = {
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(geni_i2c_suspend_noirq, NULL)
> SET_RUNTIME_PM_OPS(geni_i2c_runtime_suspend, geni_i2c_runtime_resume, NULL)
> };
>
Ok.
>
>> +};
>> +
>> +static const struct of_device_id geni_i2c_dt_match[] = {
>> + { .compatible = "qcom,geni-i2c" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
>> +
>> +static struct platform_driver geni_i2c_driver = {
>> + .probe = geni_i2c_probe,
>> + .remove = geni_i2c_remove,
>> + .driver = {
>> + .name = "geni_i2c",
>> + .pm = &geni_i2c_pm_ops,
>> + .of_match_table = geni_i2c_dt_match,
>> + },
>> +};
>> +
>> +module_platform_driver(geni_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("I2C Controller Driver for GENI based QUP cores");
>> +MODULE_LICENSE("GPL v2");
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Doug Anderson @ 2018-03-09 5:02 UTC (permalink / raw)
To: Sagar Dharia
Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
Mark Rutland, Wolfram Sang, Greg Kroah-Hartman,
Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, Jiri Slaby, evgreen, acourbot,
Girish Mahadevan, swboyd
In-Reply-To: <65ae29b5-83da-22aa-f483-c3a25802239f@codeaurora.org>
Hi,
On Thu, Mar 8, 2018 at 5:06 PM, Sagar Dharia <sdharia@codeaurora.org> wrote:
> Hi Doug
>
>
> On 3/8/2018 2:12 PM, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Mar 7, 2018 at 9:19 PM, Doug Anderson <dianders@chromium.org>
>> wrote:
>>>>>
>>>>> DMA is hard and i2c transfers > 32 bytes are rare. Do we really gain
>>>>> a lot by transferring i2c commands over DMA compared to a FIFO?
>>>>> Enough to justify the code complexity and the set of bugs that will
>>>>> show up? I'm sure it will be a controversial assertion given that the
>>>>> code's already written, but personally I'd be supportive of ripping
>>>>> DMA mode out to simplify the driver. I'd be curious if anyone else
>>>>> agrees. To me it seems like premature optimization.
>>>>
>>>>
>>>>
>>>> Yes, we have multiple clients (e.g. touch, NFC) using I2C for data
>>>> transfers
>>>> bigger than 32 bytes (some transfers are 100s of bytes). The fifo size
>>>> is
>>>> 32, so we can definitely avoid at least 1 interrupt when DMA mode is
>>>> used
>>>> with data size > 32.
>>>
>>>
>>> Does that 1-2 interrupts make any real difference, though? In theory
>>> it really shouldn't affect the transfer rate. We should be able to
>>> service the interrupt plenty fast and if we were concerned we would
>>> tweak the watermark code a little bit. So I guess we're worried about
>>> the extra CPU cycles (and power cost) to service those extra couple
>>> interrupts?
>>>
>>> In theory when touch data is coming in or NFC data is coming in then
>>> we're probably not in a super low power state to begin with. If it's
>>> touch data we likely want to have the CPU boosted a bunch to respond
>>> to the user quickly. If we've got 8 cores available all of which can
>>> run at 1GHz or more a few interrupts won't kill us. NFC data is
>>> probably not common enough that we need to optimize power/CPU
>>> utilizatoin for that.
>>>
>>>
>>> So while i can believe that you do save an interrupt or two, I still
>>> am not convinced that those interrupts are worth a bunch of complex
>>> code (and a whole second code path) to save.
>>>
>>>
>>> ...also note that above you said that coming out of runtime suspend
>>> can take several msec. That seems like it dwarfs any slight
>>> difference in timing between a FIFO-based operation and DMA.
>>
>>
>> One last note here (sorry for not thinking of this last night) is that
>> I would also be interested in considering _only_ supporting the DMA
>> path. Is it somehow intrinsically bad to use the DMA flow for a
>> 1-byte transfer? Is there a bunch of extra overhead or power draw?
>>
>> Specifically my main point is that maintaining two separate flows (the
>> FIFO flow vs the DMA flow) adds complexity, code size, and bugs. If
>> there's a really good reason to maintain both flows then fine, but we
>> should really consider if this is something that's really giving us
>> value before we agree to it.
>>
>
> FIFO mode gives us 2 advantages:
> 1. small transfers don't have to go through 'dma-map/unmap penalties.
> Some small buffers come from the stack of client caller and the
> dma-map/unmap may fail.
> 2. bring-ups are 'less eventful' (with temp. change to just not have DMA
> mode at all during bring-ups) since SMMU translation/DMA path from QUP
> (master) to memory slave may not always available when critical I2C
> peripherals need to be brought up (e.g. PMIC). CPU to QUP (slave) path
> is usually available.
>
> On the other side, DMA mode has other advantages:
> 1. Multiple android clients are still heavily using I2C in spite of
> faster peripheral buses being available in industry.
> As an example, some multi-finger Touch screens use I2C and the data to
> be transferred per transaction over the bus grows well beyond 70-100
> bytes based on number of fingers. These transactions are very frequent
> when touch is being used, and in an environment where other heavy system
> users are also running (MM/graphics).
> Another example is, NFC uses I2C (as of now) to transfer as much as 700+
> bytes. This can save us 20+ interrupts per transfer.
>
> These transfers are mostly in burst. So the RPMh penalty to resume the
> shared resources is only experienced for very first transfer. Remaining
> transfers in the burst benefit from DMA if they are too big.
>
> Goal here is to have common driver for upstream targets and android and
> android has seen proven advantages with both modes.
> If we end up keeping DMA only for downstream (or FIFO only for
> downstream), then we lose the advantage of having code in upstream since
> we have to maintain downstream patch with other mode.
OK, fair enough. Having DMA mode alone would be a pain at bringup if
nothing else. You're right.
I would still argue that perhaps those extra interrupts for FIFO mode
aren't quite as bit of a deal as you're making it out to be. I've
been on systems that get massive number of interrupts almost
constantly and really it wasn't noticeable.
In any case, I didn't think I'd really convince anyone with this one,
so unless someone out there who matters actually feels the same way as
me then feel free to just ignore this and keep supporting both DMA and
FIFO mode.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Karthik Ramasubramanian @ 2018-03-09 4:57 UTC (permalink / raw)
To: Stephen Boyd, andy.gross, corbet, david.brown, gregkh,
mark.rutland, robh+dt, wsa
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Girish Mahadevan, Sagar Dharia,
Doug Anderson
In-Reply-To: <152054835314.219802.7795128053459733073@swboyd.mtv.corp.google.com>
On 3/8/2018 3:32 PM, Stephen Boyd wrote:
> Quoting Karthik Ramasubramanian (2018-03-07 22:06:29)
>>
>>
>> On 3/6/2018 2:45 PM, Stephen Boyd wrote:
>>> Quoting Karthik Ramasubramanian (2018-03-05 16:51:23)
>>>> On 3/2/2018 3:11 PM, Stephen Boyd wrote:
>>>
>>> Ok. I've seen similar designs in some mmc drivers. For example, you can
>>> look at drivers/mmc/host/meson-gx-mmc.c and see that they register a
>>> clk_ops and then just start using that clk directly from the driver.
>>> There are also some helper functions for dividers that would hopefully
>>> make the job of calculating the best divider easier.
>> Thanks for the pointers. I will take a look at it. In the meanwhile I
>> had discussions with our clock team. They pointed out that the register
>> to write the divider value here is outside the scope of clock controller
>> which makes it trickier to implement your suggestion. They are already
>> in the mailing list and we will discuss further and get back to you in
>> this regard.
>
> Ok. Let me know if I can help answer any questions.
Ok.
>
>>>>>
>>>>> Why are these noirq variants? Please add a comment.
>>>> The intention is to enable the console UART port usage as late as
>>>> possible in the suspend cycle. Hence noirq variants. I will add this as
>>>> a comment. Please let me know if the usage does not match the intention.
>>>
>>> Hm.. Does no_console_suspend not work? Or not work well enough?
>> It works. When console suspend is disabled, the suspend operation does
>> not get triggered and the resume operation checks if the console suspend
>> is disabled and performs the needed thing.
>
> Ok so then do we need the noirq variants? Or console suspend is special
> enough for this to not matter?
I am a little confused as to whether you prefer the console to not
suspend at all or you prefer the console suspend at an earlier stage
than no_irq stage.
If it is former, then with the console_suspend_enabled flag set by
default this seems the right thing to do. Atleast my understanding is
that console is expecting the serial port to suspend as well.
If it is latter, then I will check the stage at which suspend_console()
is initiated and can suspend the serial port after that.
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Sagar Dharia @ 2018-03-09 1:27 UTC (permalink / raw)
To: Doug Anderson
Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
Mark Rutland, Wolfram Sang, Greg Kroah-Hartman,
Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, Jiri Slaby, evgreen, acourbot,
Girish Mahadevan, swboyd, harryy, adharmap
In-Reply-To: <CAD=FV=UXK_wYky83nmQdk4dQtVSCV9=o3i2590ZyUMcq4rrdKQ@mail.gmail.com>
Hi Doug,
On 3/7/2018 10:19 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 7, 2018 at 6:42 PM, Sagar Dharia <sdharia@codeaurora.org> wrote:
>> Hi Doug,
>> Thank you for reviewing the patch. I will take a stab at a few comments
>> below. We will address most of the other comments in next version of I2C
>> patch.
>>>
>>>> +
>>>> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
>>>> + {KHz(100), 7, 10, 11, 26},
>>>> + {KHz(400), 2, 5, 12, 24},
>>>> + {KHz(1000), 1, 3, 9, 18},
>>>
>>>
>>> So I guess this is all relying on an input serial clock of 19.2MHz?
>>> Maybe document that?
>>>
>>> Assuming I'm understanding the math here, is it really OK for your
>>> 100kHz and 1MHz mode to be running slightly fast?
>>>
>>> 19200. / 2 / 24
>>>>>>
>>>>>> 400.0
>>>
>>>
>>> 19200. / 7 / 26
>>>>>>
>>>>>> 105.49450549450549
>>>
>>>
>>> 19200. / 1 / 18
>>>>>>
>>>>>> 1066.6666666666667
>>>
>>>
>>> It seems like you'd want the fastest clock that you can make that's
>>> _less than_ the spec.
>>>
>>>
>>> It would also be interesting to know if it's expected that boards
>>> might need to tweak the t_high / t_low depending on their electrical
>>> characteristics. In the past I've had lots of requests from board
>>> makers to tweak things because they've got a long trace, or a stronger
>>> or weaker pull, or ... If so we might later need to add some dts
>>> properties like "i2c-scl-rising-time-ns" and make the math more
>>> dynamic here, unless your hardware somehow automatically adjusts for
>>> this type of thing...
>>> These values are derived by our HW team to comply with the t-high and
>>
>> t-low specs of I2C. We have confirmed on scope that the frequency of SCL is
>> indeed less than/equal to the spec. We have not come across slaves who have
>> needed to tweak these things. We are open to adding these properties in dts
>> if you have any such slaves not conforming due to board-layout of other
>> reasons.
>
> OK, I'm fine with leaving something like this till later if/when it
> comes up. Documenting a little bit more about how these timings work
> seems like it would be nice, though, at least mentioning what the
> source clock is...
>
Yes, we will document how t-high and t-low is derived from source.
>>> Wow, that's a cluster of arcane calls to handle a call that probably
>>> will never fail (it just enables clocks and sets pinctrl). Sigh.
>>> ...but as far as I can tell the whole sequence is right. You
>>> definitely need a "put" after a failed get and it looks like
>>> pm_runtime_set_suspended() has a special exception where it can be
>>> called if you got a runtime error...
>>
>> We didn't have this in before either. But this condition is somewhat
>> frequent if I2C transactions are called on cusp of exiting system suspend.
>> (e.g. PMIC slave getting a wakeup-IRQ and trying to read from PMIC through
>> I2C to read its status as to what caused that wake-up. At that time,
>> get_sync doesn't really enable resources (kernel 4.9) since the runtime-pm
>> ref-count isn't decremented. We run the risk of unclocked access if these
>> arcane calls aren't present. You can go through runtime-pm documentation
>> chapter 6 for more details.
>
> Yeah, I certainly agree that the calls are needed if
> pm_runtime_get_sync() and I'm not suggesting removing them. Hence the
> "as far as I can tell the whole sequence is right".
>
> ...but I'm actually kinda worried if you're saying that you actually
> ran into this case. Hopefully that got fixed and code no longer tries
> to read from the PMIC at a bad time anymore? That code should be
> fixed not to be running so late in suspend.
>
I have added Harry Y and Abhijeet D (developers for PMIC I2C client
team). They can comment if there is still a usecase of very late
transaction during suspend and/or very early transaction during resume.
>
>>>>
>>>> + /* Make sure no transactions are pending */
>>>> + ret = i2c_trylock_bus(&gi2c->adap, I2C_LOCK_SEGMENT);
>>>> + if (!ret) {
>>>> + dev_err(gi2c->se.dev, "late I2C transaction request\n");
>>>> + return -EBUSY;
>>>> + }
>>>
>>>
>>> Does this happen? How?
>>>
>>> Nothing about this code looks special for your hardware. If this is
>>> really needed, why is it not part of the i2c core since there's
>>> nothing specific about your driver here?
>>>
>> There have been some clients that don't implement sys-suspend/resume
>> callbacks (so i2c adapter has no clue they are done with their transactions)
>> and this allows us to be flexible when they call I2C transactions extremely
>> late.
>
> Still feels like this belongs in the i2c core, not your driver. Maybe
> you should send a patch for the core and remove it from here?
>
> ...and also, it seems like any i2c clients that don't implement the
> suspend/resume callbacks and try to do i2c transactions late in the
> game need to be fixed. It should be documented that this isn't a
> valid thing for a driver to do and if we end up in this error case
> then it's not an i2c issue but it's a bad driver somewhere.
>
You are right: this check is special for our HW due to usecases
mentioned above.
This check can go in i2c-core, but then it will not be necessary if
all adapters and clients that we work with are upstreamed (and all
implement system suspend/resume).
We will remove this in next version of geni i2c-adapter driver.
Thanks
Sagar
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v3, 3/4] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Sagar Dharia @ 2018-03-09 1:06 UTC (permalink / raw)
To: Doug Anderson
Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
Mark Rutland, Wolfram Sang, Greg Kroah-Hartman,
Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, Jiri Slaby, evgreen, acourbot,
Girish Mahadevan, swboyd
In-Reply-To: <CAD=FV=ULH+9aW_3m=WB3Fa4mBg3sR-S3AFY7XOKdZCDqkJdq1Q@mail.gmail.com>
Hi Doug
On 3/8/2018 2:12 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 7, 2018 at 9:19 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>> DMA is hard and i2c transfers > 32 bytes are rare. Do we really gain
>>>> a lot by transferring i2c commands over DMA compared to a FIFO?
>>>> Enough to justify the code complexity and the set of bugs that will
>>>> show up? I'm sure it will be a controversial assertion given that the
>>>> code's already written, but personally I'd be supportive of ripping
>>>> DMA mode out to simplify the driver. I'd be curious if anyone else
>>>> agrees. To me it seems like premature optimization.
>>>
>>>
>>> Yes, we have multiple clients (e.g. touch, NFC) using I2C for data transfers
>>> bigger than 32 bytes (some transfers are 100s of bytes). The fifo size is
>>> 32, so we can definitely avoid at least 1 interrupt when DMA mode is used
>>> with data size > 32.
>>
>> Does that 1-2 interrupts make any real difference, though? In theory
>> it really shouldn't affect the transfer rate. We should be able to
>> service the interrupt plenty fast and if we were concerned we would
>> tweak the watermark code a little bit. So I guess we're worried about
>> the extra CPU cycles (and power cost) to service those extra couple
>> interrupts?
>>
>> In theory when touch data is coming in or NFC data is coming in then
>> we're probably not in a super low power state to begin with. If it's
>> touch data we likely want to have the CPU boosted a bunch to respond
>> to the user quickly. If we've got 8 cores available all of which can
>> run at 1GHz or more a few interrupts won't kill us. NFC data is
>> probably not common enough that we need to optimize power/CPU
>> utilizatoin for that.
>>
>>
>> So while i can believe that you do save an interrupt or two, I still
>> am not convinced that those interrupts are worth a bunch of complex
>> code (and a whole second code path) to save.
>>
>>
>> ...also note that above you said that coming out of runtime suspend
>> can take several msec. That seems like it dwarfs any slight
>> difference in timing between a FIFO-based operation and DMA.
>
> One last note here (sorry for not thinking of this last night) is that
> I would also be interested in considering _only_ supporting the DMA
> path. Is it somehow intrinsically bad to use the DMA flow for a
> 1-byte transfer? Is there a bunch of extra overhead or power draw?
>
> Specifically my main point is that maintaining two separate flows (the
> FIFO flow vs the DMA flow) adds complexity, code size, and bugs. If
> there's a really good reason to maintain both flows then fine, but we
> should really consider if this is something that's really giving us
> value before we agree to it.
>
FIFO mode gives us 2 advantages:
1. small transfers don't have to go through 'dma-map/unmap penalties.
Some small buffers come from the stack of client caller and the
dma-map/unmap may fail.
2. bring-ups are 'less eventful' (with temp. change to just not have DMA
mode at all during bring-ups) since SMMU translation/DMA path from QUP
(master) to memory slave may not always available when critical I2C
peripherals need to be brought up (e.g. PMIC). CPU to QUP (slave) path
is usually available.
On the other side, DMA mode has other advantages:
1. Multiple android clients are still heavily using I2C in spite of
faster peripheral buses being available in industry.
As an example, some multi-finger Touch screens use I2C and the data to
be transferred per transaction over the bus grows well beyond 70-100
bytes based on number of fingers. These transactions are very frequent
when touch is being used, and in an environment where other heavy system
users are also running (MM/graphics).
Another example is, NFC uses I2C (as of now) to transfer as much as 700+
bytes. This can save us 20+ interrupts per transfer.
These transfers are mostly in burst. So the RPMh penalty to resume the
shared resources is only experienced for very first transfer. Remaining
transfers in the burst benefit from DMA if they are too big.
Goal here is to have common driver for upstream targets and android and
android has seen proven advantages with both modes.
If we end up keeping DMA only for downstream (or FIFO only for
downstream), then we lose the advantage of having code in upstream since
we have to maintain downstream patch with other mode.
Thanks
Sagar
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 4/8] Documentation: gpio: Move driver documentation to driver-api
From: Jonathan Neuschäfer @ 2018-03-08 23:40 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, linux-doc, Jonathan Neuschäfer, Linus Walleij,
Jonathan Corbet
In-Reply-To: <20180308234024.24145-1-j.neuschaefer@gmx.net>
Move gpio/driver.txt to driver-api/gpio/driver.rst and make sure it
builds cleanly as ReST.
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
.../driver.txt => driver-api/gpio/driver.rst} | 80 +++++++++++-----------
Documentation/driver-api/gpio/index.rst | 1 +
Documentation/gpio/00-INDEX | 2 -
3 files changed, 42 insertions(+), 41 deletions(-)
rename Documentation/{gpio/driver.txt => driver-api/gpio/driver.rst} (93%)
diff --git a/Documentation/gpio/driver.txt b/Documentation/driver-api/gpio/driver.rst
similarity index 93%
rename from Documentation/gpio/driver.txt
rename to Documentation/driver-api/gpio/driver.rst
index 3392a0fd4c23..505ee906d7d9 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/driver-api/gpio/driver.rst
@@ -1,3 +1,4 @@
+================================
GPIO Descriptor Driver Interface
================================
@@ -53,9 +54,9 @@ common to each controller of that type:
The code implementing a gpio_chip should support multiple instances of the
controller, possibly using the driver model. That code will configure each
-gpio_chip and issue gpiochip_add[_data]() or devm_gpiochip_add_data().
-Removing a GPIO controller should be rare; use [devm_]gpiochip_remove() when
-it is unavoidable.
+gpio_chip and issue ``gpiochip_add[_data]()`` or ``devm_gpiochip_add_data()``.
+Removing a GPIO controller should be rare; use ``[devm_]gpiochip_remove()``
+when it is unavoidable.
Often a gpio_chip is part of an instance-specific structure with states not
exposed by the GPIO interfaces, such as addressing, power management, and more.
@@ -115,7 +116,7 @@ GPIOs with open drain/source support
Open drain (CMOS) or open collector (TTL) means the line is not actively driven
high: instead you provide the drain/collector as output, so when the transistor
-is not open, it will present a high-impedance (tristate) to the external rail.
+is not open, it will present a high-impedance (tristate) to the external rail::
CMOS CONFIGURATION TTL CONFIGURATION
@@ -148,19 +149,19 @@ level-shift to the higher VDD.
Integrated electronics often have an output driver stage in the form of a CMOS
"totem-pole" with one N-MOS and one P-MOS transistor where one of them drives
the line high and one of them drives the line low. This is called a push-pull
-output. The "totem-pole" looks like so:
-
- VDD
- |
- OD ||--+
- +--/ ---o|| P-MOS-FET
- | ||--+
-IN --+ +----- out
- | ||--+
- +--/ ----|| N-MOS-FET
- OS ||--+
- |
- GND
+output. The "totem-pole" looks like so::
+
+ VDD
+ |
+ OD ||--+
+ +--/ ---o|| P-MOS-FET
+ | ||--+
+ IN --+ +----- out
+ | ||--+
+ +--/ ----|| N-MOS-FET
+ OS ||--+
+ |
+ GND
The desired output signal (e.g. coming directly from some GPIO output register)
arrives at IN. The switches named "OD" and "OS" are normally closed, creating
@@ -219,8 +220,9 @@ systems simultaneously: gpio and irq.
RT_FULL: a realtime compliant GPIO driver should not use spinlock_t or any
sleepable APIs (like PM runtime) as part of its irq_chip implementation.
-- spinlock_t should be replaced with raw_spinlock_t [1].
-- If sleepable APIs have to be used, these can be done from the .irq_bus_lock()
+
+* spinlock_t should be replaced with raw_spinlock_t [1].
+* If sleepable APIs have to be used, these can be done from the .irq_bus_lock()
and .irq_bus_unlock() callbacks, as these are the only slowpath callbacks
on an irqchip. Create the callbacks if needed [2].
@@ -232,12 +234,12 @@ GPIO irqchips usually fall in one of two categories:
system interrupt controller. This means that the GPIO irqchip handler will
be called immediately from the parent irqchip, while holding the IRQs
disabled. The GPIO irqchip will then end up calling something like this
- sequence in its interrupt handler:
+ sequence in its interrupt handler::
- static irqreturn_t foo_gpio_irq(int irq, void *data)
- chained_irq_enter(...);
- generic_handle_irq(...);
- chained_irq_exit(...);
+ static irqreturn_t foo_gpio_irq(int irq, void *data)
+ chained_irq_enter(...);
+ generic_handle_irq(...);
+ chained_irq_exit(...);
Chained GPIO irqchips typically can NOT set the .can_sleep flag on
struct gpio_chip, as everything happens directly in the callbacks: no
@@ -252,7 +254,7 @@ GPIO irqchips usually fall in one of two categories:
(for example, see [3]).
Know W/A: The generic_handle_irq() is expected to be called with IRQ disabled,
so the IRQ core will complain if it is called from an IRQ handler which is
- forced to a thread. The "fake?" raw lock can be used to W/A this problem:
+ forced to a thread. The "fake?" raw lock can be used to W/A this problem::
raw_spinlock_t wa_lock;
static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
@@ -265,11 +267,11 @@ GPIO irqchips usually fall in one of two categories:
but chained IRQ handlers are not used. Instead GPIO IRQs dispatching is
performed by generic IRQ handler which is configured using request_irq().
The GPIO irqchip will then end up calling something like this sequence in
- its interrupt handler:
+ its interrupt handler::
- static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id)
- for each detected GPIO IRQ
- generic_handle_irq(...);
+ static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id)
+ for each detected GPIO IRQ
+ generic_handle_irq(...);
RT_FULL: Such kind of handlers will be forced threaded on -RT, as result IRQ
core will complain that generic_handle_irq() is called with IRQ enabled and
@@ -282,11 +284,11 @@ GPIO irqchips usually fall in one of two categories:
in a quick IRQ handler with IRQs disabled. Instead they need to spawn a
thread and then mask the parent IRQ line until the interrupt is handled
by the driver. The hallmark of this driver is to call something like
- this in its interrupt handler:
+ this in its interrupt handler::
- static irqreturn_t foo_gpio_irq(int irq, void *data)
- ...
- handle_nested_irq(irq);
+ static irqreturn_t foo_gpio_irq(int irq, void *data)
+ ...
+ handle_nested_irq(irq);
The hallmark of threaded GPIO irqchips is that they set the .can_sleep
flag on struct gpio_chip to true, indicating that this chip may sleep
@@ -359,12 +361,12 @@ below exists.
Locking IRQ usage
-----------------
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
-to mark the GPIO as being used as an IRQ:
+to mark the GPIO as being used as an IRQ::
int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
-is released:
+is released::
void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
@@ -408,7 +410,7 @@ Sometimes it is useful to allow a GPIO chip driver to request its own GPIO
descriptors through the gpiolib API. Using gpio_request() for this purpose
does not help since it pins the module to the kernel forever (it calls
try_module_get()). A GPIO driver can use the following functions instead
-to request and free descriptors without being pinned to the kernel forever.
+to request and free descriptors without being pinned to the kernel forever::
struct gpio_desc *gpiochip_request_own_desc(struct gpio_desc *desc,
const char *label)
@@ -422,6 +424,6 @@ These functions must be used with care since they do not affect module use
count. Do not use the functions to request gpio descriptors not owned by the
calling driver.
-[1] http://www.spinics.net/lists/linux-omap/msg120425.html
-[2] https://lkml.org/lkml/2015/9/25/494
-[3] https://lkml.org/lkml/2015/9/25/495
+* [1] http://www.spinics.net/lists/linux-omap/msg120425.html
+* [2] https://lkml.org/lkml/2015/9/25/494
+* [3] https://lkml.org/lkml/2015/9/25/495
diff --git a/Documentation/driver-api/gpio/index.rst b/Documentation/driver-api/gpio/index.rst
index db47d845f473..e1fbc5408cf6 100644
--- a/Documentation/driver-api/gpio/index.rst
+++ b/Documentation/driver-api/gpio/index.rst
@@ -8,6 +8,7 @@ Contents:
:maxdepth: 2
intro
+ driver
Core
====
diff --git a/Documentation/gpio/00-INDEX b/Documentation/gpio/00-INDEX
index 52fe0fa6c964..06c25fb7604c 100644
--- a/Documentation/gpio/00-INDEX
+++ b/Documentation/gpio/00-INDEX
@@ -2,8 +2,6 @@
- This file
consumer.txt
- How to obtain and use GPIOs in a driver
-driver.txt
- - How to write a GPIO driver
drivers-on-gpio.txt:
- Drivers in other subsystems that can use GPIO to provide more
complex functionality.
--
2.16.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 7/8] Documentation: gpio: Move GPIO mapping documentation to driver-api
From: Jonathan Neuschäfer @ 2018-03-08 23:40 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, linux-doc, Jonathan Neuschäfer, Linus Walleij,
Jonathan Corbet
In-Reply-To: <20180308234024.24145-1-j.neuschaefer@gmx.net>
Move gpio/board.txt to driver-api/gpio/board.rst and make sure it builds
cleanly as ReST.
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
.../{gpio/board.txt => driver-api/gpio/board.rst} | 39 ++++++++++++----------
Documentation/driver-api/gpio/index.rst | 1 +
Documentation/gpio/00-INDEX | 2 --
3 files changed, 22 insertions(+), 20 deletions(-)
rename Documentation/{gpio/board.txt => driver-api/gpio/board.rst} (88%)
diff --git a/Documentation/gpio/board.txt b/Documentation/driver-api/gpio/board.rst
similarity index 88%
rename from Documentation/gpio/board.txt
rename to Documentation/driver-api/gpio/board.rst
index 659bb19f5b3c..25d62b2e9fd0 100644
--- a/Documentation/gpio/board.txt
+++ b/Documentation/driver-api/gpio/board.rst
@@ -1,3 +1,4 @@
+=============
GPIO Mappings
=============
@@ -23,7 +24,7 @@ device tree bindings for your controller.
GPIOs mappings are defined in the consumer device's node, in a property named
<function>-gpios, where <function> is the function the driver will request
-through gpiod_get(). For example:
+through gpiod_get(). For example::
foo_device {
compatible = "acme,foo";
@@ -40,7 +41,7 @@ it but are only supported for compatibility reasons and should not be used for
newer bindings since it has been deprecated.
This property will make GPIOs 15, 16 and 17 available to the driver under the
-"led" function, and GPIO 1 as the "power" GPIO:
+"led" function, and GPIO 1 as the "power" GPIO::
struct gpio_desc *red, *green, *blue, *power;
@@ -60,13 +61,13 @@ looked up by the gpiod functions internally) used in the device tree. With above
Internally, the GPIO subsystem prefixes the GPIO suffix ("gpios" or "gpio")
with the string passed in con_id to get the resulting string
-(snprintf(... "%s-%s", con_id, gpio_suffixes[]).
+(``snprintf(... "%s-%s", con_id, gpio_suffixes[]``).
ACPI
----
ACPI also supports function names for GPIOs in a similar fashion to DT.
The above DT example can be converted to an equivalent ACPI description
-with the help of _DSD (Device Specific Data), introduced in ACPI 5.1:
+with the help of _DSD (Device Specific Data), introduced in ACPI 5.1::
Device (FOO) {
Name (_CRS, ResourceTemplate () {
@@ -105,12 +106,12 @@ Documentation/acpi/gpio-properties.txt.
Platform Data
-------------
Finally, GPIOs can be bound to devices and functions using platform data. Board
-files that desire to do so need to include the following header:
+files that desire to do so need to include the following header::
#include <linux/gpio/machine.h>
GPIOs are mapped by the means of tables of lookups, containing instances of the
-gpiod_lookup structure. Two macros are defined to help declaring such mappings:
+gpiod_lookup structure. Two macros are defined to help declaring such mappings::
GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags)
GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags)
@@ -141,22 +142,24 @@ end. The 'dev_id' field of the table is the identifier of the device that will
make use of these GPIOs. It can be NULL, in which case it will be matched for
calls to gpiod_get() with a NULL device.
-struct gpiod_lookup_table gpios_table = {
- .dev_id = "foo.0",
- .table = {
- GPIO_LOOKUP_IDX("gpio.0", 15, "led", 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 16, "led", 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 17, "led", 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP("gpio.0", 1, "power", GPIO_ACTIVE_LOW),
- { },
- },
-};
+.. code-block:: c
+
+ struct gpiod_lookup_table gpios_table = {
+ .dev_id = "foo.0",
+ .table = {
+ GPIO_LOOKUP_IDX("gpio.0", 15, "led", 0, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 16, "led", 1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 17, "led", 2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("gpio.0", 1, "power", GPIO_ACTIVE_LOW),
+ { },
+ },
+ };
-And the table can be added by the board code as follows:
+And the table can be added by the board code as follows::
gpiod_add_lookup_table(&gpios_table);
-The driver controlling "foo.0" will then be able to obtain its GPIOs as follows:
+The driver controlling "foo.0" will then be able to obtain its GPIOs as follows::
struct gpio_desc *red, *green, *blue, *power;
diff --git a/Documentation/driver-api/gpio/index.rst b/Documentation/driver-api/gpio/index.rst
index 6ba9e0043310..2b73ea5a1fbb 100644
--- a/Documentation/driver-api/gpio/index.rst
+++ b/Documentation/driver-api/gpio/index.rst
@@ -10,6 +10,7 @@ Contents:
intro
driver
consumer
+ board
legacy
Core
diff --git a/Documentation/gpio/00-INDEX b/Documentation/gpio/00-INDEX
index f960fc00a3ef..650cb0696211 100644
--- a/Documentation/gpio/00-INDEX
+++ b/Documentation/gpio/00-INDEX
@@ -3,7 +3,5 @@
drivers-on-gpio.txt:
- Drivers in other subsystems that can use GPIO to provide more
complex functionality.
-board.txt
- - How to assign GPIOs to a consumer device and a function
sysfs.txt
- Information about the GPIO sysfs interface
--
2.16.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 6/8] Documentation: gpio: Move gpiod_* consumer documentation to driver-api
From: Jonathan Neuschäfer @ 2018-03-08 23:40 UTC (permalink / raw)
To: linux-gpio
Cc: linux-kernel, linux-doc, Jonathan Neuschäfer, Linus Walleij,
Jonathan Corbet
In-Reply-To: <20180308234024.24145-1-j.neuschaefer@gmx.net>
Move gpio/consumer.txt to driver-api/gpio/consumer.rst and make sure it
builds cleanly as ReST.
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
.../consumer.txt => driver-api/gpio/consumer.rst} | 85 +++++++++++-----------
Documentation/driver-api/gpio/index.rst | 1 +
Documentation/gpio/00-INDEX | 2 -
3 files changed, 44 insertions(+), 44 deletions(-)
rename Documentation/{gpio/consumer.txt => driver-api/gpio/consumer.rst} (89%)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/driver-api/gpio/consumer.rst
similarity index 89%
rename from Documentation/gpio/consumer.txt
rename to Documentation/driver-api/gpio/consumer.rst
index d53e5b5cfc9c..c71a50d85b50 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/driver-api/gpio/consumer.rst
@@ -1,3 +1,4 @@
+==================================
GPIO Descriptor Consumer Interface
==================================
@@ -30,10 +31,10 @@ warnings. These stubs are used for two use cases:
be met with console warnings that may be perceived as intimidating.
All the functions that work with the descriptor-based GPIO interface are
-prefixed with gpiod_. The gpio_ prefix is used for the legacy interface. No
-other function in the kernel should use these prefixes. The use of the legacy
-functions is strongly discouraged, new code should use <linux/gpio/consumer.h>
-and descriptors exclusively.
+prefixed with ``gpiod_``. The ``gpio_`` prefix is used for the legacy
+interface. No other function in the kernel should use these prefixes. The use
+of the legacy functions is strongly discouraged, new code should use
+<linux/gpio/consumer.h> and descriptors exclusively.
Obtaining and Disposing GPIOs
@@ -43,13 +44,13 @@ With the descriptor-based interface, GPIOs are identified with an opaque,
non-forgeable handler that must be obtained through a call to one of the
gpiod_get() functions. Like many other kernel subsystems, gpiod_get() takes the
device that will use the GPIO and the function the requested GPIO is supposed to
-fulfill:
+fulfill::
struct gpio_desc *gpiod_get(struct device *dev, const char *con_id,
enum gpiod_flags flags)
If a function is implemented by using several GPIOs together (e.g. a simple LED
-device that displays digits), an additional index argument can be specified:
+device that displays digits), an additional index argument can be specified::
struct gpio_desc *gpiod_get_index(struct device *dev,
const char *con_id, unsigned int idx,
@@ -84,7 +85,7 @@ occurred while trying to acquire it. This is useful to discriminate between mere
errors and an absence of GPIO for optional GPIO parameters. For the common
pattern where a GPIO is optional, the gpiod_get_optional() and
gpiod_get_index_optional() functions can be used. These functions return NULL
-instead of -ENOENT if no GPIO has been assigned to the requested function:
+instead of -ENOENT if no GPIO has been assigned to the requested function::
struct gpio_desc *gpiod_get_optional(struct device *dev,
const char *con_id,
@@ -101,14 +102,14 @@ This is helpful to driver authors, since they do not need to special case
-ENOSYS return codes. System integrators should however be careful to enable
gpiolib on systems that need it.
-For a function using multiple GPIOs all of those can be obtained with one call:
+For a function using multiple GPIOs all of those can be obtained with one call::
struct gpio_descs *gpiod_get_array(struct device *dev,
const char *con_id,
enum gpiod_flags flags)
This function returns a struct gpio_descs which contains an array of
-descriptors:
+descriptors::
struct gpio_descs {
unsigned int ndescs;
@@ -116,13 +117,13 @@ descriptors:
}
The following function returns NULL instead of -ENOENT if no GPIOs have been
-assigned to the requested function:
+assigned to the requested function::
struct gpio_descs *gpiod_get_array_optional(struct device *dev,
const char *con_id,
enum gpiod_flags flags)
-Device-managed variants of these functions are also defined:
+Device-managed variants of these functions are also defined::
struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
enum gpiod_flags flags)
@@ -149,11 +150,11 @@ Device-managed variants of these functions are also defined:
const char *con_id,
enum gpiod_flags flags)
-A GPIO descriptor can be disposed of using the gpiod_put() function:
+A GPIO descriptor can be disposed of using the gpiod_put() function::
void gpiod_put(struct gpio_desc *desc)
-For an array of GPIOs this function can be used:
+For an array of GPIOs this function can be used::
void gpiod_put_array(struct gpio_descs *descs)
@@ -161,7 +162,7 @@ It is strictly forbidden to use a descriptor after calling these functions.
It is also not allowed to individually release descriptors (using gpiod_put())
from an array acquired with gpiod_get_array().
-The device-managed variants are, unsurprisingly:
+The device-managed variants are, unsurprisingly::
void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
@@ -175,7 +176,7 @@ Setting Direction
-----------------
The first thing a driver must do with a GPIO is setting its direction. If no
direction-setting flags have been given to gpiod_get*(), this is done by
-invoking one of the gpiod_direction_*() functions:
+invoking one of the gpiod_direction_*() functions::
int gpiod_direction_input(struct gpio_desc *desc)
int gpiod_direction_output(struct gpio_desc *desc, int value)
@@ -189,7 +190,7 @@ of early board setup.
For output GPIOs, the value provided becomes the initial output value. This
helps avoid signal glitching during system startup.
-A driver can also query the current direction of a GPIO:
+A driver can also query the current direction of a GPIO::
int gpiod_get_direction(const struct gpio_desc *desc)
@@ -206,7 +207,7 @@ Most GPIO controllers can be accessed with memory read/write instructions. Those
don't need to sleep, and can safely be done from inside hard (non-threaded) IRQ
handlers and similar contexts.
-Use the following calls to access GPIOs from an atomic context:
+Use the following calls to access GPIOs from an atomic context::
int gpiod_get_value(const struct gpio_desc *desc);
void gpiod_set_value(struct gpio_desc *desc, int value);
@@ -231,11 +232,11 @@ head of a queue to transmit a command and get its response. This requires
sleeping, which can't be done from inside IRQ handlers.
Platforms that support this type of GPIO distinguish them from other GPIOs by
-returning nonzero from this call:
+returning nonzero from this call::
int gpiod_cansleep(const struct gpio_desc *desc)
-To access such GPIOs, a different set of accessors is defined:
+To access such GPIOs, a different set of accessors is defined::
int gpiod_get_value_cansleep(const struct gpio_desc *desc)
void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
@@ -271,23 +272,23 @@ As an example, if the active low property for a dedicated GPIO is set, and the
gpiod_set_(array)_value_xxx() passes "asserted" ("1"), the physical line level
will be driven low.
-To summarize:
-
-Function (example) line property physical line
-gpiod_set_raw_value(desc, 0); don't care low
-gpiod_set_raw_value(desc, 1); don't care high
-gpiod_set_value(desc, 0); default (active high) low
-gpiod_set_value(desc, 1); default (active high) high
-gpiod_set_value(desc, 0); active low high
-gpiod_set_value(desc, 1); active low low
-gpiod_set_value(desc, 0); default (active high) low
-gpiod_set_value(desc, 1); default (active high) high
-gpiod_set_value(desc, 0); open drain low
-gpiod_set_value(desc, 1); open drain high impedance
-gpiod_set_value(desc, 0); open source high impedance
-gpiod_set_value(desc, 1); open source high
-
-It is possible to override these semantics using the *set_raw/'get_raw functions
+To summarize::
+
+ Function (example) line property physical line
+ gpiod_set_raw_value(desc, 0); don't care low
+ gpiod_set_raw_value(desc, 1); don't care high
+ gpiod_set_value(desc, 0); default (active high) low
+ gpiod_set_value(desc, 1); default (active high) high
+ gpiod_set_value(desc, 0); active low high
+ gpiod_set_value(desc, 1); active low low
+ gpiod_set_value(desc, 0); default (active high) low
+ gpiod_set_value(desc, 1); default (active high) high
+ gpiod_set_value(desc, 0); open drain low
+ gpiod_set_value(desc, 1); open drain high impedance
+ gpiod_set_value(desc, 0); open source high impedance
+ gpiod_set_value(desc, 1); open source high
+
+It is possible to override these semantics using the set_raw/get_raw functions
but it should be avoided as much as possible, especially by system-agnostic drivers
which should not need to care about the actual physical line level and worry about
the logical value instead.
@@ -300,7 +301,7 @@ their device will actually receive, no matter what lies between it and the GPIO
line.
The following set of calls ignore the active-low or open drain property of a GPIO and
-work on the raw line value:
+work on the raw line value::
int gpiod_get_raw_value(const struct gpio_desc *desc)
void gpiod_set_raw_value(struct gpio_desc *desc, int value)
@@ -308,7 +309,7 @@ work on the raw line value:
void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value)
int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
-The active low state of a GPIO can also be queried using the following call:
+The active low state of a GPIO can also be queried using the following call::
int gpiod_is_active_low(const struct gpio_desc *desc)
@@ -318,7 +319,7 @@ should not have to care about the physical line level or open drain semantics.
Access multiple GPIOs with a single function call
-------------------------------------------------
-The following functions get or set the values of an array of GPIOs:
+The following functions get or set the values of an array of GPIOs::
int gpiod_get_array_value(unsigned int array_size,
struct gpio_desc **desc_array,
@@ -361,7 +362,7 @@ The functions take three arguments:
The descriptor array can be obtained using the gpiod_get_array() function
or one of its variants. If the group of descriptors returned by that function
matches the desired group of GPIOs, those GPIOs can be accessed by simply using
-the struct gpio_descs returned by gpiod_get_array():
+the struct gpio_descs returned by gpiod_get_array()::
struct gpio_descs *my_gpio_descs = gpiod_get_array(...);
gpiod_set_array_value(my_gpio_descs->ndescs, my_gpio_descs->desc,
@@ -384,7 +385,7 @@ values are stored in value_array rather than passed back as return value.
GPIOs mapped to IRQs
--------------------
GPIO lines can quite often be used as IRQs. You can get the IRQ number
-corresponding to a given GPIO using the following call:
+corresponding to a given GPIO using the following call::
int gpiod_to_irq(const struct gpio_desc *desc)
@@ -424,7 +425,7 @@ Interacting With the Legacy GPIO Subsystem
Many kernel subsystems still handle GPIOs using the legacy integer-based
interface. Although it is strongly encouraged to upgrade them to the safer
descriptor-based API, the following two functions allow you to convert a GPIO
-descriptor into the GPIO integer namespace and vice-versa:
+descriptor into the GPIO integer namespace and vice-versa::
int desc_to_gpio(const struct gpio_desc *desc)
struct gpio_desc *gpio_to_desc(unsigned gpio)
diff --git a/Documentation/driver-api/gpio/index.rst b/Documentation/driver-api/gpio/index.rst
index fd22c0d1419e..6ba9e0043310 100644
--- a/Documentation/driver-api/gpio/index.rst
+++ b/Documentation/driver-api/gpio/index.rst
@@ -9,6 +9,7 @@ Contents:
intro
driver
+ consumer
legacy
Core
diff --git a/Documentation/gpio/00-INDEX b/Documentation/gpio/00-INDEX
index 64cf61245861..f960fc00a3ef 100644
--- a/Documentation/gpio/00-INDEX
+++ b/Documentation/gpio/00-INDEX
@@ -1,7 +1,5 @@
00-INDEX
- This file
-consumer.txt
- - How to obtain and use GPIOs in a driver
drivers-on-gpio.txt:
- Drivers in other subsystems that can use GPIO to provide more
complex functionality.
--
2.16.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox