Re: [PATCH v2 2/2] devicetree: Document the max31760 device binding.

[Rob Herring]

On Tue, Apr 11, 2017 at 02:32:18PM -0700, John Muir wrote:
> v2:
> - Fixup based on comments.

While you should have a commit msg here, the patch versioning goes below
the '---'.

This history is not too useful either. You should describe what you 
changed so reviewers don't have to go find the previous versions. I may 
remember reviewing a patch, but I likely don't remember what I said.

> 
> Signed-off-by: John Muir 
> ---
>  .../devicetree/bindings/hwmon/max31760.txt | 72 
> ++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/max31760.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/max31760.txt 
> b/Documentation/devicetree/bindings/hwmon/max31760.txt
> new file mode 100644
> index ..760fdf0b55e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/max31760.txt
> @@ -0,0 +1,72 @@
> +MAX31760 fan controller
> +---
> +
> +This device supports I2C only. Fan sub-nodes must be defined in order to 
> enable
> +the fan tachometer input. See also the datasheet:
> +https://datasheets.maximintegrated.com/en/ds/MAX31760.pdf
> +
> +Required node properties:
> + - compatible: "maxim,max31760"
> + - reg: The I2C address of the device. This is 0x50 - 0x57 depending on the
> +   hardware configuration.
> + - #address-cells: Must be 1.
> + - #size-cells: Must be 0.
> +
> +Optional node properties:
> + - maxim,fan-fail-full-only: Boolean; Assert a fan failure only when the PWM 
> is
> +   at 100%.
> + - maxim,fan-rd-signal: Boolean; Fans provide a rotation detection (RD) 
> signal
> +   instead of generating square-wave pulses.
> + - maxim,fan-rd-polarity-high: Boolean; RD is high when the fan is running, 
> not
> +   low. Only relevant when fan-rd-signal is true.
> + - maxim,fan-signal-enabled: Boolean; Externally driving FF/FS low should 
> force
> +   PWM output to 100%.
> + - maxim,fan-spin-up-enabled: Boolean; For fan startup set the PWM to 100% 
> until
> +   tach is detected or two seconds have passed before reducing to the target
> +   value.
> + - maxim,pwm-polarity-negative: Boolean; 100% PWM is when PWM is low, not 
> high.
> + - maxim,pwm-pulse-stretch-enabled: Boolean; Enable PWM pulse stretching.
> + - maxim,pwm-zero-fan-can-fail: Boolean; Enable fan failure detection while
> +   ramping to 0% PWM.
> +
> +Fan sub-nodes must be present in order to enable the fan.
> +
> +Required fan sub-node properties:
> + - reg: Fan address. Must be <0x00> or <0x01>.
> +
> +Optional fan sub-node properties:
> + - label: String; Assigned to the hwmon fanX_label property.
> +
> +Temperature sub-nodes are optional.
> +
> +Required temp sub-node properties:
> + - reg: Temperature sensor address. Must be <0x00> or <0x01>.
> +
> +Optional temp sub-node properties:
> + - label: String; Assigned to the hwmon tempX_label property.
> + - ideality: For temperature node with reg 1 only: Set ideality factor for 
> the

This too needs a maxim prefix.

> +   remote temperature sensor. Integer with range 0 to 63, representing a
> +   multiplication factor of 0.9844 to 1.0489. Default: 24 (1.0080).
> +
> +Example:
> + max31760@50 {
> + compatible = "maxim,max31760";
> + reg = <0x50>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + maxim,fan-spin-up-enabled;
> +
> + fan@0 {
> + reg = <0x00>;
> + label = "Left";
> + };
> + fan@1 {
> + reg = <0x01>;
> + label = "Right";
> + };
> + temp@1 {
> + reg = <0x01>;
> + label = "CPU";
> + };
> + };
> -- 
> 2.12.2.715.g7642488e1d-goog
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-19 15:49, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
> [...]
 +int mux_control_select(struct mux_control *mux, int state)
>>>
>>> If we let two of these race, ...
>>
>> The window for this "race" is positively huge. If there are several
>> mux consumers of a single mux controller, it is self-evident that
>> if one of them grabs the mux for a long time, the others will suffer.
>>
>> The design is that the rwsem is reader-locked for the full duration
>> of a select/deselect operation by the mux consumer.
> 
> I was not clear. I meant: I think this can also happen if we let them
> race with the same state target.

Right, but there is another glaring problem with the v13 implementation
of select/deselect. If there are three consumers and the first one
holds the mux while the other two tries to select it to some other
position, then even if the two "new" consumers agree on the mux state,
then both of them will end up in the "it's just contended" case and
then be serialized. So, yes, the select/deselect implementation is not
perfect. To quote the cover letter:

I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.

But with your discovery that there's a race when two consumers go
for the same state in a free mux, in addition to the above contention
problem, I'm about ready to go with Greg's suggestion and just use a
mutex. I.e. ignore the desire to allow concurrent use. Because it's
not like the sketchy thing I threw out in the other part of the thread
solves any of these problems. I can live without the concurrency, and
I guess I can also live with passing the buck to the poor sod that
eventually needs it.

 +{
 +  int ret;
 +
 +  if (down_read_trylock(>lock)) {
 +  if (mux->cached_state == state)
 +  return 0;
> 
> This check makes it clear that a second select call is not intended to
> block if the intended state is already selected. But if the instance we
> will lose the race against has not yet updated cached_state, ...
> 
 +  /* Sigh, the mux needs updating... */
 +  up_read(>lock);
 +  }
 +
 +  /* ...or it's just contended. */
 +  down_write(>lock);
> 
> ... we are blocking here until the other instance calls up_read. Even
> though in this case (same state target) we would only have to block
> until the other instance calls downgrade_write after the mux control is
> set to the correct state.
> 
> Basically there is a small window before down_write with no lock at all,
> where multiple instances can already have decided they must change the
> mux (to the same state). If this happens, they go on to block each other
> unnecessarily.
> 
>>> ... then the last to get to down_write will just wait here forever (or
>>> until the first consumer calls mux_control_deselect, which may never
>>> happen)?
>>
>> It is vital that the mux consumer call _deselect when it is done with
>> the mux. Not doing so will surely starve out any other mux consumers.
>> The whole thing is designed around the fact that mux consumers should
>> deselect the mux as soon as it's no longer needed.
> 
> I'd like to use this for video bus multiplexers. Those would not be
> selected for the duration of an i2c transfer, but for the duration of a
> running video capture stream, or for the duration of an enabled display
> path. While I currently have no use case for multiple consumers
> controlling the same mux, this scenario is what shapes my perspective.
> For such long running selections the consumer should have the option to
> return -EBUSY instead of blocking when the lock can't be taken.

I'll see if I can implement a try variant for the mutex based select I
will probably end up with in v14...

Cheers,
peda
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] docs: process/4.Coding.rst: Re-flow a couple of paragraphs

[Andrew Clayton]

In Documentation/process/4.Coding.rst there were a couple of paragraphs
that spilled over the 80 character line length.

This brings them back in line with the rest of the document.

Signed-off-by: Andrew Clayton 
---
 Documentation/process/4.Coding.rst | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/process/4.Coding.rst 
b/Documentation/process/4.Coding.rst
index 2a728d898fc5..017f22fa970c 100644
--- a/Documentation/process/4.Coding.rst
+++ b/Documentation/process/4.Coding.rst
@@ -22,10 +22,10 @@ Coding style
 
 
 The kernel has long had a standard coding style, described in
-Documentation/process/coding-style.rst.  For much of that time, the policies 
described
-in that file were taken as being, at most, advisory.  As a result, there is
-a substantial amount of code in the kernel which does not meet the coding
-style guidelines.  The presence of that code leads to two independent
+Documentation/process/coding-style.rst.  For much of that time, the policies
+described in that file were taken as being, at most, advisory.  As a result,
+there is a substantial amount of code in the kernel which does not meet the
+coding style guidelines.  The presence of that code leads to two independent
 hazards for kernel developers.
 
 The first of these is to believe that the kernel coding standards do not
@@ -343,9 +343,9 @@ user-space developers to know what they are working with.  
See
 Documentation/ABI/README for a description of how this documentation should
 be formatted and what information needs to be provided.
 
-The file Documentation/admin-guide/kernel-parameters.rst describes all of the 
kernel's
-boot-time parameters.  Any patch which adds new parameters should add the
-appropriate entries to this file.
+The file Documentation/admin-guide/kernel-parameters.rst describes all of
+the kernel's boot-time parameters.  Any patch which adds new parameters
+should add the appropriate entries to this file.
 
 Any new configuration options must be accompanied by help text which
 clearly explains the options and when the user might want to select them.
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug

[Mathieu Poirier]

On 6 April 2017 at 07:30, Leo Yan  wrote:
> Update kernel-parameters.txt to add new parameter:
> coresight_cpu_debug.enable is a knob to enable debugging at boot time.
>
> Add detailed documentation, which contains the implementation, Mike
> Leach excellent summary for "clock and power domain". At the end some
> examples on how to enable the debugging functionality are provided.
>
> Suggested-by: Mike Leach 
> Signed-off-by: Leo Yan 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |   7 +
>  Documentation/trace/coresight-cpu-debug.txt | 173 
> 
>  2 files changed, 180 insertions(+)
>  create mode 100644 Documentation/trace/coresight-cpu-debug.txt
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index facc20a..cf90146 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -650,6 +650,13 @@
> /proc//coredump_filter.
> See also Documentation/filesystems/proc.txt.
>
> +   coresight_cpu_debug.enable
> +   [ARM,ARM64]
> +   Format: 
> +   Enable/disable the CPU sampling based debugging.
> +   0: default value, disable debugging
> +   1: enable debugging at boot time
> +
> cpuidle.off=1   [CPU_IDLE]
> disable the cpuidle sub-system
>
> diff --git a/Documentation/trace/coresight-cpu-debug.txt 
> b/Documentation/trace/coresight-cpu-debug.txt
> new file mode 100644
> index 000..e7ad05e
> --- /dev/null
> +++ b/Documentation/trace/coresight-cpu-debug.txt
> @@ -0,0 +1,173 @@
> +   Coresight CPU Debug Module
> +   ==
> +
> +   Author:   Leo Yan 
> +   Date: April 5th, 2017
> +
> +Introduction
> +
> +
> +Coresight CPU debug module is defined in ARMv8-a architecture reference 
> manual
> +(ARM DDI 0487A.k) Chapter 'Part H: External debug', the CPU can integrate
> +debug module and it is mainly used for two modes: self-hosted debug and
> +external debug. Usually the external debug mode is well known as the external
> +debugger connects with SoC from JTAG port; on the other hand the program can
> +explore debugging method which rely on self-hosted debug mode, this document
> +is to focus on this part.
> +
> +The debug module provides sample-based profiling extension, which can be used
> +to sample CPU program counter, secure state and exception level, etc; usually
> +every CPU has one dedicated debug module to be connected. Based on 
> self-hosted
> +debug mechanism, Linux kernel can access these related registers from mmio
> +region when the kernel panic happens. The callback notifier for kernel panic
> +will dump related registers for every CPU; finally this is good for assistant
> +analysis for panic.
> +
> +
> +Implementation
> +--
> +
> +- During driver registration, use EDDEVID and EDDEVID1 two device ID
> +  registers to decide if sample-based profiling is implemented or not. On 
> some
> +  platforms this hardware feature is fully or partialy implemented; and if
> +  this feature is not supported then registration will fail.
> +
> +- When write this doc, the debug driver mainly relies on three sampling
> +  registers. The kernel panic callback notifier gathers info from EDPCSR
> +  EDVIDSR and EDCIDSR; from EDPCSR we can get program counter, EDVIDSR has
> +  information for secure state, exception level, bit width, etc; EDCIDSR is
> +  context ID value which contains the sampled value of CONTEXTIDR_EL1.
> +
> +- The driver supports CPU running mode with either AArch64 or AArch32. The
> +  registers naming convention is a bit different between them, AArch64 uses
> +  'ED' for register prefix (ARM DDI 0487A.k, chapter H9.1) and AArch32 uses
> +  'DBG' as prefix (ARM DDI 0487A.k, chapter G5.1). The driver is unified to
> +  use AArch64 naming convention.
> +
> +- ARMv8-a (ARM DDI 0487A.k) and ARMv7-a (ARM DDI 0406C.b) have different
> +  register bits definition. So the driver consolidates two difference:
> +
> +  If PCSROffset=0b. ARMv8-a doesn't apply offset on PCSR, but ARMv7-a
> +  defines "PCSR samples are offset by a value that depends on the instruction
> +  set state".

I had to look in the documentation (ID092916) a little here.  On ARMv8
if DEVID1.PCSROffset is 0 then the feature is not implemented - as
such the first part of the sentence should be corrected.  The rest of
the sentence about ARMv7 is correct.

> For ARMv7-a, the driver checks furthermore if CPU runs with ARM
> +  or thumb instruction set and calibrate PCSR value, the detailed description
> +  for offset is in ARMv7-a ARM (ARM DDI 0406C.b) chapter C11.11.34 "DBGPCSR,
> +  Program Counter Sampling Register".
> +
> +  If 

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Mathieu Poirier]

On 19 April 2017 at 09:30, Leo Yan  wrote:
> On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> >> > +static bool debug_enable;
>> >> > +module_param_named(enable, debug_enable, bool, 0600);
>> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> >> > +"(default is 0, which means is disabled by default)");
>> >>
>> >> For this driver we have a debugFS interface so I question the validity of 
>> >> a
>> >> kernel module parameter.  Other than adding complexity to the code it 
>> >> offers no
>> >> real added value.  If a user is to insmod a module, it is just as easy to 
>> >> switch
>> >> on the functionality using debugFS in a second step.
>> >
>> > This module parameter can be used for kernel command line, so
>> > it's useful when user wants to dynamically turn on/off the
>> > functionality at boot up time.
>> >
>> > Does this make sense for you? Removing this parameter is okay for
>> > me, but this means users need to decide if use it by Kernel config
>> > with static building in. This is a bit contradictory with before's
>> > discussion.
>>
>> My hope was to use the kernel command line and the debugFS interface,
>> avoiding the module parameter.  Look at what the baycom_par and
>> blacklist drivers are doing with the "__setup()" function and see if
>> we can void a module parameter.  If not then let it be, unless someone
>> else has a better idea.
>>
>> [1]. drivers/net/hamradio/baycom_par.c
>> [2]. drivers/s390/cio/blacklist.c
>
> This driver supports module mode. So we can choose to use either module
> parameter or __setup(). But as described in the file
> Documentation/admin-guide/kernel-parameters.rst, the module parameter
> is more flexible to be used at boot time or insmod the module:
>
> "Parameters for modules which are built into the kernel need to be
> specified on the kernel command line.  modprobe looks through the
> kernel command line (/proc/cmdline) and collects module parameters
> when it loads a module, so the kernel command line can be used for
> loadable modules too."
>
> __setup() cannot support module mode, and when use __setup(), we need
> register one callback function for it. The callback function is
> friendly to parse complex parameters rather than module parameter.
> but it's not necessary for this case.

__setup() definitely supports module - the baycom driver is a good
example of that.

But as you pointed out kernel-parameters.rst is pretty clear on how to
proceed.  As such disregard my comment and proceed with a module
parameter.

>
> So I'm bias to use module parameter :)
>
> Thanks,
> Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux

[Philipp Zabel]

On Wed, 2017-04-19 at 13:23 +0200, Peter Rosin wrote:
> On 2017-04-19 13:05, Philipp Zabel wrote:
> > On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
> >> On 2017-04-19 11:17, Philipp Zabel wrote:
> >>> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
>  If I got things wrong when I skimmed whatever I came across, and if the
>  mmio register is the only mux control option in the stars, it becomes
>  less obvious... It's of course still possible to hook into the mux
>  subsystem, but the benefit is questionable. And you do get the extra
>  device tree node. You could of course also implement a mux driver
>  outside of drivers/mux and thus make use of the mux api, but it's tiny
>  and any benefit is truly small.
> >>>
> >>> What I wondered mostly is whether it would be a good idea to move the
> >>> OF-graph ports into the mux controller node, and let the video capture
> >>> device be the consumer of the mux.
> >>> But this wouldn't fit well with the clear split between the mux
> >>> controller and the actual mux hardware in the mux DT bindings.
> >>
> >> I have tried to do something similar. I think. The current
> >> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
> >> IIUC.
> >>
> >> That dedicated driver and the general purpose i2c mux driver does pretty
> >> much the same thing with these two DT snippets:
> >>
> >> Dedicated i2c-mux-gpio DT snippet:
> >>
> >>i2c-mux {
> >>compatible = "i2c-mux-gpio";
> >>i2c-parent = <>;
> >>
> >>mux-gpios = < 22 0  23 0>;
> >>
> >>#address-cells = <1>;
> >>#size-cells = <0>;
> >>
> >>i2c@1 {
> >>...
> >>};
> >>
> >>i2c@3 {
> >>...
> >>};
> >>};
> >>
> >> General purpose mux DT snippet:
> >>
> >>mux: mux-controller {
> >>compatible = "gpio-mux";
> >>#mux-control-cells = <0>;
> >>
> >>mux-gpios = < 22 0  23 0>;
> >>};
> >>
> >>i2c-mux {
> >>compatible = "i2c-mux";
> >>i2c-parent = <>;
> >>
> >>mux-controls = <>;
> >>
> >>#address-cells = <1>;
> >>#size-cells = <0>;
> >>
> >>i2c@1 {
> >>...
> >>};
> >>
> >>i2c@3 {
> >>...
> >>};
> >>};
> > 
> > Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
> > nodes, and this is very close to what I am thinking about.
> > 
> >> I would love to find a way to cleanly get the mux framework to handle
> >> the first DT as well, and thus being able to obsolete the dedicated
> >> i2c-mux-gpio driver. I have not figured out how to accomplish that
> >> without abusing the driver-model to a point that it's not working.
> >> Help with that task is dearly appreciated.
> >>
> >> What I have stumbled on, I think, is that two drivers needs to be
> >> instantiated from the same DT node. At the same time, I need the
> >> mux framework to handle the current out-of-node thing with a
> >> phandle as well, so that several mux consumers can share a common
> >> mux controller. My understanding of these matters are apparently not
> >> deep enough...
> > 
> > Not necessarily, if the framework could export a function to create a
> > gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
> > drivers just reuse that.
> 
> I've been up that creek. Why should the gpio mux be special cased?

You are right, this does not scale.

> That's not clean, the implication is that all mux consumers need
> to handle the gpio case and have a special compatible for that
> case etc. Then someone thinks the DT should look equally "clean" for
> some i2c based mux, and the weeds start piling up. This is exactly
> what we don't want. We want the mux consumer drivers to be totally
> agnostic about the fact that they happen to use a gpio mux.

If you want to have i2c-mux-gpio and i2c-mux compatibles, and a single
driver to handle them both, it must at least match both compatibles, so
it can't be completely agnostic.

Why not then have it call

if (/* compatible == "i2c-mux" */)
mux = devm_mux_control_get(dev, NULL);
else /* if (compatible == "i2c-mux-gpio/mmio/etc.") */
mux = devm_mux_control_create(dev);

? The mux framework core could hold a list of those -mux-
compatibles and dispatch creation of the correct mux (or mux platform
device, if necessary).

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:

[...]

> >> > +static bool debug_enable;
> >> > +module_param_named(enable, debug_enable, bool, 0600);
> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> >> > +"(default is 0, which means is disabled by default)");
> >>
> >> For this driver we have a debugFS interface so I question the validity of a
> >> kernel module parameter.  Other than adding complexity to the code it 
> >> offers no
> >> real added value.  If a user is to insmod a module, it is just as easy to 
> >> switch
> >> on the functionality using debugFS in a second step.
> >
> > This module parameter can be used for kernel command line, so
> > it's useful when user wants to dynamically turn on/off the
> > functionality at boot up time.
> >
> > Does this make sense for you? Removing this parameter is okay for
> > me, but this means users need to decide if use it by Kernel config
> > with static building in. This is a bit contradictory with before's
> > discussion.
> 
> My hope was to use the kernel command line and the debugFS interface,
> avoiding the module parameter.  Look at what the baycom_par and
> blacklist drivers are doing with the "__setup()" function and see if
> we can void a module parameter.  If not then let it be, unless someone
> else has a better idea.
> 
> [1]. drivers/net/hamradio/baycom_par.c
> [2]. drivers/s390/cio/blacklist.c

This driver supports module mode. So we can choose to use either module
parameter or __setup(). But as described in the file
Documentation/admin-guide/kernel-parameters.rst, the module parameter
is more flexible to be used at boot time or insmod the module:

"Parameters for modules which are built into the kernel need to be
specified on the kernel command line.  modprobe looks through the 
kernel command line (/proc/cmdline) and collects module parameters
when it loads a module, so the kernel command line can be used for 
loadable modules too."

__setup() cannot support module mode, and when use __setup(), we need
register one callback function for it. The callback function is
friendly to parse complex parameters rather than module parameter.
but it's not necessary for this case.

So I'm bias to use module parameter :)

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Leo Yan]

On Wed, Apr 19, 2017 at 03:32:39PM +0100, Suzuki K Poulose wrote:

[...]

> >>>+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >>>+{
> >>>+  void __iomem *base;
> >>>+  struct device *dev = >dev;
> >>>+  struct debug_drvdata *drvdata;
> >>>+  struct resource *res = >res;
> >>>+  struct device_node *np = adev->dev.of_node;
> >>>+  int ret;
> >>>+
> >>>+  drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >>>+  if (!drvdata)
> >>>+  return -ENOMEM;
> >>>+
> >>>+  drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >>>+  if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >>>+  dev_err(dev, "CPU%d drvdata has been initialized\n",
> >>>+  drvdata->cpu);
> >>
> >>May be we could warn about a possible issue in the DT ?
> >
> >So can I understand the suggestion is to add warning in function
> >of_coresight_get_cpu() when cannot find CPU number, and here directly
> >bail out?
> 
> No. One could have single CPU DT, where he doesn't need to provide the CPU 
> number.
> Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().
> 
> But when we hit the case above, we find that the some node was registered for
> the given CPU (be it 0 or any other), which is definitely an error in DT. Due 
> to
> 
> 1) Hasn't specified the CPU number for more than one node
> 
> OR
> 
> 2) CPU number duplicated in the more than one nodes.

Thanks for explaination. It's clear for me now.

> Cheers
> Suzuki
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 19/04/17 15:28, Leo Yan wrote:

Hi Suzuki,

On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:

Hi Leo,

This version looks good to me. I have two minor comments below.


Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.

[...]


+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
+{
+   void __iomem *base;
+   struct device *dev = >dev;
+   struct debug_drvdata *drvdata;
+   struct resource *res = >res;
+   struct device_node *np = adev->dev.of_node;
+   int ret;
+
+   drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+   if (!drvdata)
+   return -ENOMEM;
+
+   drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
+   if (per_cpu(debug_drvdata, drvdata->cpu)) {
+   dev_err(dev, "CPU%d drvdata has been initialized\n",
+   drvdata->cpu);


May be we could warn about a possible issue in the DT ?


So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?


No. One could have single CPU DT, where he doesn't need to provide the CPU 
number.
Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().

But when we hit the case above, we find that the some node was registered for
the given CPU (be it 0 or any other), which is definitely an error in DT. Due to

1) Hasn't specified the CPU number for more than one node

OR

2) CPU number duplicated in the more than one nodes.

Cheers
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Leo Yan]

Hi Suzuki,

On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
> Hi Leo,
> 
> This version looks good to me. I have two minor comments below.

Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.

[...]

> >+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >+{
> >+void __iomem *base;
> >+struct device *dev = >dev;
> >+struct debug_drvdata *drvdata;
> >+struct resource *res = >res;
> >+struct device_node *np = adev->dev.of_node;
> >+int ret;
> >+
> >+drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >+if (!drvdata)
> >+return -ENOMEM;
> >+
> >+drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >+if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >+dev_err(dev, "CPU%d drvdata has been initialized\n",
> >+drvdata->cpu);
> 
> May be we could warn about a possible issue in the DT ?

So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/32] x86: Documentation for AMD Secure Memory Encryption (SME)

[Tom Lendacky]

On 4/19/2017 4:02 AM, Borislav Petkov wrote:

Always have a verb in the Subject to form a "do this" or "do that"
sentence to better explain what the patch does:

"Subject: [PATCH v5 01/32] x86: Add documentation for AMD Secure Memory Encryption 
(SME)"


Will do.

Btw, I tried to update all the subjects and descriptions to be
more descriptive but I'm sure there is still room for improvement
so keep the comments on them coming.



On Tue, Apr 18, 2017 at 04:16:25PM -0500, Tom Lendacky wrote:

Create a Documentation entry to describe the AMD Secure Memory
Encryption (SME) feature and add documentation for the mem_encrypt=
kernel parameter.

Signed-off-by: Tom Lendacky 
---
 Documentation/admin-guide/kernel-parameters.txt |   11 
 Documentation/x86/amd-memory-encryption.txt |   60 +++
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/x86/amd-memory-encryption.txt

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 3dd6d5d..84c5787 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2165,6 +2165,17 @@
memory contents and reserves bad memory
regions that are detected.

+   mem_encrypt=[X86-64] AMD Secure Memory Encryption (SME) control
+   Valid arguments: on, off
+   Default (depends on kernel configuration option):
+ on  (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=y)
+ off (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=n)
+   mem_encrypt=on: Activate SME
+   mem_encrypt=off:Do not activate SME
+
+   Refer to Documentation/x86/amd-memory-encryption.txt
+   for details on when memory encryption can be activated.
+
mem_sleep_default=  [SUSPEND] Default system suspend mode:
s2idle  - Suspend-To-Idle
shallow - Power-On Suspend or equivalent (if supported)
diff --git a/Documentation/x86/amd-memory-encryption.txt 
b/Documentation/x86/amd-memory-encryption.txt
new file mode 100644
index 000..0b72ff2
--- /dev/null
+++ b/Documentation/x86/amd-memory-encryption.txt
@@ -0,0 +1,60 @@
+Secure Memory Encryption (SME) is a feature found on AMD processors.
+
+SME provides the ability to mark individual pages of memory as encrypted using
+the standard x86 page tables.  A page that is marked encrypted will be
+automatically decrypted when read from DRAM and encrypted when written to
+DRAM.  SME can therefore be used to protect the contents of DRAM from physical
+attacks on the system.
+
+A page is encrypted when a page table entry has the encryption bit set (see
+below on how to determine its position).  The encryption bit can be specified
+in the cr3 register, allowing the PGD table to be encrypted. Each successive


I missed that the last time: do you mean here, "The encryption bit can
be specified in the %cr3 register allowing for the page table hierarchy
itself to be encrypted."?


+level of page tables can also be encrypted.


Right, judging by the next sentence, it looks like it.


Correct. I like the hierarchy term so I'll add that to the text.

Note, just because the bit is set in %cr3 doesn't mean the full
hierarchy is encrypted. Each level in the hierarchy needs to have the
encryption bit set. So, theoretically, you could have the encryption
bit set in %cr3 so that the PGD is encrypted, but not set the encryption
bit in the PGD entry for a PUD and so the PUD pointed to by that entry
would not be encrypted.

Thanks,
Tom



The rest looks and reads really nice to me, so feel free to add:

Reviewed-by: Borislav Petkov 

after addressing those minor nitpicks on your next submission.

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
[...]
> >> +int mux_control_select(struct mux_control *mux, int state)
> > 
> > If we let two of these race, ...
>
> The window for this "race" is positively huge. If there are several
> mux consumers of a single mux controller, it is self-evident that
> if one of them grabs the mux for a long time, the others will suffer.
> 
> The design is that the rwsem is reader-locked for the full duration
> of a select/deselect operation by the mux consumer.

I was not clear. I meant: I think this can also happen if we let them
race with the same state target.

> >> +{
> >> +  int ret;
> >> +
> >> +  if (down_read_trylock(>lock)) {
> >> +  if (mux->cached_state == state)
> >> +  return 0;

This check makes it clear that a second select call is not intended to
block if the intended state is already selected. But if the instance we
will lose the race against has not yet updated cached_state, ...

> >> +  /* Sigh, the mux needs updating... */
> >> +  up_read(>lock);
> >> +  }
> >> +
> >> +  /* ...or it's just contended. */
> >> +  down_write(>lock);

... we are blocking here until the other instance calls up_read. Even
though in this case (same state target) we would only have to block
until the other instance calls downgrade_write after the mux control is
set to the correct state.

Basically there is a small window before down_write with no lock at all,
where multiple instances can already have decided they must change the
mux (to the same state). If this happens, they go on to block each other
unnecessarily.

> > ... then the last to get to down_write will just wait here forever (or
> > until the first consumer calls mux_control_deselect, which may never
> > happen)?
> 
> It is vital that the mux consumer call _deselect when it is done with
> the mux. Not doing so will surely starve out any other mux consumers.
> The whole thing is designed around the fact that mux consumers should
> deselect the mux as soon as it's no longer needed.

I'd like to use this for video bus multiplexers. Those would not be
selected for the duration of an i2c transfer, but for the duration of a
running video capture stream, or for the duration of an enabled display
path. While I currently have no use case for multiple consumers
controlling the same mux, this scenario is what shapes my perspective.
For such long running selections the consumer should have the option to
return -EBUSY instead of blocking when the lock can't be taken.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/8] coresight: add support for CPU debug module

[Suzuki K Poulose]

On 06/04/17 14:30, Leo Yan wrote:

Coresight includes debug module and usually the module connects with CPU
debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
description for related info in "Part H: External Debug".

Chapter H7 "The Sample-based Profiling Extension" introduces several
sampling registers, e.g. we can check program counter value with
combined CPU exception level, secure state, etc. So this is helpful for
analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
loop with IRQ disabled. In this case the CPU cannot switch context and
handle any interrupt (including IPIs), as the result it cannot handle
SMP call for stack dump.

This patch is to enable coresight debug module, so firstly this driver
is to bind apb clock for debug module and this is to ensure the debug
module can be accessed from program or external debugger. And the driver
uses sample-based registers for debug purpose, e.g. when system triggers
panic, the driver will dump program counter and combined context
registers (EDCIDSR, EDVIDSR); by parsing context registers so can
quickly get to know CPU secure state, exception level, etc.

Some of the debug module registers are located in CPU power domain, so
this requires the CPU power domain stays on when access related debug
registers, but the power management for CPU power domain is quite
dependent on SoC integration for power management. For the platforms
which with sane power controller implementations, this driver follows
the method to set EDPRCR to try to pull the CPU out of low power state
and then set 'no power down request' bit so the CPU has no chance to
lose power.

If the SoC has not followed up this design well for power management
controller, the user should use the command line parameter or sysfs
to constrain all or partial idle states to ensure the CPU power
domain is enabled and access coresight CPU debug component safely.


Hi Leo,

This version looks good to me. I have two minor comments below.



+
+static struct notifier_block debug_notifier = {
+   .notifier_call = debug_notifier_call,
+};
+
+static int debug_enable_func(void)
+{
+   struct debug_drvdata *drvdata;
+   int cpu;
+
+   for_each_possible_cpu(cpu) {
+   drvdata = per_cpu(debug_drvdata, cpu);
+   if (!drvdata)
+   continue;
+
+   pm_runtime_get_sync(drvdata->dev);
+   }
+
+   return atomic_notifier_chain_register(_notifier_list,
+ _notifier);
+}
+
+static int debug_disable_func(void)
+{
+   struct debug_drvdata *drvdata;
+   int cpu;
+
+   for_each_possible_cpu(cpu) {
+   drvdata = per_cpu(debug_drvdata, cpu);
+   if (!drvdata)
+   continue;
+
+   pm_runtime_put(drvdata->dev);
+   }
+
+   return atomic_notifier_chain_unregister(_notifier_list,
+   _notifier);
+}


I believe you should, reverse the order of these operations in 
debug_disable_func()
to prevent getting a panic notifier after we have released the power domain for 
the
debug.
i.e, :
atomic_notifier_chain_unregister(...);

for_each_possible_cpu(cpu) {}




+
+static ssize_t debug_func_knob_write(struct file *f,
+   const char __user *buf, size_t count, loff_t *ppos)
+{
+   u8 val;
+   int ret;
+
+   ret = kstrtou8_from_user(buf, count, 2, );
+   if (ret)
+   return ret;
+
+   mutex_lock(_lock);
+
+   if (val == debug_enable)
+   goto out;
+
+   if (val)
+   ret = debug_enable_func();
+   else
+   ret = debug_disable_func();
+
+   if (ret) {
+   pr_err("%s: unable to %s debug function: %d\n",
+  __func__, val ? "enable" : "disable", ret);
+   goto err;
+   }
+
+   debug_enable = val;
+out:
+   ret = count;
+err:
+   mutex_unlock(_lock);
+   return ret;
+}
+
+static ssize_t debug_func_knob_read(struct file *f,
+   char __user *ubuf, size_t count, loff_t *ppos)
+{
+   ssize_t ret;
+   char buf[2];
+
+   mutex_lock(_lock);
+
+   buf[0] = '0' + debug_enable;
+   buf[1] = '\n';
+   ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
+
+   mutex_unlock(_lock);
+   return ret;
+}
+
+static const struct file_operations debug_func_knob_fops = {
+   .open   = simple_open,
+   .read   = debug_func_knob_read,
+   .write  = debug_func_knob_write,
+};
+
+static int debug_func_init(void)
+{
+   struct dentry *file;
+   int ret;
+
+   /* Create debugfs node */
+   debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
+   if (!debug_debugfs_dir) {
+   pr_err("%s: unable to create debugfs directory\n", __func__);
+   return -ENOMEM;
+   }
+
+   file = 

Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux

[Peter Rosin]

On 2017-04-19 13:05, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
>> On 2017-04-19 11:17, Philipp Zabel wrote:
>>> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
 If I got things wrong when I skimmed whatever I came across, and if the
 mmio register is the only mux control option in the stars, it becomes
 less obvious... It's of course still possible to hook into the mux
 subsystem, but the benefit is questionable. And you do get the extra
 device tree node. You could of course also implement a mux driver
 outside of drivers/mux and thus make use of the mux api, but it's tiny
 and any benefit is truly small.
>>>
>>> What I wondered mostly is whether it would be a good idea to move the
>>> OF-graph ports into the mux controller node, and let the video capture
>>> device be the consumer of the mux.
>>> But this wouldn't fit well with the clear split between the mux
>>> controller and the actual mux hardware in the mux DT bindings.
>>
>> I have tried to do something similar. I think. The current
>> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
>> IIUC.
>>
>> That dedicated driver and the general purpose i2c mux driver does pretty
>> much the same thing with these two DT snippets:
>>
>> Dedicated i2c-mux-gpio DT snippet:
>>
>>  i2c-mux {
>>  compatible = "i2c-mux-gpio";
>>  i2c-parent = <>;
>>
>>  mux-gpios = < 22 0  23 0>;
>>
>>  #address-cells = <1>;
>>  #size-cells = <0>;
>>
>>  i2c@1 {
>>  ...
>>  };
>>
>>  i2c@3 {
>>  ...
>>  };
>>  };
>>
>> General purpose mux DT snippet:
>>
>>  mux: mux-controller {
>>  compatible = "gpio-mux";
>>  #mux-control-cells = <0>;
>>
>>  mux-gpios = < 22 0  23 0>;
>>  };
>>
>>  i2c-mux {
>>  compatible = "i2c-mux";
>>  i2c-parent = <>;
>>
>>  mux-controls = <>;
>>
>>  #address-cells = <1>;
>>  #size-cells = <0>;
>>
>>  i2c@1 {
>>  ...
>>  };
>>
>>  i2c@3 {
>>  ...
>>  };
>>  };
> 
> Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
> nodes, and this is very close to what I am thinking about.
> 
>> I would love to find a way to cleanly get the mux framework to handle
>> the first DT as well, and thus being able to obsolete the dedicated
>> i2c-mux-gpio driver. I have not figured out how to accomplish that
>> without abusing the driver-model to a point that it's not working.
>> Help with that task is dearly appreciated.
>>
>> What I have stumbled on, I think, is that two drivers needs to be
>> instantiated from the same DT node. At the same time, I need the
>> mux framework to handle the current out-of-node thing with a
>> phandle as well, so that several mux consumers can share a common
>> mux controller. My understanding of these matters are apparently not
>> deep enough...
> 
> Not necessarily, if the framework could export a function to create a
> gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
> drivers just reuse that.

I've been up that creek. Why should the gpio mux be special cased?
That's not clean, the implication is that all mux consumers need
to handle the gpio case and have a special compatible for that
case etc. Then someone thinks the DT should look equally "clean" for
some i2c based mux, and the weeds start piling up. This is exactly
what we don't want. We want the mux consumer drivers to be totally
agnostic about the fact that they happen to use a gpio mux.

Cheers,
peda

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux

[Philipp Zabel]

On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
> On 2017-04-19 11:17, Philipp Zabel wrote:
> > On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> >> If I got things wrong when I skimmed whatever I came across, and if the
> >> mmio register is the only mux control option in the stars, it becomes
> >> less obvious... It's of course still possible to hook into the mux
> >> subsystem, but the benefit is questionable. And you do get the extra
> >> device tree node. You could of course also implement a mux driver
> >> outside of drivers/mux and thus make use of the mux api, but it's tiny
> >> and any benefit is truly small.
> > 
> > What I wondered mostly is whether it would be a good idea to move the
> > OF-graph ports into the mux controller node, and let the video capture
> > device be the consumer of the mux.
> > But this wouldn't fit well with the clear split between the mux
> > controller and the actual mux hardware in the mux DT bindings.
> 
> I have tried to do something similar. I think. The current
> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
> IIUC.
>
> That dedicated driver and the general purpose i2c mux driver does pretty
> much the same thing with these two DT snippets:
> 
> Dedicated i2c-mux-gpio DT snippet:
> 
>   i2c-mux {
>   compatible = "i2c-mux-gpio";
>   i2c-parent = <>;
> 
>   mux-gpios = < 22 0  23 0>;
> 
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   i2c@1 {
>   ...
>   };
> 
>   i2c@3 {
>   ...
>   };
>   };
> 
> General purpose mux DT snippet:
> 
>   mux: mux-controller {
>   compatible = "gpio-mux";
>   #mux-control-cells = <0>;
> 
>   mux-gpios = < 22 0  23 0>;
>   };
> 
>   i2c-mux {
>   compatible = "i2c-mux";
>   i2c-parent = <>;
> 
>   mux-controls = <>;
> 
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   i2c@1 {
>   ...
>   };
> 
>   i2c@3 {
>   ...
>   };
>   };

Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
nodes, and this is very close to what I am thinking about.

> I would love to find a way to cleanly get the mux framework to handle
> the first DT as well, and thus being able to obsolete the dedicated
> i2c-mux-gpio driver. I have not figured out how to accomplish that
> without abusing the driver-model to a point that it's not working.
> Help with that task is dearly appreciated.
> 
> What I have stumbled on, I think, is that two drivers needs to be
> instantiated from the same DT node. At the same time, I need the
> mux framework to handle the current out-of-node thing with a
> phandle as well, so that several mux consumers can share a common
> mux controller. My understanding of these matters are apparently not
> deep enough...

Not necessarily, if the framework could export a function to create a
gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
drivers just reuse that.

> I think you would like a DT that looks more like the first DT
> snippet but still enjoy the flexibility of the mux framework and w/o
> implementing a (another) full muxing sub-sub-system like the i2c
> sub-system has done. Correct?

Correct.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux

[Peter Rosin]

On 2017-04-19 11:17, Philipp Zabel wrote:
> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
>> If I got things wrong when I skimmed whatever I came across, and if the
>> mmio register is the only mux control option in the stars, it becomes
>> less obvious... It's of course still possible to hook into the mux
>> subsystem, but the benefit is questionable. And you do get the extra
>> device tree node. You could of course also implement a mux driver
>> outside of drivers/mux and thus make use of the mux api, but it's tiny
>> and any benefit is truly small.
> 
> What I wondered mostly is whether it would be a good idea to move the
> OF-graph ports into the mux controller node, and let the video capture
> device be the consumer of the mux.
> But this wouldn't fit well with the clear split between the mux
> controller and the actual mux hardware in the mux DT bindings.

I have tried to do something similar. I think. The current
drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
IIUC.

That dedicated driver and the general purpose i2c mux driver does pretty
much the same thing with these two DT snippets:

Dedicated i2c-mux-gpio DT snippet:

i2c-mux {
compatible = "i2c-mux-gpio";
i2c-parent = <>;

mux-gpios = < 22 0  23 0>;

#address-cells = <1>;
#size-cells = <0>;

i2c@1 {
...
};

i2c@3 {
...
};
};

General purpose mux DT snippet:

mux: mux-controller {
compatible = "gpio-mux";
#mux-control-cells = <0>;

mux-gpios = < 22 0  23 0>;
};

i2c-mux {
compatible = "i2c-mux";
i2c-parent = <>;

mux-controls = <>;

#address-cells = <1>;
#size-cells = <0>;

i2c@1 {
...
};

i2c@3 {
...
};
};

I would love to find a way to cleanly get the mux framework to handle
the first DT as well, and thus being able to obsolete the dedicated
i2c-mux-gpio driver. I have not figured out how to accomplish that
without abusing the driver-model to a point that it's not working.
Help with that task is dearly appreciated.

What I have stumbled on, I think, is that two drivers needs to be
instantiated from the same DT node. At the same time, I need the
mux framework to handle the current out-of-node thing with a
phandle as well, so that several mux consumers can share a common
mux controller. My understanding of these matters are apparently not
deep enough...

I think you would like a DT that looks more like the first DT
snippet but still enjoy the flexibility of the mux framework and w/o
implementing a (another) full muxing sub-sub-system like the i2c
sub-system has done. Correct?

Cheers,
peda

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 01/32] x86: Documentation for AMD Secure Memory Encryption (SME)

[David Howells]

Borislav Petkov  wrote:

> "Subject: [PATCH v5 01/32] x86: Add documentation for AMD Secure Memory 
> Encryption (SME)"

Or:

x86: Document AMD Secure Memory Encryption (SME) support

David
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux

[Philipp Zabel]

On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> On 2017-04-18 12:06, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> >> Allow specifying that a single multiplexer controller can be used to
> >> control several parallel multiplexers, thus enabling sharing of the
> >> multiplexer controller by different consumers.
> >>
> >> Add a binding for a first mux controller in the form of a GPIO based mux
> >> controller.
> >>
> >> Acked-by: Jonathan Cameron 
> >> Acked-by: Rob Herring 
> >> Signed-off-by: Peter Rosin 
> >> ---
> >>  Documentation/devicetree/bindings/mux/gpio-mux.txt |  69 +
> >>  .../devicetree/bindings/mux/mux-controller.txt | 157 
> >> +
> >>  MAINTAINERS|   6 +
> >>  include/dt-bindings/mux/mux.h  |  16 +++
> >>  4 files changed, 248 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mux/gpio-mux.txt
> >>  create mode 100644 
> >> Documentation/devicetree/bindings/mux/mux-controller.txt
> >>  create mode 100644 include/dt-bindings/mux/mux.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.txt 
> >> b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> >> new file mode 100644
> >> index ..b8f746344d80
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> >> @@ -0,0 +1,69 @@
> >> +GPIO-based multiplexer controller bindings
> >> +
> >> +Define what GPIO pins are used to control a multiplexer. Or several
> >> +multiplexers, if the same pins control more than one multiplexer.
> >> +
> >> +Required properties:
> >> +- compatible : "gpio-mux"
> >> +- mux-gpios : list of gpios used to control the multiplexer, least
> >> +significant bit first.
> >> +- #mux-control-cells : <0>
> >> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >> +
> >> +Optional properties:
> >> +- idle-state : if present, the state the mux will have when idle. The
> >> + special state MUX_IDLE_AS_IS is the default.
> >> +
> >> +The multiplexer state is defined as the number represented by the
> >> +multiplexer GPIO pins, where the first pin is the least significant
> >> +bit. An active pin is a binary 1, an inactive pin is a binary 0.
> >> +
> >> +Example:
> >> +
> >> +  mux: mux-controller {
> >> +  compatible = "gpio-mux";
> >> +  #mux-control-cells = <0>;
> >> +
> >> +  mux-gpios = < 0 GPIO_ACTIVE_HIGH>,
> >> +  < 1 GPIO_ACTIVE_HIGH>;
> >> +  };
> >> +
> >> +  adc-mux {
> >> +  compatible = "io-channel-mux";
> >> +  io-channels = < 0>;
> >> +  io-channel-names = "parent";
> >> +
> >> +  mux-controls = <>;
> >> +
> >> +  channels = "sync-1", "in", "out", "sync-2";
> >> +  };
> > 
> > Could you explain in more detail the reasoning behind this split between
> > the mux controller and the actual mux?
> > For SoC internal video bus muxes that are controlled by a register
> > bitfield, it seems a bit strange to have to split them into two device
> > tree nodes.
> 
> The background for the split is in the cover letter.

Thanks for explaining anyway, I didn't read past the changelog earlier.

[...]
> > Basically I'm trying to figure out whether a video mux (which has a mux
> > control plus OF-graph bindings to describe its ports and connections)
> > would fit into the same category as an adc-mux or i2c-mux, or whether it
> > would be better to handle them as a specialized form of mux-controller.
> 
> I did read some earlier thread about your muxing requirements and I got
> the impression that you also had HW which controlled the mux with
> gpio lines? In that case, the mux subsystem seems like a perfect fit
> with a new syscon/mmio/reg based mux driver (or whatever the name should
> be, I think I'd go with syscon) pretty much as suggested in your RFC
> patches. And then of course reuse the existing gpio-mux driver for the
> other case.

Yes, the requirement on hand is for MMIO controlled SoC internal muxes
for the i.MX6 video capture subsystem, but I'd like to also support GPIO
controlled external muxes to switch between two camera sources on those
boards that have them.

> The video-mux would fit as a mux consumer just like the iio-mux and the
> i2c-mux are mux consumers, with input 0/input 1 being the port that
> would be selected with the mux I guess.

Exactly. An N-input mux would have N+1 ports with port N being the
output.

[...]
> If I got things wrong when I skimmed whatever I came across, and if the
> mmio register is the only mux control option in the stars, it becomes
> less obvious... It's of course still possible to hook into the mux
> subsystem, but the benefit is questionable. And you do get the extra
> device tree node. You could of course also implement a mux driver
> outside of drivers/mux and thus make use of the mux api, but it's 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> Add a new minimalistic subsystem that handles multiplexer controllers.
> When multiplexers are used in various places in the kernel, and the
> same multiplexer controller can be used for several independent things,
> there should be one place to implement support for said multiplexer
> controller.
> 
> A single multiplexer controller can also be used to control several
> parallel multiplexers, that are in turn used by different subsystems
> in the kernel, leading to a need to coordinate multiplexer accesses.
> The multiplexer subsystem handles this coordination.
> 
> This new mux controller subsystem initially comes with a single backend
> driver that controls gpio based multiplexers. Even though not needed by
> this initial driver, the mux controller subsystem is prepared to handle
> chips with multiple (independent) mux controllers.
> 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Peter Rosin 
> ---
>  Documentation/driver-model/devres.txt |   8 +
>  MAINTAINERS   |   2 +
>  drivers/Kconfig   |   2 +
>  drivers/Makefile  |   1 +
>  drivers/mux/Kconfig   |  34 +++
>  drivers/mux/Makefile  |   6 +
>  drivers/mux/mux-core.c| 422 
> ++
>  drivers/mux/mux-gpio.c| 114 +
>  include/linux/mux.h   | 252 
>  9 files changed, 841 insertions(+)
>  create mode 100644 drivers/mux/Kconfig
>  create mode 100644 drivers/mux/Makefile
>  create mode 100644 drivers/mux/mux-core.c
>  create mode 100644 drivers/mux/mux-gpio.c
>  create mode 100644 include/linux/mux.h
> 
> diff --git a/Documentation/driver-model/devres.txt 
> b/Documentation/driver-model/devres.txt
> index efb8200819d6..e2343d9cbec7 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -337,6 +337,14 @@ MEM
>  MFD
>devm_mfd_add_devices()
>  
> +MUX
> +  devm_mux_chip_alloc()
> +  devm_mux_chip_free()
> +  devm_mux_chip_register()
> +  devm_mux_chip_unregister()
> +  devm_mux_control_get()
> +  devm_mux_control_put()
> +
>  PER-CPU MEM
>devm_alloc_percpu()
>devm_free_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7fc06739c8ad..591eba737678 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8563,6 +8563,8 @@ M:  Peter Rosin 
>  S:   Maintained
>  F:   Documentation/devicetree/bindings/mux/
>  F:   include/linux/dt-bindings/mux/
> +F:   include/linux/mux.h
> +F:   drivers/mux/
>  
>  MULTISOUND SOUND DRIVER
>  M:   Andrew Veliath 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 117ca14ccf85..a7ea13e1b869 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
>  
>  source "drivers/fsi/Kconfig"
>  
> +source "drivers/mux/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2eced9afba53..c0436f6dd5a9 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID) += android/
>  obj-$(CONFIG_NVMEM)  += nvmem/
>  obj-$(CONFIG_FPGA)   += fpga/
>  obj-$(CONFIG_FSI)+= fsi/
> +obj-$(CONFIG_MULTIPLEXER)+= mux/
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> new file mode 100644
> index ..41dfe08ead84
> --- /dev/null
> +++ b/drivers/mux/Kconfig
> @@ -0,0 +1,34 @@
> +#
> +# Multiplexer devices
> +#
> +
> +menuconfig MULTIPLEXER
> + tristate "Multiplexer subsystem"
> + help
> +   Multiplexer controller subsystem. Multiplexers are used in a
> +   variety of settings, and this subsystem abstracts their use
> +   so that the rest of the kernel sees a common interface. When
> +   multiple parallel multiplexers are controlled by one single
> +   multiplexer controller, this subsystem also coordinates the
> +   multiplexer accesses.
> +
> +   To compile the subsystem as a module, choose M here: the module will
> +   be called mux-core.
> +
> +if MULTIPLEXER
> +
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB
> + help
> +   GPIO-controlled Multiplexer controller.
> +
> +   The driver builds a single multiplexer controller using a number
> +   of gpio pins. For N pins, there will be 2^N possible multiplexer
> +   states. The GPIO pins can be connected (by the hardware) to several
> +   multiplexers, which in that case will be operated in parallel.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index ..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for