* [PATCH v4 2/2] power: supply: ltc2941-battery-gauge: Add OF device ID table
From: Javier Martinez Canillas @ 2017-03-29 16:00 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, Mark Rutland, Rob Herring, Javier Martinez Canillas,
Sebastian Reichel, linux-pm
In-Reply-To: <20170329160057.8298-1-javier@osg.samsung.com>
The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:<device>.
But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
Changes in v4:
- Add a vendor prefix to OF device compatible strings (Sebastian Reichel).
Changes in v3:
- Don't use of_match_ptr() to avoid build warning when CONFIG_OF is disabled.
Changes in v2:
- Fix build warning reported by kbuild test robot.
- Fix wrong compatible strings due a copy & paste error.
drivers/power/supply/ltc2941-battery-gauge.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/ltc2941-battery-gauge.c b/drivers/power/supply/ltc2941-battery-gauge.c
index 4adf2ba021ce..7efb908f4451 100644
--- a/drivers/power/supply/ltc2941-battery-gauge.c
+++ b/drivers/power/supply/ltc2941-battery-gauge.c
@@ -9,6 +9,7 @@
*/
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of_device.h>
#include <linux/types.h>
#include <linux/errno.h>
#include <linux/swab.h>
@@ -61,7 +62,7 @@ struct ltc294x_info {
struct power_supply *supply; /* Supply pointer */
struct power_supply_desc supply_desc; /* Supply description */
struct delayed_work work; /* Work scheduler */
- int num_regs; /* Number of registers (chip type) */
+ unsigned long num_regs; /* Number of registers (chip type) */
int charge; /* Last charge register content */
int r_sense; /* mOhm */
int Qlsb; /* nAh */
@@ -387,7 +388,7 @@ static int ltc294x_i2c_probe(struct i2c_client *client,
np = of_node_get(client->dev.of_node);
- info->num_regs = id->driver_data;
+ info->num_regs = (unsigned long)of_device_get_match_data(&client->dev);
info->supply_desc.name = np->name;
/* r_sense can be negative, when sense+ is connected to the battery
@@ -497,9 +498,23 @@ static const struct i2c_device_id ltc294x_i2c_id[] = {
};
MODULE_DEVICE_TABLE(i2c, ltc294x_i2c_id);
+static const struct of_device_id ltc294x_i2c_of_match[] = {
+ {
+ .compatible = "lltc,ltc2941",
+ .data = (void *)LTC2941_NUM_REGS
+ },
+ {
+ .compatible = "lltc,ltc2943",
+ .data = (void *)LTC2943_NUM_REGS
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ltc294x_i2c_of_match);
+
static struct i2c_driver ltc294x_driver = {
.driver = {
.name = "LTC2941",
+ .of_match_table = ltc294x_i2c_of_match,
.pm = LTC294X_PM_OPS,
},
.probe = ltc294x_i2c_probe,
--
2.9.3
^ permalink raw reply related
* [PATCH v4 1/2] power: supply: ltc2941-battery-gauge: Add vendor to compatibles in binding
From: Javier Martinez Canillas @ 2017-03-29 16:00 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Rob Herring,
Javier Martinez Canillas, linux-pm-u79uwXL29TY76Z2rM5mHXA,
Sebastian Reichel
The DT binding document for LTC2941 and LTC2943 battery gauges did not use
a vendor prefix in the listed compatible strings. The driver says that the
manufacturer is Linear Technology which is "lltc" in vendor-prefixes.txt.
There isn't an upstream Device Tree source file that has nodes defined for
these devices, so there's no need to keep the old compatible strings.
Signed-off-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
Documentation/devicetree/bindings/power/supply/ltc2941.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/power/supply/ltc2941.txt b/Documentation/devicetree/bindings/power/supply/ltc2941.txt
index ea42ae12d924..a9d7aa60558b 100644
--- a/Documentation/devicetree/bindings/power/supply/ltc2941.txt
+++ b/Documentation/devicetree/bindings/power/supply/ltc2941.txt
@@ -6,8 +6,8 @@ temperature monitoring, and uses a slightly different conversion
formula for the charge counter.
Required properties:
-- compatible: Should contain "ltc2941" or "ltc2943" which also indicates the
- type of I2C chip attached.
+- compatible: Should contain "lltc,ltc2941" or "lltc,ltc2943" which also
+ indicates the type of I2C chip attached.
- reg: The 7-bit I2C address.
- lltc,resistor-sense: The sense resistor value in milli-ohms. Can be a 32-bit
negative value when the battery has been connected to the wrong end of the
@@ -20,7 +20,7 @@ Required properties:
Example from the Topic Miami Florida board:
fuelgauge: ltc2943@64 {
- compatible = "ltc2943";
+ compatible = "lltc,ltc2943";
reg = <0x64>;
lltc,resistor-sense = <15>;
lltc,prescaler-exponent = <5>; /* 2^(2*5) = 1024 */
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: imx-gpc: add i.MX6 QuadPlus compatible
From: Rob Herring @ 2017-03-29 15:59 UTC (permalink / raw)
To: Lucas Stach
Cc: Shawn Guo, Fabio Estevam, Dong Aisheng,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
patchwork-lst-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170323144418.30977-2-l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Thu, Mar 23, 2017 at 03:44:17PM +0100, Lucas Stach wrote:
> While the GPC on i.MX6QP is mostly comptible to the i.MX6Q one,
> the QuadPlus requires special workarounds for hardware erratum
> ERR009619.
>
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Documentation/devicetree/bindings/power/fsl,imx-gpc.txt | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] i2c/muxes/i2c-mux-ltc4306: LTC4306 and LTC4305 I2C multiplexer/switch
From: Rob Herring @ 2017-03-29 15:58 UTC (permalink / raw)
To: michael.hennerich
Cc: wsa, peda, mark.rutland, linus.walleij, linux-i2c, devicetree,
linux-gpio, linux-kernel
In-Reply-To: <1490278978-28357-1-git-send-email-michael.hennerich@analog.com>
On Thu, Mar 23, 2017 at 03:22:58PM +0100, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> This patch adds support for the Analog Devices / Linear Technology
> LTC4306 and LTC4305 4/2 Channel I2C Bus Multiplexer/Switches.
> The LTC4306 optionally provides two general purpose input/output pins
> (GPIOs) that can be configured as logic inputs, opendrain outputs or
> push-pull outputs via the generic GPIOLIB framework.
>
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
> .../devicetree/bindings/i2c/i2c-mux-ltc4306.txt | 61 ++++
It's preferred to split bindings to separate patch. In any case,
Acked-by: Rob Herring <robh@kernel.org>
> MAINTAINERS | 8 +
> drivers/i2c/muxes/Kconfig | 10 +
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-ltc4306.c | 365 +++++++++++++++++++++
> 5 files changed, 445 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-ltc4306.txt
> create mode 100644 drivers/i2c/muxes/i2c-mux-ltc4306.c
^ permalink raw reply
* Re: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: jacopo @ 2017-03-29 15:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
Laurent Pinchart, Chris Brandt, Rob Herring, Mark Rutland,
Russell King, Linux-Renesas, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdZ4Z6c2YDu-e6kZ8-K7Dv_tRGxZj0vAqgPBx7rWwmHU-g@mail.gmail.com>
Hi Linus,
another reply to your email, please don't feel assaulted :)
On Wed, Mar 29, 2017 at 03:22:23PM +0200, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR (1 << 3)
>
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in
> drivers/pinctrl/pinconf-generic.c
> from the (new) DT property "bidirectional" simply?
>
I can try to give you a few reasons why I don't see those flags fit in
the pin configuration flags definition.
*) those flags are used during pin multiplexing procedure only and that
procedure has a specific order to be respected:
You can have a look here:
https://www.spinics.net/lists/linux-renesas-soc/msg12793.html
In "rza1_alternate_function_conf()" function, we need to set bidir
before setting every other register.
The same applies some lines below:, the PIPC, PMC and PM register set order
has to be respected, and depends on those BIDIR and SWIO_* parameters.
This implies those configuration cannot be applied after pin muxing,
certainly not in pin_config[_group]_set() whose invocation time
is independent from pin_mux_set()'s one.
One way forward would be, every time we mux a pin, look for a pinconf group
that includes the pin we're muxing. That would happen for each pin,
for no added benefits imo.
*) as Geert already pointed out, we may need dedicated subnodes to
specify those pin configuration flags, not only because of what Chris
said, but because pinconf_generic_dt_subnode_to_map() wants "pins" or
"groups" to be there in the subnode, and in our pin multiplexing
sub-nodes we only have "pinmux" property (say: we cannot specify
pin_conf flags in the same sub-node where we describe pin
multiplexing, we always need a dedicated sub-node).
Chris and Geert gave some examples in their replies on how that would
like, and how it makes the bindings a little more complex.
*) those flags, according to Chris, won't be used in RZ/A2, and
reasonably not in any other RZ device. Do we want to add them to the
generic bindings, being them so specific to this hardware platform?
One thing I suggest considering is to get rid of those flags, at
least in bindings, and introduce 3 variants for each pin multiplexing
function identifier.
Say:
include/dt-bindings/pinctrl/r7s72100-pinctrl.h:
#define MUX_1 (1 << 16)
#define MUX_1_BIDIR (1 << 16 | 1 << 24)
#define MUX_1_SWIO_IN (1 << 16 | 2 << 24)
#define MUX_1_SWIO_OUT (1 << 16 | 3 << 24)
...
#define MUX_8 (8 << 16)
#define MUX_8_BIDIR (8 << 16 | 1 << 24)
....
this way we get rid of those extra flag values and squeeze pin id
and mux function id in a single integer, part of the "pinmux"
arguments list.
i2c_pins {
pinmux = <(PIN(1, 6) | MUX_1_BIDIR)>,
<(PIN(1, 7) | MUX_2_BIDIR)>;
};
The driver will parse the bits from [31:24] to find out if it needs
to enable some special multiplexing property, and performs
multiplexing as it is doing right now.
> > +/*
> > + * Flags used to ask software to drive the pin I/O direction overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN (1 << 4)
> > +#define SWIO_OUT (1 << 5)
>
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT
> (ignoring the argument)?
>
> In the device tree use input-enable and add a new output-enable
> (with unspecified value) with proper description and DT bindings?
>
> And if you think these have no general applicability, by the end
> of the day they are *still* pin config, not magic flags we can choose to
> toss in with the muxing, so you can do what the Qualcomm driver
> does and add custom pin configurations extending the generic
> pin config, see drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.
>
I see, but that custom pin configuration flag can be applied
independently from pin muxing procedure and it can be applied to pins
while they're configured in GPIO mode.
Our "flags" are not of that nature, and only apply to some register
setting during pinmux, as I hopefully tried to clarify above.
Thanks for time and patience in this long email thread.
j
> Yours,
> Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 1/2] PCI: Add tango MSI controller support
From: Mason @ 2017-03-29 15:50 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner
Cc: Marc Gonzalez, Bjorn Helgaas, Robin Murphy, Lorenzo Pieralisi,
Liviu Dudau, David Laight, linux-pci, Linux ARM, Thibaud Cornic,
Phuong Nguyen, LKML, DT
In-Reply-To: <0541e62a-c87a-2e69-a7d1-28f886ad04c2-GANU6spQydw@public.gmane.org>
On 29/03/2017 15:16, Mason wrote:
> But I don't understand how to get my pcie pointer back in irq_ack
> or irq_unmask, or the relevant msi. Can you throw me a clue?
Let's see... the irq_chip call-backs receive an irq_data pointer.
struct irq_data - per irq chip data passed down to chip functions
struct irq_chip - hardware interrupt chip descriptor
irq_data contains a void *chip_data member.
Can I use chip_data to stash a struct tango_pcie pointer?
/**
* irq_set_chip_data - set irq chip data for an irq
* @irq: Interrupt number
* @data: Pointer to chip specific data
*
* Set the hardware irq chip data for an irq
*/
int irq_set_chip_data(unsigned int irq, void *data)
But where would I call it? And for which irq?
Otherwise, I found by trial-and-error that I can reach pcie through
data->domain->parent->host_data
data->domain == msi_dom
msi_dom->parent == irq_dom
irq_dom->host_data == pcie
But data->irq and data->hwirq don't hold the MSI index :-(
I suppose I have to ask for some mapping?
Regards.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Suzuki K Poulose @ 2017-03-29 15:50 UTC (permalink / raw)
To: Leo Yan
Cc: Mark Rutland, linux-doc, Catalin Marinas, Michael Turquette,
Will Deacon, David Brown, linux-clk, Jonathan Corbet, Wei Xu,
Andy Gross, mike.leach, devicetree, linux-arm-msm, Rob Herring,
John Stultz, linux-soc, linux-arm-kernel, Mathieu Poirier,
Guodong Xu, Stephen Boyd, linux-kernel, sudeep.holla
In-Reply-To: <20170329103712.GA22480@leoy-linaro>
On 29/03/17 11:37, Leo Yan wrote:
> On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:
>> On 29/03/17 11:27, Leo Yan wrote:
>>> On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
>>>
>>> [...]
>>>
>>>>>>> + if (mode == EDDEVID_IMPL_NONE) {
>>>>>>> + drvdata->edpcsr_present = false;
>>>>>>> + drvdata->edcidsr_present = false;
>>>>>>> + drvdata->edvidsr_present = false;
>>>>>>> + } else if (mode == EDDEVID_IMPL_EDPCSR) {
>>>>>>> + drvdata->edpcsr_present = true;
>>>>>>> + drvdata->edcidsr_present = false;
>>>>>>> + drvdata->edvidsr_present = false;
>>>>>>> + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>>>>>>> + if (!IS_ENABLED(CONFIG_64BIT) &&
>>>>>>> + (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>>>>>>> + drvdata->edpcsr_present = false;
>>>>>>> + else
>>>>>>> + drvdata->edpcsr_present = true;
>>>>>>
>>>>>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>>>>>> common to all modes (of course which implies PCSR is present) ?
>>>>>
>>>>> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
>>>>> simplize PCSROffset value :
>>>>> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
>>>>> 0001 - No offset applies.
>>>>> 0010 - No offset applies, but do not use in AArch32 mode!
>>>>>
>>>>> So we need handle the corner case is when CPU runs AArch32 mode and
>>>>> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>>>>
>>>> I understand that reasoning. But my question is, why do we check for PCSROffset
>>>> only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or
>>>> any other mode where PCSR is present.
>>>
>>> Sorry I misunderstood your question.
>>>
>>> I made mistake when I analyzed the possbile combination for mode and
>>> PCSROffset so I thought it's the only case should handle:
>>> { EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>>
>>> Below three combinations are possible to exist; so you are right, I
>>> should move this out for the checking:
>>> { EDDEVID_IMPL_NONE, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>
>> That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
>> don't worry about anything as the functionality is missing. This should rather be:
>> EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.
>
> I think below combination doesn't really exist:
> { EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };
>
> EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
> EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.
It is not wrong to check the PCSROffset in all cases where PCSR is available, as if
we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that
would make the code a bit more cleaner. Anyways, I am not particular about this.
Suzuki
^ permalink raw reply
* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-03-29 15:41 UTC (permalink / raw)
To: Leo Yan
Cc: Mark Rutland, linux-doc, Catalin Marinas, Michael Turquette,
Will Deacon, David Brown, linux-clk, Jonathan Corbet, Wei Xu,
Andy Gross, mike.leach, devicetree, Suzuki K Poulose,
linux-arm-msm, Rob Herring, John Stultz, linux-soc,
linux-arm-kernel, Guodong Xu, Stephen Boyd, linux-kernel,
sudeep.holla
In-Reply-To: <20170329030735.GA23889@leoy-linaro>
On Wed, Mar 29, 2017 at 11:07:35AM +0800, Leo Yan wrote:
> Hi Suzuki,
>
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
> > On 25/03/17 18:23, Leo Yan wrote:
>
> [...]
>
> > Leo,
> >
> > Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the
> > idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert
> > in that area. Some minor comments below.
>
> Thanks a lot for quick reviewing :)
>
> > >Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > >---
> > > drivers/hwtracing/coresight/Kconfig | 11 +
> > > drivers/hwtracing/coresight/Makefile | 1 +
> > > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++
> > > 3 files changed, 716 insertions(+)
> > > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
> > >
> > >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> > >index 130cb21..18d7931 100644
> > >--- a/drivers/hwtracing/coresight/Kconfig
> > >+++ b/drivers/hwtracing/coresight/Kconfig
> > >@@ -89,4 +89,15 @@ config CORESIGHT_STM
> > > logging useful software events or data coming from various entities
> > > in the system, possibly running different OSs
> > >
> > >+config CORESIGHT_CPU_DEBUG
> > >+ tristate "CoreSight CPU Debug driver"
> > >+ depends on ARM || ARM64
> > >+ depends on DEBUG_FS
> > >+ help
> > >+ This driver provides support for coresight debugging module. This
> > >+ is primarily used to dump sample-based profiling registers when
> > >+ system triggers panic, the driver will parse context registers so
> > >+ can quickly get to know program counter (PC), secure state,
> > >+ exception level, etc.
> >
> > May be we should mention/warn the user about the possible caveats of using
> > this feature to help him make a better decision ? And / Or we should add a documentation
> > for it. We have collected some real good information over the discussions and
> > it is a good idea to capture it somewhere.
>
> Sure, I will add a documentation for this.
>
> [...]
>
> > >+static struct pm_qos_request debug_qos_req;
> > >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
> > >+module_param(idle_constraint, int, 0600);
> > >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU "
> > >+ "idle states (default is -1, which means have no limiation "
> > >+ "to CPU idle states; 0 means disabling all idle states; user "
> > >+ "can choose other platform dependent values so can disable "
> > >+ "specific idle states for the platform)");
> >
> > Correct me if I am wrong,
> >
> > All we want to do is disable the CPUIdle explicitly if the user knows that this
> > could be a problem to use CPU debug on his platform. So, in effect, we should
> > only be using idle_constraint = 0 or -1.
> >
> > In which case, we could make it easier for the user to tell us, either
> >
> > 0 - Don't do anything with CPUIdle (default)
> > 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle.
>
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
>
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
>
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
>
> > than explaining the miscrosecond latency etc and make the appropriate calls underneath.
> > something like (not necessarily the same name) :
> >
> > module_param(broken_with_cpuidle, bool, 0600);
> > MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on"
> > " the platform. Non-zero value implies CPUIdle has to be"
> > " explicitly disabled.",);
>
> [...]
>
> > >+ /*
> > >+ * Unfortunately the CPU cannot be powered up, so return
> > >+ * back and later has no permission to access other
> > >+ * registers. For this case, should set 'idle_constraint'
> > >+ * to ensure CPU power domain is enabled!
> > >+ */
> > >+ if (!(drvdata->edprsr & EDPRSR_PU)) {
> > >+ pr_err("%s: power up request for CPU%d failed\n",
> > >+ __func__, drvdata->cpu);
> > >+ goto out;
> > >+ }
> > >+
> > >+out_powered_up:
> > >+ debug_os_unlock(drvdata);
> >
> > Question: Do we need a matching debug_os_lock() once we are done ?
>
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> no debug_os_lock() related operations.
>
> Mike, Mathieu, could you also help confirm this?
I'm not strongly opiniated on the usage of the OS lock, hence being a little
nonchalent in the coresight-etmv4 driver.
>
> [...]
>
> > >+static void debug_init_arch_data(void *info)
> > >+{
> > >+ struct debug_drvdata *drvdata = info;
> > >+ u32 mode, pcsr_offset;
> > >+
> > >+ CS_UNLOCK(drvdata->base);
> > >+
> > >+ debug_os_unlock(drvdata);
> > >+
> > >+ /* Read device info */
> > >+ drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID);
> > >+ drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> >
> > As mentioned above, both of these registers are only need at init time to
> > figure out the flags we set here. So we could remove them.
> >
> > >+
> > >+ CS_LOCK(drvdata->base);
> > >+
> > >+ /* Parse implementation feature */
> > >+ mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
> > >+ pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> >
> >
> > >+
> > >+ if (mode == EDDEVID_IMPL_NONE) {
> > >+ drvdata->edpcsr_present = false;
> > >+ drvdata->edcidsr_present = false;
> > >+ drvdata->edvidsr_present = false;
> > >+ } else if (mode == EDDEVID_IMPL_EDPCSR) {
> > >+ drvdata->edpcsr_present = true;
> > >+ drvdata->edcidsr_present = false;
> > >+ drvdata->edvidsr_present = false;
> > >+ } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
> > >+ if (!IS_ENABLED(CONFIG_64BIT) &&
> > >+ (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
> > >+ drvdata->edpcsr_present = false;
> > >+ else
> > >+ drvdata->edpcsr_present = true;
> >
> > Sorry, I forgot why we do this check only in this mode. Shouldn't this be
> > common to all modes (of course which implies PCSR is present) ?
>
> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> simplize PCSROffset value :
> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
> 0001 - No offset applies.
> 0010 - No offset applies, but do not use in AArch32 mode!
>
> So we need handle the corner case is when CPU runs AArch32 mode and
> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>
> [...]
>
> Other suggestions are good for me, will take them in next version.
>
> Thanks,
> Leo Yan
^ permalink raw reply
* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 15:39 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
Linux-Renesas, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SG2PR06MB1165F5D4CFC50CADC609451C8A350@SG2PR06MB1165.apcprd06.prod.outlook.com>
On Wednesday, March 29, 2017, Chris Brandt wrote:
> On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > > But, what do we do for Ethernet? All the pins are "normal" except
> > > just the MDIO pin needs to be bidirectional.
> > > That's the part I'm confused by.
> > > How do we flag that just the ET_MDIO needs "bidirectional"?
> >
> > You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-
> x.dts:
> >
> > avb_pins: avb {
> > mux {
> > groups = "avb_link", "avb_phy_int", "avb_mdc",
> > "avb_mii";
> > function = "avb";
> > };
> >
> > pins_mdc {
> > groups = "avb_mdc";
> > drive-strength = <24>;
> > };
> >
> > pins_mii_tx {
> > pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> > "PIN_AVB_TD0",
> > "PIN_AVB_TD1", "PIN_AVB_TD2",
> > "PIN_AVB_TD3";
> > drive-strength = <12>;
> > };
> > };
>
> Oh, so there is a way!
>
> Thank you.
>
>
> So, for clarity:
Actually, Linus's request was to use "pinmux", not "pins".
So, it should be:
/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pinmux = <PIN(1, 6) | FUNC_1>,
<PIN(1, 7) | FUNC_1>;
bidirectional;
};
/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pinmux = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
<PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */
<PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */
<PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */
<PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */
<PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */
<PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */
<PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */
<PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */
<PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */
<PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */
<PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */
<PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */
<PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */
<PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */
<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pinmux = <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO */
bidirectional;
};
};
NOTE: "FUNC_2" can just be "2" as per Geert's request.
Chris
^ permalink raw reply
* Re: [PATCH v3 2/5] clk: sunxi-ng: add support for PRCM CCUs
From: Icenowy Zheng @ 2017-03-29 15:27 UTC (permalink / raw)
To: Michael Turquette, Maxime Ripard, Rob Herring, Chen-Yu Tsai
Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20170329104246.34204-3-icenowy-h8G6r0blFSE@public.gmane.org>
On Wed, 29 Mar 2017 18:42:43 +0800
Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> From: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
>
> SoCs after A31 has a clock controller module in the PRCM part.
>
> Support the clock controller module on H3/5 and A64 now.
>
> Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> ---
> Changes in v3:
> - Change osc32000 mux to iosc, as its frequency varies between SoCs.
> (And none of them is really 32000Hz)
> Changes in v2:
> - Replace all sun6i to sun8i, as this driver currently doesn't really
> support sun6i(A31).
> - Add osc32000 mux for ar100 clk. (Note: the frequency is proven to
> be wrong during the development of PATCH v3).
> - Rename some clocks.
> - Add gate of TWD (Trusted Watchdog). There's no reset for TWD.
> - Removed reset of PIO, which doesn't exist really.
>
> drivers/clk/sunxi-ng/Kconfig | 6 +
> drivers/clk/sunxi-ng/Makefile | 1 +
> drivers/clk/sunxi-ng/ccu-sun8i-r.c | 211
> ++++++++++++++++++++++++++++++++
> drivers/clk/sunxi-ng/ccu-sun8i-r.h | 27 ++++
> include/dt-bindings/clock/sun8i-r-ccu.h | 59 +++++++++
> include/dt-bindings/reset/sun8i-r-ccu.h | 53 ++++++++ 6 files
> changed, 357 insertions(+) create mode 100644
> drivers/clk/sunxi-ng/ccu-sun8i-r.c create mode 100644
> drivers/clk/sunxi-ng/ccu-sun8i-r.h create mode 100644
> include/dt-bindings/clock/sun8i-r-ccu.h create mode 100644
> include/dt-bindings/reset/sun8i-r-ccu.h
>
> diff --git a/drivers/clk/sunxi-ng/Kconfig
> b/drivers/clk/sunxi-ng/Kconfig index 8af8f4be8e3b..fbd3f8cd5c22 100644
> --- a/drivers/clk/sunxi-ng/Kconfig
> +++ b/drivers/clk/sunxi-ng/Kconfig
> @@ -151,4 +151,10 @@ config SUN9I_A80_CCU
> default MACH_SUN9I
> depends on MACH_SUN9I || COMPILE_TEST
>
> +config SUN8I_R_CCU
> + bool "Support for Allwinner SoCs' PRCM CCUs"
> + select SUNXI_CCU_DIV
> + select SUNXI_CCU_GATE
> + default MACH_SUN8I || (ARCH_SUNXI && ARM64)
> +
> endif
> diff --git a/drivers/clk/sunxi-ng/Makefile
> b/drivers/clk/sunxi-ng/Makefile index 6feaac0c5600..0ec02fe14c50
> 100644 --- a/drivers/clk/sunxi-ng/Makefile
> +++ b/drivers/clk/sunxi-ng/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_SUN8I_A23_CCU) +=
> ccu-sun8i-a23.o obj-$(CONFIG_SUN8I_A33_CCU) += ccu-sun8i-a33.o
> obj-$(CONFIG_SUN8I_H3_CCU) += ccu-sun8i-h3.o
> obj-$(CONFIG_SUN8I_V3S_CCU) += ccu-sun8i-v3s.o
> +obj-$(CONFIG_SUN8I_R_CCU) += ccu-sun8i-r.o
> obj-$(CONFIG_SUN9I_A80_CCU) += ccu-sun9i-a80.o
> obj-$(CONFIG_SUN9I_A80_CCU) += ccu-sun9i-a80-de.o
> obj-$(CONFIG_SUN9I_A80_CCU) += ccu-sun9i-a80-usb.o
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.c
> b/drivers/clk/sunxi-ng/ccu-sun8i-r.c new file mode 100644
> index 000000000000..ddbb34018086
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright (c) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This software is licensed under the terms of the GNU General
> Public
> + * License version 2, as published by the Free Software Foundation,
> and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#include "ccu_common.h"
> +#include "ccu_reset.h"
> +
> +#include "ccu_div.h"
> +#include "ccu_gate.h"
> +#include "ccu_mp.h"
> +#include "ccu_nm.h"
> +
> +#include "ccu-sun8i-r.h"
> +
> +static const char * const ar100_parents[] = { "osc32k", "osc24M",
> + "pll-periph0", "iosc" };
> +
> +static struct ccu_div ar100_clk = {
> + .div = _SUNXI_CCU_DIV_FLAGS(4, 2,
> CLK_DIVIDER_POWER_OF_TWO), +
> + .mux = {
> + .shift = 16,
> + .width = 2,
> +
> + .variable_prediv = {
> + .index = 2,
> + .shift = 8,
> + .width = 5,
> + },
> + },
> +
> + .common = {
> + .reg = 0x00,
> + .features = CCU_FEATURE_VARIABLE_PREDIV,
> + .hw.init = CLK_HW_INIT_PARENTS("ar100",
> + ar100_parents,
> + &ccu_div_ops,
> + 0),
> + },
> +};
> +
> +static CLK_FIXED_FACTOR(ahb0_clk, "ahb0", "ar100", 1, 1, 0);
> +
> +static struct ccu_div apb0_clk = {
> + .div = _SUNXI_CCU_DIV_FLAGS(0, 2,
> CLK_DIVIDER_POWER_OF_TWO), +
> + .common = {
> + .reg = 0x0c,
> + .hw.init = CLK_HW_INIT("apb0",
> + "ahb0",
> + &ccu_div_ops,
> + 0),
> + },
> +};
> +
> +static SUNXI_CCU_GATE(apb0_pio_clk, "apb0-pio", "apb0",
> + 0x28, BIT(0), 0);
> +static SUNXI_CCU_GATE(apb0_ir_clk, "apb0-ir", "apb0",
> + 0x28, BIT(1), 0);
> +static
> SUNXI_CCU_GATE(apb0_timer_clk, "apb0-timer", "apb0",
> + 0x28, BIT(2), 0);
> +static SUNXI_CCU_GATE(apb0_rsb_clk, "apb0-rsb", "apb0",
> + 0x28, BIT(3), 0);
> +static
> SUNXI_CCU_GATE(apb0_uart_clk, "apb0-uart", "apb0",
> + 0x28, BIT(4), 0);
> +static SUNXI_CCU_GATE(apb0_i2c_clk, "apb0-i2c", "apb0",
> + 0x28, BIT(6), 0);
> +static SUNXI_CCU_GATE(apb0_twd_clk, "apb0-twd", "apb0",
> + 0x28, BIT(7), 0);
> +
> +static const char * const r_mod0_default_parents[] = { "osc32K",
> "osc24M" }; +static SUNXI_CCU_MP_WITH_MUX_GATE(ir_clk, "ir",
> + r_mod0_default_parents, 0x54,
> + 0, 4, /* M */
> + 16, 2, /* P */
> + 24, 2, /* mux */
> + BIT(31), /* gate */
> + 0);
> +
> +static struct ccu_common *sun8i_h3_r_ccu_clks[] = {
> + &ar100_clk.common,
> + &apb0_clk.common,
> + &apb0_pio_clk.common,
> + &apb0_ir_clk.common,
> + &apb0_timer_clk.common,
> + &apb0_uart_clk.common,
> + &apb0_i2c_clk.common,
I'm sorry but I missed something here:
+ &apb0_twd_clk.common,
> + &ir_clk.common,
> +};
> +
> +static struct ccu_common *sun50i_a64_r_ccu_clks[] = {
> + &ar100_clk.common,
> + &apb0_clk.common,
> + &apb0_pio_clk.common,
> + &apb0_ir_clk.common,
> + &apb0_timer_clk.common,
> + &apb0_rsb_clk.common,
> + &apb0_uart_clk.common,
> + &apb0_i2c_clk.common,
+ &apb0_twd_clk.common,
> + &ir_clk.common,
> +};
> +
> +static struct clk_hw_onecell_data sun8i_h3_r_hw_clks = {
> + .hws = {
> + [CLK_AR100] = &ar100_clk.common.hw,
> + [CLK_AHB0] = &ahb0_clk.hw,
> + [CLK_APB0] = &apb0_clk.common.hw,
> + [CLK_APB0_PIO] =
> &apb0_pio_clk.common.hw,
> + [CLK_APB0_IR] =
> &apb0_ir_clk.common.hw,
> + [CLK_APB0_TIMER] = &apb0_timer_clk.common.hw,
> + [CLK_APB0_UART] =
> &apb0_uart_clk.common.hw,
> + [CLK_APB0_I2C] =
> &apb0_i2c_clk.common.hw,
> + [CLK_APB0_TWD] =
> &apb0_twd_clk.common.hw,
> + [CLK_IR] = &ir_clk.common.hw,
> + },
> + .num = CLK_NUMBER,
> +};
> +
> +static struct clk_hw_onecell_data sun50i_a64_r_hw_clks = {
> + .hws = {
> + [CLK_AR100] = &ar100_clk.common.hw,
> + [CLK_AHB0] = &ahb0_clk.hw,
> + [CLK_APB0] = &apb0_clk.common.hw,
> + [CLK_APB0_PIO] =
> &apb0_pio_clk.common.hw,
> + [CLK_APB0_IR] =
> &apb0_ir_clk.common.hw,
> + [CLK_APB0_TIMER] = &apb0_timer_clk.common.hw,
> + [CLK_APB0_RSB] =
> &apb0_rsb_clk.common.hw,
> + [CLK_APB0_UART] =
> &apb0_uart_clk.common.hw,
> + [CLK_APB0_I2C] =
> &apb0_i2c_clk.common.hw,
> + [CLK_APB0_TWD] =
> &apb0_twd_clk.common.hw,
> + [CLK_IR] = &ir_clk.common.hw,
> + },
> + .num = CLK_NUMBER,
> +};
> +
> +static struct ccu_reset_map sun8i_h3_r_ccu_resets[] = {
> + [RST_APB0_IR] = { 0xb0, BIT(1) },
> + [RST_APB0_TIMER] = { 0xb0, BIT(2) },
> + [RST_APB0_UART] = { 0xb0, BIT(4) },
> + [RST_APB0_I2C] = { 0xb0, BIT(6) },
> +};
> +
> +static struct ccu_reset_map sun50i_a64_r_ccu_resets[] = {
> + [RST_APB0_IR] = { 0xb0, BIT(1) },
> + [RST_APB0_TIMER] = { 0xb0, BIT(2) },
> + [RST_APB0_RSB] = { 0xb0, BIT(3) },
> + [RST_APB0_UART] = { 0xb0, BIT(4) },
> + [RST_APB0_I2C] = { 0xb0, BIT(6) },
> +};
> +
> +static const struct sunxi_ccu_desc sun8i_h3_r_ccu_desc = {
> + .ccu_clks = sun8i_h3_r_ccu_clks,
> + .num_ccu_clks = ARRAY_SIZE(sun8i_h3_r_ccu_clks),
> +
> + .hw_clks = &sun8i_h3_r_hw_clks,
> +
> + .resets = sun8i_h3_r_ccu_resets,
> + .num_resets = ARRAY_SIZE(sun8i_h3_r_ccu_resets),
> +};
> +
> +static const struct sunxi_ccu_desc sun50i_a64_r_ccu_desc = {
> + .ccu_clks = sun50i_a64_r_ccu_clks,
> + .num_ccu_clks = ARRAY_SIZE(sun50i_a64_r_ccu_clks),
> +
> + .hw_clks = &sun50i_a64_r_hw_clks,
> +
> + .resets = sun50i_a64_r_ccu_resets,
> + .num_resets = ARRAY_SIZE(sun50i_a64_r_ccu_resets),
> +};
> +
> +static void __init sunxi_r_ccu_init(struct device_node *node,
> + const struct sunxi_ccu_desc
> *desc) +{
> + void __iomem *reg;
> +
> + reg = of_io_request_and_map(node, 0,
> of_node_full_name(node));
> + if (IS_ERR(reg)) {
> + pr_err("%s: Could not map the clock registers\n",
> + of_node_full_name(node));
> + return;
> + }
> +
> + sunxi_ccu_probe(node, reg, desc);
> +}
> +
> +static void __init sun8i_h3_r_ccu_setup(struct device_node *node)
> +{
> + sunxi_r_ccu_init(node, &sun8i_h3_r_ccu_desc);
> +}
> +CLK_OF_DECLARE(sun8i_h3_r_ccu, "allwinner,sun8i-h3-r-ccu",
> + sun8i_h3_r_ccu_setup);
> +
> +static void __init sun50i_a64_r_ccu_setup(struct device_node *node)
> +{
> + sunxi_r_ccu_init(node, &sun50i_a64_r_ccu_desc);
> +}
> +CLK_OF_DECLARE(sun50i_a64_r_ccu, "allwinner,sun50i-a64-r-ccu",
> + sun50i_a64_r_ccu_setup);
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.h
> b/drivers/clk/sunxi-ng/ccu-sun8i-r.h new file mode 100644
> index 000000000000..eaa431fd1d8f
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright 2016 Icenowy <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _CCU_SUN8I_R_H
> +#define _CCU_SUN8I_R_H_
> +
> +#include <dt-bindings/clock/sun8i-r-ccu.h>
> +#include <dt-bindings/reset/sun8i-r-ccu.h>
> +
> +/* AHB/APB bus clocks are not exported */
> +#define CLK_AHB0 1
> +#define CLK_APB0 2
> +
> +#define CLK_NUMBER (CLK_APB0_TWD + 1)
> +
> +#endif /* _CCU_SUN8I_R_H */
> diff --git a/include/dt-bindings/clock/sun8i-r-ccu.h
> b/include/dt-bindings/clock/sun8i-r-ccu.h new file mode 100644
> index 000000000000..779d20aa0d05
> --- /dev/null
> +++ b/include/dt-bindings/clock/sun8i-r-ccu.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (c) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2
> of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the
> Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_SUN8I_R_CCU_H_
> +#define _DT_BINDINGS_CLK_SUN8I_R_CCU_H_
> +
> +#define CLK_AR100 0
> +
> +#define CLK_APB0_PIO 3
> +#define CLK_APB0_IR 4
> +#define CLK_APB0_TIMER 5
> +#define CLK_APB0_RSB 6
> +#define CLK_APB0_UART 7
> +/* 8 is reserved for CLK_APB0_W1 on A31 */
> +#define CLK_APB0_I2C 9
> +#define CLK_APB0_TWD 10
> +
> +#define CLK_IR 11
> +
> +#endif /* _DT_BINDINGS_CLK_SUN8I_R_CCU_H_ */
> diff --git a/include/dt-bindings/reset/sun8i-r-ccu.h
> b/include/dt-bindings/reset/sun8i-r-ccu.h new file mode 100644
> index 000000000000..4ba64f3d6fc9
> --- /dev/null
> +++ b/include/dt-bindings/reset/sun8i-r-ccu.h
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright (C) 2016 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2
> of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the
> Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _DT_BINDINGS_RST_SUN8I_R_CCU_H_
> +#define _DT_BINDINGS_RST_SUN8I_R_CCU_H_
> +
> +#define RST_APB0_IR 0
> +#define RST_APB0_TIMER 1
> +#define RST_APB0_RSB 2
> +#define RST_APB0_UART 3
> +/* 4 is reserved for RST_APB0_W1 on A31 */
> +#define RST_APB0_I2C 5
> +
> +#endif /* _DT_BINDINGS_RST_SUN8I_R_CCU_H_ */
^ permalink raw reply
* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 15:18 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
Linux-Renesas, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdVx_fyGsw0p-s4ZAoe2k+r0RuwxjwNV_-8fXY7Yv8y3kQ@mail.gmail.com>
Hi Geert,
On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> > But, what do we do for Ethernet? All the pins are "normal" except just
> > the MDIO pin needs to be bidirectional.
> > That's the part I'm confused by.
> > How do we flag that just the ET_MDIO needs "bidirectional"?
>
> You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:
>
> avb_pins: avb {
> mux {
> groups = "avb_link", "avb_phy_int", "avb_mdc",
> "avb_mii";
> function = "avb";
> };
>
> pins_mdc {
> groups = "avb_mdc";
> drive-strength = <24>;
> };
>
> pins_mii_tx {
> pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC",
> "PIN_AVB_TD0",
> "PIN_AVB_TD1", "PIN_AVB_TD2",
> "PIN_AVB_TD3";
> drive-strength = <12>;
> };
> };
Oh, so there is a way!
Thank you.
So, for clarity:
/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
mux {
pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
<PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */
<PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */
<PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */
<PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */
<PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */
<PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */
<PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */
<PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */
<PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */
<PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */
<PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */
<PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */
<PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */
<PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */
<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
};
pins_bidir {
pins = <PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO */
bidirectional;
};
};
Chris
^ permalink raw reply
* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mike Leach @ 2017-03-29 15:17 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Jonathan Corbet, Rob Herring, Mark Rutland,
Wei Xu, Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Michael Turquette, Stephen Boyd, Mathieu Poirier, Guodong Xu,
John Stultz, linux-doc, linux-kernel, devicetree,
linux-arm-kernel, linux-arm-msm, linux-soc, linux-clk,
Sudeep Holla <sudee>
In-Reply-To: <20170329030735.GA23889@leoy-linaro>
On 29 March 2017 at 04:07, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Suzuki,
>
> On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote:
>> On 25/03/17 18:23, Leo Yan wrote:
>
> [...]
>
>> Leo,
>>
>> Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the
>> idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert
>> in that area. Some minor comments below.
>
> Thanks a lot for quick reviewing :)
>
>> >Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> >---
>> > drivers/hwtracing/coresight/Kconfig | 11 +
>> > drivers/hwtracing/coresight/Makefile | 1 +
>> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++
>> > 3 files changed, 716 insertions(+)
>> > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>> >
>> >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> >index 130cb21..18d7931 100644
>> >--- a/drivers/hwtracing/coresight/Kconfig
>> >+++ b/drivers/hwtracing/coresight/Kconfig
>> >@@ -89,4 +89,15 @@ config CORESIGHT_STM
>> > logging useful software events or data coming from various entities
>> > in the system, possibly running different OSs
>> >
>> >+config CORESIGHT_CPU_DEBUG
>> >+ tristate "CoreSight CPU Debug driver"
>> >+ depends on ARM || ARM64
>> >+ depends on DEBUG_FS
>> >+ help
>> >+ This driver provides support for coresight debugging module. This
>> >+ is primarily used to dump sample-based profiling registers when
>> >+ system triggers panic, the driver will parse context registers so
>> >+ can quickly get to know program counter (PC), secure state,
>> >+ exception level, etc.
>>
>> May be we should mention/warn the user about the possible caveats of using
>> this feature to help him make a better decision ? And / Or we should add a documentation
>> for it. We have collected some real good information over the discussions and
>> it is a good idea to capture it somewhere.
>
> Sure, I will add a documentation for this.
>
> [...]
>
>> >+static struct pm_qos_request debug_qos_req;
>> >+static int idle_constraint = PM_QOS_DEFAULT_VALUE;
>> >+module_param(idle_constraint, int, 0600);
>> >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU "
>> >+ "idle states (default is -1, which means have no limiation "
>> >+ "to CPU idle states; 0 means disabling all idle states; user "
>> >+ "can choose other platform dependent values so can disable "
>> >+ "specific idle states for the platform)");
>>
>> Correct me if I am wrong,
>>
>> All we want to do is disable the CPUIdle explicitly if the user knows that this
>> could be a problem to use CPU debug on his platform. So, in effect, we should
>> only be using idle_constraint = 0 or -1.
>>
>> In which case, we could make it easier for the user to tell us, either
>>
>> 0 - Don't do anything with CPUIdle (default)
>> 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle.
>
> The reason for not using bool flag is: usually SoC may have many idle
> states, so if user wants to partially enable some states then can set
> the latency to constraint.
>
> But of course, we can change this to binary value as you suggested,
> this means turn on of turn off all states. The only one reason to use
> latency value is it is more friendly for hardware design, e.g. some
> platforms can enable partial states to save power and avoid overheat
> after using this driver.
>
> If you guys think this is a bit over design, I will follow up your
> suggestion. I also have some replying in Mathieu's reviewing, please
> help review as well.
>
>> than explaining the miscrosecond latency etc and make the appropriate calls underneath.
>> something like (not necessarily the same name) :
>>
>> module_param(broken_with_cpuidle, bool, 0600);
>> MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on"
>> " the platform. Non-zero value implies CPUIdle has to be"
>> " explicitly disabled.",);
>
> [...]
>
>> >+ /*
>> >+ * Unfortunately the CPU cannot be powered up, so return
>> >+ * back and later has no permission to access other
>> >+ * registers. For this case, should set 'idle_constraint'
>> >+ * to ensure CPU power domain is enabled!
>> >+ */
>> >+ if (!(drvdata->edprsr & EDPRSR_PU)) {
>> >+ pr_err("%s: power up request for CPU%d failed\n",
>> >+ __func__, drvdata->cpu);
>> >+ goto out;
>> >+ }
>> >+
>> >+out_powered_up:
>> >+ debug_os_unlock(drvdata);
>>
>> Question: Do we need a matching debug_os_lock() once we are done ?
>
> I have checked ARM ARMv8, but there have no detailed description for
> this. I refered coresight-etmv4 code and Mike's pseudo code, ther have
> no debug_os_lock() related operations.
>
> Mike, Mathieu, could you also help confirm this?
>
Debug OS lock / unlock allows the power management code running on the
core to lock out the external debugger while the debug registers are
saved/restored during a core power event.
e.g. A sequence such as this might occur in a correctly programmed system....
debug_os_lock()
save_debug_regs() // visible from core power domain - incl breakpoints etc
save_etm_regs()
... // other stuff prior to core power down,
<power_down_core>
Followed by...
<power_up_core>
restore_etm_regs()
restore_debug_regs() // visible from core power domain - incl breakpoints etc
debug_os_unlock()
The value is 1 (locked) if cold resetting into AArch64 - it is
expected that some system software will set this to 0 as part of the
boot process.
The lock prevents write access to the external debug registers so we
need to clear it to set up the external debug registers we are using.
This suggests that it should be restored as we found it when done.
Mike
> [...]
>
>> >+static void debug_init_arch_data(void *info)
>> >+{
>> >+ struct debug_drvdata *drvdata = info;
>> >+ u32 mode, pcsr_offset;
>> >+
>> >+ CS_UNLOCK(drvdata->base);
>> >+
>> >+ debug_os_unlock(drvdata);
>> >+
>> >+ /* Read device info */
>> >+ drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID);
>> >+ drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
>>
>> As mentioned above, both of these registers are only need at init time to
>> figure out the flags we set here. So we could remove them.
>>
>> >+
>> >+ CS_LOCK(drvdata->base);
>> >+
>> >+ /* Parse implementation feature */
>> >+ mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
>> >+ pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
>>
>>
>> >+
>> >+ if (mode == EDDEVID_IMPL_NONE) {
>> >+ drvdata->edpcsr_present = false;
>> >+ drvdata->edcidsr_present = false;
>> >+ drvdata->edvidsr_present = false;
>> >+ } else if (mode == EDDEVID_IMPL_EDPCSR) {
>> >+ drvdata->edpcsr_present = true;
>> >+ drvdata->edcidsr_present = false;
>> >+ drvdata->edvidsr_present = false;
>> >+ } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>> >+ if (!IS_ENABLED(CONFIG_64BIT) &&
>> >+ (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>> >+ drvdata->edpcsr_present = false;
>> >+ else
>> >+ drvdata->edpcsr_present = true;
>>
>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>> common to all modes (of course which implies PCSR is present) ?
>
> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
> simplize PCSROffset value :
> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
> 0001 - No offset applies.
> 0010 - No offset applies, but do not use in AArch32 mode!
>
> So we need handle the corner case is when CPU runs AArch32 mode and
> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>
> [...]
>
> Other suggestions are good for me, will take them in next version.
>
> Thanks,
> Leo Yan
--
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK
^ permalink raw reply
* Re: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Geert Uytterhoeven @ 2017-03-29 14:59 UTC (permalink / raw)
To: Chris Brandt
Cc: Linus Walleij, Jacopo Mondi, Bjorn Andersson, Geert Uytterhoeven,
Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
Linux-Renesas, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SG2PR06MB11659AB956F83E8DCE2C9AA18A350@SG2PR06MB1165.apcprd06.prod.outlook.com>
Hi Chris,
On Wed, Mar 29, 2017 at 4:55 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Wednesday, March 29, 2017, Linus Walleij wrote:
>> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> > +/*
>> > + * Flags used to ask software to drive the pin I/O direction
>> > +overriding the
>> > + * alternate function configuration.
>> > + * Some alternate functions require software to force I/O direction
>> > +of a pin,
>> > + * overriding the designated one.
>> > + * Refer to the HW manual to know when this flag shall be used.
>> > + */
>> > +#define SWIO_IN (1 << 4)
>> > +#define SWIO_OUT (1 << 5)
>>
>> What is wrong in doing this with generic pin config using
>> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
>>
>> In the device tree use input-enable and add a new output-enable (with
>> unspecified value) with proper description and DT bindings?
>
> Again, that's probably fine. It seems we are still doing the same thing
> which is using the DT to pass extra config information to the driver.
> And, we can do whatever we want with that info.
>
>
>> And if you think these have no general applicability, by the end of the
>> day they are *still* pin config, not magic flags we can choose to toss in
>> with the muxing, so you can do what the Qualcomm driver does and add
>> custom pin configurations extending the generic pin config, see
>> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>> qcom,pull-up-strength etc.
>
> But, it seems that when you set a config option, it applies to everything
> in "pins"?
>
> I2C Example: (seem OK)
> /* P1_6 = RIIC3SCL (bi dir) */
> /* P1_7 = RIIC3SDA (bi dir) */
> i2c3_pins: i2c3 {
> pins = <PIN(1, 6) | FUNC_1>,
> <PIN(1, 7) | FUNC_1>;
> bidirectional;
> };
Correct.
> But, what do we do for Ethernet? All the pins are "normal" except just
> the MDIO pin needs to be bidirectional.
> That's the part I'm confused by.
> How do we flag that just the ET_MDIO needs "bidirectional"?
You add subnodes, cfr. arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts:
avb_pins: avb {
mux {
groups = "avb_link", "avb_phy_int", "avb_mdc",
"avb_mii";
function = "avb";
};
pins_mdc {
groups = "avb_mdc";
drive-strength = <24>;
};
pins_mii_tx {
pins = "PIN_AVB_TX_CTL", "PIN_AVB_TXC", "PIN_AVB_TD0",
"PIN_AVB_TD1", "PIN_AVB_TD2", "PIN_AVB_TD3";
drive-strength = <12>;
};
};
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v5 6/9] coresight: add support for CPU debug module
From: Mike Leach @ 2017-03-29 14:56 UTC (permalink / raw)
To: Leo Yan
Cc: Mathieu Poirier, Jonathan Corbet, Rob Herring, Mark Rutland,
Wei Xu, Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Michael Turquette, Stephen Boyd, Guodong Xu, John Stultz,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Suzuki K. Poulose, Sudeep
In-Reply-To: <20170329015423.GA5035@leoy-linaro>
Hi Leo
On 29 March 2017 at 02:54, Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Mathieu,
>
> On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote:
>> On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote:
>
> [...]
>
>> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
>> > +{
>> > + int timeout = DEBUG_WAIT_TIMEOUT;
>> > +
>> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +
>> > + CS_UNLOCK(drvdata->base);
>> > +
>> > + /* Bail out if CPU is powered up yet */
>> > + if (drvdata->edprsr & EDPRSR_PU)
>> > + goto out_powered_up;
>> > +
>> > + /*
>> > + * Send request to power management controller and assert
>> > + * DBGPWRUPREQ signal; if power management controller has
>> > + * sane implementation, it should enable CPU power domain
>> > + * in case CPU is in low power state.
>> > + */
>> > + drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > + drvdata->edprsr |= EDPRCR_COREPURQ;
>> > + writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> Here ->edprsr is used but EDPRCR is accessed. Is this intentional or a
>> copy/paste error? The same is true for accesses in the out_powered_up section.
>
> Thanks for pointing out. This is a typo error and will fix.
>
>> > +
>> > + /* Wait for CPU to be powered up (timeout~=32ms) */
>> > + while (timeout--) {
>> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > + if (drvdata->edprsr & EDPRSR_PU)
>> > + break;
>> > +
>> > + usleep_range(1000, 2000);
>> > + }
>>
>> See if function readx_poll_timeout() can be used.
>
> Will use it.
>
>> > +
>> > + /*
>> > + * Unfortunately the CPU cannot be powered up, so return
>> > + * back and later has no permission to access other
>> > + * registers. For this case, should set 'idle_constraint'
>> > + * to ensure CPU power domain is enabled!
>> > + */
>> > + if (!(drvdata->edprsr & EDPRSR_PU)) {
>> > + pr_err("%s: power up request for CPU%d failed\n",
>> > + __func__, drvdata->cpu);
>> > + goto out;
>> > + }
>> > +
>> > +out_powered_up:
>> > + debug_os_unlock(drvdata);
>> > +
>> > + /*
>> > + * At this point the CPU is powered up, so set the no powerdown
>> > + * request bit so we don't lose power and emulate power down.
>> > + */
>> > + drvdata->edprsr = readl(drvdata->base + EDPRCR);
>> > + drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
>>
>> If we are here the core is already up. Shouldn't we need to set
>> EDPRCR_CORENPDRQ only?
>
> Yeah. Will fix.
No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes
EDPRCR_COREPURQ is in the debug power domain an is tied to an external
debug request that should be an input to the external (to the PE)
system power controller.
The requirement is that the system power controller powers up the core
domain and does not power it down while it remains asserted.
EDPRCR_CORENPDRQ is in the core power domain and thus to the specific
core only. This ensures that any power control software running on
that core should emulate a power down if this is set to one.
We cannot know the power control design of the system, so the safe
solution is to set both bits.
Mike
>
>> > + writel(drvdata->edprsr, drvdata->base + EDPRCR);
>>
>> This section is a little racy - between the time the PU bit has been
>> checked and the time COREPDRQ has been flipped, the state of PU may have
>> changed. You can probably get around this by checking edprsr.PU rigth here. If
>> it is not set you go through the process again. Note that doing this will
>> probably force a refactoring of the whole function.
>
> Agree. Will handle this.
>
> [...]
>
>> > +static ssize_t debug_func_knob_write(struct file *f,
>> > + const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > e u8 on;
>> > + int ret;
>> > +
>> > + ret = kstrtou8_from_user(buf, count, 2, &on);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + mutex_lock(&debug_lock);
>> > +
>> > + if (!on ^ debug_enable)
>> > + goto out;
>>
>> I had to read this condition too many times - please refactor.
>
> Will do it.
>
>> > +
>> > + if (on) {
>> > + ret = debug_enable_func();
>> > + if (ret) {
>> > + pr_err("%s: unable to disable debug function: %d\n",
>> > + __func__, ret);
>>
>> Based on the semantic this is the wrong error message.
>
> Yeah. Will fix.
>
>> > + goto err;
>> > + }
>> > + } else
>> > + debug_disable_func();
>>
>> Did checkpatch.pl complain about extra curly braces? If not please add them.
>
> checkpatch.pl doesn't report for this. Will add.
>
>> > +
>> > + debug_enable = on;
>>
>> Here we can't set debug_enable if we just called debug_disable_func(). Maybe
>> I'm missing something. If that's the case a comment in the code would be worth
>> it.
>
> After called debug_disable_func(), debug_enable is set to 0 (on = 0).
>
>> > +
>> > +out:
>> > + ret = count;
>> > +err:
>> > + mutex_unlock(&debug_lock);
>> > + return ret;
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_read(struct file *f,
>> > + char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > + char val[] = { '0' + debug_enable, '\n' };
>> > +
>> > + return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val));
>>
>> Use the debug_lock to avoid race conditions.
>
> Will do it.
>
>> > +}
>> > +
>> > +static ssize_t debug_idle_constraint_write(struct file *f,
>> > + const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > + int val;
>> > + ssize_t ret;
>> > +
>> > + ret = kstrtoint_from_user(buf, count, 10, &val);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + mutex_lock(&debug_lock);
>> > + idle_constraint = val;
>> > +
>> > + if (debug_enable)
>> > + pm_qos_update_request(&debug_qos_req, idle_constraint);
>> > +
>> > + mutex_unlock(&debug_lock);
>> > + return count;
>> > +}
>> > +
>> > +static ssize_t debug_idle_constraint_read(struct file *f,
>> > + char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > + char buf[32];
>> > + int len;
>> > +
>> > + if (*ppos)
>> > + return 0;
>> > +
>> > + len = sprintf(buf, "%d\n", idle_constraint);
>> > + return simple_read_from_buffer(ubuf, count, ppos, buf, len);
>>
>> Use the debug_lock to avoid race conditions.
>
> Will do it.
>
>> > +}
>> > +
>> > +static const struct file_operations debug_func_knob_fops = {
>> > + .open = simple_open,
>> > + .read = debug_func_knob_read,
>> > + .write = debug_func_knob_write,
>> > +};
>> > +
>> > +static const struct file_operations debug_idle_constraint_fops = {
>> > + .open = simple_open,
>> > + .read = debug_idle_constraint_read,
>> > + .write = debug_idle_constraint_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;
>>
>> return PTR_ERR(debug_debugfs_dir);
>
> Here cannot use PTR_ERR(debug_debugfs_dir). If create debugfs failed
> the pointer is NULL value, so finally we will return zero value for
> PTR_ERR(debug_debugfs_dir).
>
> [...]
>
>> > + }
>> > +
>> > + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
>> > + debug_debugfs_dir, NULL, &debug_func_knob_fops);
>> > + if (!file) {
>> > + pr_err("%s: unable to create enable knob file\n", __func__);
>> > + ret = -ENOMEM;
>>
>> Same as above.
>>
>> > + goto err;
>> > + }
>> > +
>> > + file = debugfs_create_file("idle_constraint", S_IRUGO | S_IWUSR,
>> > + debug_debugfs_dir, NULL, &debug_idle_constraint_fops);
>> > + if (!file) {
>> > + pr_err("%s: unable to create idle constraint file\n", __func__);
>> > + ret = -ENOMEM;
>>
>> Same as above.
>>
>> > + goto err;
>> > + }
>> > +
>> > + /* Use sysfs node to enable functionality */
>> > + if (!debug_enable)
>> > + return 0;
>> > +
>> > + /* Enable debug module at boot time */
>> > + ret = debug_enable_func();
>> > + if (ret) {
>> > + pr_err("%s: unable to disable debug function: %d\n",
>> > + __func__, ret);
>> > + goto err;
>> > + }
>>
>> Use the debug_lock to avoid race conditions.
>
> I'm struggling to understand what's race condition at here? The
> function pairs debug_func_init()/debug_func_exit() are used for
> module's probing and removing, so naturally module's probing and
> removing are sequential, right?
>
>> > +
>> > + return 0;
>> > +
>> > +err:
>> > + debugfs_remove_recursive(debug_debugfs_dir);
>> > + return ret;
>> > +}
>> > +
>> > +static void debug_func_exit(void)
>> > +{
>> > + debugfs_remove_recursive(debug_debugfs_dir);
>> > +
>> > + /* Disable functionality if has enabled */
>> > + if (debug_enable)
>> > + debug_disable_func();
>> > +}
>> > +
>> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>> > +{
>> > + void __iomem *base;
>> > + struct device *dev = &adev->dev;
>> > + struct debug_drvdata *drvdata;
>> > + struct resource *res = &adev->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's drvdata has been initialized\n");
>>
>> Might be worth adding the CPU number in the error message.
>
> Yeah, will add it.
>
> [...]
>
>> This driver doesn't call the pm_runtime_put/get() operations needed to handle the
>> debug power domain. See the other CoreSight drivers for details.
>
> Sure, will do it.
>
>> Also, from the conversation that followed the previous post we agreed that we wouldn't
>> deal with CPUidle issues in this driver. We deal with the CPU power domain
>> using the EDPRCR register (like you did) and that's it. System that don't honor that register
>> can use other (external) means to solve this. As such please remove the
>> pm_qos_xyz() functions.
>
> From previous discussion, Mike reminds the CPU power domain design is
> quite SoC specific and usually the SoC has many different low power
> states, e.g. except CPU level and cluster level low power states, they
> also can define SoC level low power states. Any SoC with any power
> state is possible finally impact CPU power domain, so this is why I add
> this interface to let user can have the final decision based on their
> working platform.
>
> We can rely on "nohlt" and "cpuidle.off=1" in kernel command line to
> disable whole SoC low power states at boot time; or we can use sysfs
> node "echo 1 > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disble" to
> disable CPU low power states at runtime. But that means we need use
> different interfaces to control CPU power domain for booting and
> runtime, it's not nice for usage.
>
> So this is why add "idle_constraint" as a central place to control
> power domain for CPU debug purpose and I also think this is more
> friendly for hardware design, e.g. some platforms can enable partial
> low power states to save power and avoid overheat after using this
> driver.
>
> How about you think for this?
>
> Thanks,
> Leo Yan
--
Mike Leach
Principal Engineer, ARM Ltd.
Blackburn Design Centre. UK
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl header
From: Chris Brandt @ 2017-03-29 14:55 UTC (permalink / raw)
To: Linus Walleij, Jacopo Mondi, Bjorn Andersson
Cc: Geert Uytterhoeven, Laurent Pinchart, Rob Herring, Mark Rutland,
Russell King, Linux-Renesas, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdZ4Z6c2YDu-e6kZ8-K7Dv_tRGxZj0vAqgPBx7rWwmHU-g@mail.gmail.com>
On Wednesday, March 29, 2017, Linus Walleij wrote:
> On Fri, Mar 24, 2017 at 4:22 PM, Jacopo Mondi <jacopo+renesas@jmondi.org>
> wrote:
>
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> > +/*
> > + * Pin is bi-directional.
> > + * An alternate function that needs both input/output functionalities
> > +shall
> > + * be configured as bidirectional.
> > + * Eg. SDA/SCL pins of an I2c interface.
> > + */
> > +#define BI_DIR (1 << 3)
>
> Any specific reason why this should not simply be added to
> include/linux/pinctrl/pinconf-generic.h
> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> generic.c from the (new) DT property "bidirectional" simply?
I see your point. It would cut down from every driver out there
inventing some new property or config instead of everyone just sharing
a fixed set.
Maybe someone else out there will end up having a need for a
"bidirectional" option.
> > +/*
> > + * Flags used to ask software to drive the pin I/O direction
> > +overriding the
> > + * alternate function configuration.
> > + * Some alternate functions require software to force I/O direction
> > +of a pin,
> > + * overriding the designated one.
> > + * Refer to the HW manual to know when this flag shall be used.
> > + */
> > +#define SWIO_IN (1 << 4)
> > +#define SWIO_OUT (1 << 5)
>
> What is wrong in doing this with generic pin config using
> PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT (ignoring the argument)?
>
> In the device tree use input-enable and add a new output-enable (with
> unspecified value) with proper description and DT bindings?
Again, that's probably fine. It seems we are still doing the same thing
which is using the DT to pass extra config information to the driver.
And, we can do whatever we want with that info.
> And if you think these have no general applicability, by the end of the
> day they are *still* pin config, not magic flags we can choose to toss in
> with the muxing, so you can do what the Qualcomm driver does and add
> custom pin configurations extending the generic pin config, see
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> qcom,pull-up-strength etc.
But, it seems that when you set a config option, it applies to everything
in "pins"?
I2C Example: (seem OK)
/* P1_6 = RIIC3SCL (bi dir) */
/* P1_7 = RIIC3SDA (bi dir) */
i2c3_pins: i2c3 {
pins = <PIN(1, 6) | FUNC_1>,
<PIN(1, 7) | FUNC_1>;
bidirectional;
};
But, what do we do for Ethernet? All the pins are "normal" except just
the MDIO pin needs to be bidirectional.
That's the part I'm confused by.
How do we flag that just the ET_MDIO needs "bidirectional"?
/* Ethernet */
ether_pins: ether {
/* Ethernet on Ports 1,2,3,5 */
pins = <PIN(1, 14) | FUNC_4)>, /* P1_14 = ET_COL */
<PIN(5, 9) | FUNC_2)>, /* P5_9 = ET_MDC */
<PIN(3, 3) | FUNC_2)>, /* P3_3 = ET_MDIO !!!! (bi dir) !!!!!! */
<PIN(3, 4) | FUNC_2)>, /* P3_4 = ET_RXCLK */
<PIN(3, 5) | FUNC_2)>, /* P3_5 = ET_RXER */
<PIN(3, 6) | FUNC_2)>, /* P3_6 = ET_RXDV */
<PIN(2, 0) | FUNC_2)>, /* P2_0 = ET_TXCLK */
<PIN(2, 1) | FUNC_2)>, /* P2_1 = ET_TXER */
<PIN(2, 2) | FUNC_2)>, /* P2_2 = ET_TXEN */
<PIN(2, 3) | FUNC_2)>, /* P2_3 = ET_CRS */
<PIN(2, 4) | FUNC_2)>, /* P2_4 = ET_TXD0 */
<PIN(2, 5) | FUNC_2)>, /* P2_5 = ET_TXD1 */
<PIN(2, 6) | FUNC_2)>, /* P2_6 = ET_TXD2 */
<PIN(2, 7) | FUNC_2)>, /* P2_7 = ET_TXD3 */
<PIN(2, 8) | FUNC_2)>, /* P2_8 = ET_RXD0 */
<PIN(2, 9) | FUNC_2)>, /* P2_9 = ET_RXD1 */
<PIN(2, 10) | FUNC_2)>, /* P2_10 = ET_RXD2 */
<PIN(2, 11) | FUNC_2)>; /* P2_11 = ET_RXD3 */
};
Chris
^ permalink raw reply
* RE: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support
From: David Laight @ 2017-03-29 14:38 UTC (permalink / raw)
To: 'Robin Murphy', Mason
Cc: Marc Gonzalez, Bjorn Helgaas, Marc Zyngier, Thomas Gleixner,
Lorenzo Pieralisi, Liviu Dudau, linux-pci, Linux ARM,
Thibaud Cornic, Phuong Nguyen, LKML, DT
In-Reply-To: <84a8b26d-7d24-df29-3959-45fce880916d@arm.com>
> > For my education, what is the API to send an IPI?
> > And the API to handle an IPI?
>
> There are a few ways you could implement some custom cross-call,
> although in this case I think stop_machine() would probably be the most
> appropriate candidate. However, you're right that in general it may not
> actually help enough to be worthwhile - a DSB SY would ensure that
> in-flight transactions have at least been observed by the CPUs and any
> other coherent masters, but for any writes with a memory type allowing
> early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
> doesn't necessarily correlate with them having reached their ultimate
> destination. For a PCI destination in particular, I think the normal way
> to ensure all posted writes have completed would be to read from config
> space; ah...
He almost certainly doesn't need to wait for the cycle to complete,
just long enough for the cycle to have been sent.
David
^ permalink raw reply
* Re: [PATCH v7 1/7] clocksource/drivers/clksrc-evt-probe: Describe with the DT both the clocksource and the clockevent
From: Mark Rutland @ 2017-03-29 14:34 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rob Herring, Alexander Kochetkov, Heiko Stuebner,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Thomas Gleixner,
Russell King, Caesar Wang, Huang Tao
In-Reply-To: <20170329134134.GJ2123@mai>
On Wed, Mar 29, 2017 at 03:41:34PM +0200, Daniel Lezcano wrote:
> On Wed, Mar 29, 2017 at 01:57:14PM +0100, Mark Rutland wrote:
> > On Wed, Mar 29, 2017 at 02:36:38PM +0200, Daniel Lezcano wrote:
> > > arch/arc/boot/dts/abilis_tb10x.dtsi:
> > >
> > > /* TIMER0 with interrupt for clockevent */
> > > timer0 {
> > > compatible = "snps,arc-timer";
> > > interrupts = <3>;
> > > interrupt-parent = <&intc>;
> > > clocks = <&cpu_clk>;
> > > };
> > >
> > > /* TIMER1 for free running clocksource */
> > > timer1 {
> > > compatible = "snps,arc-timer";
> > > clocks = <&cpu_clk>;
> > > };
> > >
> > > drivers/clocksource/arc_timer.c:
> > >
> > > static int __init arc_of_timer_init(struct device_node *np)
> > > {
> > > static int init_count = 0;
> > > int ret;
> > >
> > > if (!init_count) {
> > > init_count = 1;
> > > ret = arc_clockevent_setup(np);
> > > } else {
> > > ret = arc_cs_setup_timer1(np);
> > > }
> > >
> > > return ret;
> > > }
> > >
> > > So the purpose of these changes is to provide a stronger timer declaration in
> > > order to clearly split in the kernel a clocksource and a clockevent
> > > initialization.
> >
> > I agree that this pattern is not nice. However, I think that splitting
> > devices as this level makes the problem *worse*.
> >
> > Users care that they have a clocksource and a clockevent device. They
> > do not care *which* particular device is used as either. The comments in
> > the DT above are at best misleading.
>
> Agree.
>
> And the driver is assuming the first node is the clockevent and the second one
> is the clocksource. If the DT invert these nodes, that breaks the driver.
Sure, but that is something we can and should fix within Linux.
> > What we need is for the kernel to understand that devices can be both
> > clockevent and clocksource (perhaps mutually exclusively), such that the
> > kernel can decide how to make use of devices.
> >
> > That way, for the above the kernel can figure out that timer0 could be
> > used as clocksource or clockevent, while timer1 can only be used as a
> > clocksource due to the lack of an interrupt. Thus, it can choose to use
> > timer0 as a clockevent, and timer1 and a clocksource.
>
> Well, 'interrupt' gives an indication the timer can be used as a clockevent and
> clocksource, not the clockevent only.
Which is exactly what I said above, when I said:
the kernel can figure out that timer0 could be used as
clocksource or clockevent
Considering both timer0 and timer1 is how we can figure out timer0 must
be the clockevent, since timer1 cannot be.
> If we take the case of the rockchip, the arm_arch_timer clocksource is stopped
> when the CPU is clock gated. So specifically, we don't want to use this
> clocksource but we want to use the arch clockevents because they are better.
Sure. As I pointed out, we want to consider the holistic details to make
the right decision. i.e. the infrastructure should make the choice, not
the individual drivers.
Consider that the kernel may need to make decisions that differ it a
kernel is built wihout certain drivers. That cannot work if the use is
allocated in the DT.
[...]
> > > 2. A node can have a clockevent and|or a clocksource attributes
> >
> > As above, this should not be in the DT given it's describing a
> > (Linux-specific) SW policy and not a HW detail.
> >
> > So I must disagree with this.
>
> IIUC my discussion with Rob, an attribute is acceptable (btw if
> 'clocksource'|'clockevent' names are too Linux specific (+1), what
> about a more generic name like 'tick' and 'time' ?).
The *meaning* of these is Linux specific. The naming is irrelevant.
> > > 3. The timer_probe pass a flag to the driver's init function, so this one knows
> > > if it should invoke the clockevent/clocksource init functions.
> > > No attribute defaults to clocksource|clockevent.
> > >
> > > That would be backward compatible and will let to create drivers with clutch
> > > activated device via DT. Also, it will give the opportunity to the existing
> > > drivers to change consolidate their initialization routines.
> >
> > I think that if anything, we need a combined clocksource+clockevent
> > device that we register to the core code. That means all
> > clocksource/clockevent drivers have a consolidated routine.
> >
> > Subsequently, core code should determine how specifically to use the
> > device (e.g. based on what other devices are registered, and their
> > capabilities).
>
> IMO, the core code is complex enough and that may imply more heuristics.
Given that the majority of cases are going to be multiple instances of
the same IP, I cannot imagine it is complex to get something that works.
A generally optimal configuration may require some heuristics, but
that's a different matter to a correct and functional configuration.
I must disagree with trying to push that complexity into the DT by means
of static SW policy, rather than solving the problem in SW where we have
all the information to do so.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support
From: Robin Murphy @ 2017-03-29 14:33 UTC (permalink / raw)
To: Mason
Cc: Marc Gonzalez, Bjorn Helgaas, Marc Zyngier, Thomas Gleixner,
Lorenzo Pieralisi, Liviu Dudau, David Laight, linux-pci,
Linux ARM, Thibaud Cornic, Phuong Nguyen, LKML, DT
In-Reply-To: <96da696c-141a-3e8d-1fb7-2c024a295f58@free.fr>
On 29/03/17 13:53, Mason wrote:
> On 29/03/2017 14:19, Robin Murphy wrote:
>
>> On 29/03/17 12:34, Marc Gonzalez wrote:
>>
>>> + /*
>>> + * QUIRK #3
>>> + * Unfortunately, config and mem spaces are muxed.
>>> + * Linux does not support such a setting, since drivers are free
>>> + * to access mem space directly, at any time.
>>> + * Therefore, we can only PRAY that config and mem space accesses
>>> + * NEVER occur concurrently.
>>> + */
>>
>> What about David's suggestion of using an IPI for safe mutual exclusion?
>
> I was left with the impression that this wouldn't solve the problem.
> If a mem space access is "in flight" on core0 when core1 starts a
> config space access, an IPI will not prevent breakage.
>
> Did I misunderstand?
>
> For my education, what is the API to send an IPI?
> And the API to handle an IPI?
There are a few ways you could implement some custom cross-call,
although in this case I think stop_machine() would probably be the most
appropriate candidate. However, you're right that in general it may not
actually help enough to be worthwhile - a DSB SY would ensure that
in-flight transactions have at least been observed by the CPUs and any
other coherent masters, but for any writes with a memory type allowing
early acknowledgement (i.e. a Normal or Device mapping of a BAR) that
doesn't necessarily correlate with them having reached their ultimate
destination. For a PCI destination in particular, I think the normal way
to ensure all posted writes have completed would be to read from config
space; ah...
>>> + if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
>>> + smp8759_init(pcie, base);
>>
>> ...then retrieve it with of_device_get_match_data() here. No need to
>> reinvent the wheel (or have to worry about the ordering of multiple
>> compatibles once rev. n+1 comes around).
>
> I actually asked about this on IRC. The consensus was "use what
> best fits your use case". I need to do some processing based on
> the revision, so I thought
>
> if (chip_x)
> do_chip_x_init()
>
> was a good way to express my intent. Did I misunderstand?
No, I'm in no way disputing that; what I'm pointing out is that you
already have an explicitly provided way to associate a value of "chip_x"
with a given compatible string - see other callers of
of_device_get_match_data() for inspiration. I don't have much of an
opinion as to whether it's an enum, a static structure of offsets and
callbacks, or you embrace the nasal demons and just wedge the init
function pointer in there directly (this'll never run on
IA-64/M68k/etc., right? :P). The point is that not only is it cleaner
and scales better as the driver grows, it stops you having to worry at
all about setting this trap for yourself:
compatible = "rev3-with-extra-fun", "rev3";
...
if (of_device_is_compatible(dev, "rev3"))
boring_init_without_extra_fun(); /* :( */
because once you've made your code robust against that, you'll realise
that what you've done is wasted your time open-coding a creaky
approximation of of_match_device().
Robin.
> For example, the init function for rev2 currently looks like this:
>
> static void rev2_init(struct tango_pcie *pcie, void __iomem *base)
> {
> void __iomem *misc_irq = base + 0x40;
> void __iomem *doorbell = base + 0x8c;
>
> pcie->mux = base + 0x2c;
> pcie->msi_status = base + 0x4c;
> pcie->msi_mask = base + 0x6c;
> pcie->msi_doorbell = 0x80000000;
>
> writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0);
> writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4);
>
> /* Enable legacy PCI interrupts */
> writel(BIT(15), misc_irq);
> writel(0xf << 4, misc_irq + 4);
> }
>
>>> +#define VENDOR_SIGMA 0x1105
>>
>> Should this not be in include/linux/pci_ids.h?
>
> Doh! Very likely. Thanks.
>
> Regards.
>
^ permalink raw reply
* Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier
From: Mark Brown @ 2017-03-29 14:23 UTC (permalink / raw)
To: Ryan Lee
Cc: mark.rutland, alsa-devel, kuninori.morimoto.gx, lgirdwood, tiwai,
srinivas.kandagatla, romain.perier, bardliao, lars, axel.lin,
Paul.Handrigan, devicetree, arnd, nh6z, robh+dt, ckeepax, dgreid,
oder_chiou, ryan.lee.maxim, KCHSU0, linux-kernel
In-Reply-To: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com>
[-- Attachment #1.1: Type: text/plain, Size: 3776 bytes --]
On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote:
> + case SND_SOC_DAIFMT_CBS_CFM:
> + mode = MAX98927_PCM_MASTER_MODE_HYBRID;
> + default:
> + dev_err(codec->dev, "DAI clock mode unsupported");
Either we don't support _CBS_CFM or there's a missing break there.
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> + case SND_SOC_DAIFMT_I2S:
> + max98927->iface |= SND_SOC_DAIFMT_I2S;
> +
> + break;
Please use the kernel coding style as you do in the rest of this
function.
> + int reg = MAX98927_R0022_PCM_CLK_SETUP;
> + int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK;
> + regmap_update_bits(max98927->regmap, reg, mask, value);
reg and mask have exactly one user (as you'd expect), just use the
constants directly here to make things clearer.
> + switch (snd_pcm_format_width(params_format(params))) {
> + case 16:
> + chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16;
> + max98927->ch_size = 16;
You could just assign ch_size directly.
> + /* sampling rate configuration */
> + switch (params_rate(params)) {
> + case 8000:
> + sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000;
sampling_rate is only ever set to a real value in this switch statement,
you're not oring with anything else so you can just use an assignment
which would be a lot clearer (it's not obvious what else might go in
there) and the initial assignment to 0 for this and chan_sz can only
mask errors.
> + /* set sampling rate of IV */
> + if (max98927->interleave_mode &&
> + sampling_rate > MAX98927_PCM_SR_SET1_SR_16000)
> + regmap_update_bits(max98927->regmap,
Please use the kernel coding style, indent the second line of the if
with the ( so that it doesn't look like part of the conditional code.
> + switch (reg) {
> + case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B:
> + return true;
> + }
> + return false;
It'd be clearer to put the return false in the switch statement like the
return true, and also consistent with the _volatile_reg() function.
> +static const char * const max98927_speaker_source_text[] = {
> + "i2s", "reserved", "tone", "pdm"
> +};
This looks like it should be a DAPM control.
> +static const char * const max98927_monomix_output_text[] = {
> + "ch_0", "ch_1", "ch_1_2_div"
> +};
Similarly here.
> + SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL,
> + 0, (1<<MAX98927_AMP_VOL_WIDTH)-1, 0,
> + max98927_digital_tlv),
All volume controls should end with Volume as per control-names.rst.
> + SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN,
> + MAX98927_BROWNOUT_DSP_SHIFT, 1, 0),
All on/off switches should end with Switch as per control-names.rst.
> +static int max98927_probe(struct snd_soc_codec *codec)
> +{
> + /* Check Revision ID */
> + ret = regmap_read(max98927->regmap,
> + MAX98927_R01FF_REV_ID, ®);
Basic device identification and setup should be done at the chip level
probe not at the CODEC level so that if there are problems we fail as
early as possible and so that diagnostic information is available to
users as soon as possible, even if there's no sound card for the device
in the system.
> + /* Set inital volume (+13dB) */
As with all other CODEC drivers you should leave the hardware defaults
alone, what makes sense for your system may not make sense for other
systems and the hardware defaults are a fixed thing.
> + /* Boost Output Voltage & Current limit */
> + regmap_write(max98927->regmap,
> + MAX98927_R0040_BOOST_CTRL0,
> + 0x1C);
> + regmap_write(max98927->regmap,
> + MAX98927_R0042_BOOST_CTRL1,
> + 0x3E);
This should be system specific, these values might be unsafe in some
systems.
> +err:
> + if (max98927)
> + devm_kfree(&i2c->dev, max98927);
There is no need to explicitly free devm_ allocated memory, that's the
point of devm.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* [PATCHv5 11/11] arm: sti: update sti-cec for CEC notifier support
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
To: linux-media
Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>
From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
To use CEC notifier sti CEC driver needs to get phandle
of the hdmi device.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: devicetree@vger.kernel.org
---
arch/arm/boot/dts/stih407-family.dtsi | 12 ------------
arch/arm/boot/dts/stih410.dtsi | 13 +++++++++++++
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index d753ac36788f..044184580326 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -742,18 +742,6 @@
<&clk_s_c0_flexgen CLK_ETH_PHY>;
};
- cec: sti-cec@094a087c {
- compatible = "st,stih-cec";
- reg = <0x94a087c 0x64>;
- clocks = <&clk_sysin>;
- clock-names = "cec-clk";
- interrupts = <GIC_SPI 140 IRQ_TYPE_NONE>;
- interrupt-names = "cec-irq";
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_cec0_default>;
- resets = <&softreset STIH407_LPM_SOFTRESET>;
- };
-
rng10: rng@08a89000 {
compatible = "st,rng";
reg = <0x08a89000 0x1000>;
diff --git a/arch/arm/boot/dts/stih410.dtsi b/arch/arm/boot/dts/stih410.dtsi
index 3c9672c5b09f..21fe72b183d8 100644
--- a/arch/arm/boot/dts/stih410.dtsi
+++ b/arch/arm/boot/dts/stih410.dtsi
@@ -281,5 +281,18 @@
<&clk_s_c0_flexgen CLK_ST231_DMU>,
<&clk_s_c0_flexgen CLK_FLASH_PROMIP>;
};
+
+ sti-cec@094a087c {
+ compatible = "st,stih-cec";
+ reg = <0x94a087c 0x64>;
+ clocks = <&clk_sysin>;
+ clock-names = "cec-clk";
+ interrupts = <GIC_SPI 140 IRQ_TYPE_NONE>;
+ interrupt-names = "cec-irq";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_cec0_default>;
+ resets = <&softreset STIH407_LPM_SOFTRESET>;
+ hdmi-phandle = <&sti_hdmi>;
+ };
};
};
--
2.11.0
^ permalink raw reply related
* [PATCHv5 10/11] stih-cec: add CEC notifier support
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
To: linux-media
Cc: devicetree, linux-samsung-soc, Russell King, Krzysztof Kozlowski,
Javier Martinez Canillas, Hans Verkuil, dri-devel, Daniel Vetter,
Marek Szyprowski
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>
From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
By using the CEC notifier framework there is no longer any reason
to manually set the physical address. This was the one blocking
issue that prevented this driver from going out of staging, so do
this move as well.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: devicetree@vger.kernel.org
---
drivers/media/platform/Kconfig | 10 +++++++
drivers/media/platform/Makefile | 1 +
.../st-cec => media/platform/sti/cec}/Makefile | 0
.../st-cec => media/platform/sti/cec}/stih-cec.c | 31 +++++++++++++++++++---
drivers/staging/media/Kconfig | 2 --
drivers/staging/media/Makefile | 1 -
drivers/staging/media/st-cec/Kconfig | 8 ------
drivers/staging/media/st-cec/TODO | 7 -----
8 files changed, 39 insertions(+), 21 deletions(-)
rename drivers/{staging/media/st-cec => media/platform/sti/cec}/Makefile (100%)
rename drivers/{staging/media/st-cec => media/platform/sti/cec}/stih-cec.c (93%)
delete mode 100644 drivers/staging/media/st-cec/Kconfig
delete mode 100644 drivers/staging/media/st-cec/TODO
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 2c449b88fc94..7321f6123659 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -476,6 +476,16 @@ config VIDEO_SAMSUNG_S5P_CEC
CEC bus is present in the HDMI connector and enables communication
between compatible devices.
+config VIDEO_STI_HDMI_CEC
+ tristate "STMicroelectronics STiH4xx HDMI CEC driver"
+ depends on VIDEO_DEV && MEDIA_CEC_SUPPORT && (ARCH_STI || COMPILE_TEST)
+ select MEDIA_CEC_NOTIFIER
+ ---help---
+ This is a driver for STIH4xx HDMI CEC interface. It uses the
+ generic CEC framework interface.
+ CEC bus is present in the HDMI connector and enables communication
+ between compatible devices.
+
endif #V4L_CEC_DRIVERS
menuconfig V4L_TEST_DRIVERS
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 2f94d82afa4c..940724ab9b70 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS_GSC) += exynos-gsc/
obj-$(CONFIG_VIDEO_STI_BDISP) += sti/bdisp/
obj-$(CONFIG_VIDEO_STI_HVA) += sti/hva/
obj-$(CONFIG_DVB_C8SECTPFE) += sti/c8sectpfe/
+obj-$(CONFIG_VIDEO_STI_HDMI_CEC) += sti/cec/
obj-$(CONFIG_VIDEO_STI_DELTA) += sti/delta/
diff --git a/drivers/staging/media/st-cec/Makefile b/drivers/media/platform/sti/cec/Makefile
similarity index 100%
rename from drivers/staging/media/st-cec/Makefile
rename to drivers/media/platform/sti/cec/Makefile
diff --git a/drivers/staging/media/st-cec/stih-cec.c b/drivers/media/platform/sti/cec/stih-cec.c
similarity index 93%
rename from drivers/staging/media/st-cec/stih-cec.c
rename to drivers/media/platform/sti/cec/stih-cec.c
index 3c25638a9610..636281c64c04 100644
--- a/drivers/staging/media/st-cec/stih-cec.c
+++ b/drivers/media/platform/sti/cec/stih-cec.c
@@ -1,6 +1,4 @@
/*
- * drivers/staging/media/st-cec/stih-cec.c
- *
* STIH4xx CEC driver
* Copyright (C) STMicroelectronic SA 2016
*
@@ -15,9 +13,11 @@
#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <media/cec.h>
+#include <media/cec-notifier.h>
#define CEC_NAME "stih-cec"
@@ -129,6 +129,7 @@ struct stih_cec {
void __iomem *regs;
int irq;
u32 irq_status;
+ struct cec_notifier *notifier;
};
static int stih_cec_adap_enable(struct cec_adapter *adap, bool enable)
@@ -303,12 +304,29 @@ static int stih_cec_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct resource *res;
struct stih_cec *cec;
+ struct device_node *np;
+ struct platform_device *hdmi_dev;
int ret;
cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
if (!cec)
return -ENOMEM;
+ np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
+
+ if (!np) {
+ dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
+ return -ENODEV;
+ }
+
+ hdmi_dev = of_find_device_by_node(np);
+ if (!hdmi_dev)
+ return -EPROBE_DEFER;
+
+ cec->notifier = cec_notifier_get(&hdmi_dev->dev);
+ if (!cec->notifier)
+ return -ENOMEM;
+
cec->dev = dev;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -335,7 +353,7 @@ static int stih_cec_probe(struct platform_device *pdev)
cec->adap = cec_allocate_adapter(&sti_cec_adap_ops, cec,
CEC_NAME,
CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH |
- CEC_CAP_PHYS_ADDR | CEC_CAP_TRANSMIT, 1);
+ CEC_CAP_TRANSMIT, 1);
ret = PTR_ERR_OR_ZERO(cec->adap);
if (ret)
return ret;
@@ -346,12 +364,19 @@ static int stih_cec_probe(struct platform_device *pdev)
return ret;
}
+ cec_register_cec_notifier(cec->adap, cec->notifier);
+
platform_set_drvdata(pdev, cec);
return 0;
}
static int stih_cec_remove(struct platform_device *pdev)
{
+ struct stih_cec *cec = platform_get_drvdata(pdev);
+
+ cec_unregister_adapter(cec->adap);
+ cec_notifier_put(cec->notifier);
+
return 0;
}
diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
index c0d83cecf528..8ed8202da57a 100644
--- a/drivers/staging/media/Kconfig
+++ b/drivers/staging/media/Kconfig
@@ -32,6 +32,4 @@ source "drivers/staging/media/platform/bcm2835/Kconfig"
# Keep LIRC at the end, as it has sub-menus
source "drivers/staging/media/lirc/Kconfig"
-source "drivers/staging/media/st-cec/Kconfig"
-
endif
diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
index 97b29ece9a2c..3a6adeabede1 100644
--- a/drivers/staging/media/Makefile
+++ b/drivers/staging/media/Makefile
@@ -4,4 +4,3 @@ obj-$(CONFIG_LIRC_STAGING) += lirc/
obj-$(CONFIG_VIDEO_BCM2835) += platform/bcm2835/
obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/
obj-$(CONFIG_VIDEO_OMAP4) += omap4iss/
-obj-$(CONFIG_VIDEO_STI_HDMI_CEC) += st-cec/
diff --git a/drivers/staging/media/st-cec/Kconfig b/drivers/staging/media/st-cec/Kconfig
deleted file mode 100644
index c04283db58d6..000000000000
--- a/drivers/staging/media/st-cec/Kconfig
+++ /dev/null
@@ -1,8 +0,0 @@
-config VIDEO_STI_HDMI_CEC
- tristate "STMicroelectronics STiH4xx HDMI CEC driver"
- depends on VIDEO_DEV && MEDIA_CEC_SUPPORT && (ARCH_STI || COMPILE_TEST)
- ---help---
- This is a driver for STIH4xx HDMI CEC interface. It uses the
- generic CEC framework interface.
- CEC bus is present in the HDMI connector and enables communication
- between compatible devices.
diff --git a/drivers/staging/media/st-cec/TODO b/drivers/staging/media/st-cec/TODO
deleted file mode 100644
index c61289742c5c..000000000000
--- a/drivers/staging/media/st-cec/TODO
+++ /dev/null
@@ -1,7 +0,0 @@
-This driver requires that userspace sets the physical address.
-However, this should be passed on from the corresponding
-ST HDMI driver.
-
-We have to wait until the HDMI notifier framework has been merged
-in order to handle this gracefully, until that time this driver
-has to remain in staging.
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCHv5 09/11] stih-cec.txt: document new hdmi phandle
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
To: linux-media
Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>
From: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Update the bindings documentation with the new hdmi phandle.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Rob Herring <robh@kernel.org>
CC: devicetree@vger.kernel.org
---
Documentation/devicetree/bindings/media/stih-cec.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/stih-cec.txt b/Documentation/devicetree/bindings/media/stih-cec.txt
index 71c4b2f4bcef..289a08b33651 100644
--- a/Documentation/devicetree/bindings/media/stih-cec.txt
+++ b/Documentation/devicetree/bindings/media/stih-cec.txt
@@ -9,6 +9,7 @@ Required properties:
- pinctrl-names: Contains only one value - "default"
- pinctrl-0: Specifies the pin control groups used for CEC hardware.
- resets: Reference to a reset controller
+ - hdmi-phandle: Phandle to the HDMI controller
Example for STIH407:
@@ -22,4 +23,5 @@ sti-cec@094a087c {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_cec0_default>;
resets = <&softreset STIH407_LPM_SOFTRESET>;
+ hdmi-phandle = <&hdmi>;
};
--
2.11.0
^ permalink raw reply related
* [PATCHv5 06/11] s5p-cec.txt: document the HDMI controller phandle
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
To: linux-media
Cc: devicetree, linux-samsung-soc, Russell King, Krzysztof Kozlowski,
Javier Martinez Canillas, Hans Verkuil, dri-devel, Daniel Vetter,
Marek Szyprowski
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>
From: Hans Verkuil <hans.verkuil@cisco.com>
Update the bindings documenting the new hdmi phandle.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
CC: linux-samsung-soc@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: Krzysztof Kozlowski <krzk@kernel.org>
---
Documentation/devicetree/bindings/media/s5p-cec.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/s5p-cec.txt b/Documentation/devicetree/bindings/media/s5p-cec.txt
index 925ab4d72eaa..4bb08d9d940b 100644
--- a/Documentation/devicetree/bindings/media/s5p-cec.txt
+++ b/Documentation/devicetree/bindings/media/s5p-cec.txt
@@ -15,6 +15,7 @@ Required properties:
- clock-names : from common clock binding: must contain "hdmicec",
corresponding to entry in the clocks property.
- samsung,syscon-phandle - phandle to the PMU system controller
+ - hdmi-phandle - phandle to the HDMI controller
Example:
@@ -25,6 +26,7 @@ hdmicec: cec@100B0000 {
clocks = <&clock CLK_HDMI_CEC>;
clock-names = "hdmicec";
samsung,syscon-phandle = <&pmu_system_controller>;
+ hdmi-phandle = <&hdmi>;
pinctrl-names = "default";
pinctrl-0 = <&hdmi_cec>;
status = "okay";
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCHv5 05/11] ARM: dts: exynos: add HDMI controller phandle to exynos4.dtsi
From: Hans Verkuil @ 2017-03-29 14:15 UTC (permalink / raw)
To: linux-media
Cc: Daniel Vetter, Russell King, dri-devel, linux-samsung-soc,
Krzysztof Kozlowski, Inki Dae, Marek Szyprowski,
Javier Martinez Canillas, Benjamin Gaignard, Hans Verkuil,
devicetree
In-Reply-To: <20170329141543.32935-1-hverkuil@xs4all.nl>
From: Hans Verkuil <hans.verkuil@cisco.com>
Add the new hdmi phandle to exynos4.dtsi. This phandle is needed by the
s5p-cec driver to initialize the CEC notifier framework.
Tested with my Odroid U3.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
CC: linux-samsung-soc@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: Krzysztof Kozlowski <krzk@kernel.org>
---
arch/arm/boot/dts/exynos4.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 18def1c774d5..84fcdff140ae 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -771,6 +771,7 @@
clocks = <&clock CLK_HDMI_CEC>;
clock-names = "hdmicec";
samsung,syscon-phandle = <&pmu_system_controller>;
+ hdmi-phandle = <&hdmi>;
pinctrl-names = "default";
pinctrl-0 = <&hdmi_cec>;
status = "disabled";
--
2.11.0
^ permalink raw reply related
* RE: [PATCH v2 2/7] rtc: Add rtc-sh
From: Chris Brandt @ 2017-03-29 14:14 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring
Cc: Alessandro Zummo, Alexandre Belloni, Mark Rutland, Simon Horman,
RTCLINUX, devicetree@vger.kernel.org, Linux-Renesas
In-Reply-To: <CAMuHMdVmwJYMzspVeDqToBrkfK+zGLMNhxGvEbqCA-vuCUXqoQ@mail.gmail.com>
On Wednesday, March 29, 2017, Geert Uytterhoeven wrote:
> >> +Example:
> >> +rtc: rtc@fcff1000 {
> >> + compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> >> + reg = <0xfcff1000 0x2e>;
> >> + interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> >> + GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> >> + GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> >> + interrupt-names = "alarm", "period", "carry";
> >> + clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> >> + <&rtc_x3_clk>, <&extal_clk>;
> >> + clock-names = "fck", "rtc_x1", "rtc_x3", "extal";
> >> + power-domains = <&cpg_clocks>;
> >
> > Not documented.
>
> "power-domains" is a platform property.
>
> All hardware components need power.
> All synchronous hardware components need a clock.
> Most hardware components have a reset signal.
>
> Whether these are exposed and can be controlled depends on the
> platform/SoC.
> So documenting them in each and every device binding looks overkill to me.
> I think this is something to be addressed by devicetree-specification
> (which doesn't handle clocks, power-domains, resets yet).
>
> If you prefer, the property can be removed from the example, though.
I'll go ahead and pull it out (since I have to update the commit title
anyway as per Rob's request)
Besides...it's probably the most least important part of the example ;)
Thanks,
Chris
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox