Re: [PATCH 4/6] clk: tegra: add nvidia,tegra132-ccplex-clk binding

[Stephen Warren]

On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> Tegra132 has a few new clocks for the CPU complex (ccplex).

> diff --git 
> a/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt 
> b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt

> +Example SoC include file:
> +
> +/ {
> + ccplex-clock@0,7004 {
> + compatible = "nvidia,tegra132-ccplex-clk";
> + reg = <0x0 0x7004 0x0 0x1000>;
> + status = "okay";

That's the default, so it's not worth including that property.

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

Re: [PATCH 1/6] clk: tegra: don't abort clk init on error

[Stephen Warren]

On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> Just continue initializing clocks if there's an error on one of them. This
> is useful if there's a mistake in the inittable, because the system could
> hang if clk_disable_unused() disables some of the critical clocks in this
> table.

If there's a problem in the init table, we should simply fix it instead
of working around it.

At the very least, we need to WARN on this rather than just ignoring
problems.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] clk: tegra: make tegra_clocks_apply_init_table arch_initcall

[Stephen Warren]

On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> tegra_clocks_apply_init_table needs to be called after the udelay loop has
> been calibrated (see 441f199a37cfd66c5dd8dd45490bd3ea6971117d why that is).

Instead of just the commit ID, can you please mention the commit subject
too:

441f199a37cf "clk: tegra: defer application of init table"

> On existing Tegra SoCs this was done by calling tegra_clocks_apply_init_table
> from tegra_dt_init. To make this also work on ARM64, we need to
> change this into an initcall. tegra_dt_init is called from customize_machine
> which is an arch_initcall. Therefore this should also work on existing 32bit
> Tegra SoCs.

I still strongly dislike performing this basic initialization from
random separate initcalls. I think we should create a single initcall
for all the Tegra initialization, even if it isn't able to be a machine
descriptor hook function any more.

That said, discussions re: that are ongoing in other threads, so it's no
worth reworking this patch yet.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] clk: tegra: Add EMC clock driver

[Stephen Warren]

On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> The driver is currently only tested on Tegra124 Jetson TK1, but should
> work with other Tegra124 boards, provided that correct EMC tables are
> provided through the device tree. Older chip models have differing
> timing change sequences, so they are not currently supported.

> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c

> +struct emc_timing {
> + unsigned long rate, parent_rate;
> + u8 parent_index;
> + struct clk *parent;
> +
> + /* Store EMC burst data in a union to minimize mistakes. This allows
> +  * us to use the same burst data lists as used by the downstream and
> +  * ChromeOS kernels. */

Nit: */ should be on its own line. This applies to many comments in the
file.

> +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Timing change sequence functions*
> + * * * * * * * * * * * * * * * * * * * * * * * * * */

Nit: This kind of banner comment is unusual, but I guess it's fine.

> +static void emc_seq_update_timing(struct tegra_emc *tegra)
...
> + dev_err(&tegra->pdev->dev, "timing update failed\n");
> + BUG();
> +}

Is there any way to avoid all these BUGs? Can we just continue on and
retry the next time, or disallow any further clock rate changes or
something?

> +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Debugfs entry   *
> + * * * * * * * * * * * * * * * * * * * * * * * * * */
> +
> +static int emc_debug_rate_get(void *data, u64 *rate)
> +{
> + struct tegra_emc *tegra = data;
> +
> + *rate = clk_get_rate(tegra->hw.clk);
> +
> + return 0;
> +}
> +
> +static int emc_debug_rate_set(void *data, u64 rate)
> +{
> + struct tegra_emc *tegra = data;
> +
> + return clk_set_rate(tegra->hw.clk, rate);
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(emc_debug_rate_fops, emc_debug_rate_get,
> + emc_debug_rate_set, "%lld\n");

I think the rate can already be obtained through
...debug/clock/clock_summary. I'm not sure about changing the rate, but
shouldn't that be a feature of the common clock core, not individual
drivers?

> +static int load_timings_from_dt(struct tegra_emc *tegra,
> + struct device_node *node)
> +{
...
> + for_each_child_of_node(node, child) {
...
> + if (timing->rate <= prev_rate) {
> + dev_err(&tegra->pdev->dev,
> + "timing %s: rate not increasing\n",
> + child->name);

I don't believe there's any guaranteed node enumeration order. If the
driver needs the child nodes sorted, it should sort them itself.

> +static const struct of_device_id tegra_car_of_match[] = {
> + { .compatible = "nvidia,tegra124-car" },
> + {}
> +};
> +
> +static const struct of_device_id tegra_mc_of_match[] = {
> + { .compatible = "nvidia,tegra124-mc" },
> + {}
> +};

It would be better if this driver explicitly called into the driver for
other modules, rather than directly touching their registers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] of: Add Tegra124 EMC bindings

[Stephen Warren]

On 07/21/2014 04:52 PM, Andrew Bresticker wrote:
> On Mon, Jul 21, 2014 at 2:28 PM, Stephen Warren  wrote:
>> On 07/11/2014 10:43 AM, Andrew Bresticker wrote:
>>> On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen  
>>> wrote:
>>>> Add binding documentation for the nvidia,tegra124-emc device tree
>>>> node.
>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt 
>>>> b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>>>
>>>> +Required properties :
>>>> +- compatible : "nvidia,tegra124-emc".
>>>> +- reg : Should contain 1 or 2 entries:
>>>> +  - EMC register set
>>>> +  - MC register set : Required only if no node with
>>>> +'compatible = "nvidia,tegra124-mc"' exists. The MC register set
>>>> +is first read from the MC node. If it doesn't exist, it is read
>>>> +from this property.
>>>> +- timings : Should contain 1 entry for each supported clock rate.
>>>> +  Entries should be named "timing@n" where n is a 0-based increasing
>>>> +  number. The timings must be listed in rate-ascending order.
>>>
>>> There are upcoming boards which support multiple DRAM configurations
>>> and require a separate set of timings for each configuration.  Could
>>> we instead have multiple sets of timings with the proper one selected
>>> at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0?
>>> Something like:
>>>
>>> emc {
>>> emc-table@0 {
>>> nvidia,ram-code = <0>;
>>> timing@0 {
>>> ...
>>> };
>>> ...
>>> };
>>
>> Until recently, there was a binding for Tegra20 EMC in mainline. We
>> should make sure the Tegra124 driver (or rather binding...) is at least
>> as feature-ful as that was.
>>
>> Furthermore, I thought that with some boards, there were more RAM
>> options that there were available RAM code strap bits. I assume that in
>> mainline, we'll simply have different base DT files for the different
>> sets of configurations? Or, will we want to add another level to the DT
>> to represent different sets of RAM code values? I'm not sure what data
>> source the bootloader uses to determine which set of RAM configuration
>> to use when there aren't enough entries in the BCT for the boot ROM to
>> do this automatically, and whether we have any way to get that value
>> into the kernel, so it could use it for the extra DT level lookup?
> 
> For the ChromeOS boards at least we are neither limited by the number
> of strapping bits (4) nor the number of BCT entries supported by the
> boot ROM (since coreboot does not rely on the boot ROM for SDRAM
> initialization), so having a single set of SDRAM configurations for
> each board, indexed by APBDEV_PMC_STRAPPING_OPT_A_0[7:4] works just
> fine.

Does the bootloader adjust the DT that's passed to the kernel so that
only the relevant single set of EMC timings is contained in the DT?

On a system where the boot ROM initializes RAM, and where the HW design
might have multiple SDRAM configuration, here's what usually happens:

a) The BCT contains N sets of SDRAM configurations.

b) The boot ROM reads the SDRAM strapping bits, and uses them to pick
the correct SDRAM configuration from the N sets in the BCT.

c) The kernel DT has N sets of SDRAM configurations.

d) The kernel reads the SDRAM strapping bits, and uses them to pick the
correct SDRAM configuration from the N sets in the DT.

On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is
too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d)
won't work. I assume the kernel DT therefore must be adjusted to only
contain the single SDRAM configuration that is relevant for the current HW?

(isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index
and 2 bits for boot flash index, so max N is quite small?)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] of: Add Tegra124 EMC bindings

[Stephen Warren]

On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> Add binding documentation for the nvidia,tegra124-emc device tree
> node.

> diff --git 
> a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt 
> b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt

> +Required properties :
> +- compatible : "nvidia,tegra124-emc".
> +- reg : Should contain 1 or 2 entries:
> +  - EMC register set
> +  - MC register set : Required only if no node with
> +'compatible = "nvidia,tegra124-mc"' exists. The MC register set
> +is first read from the MC node. If it doesn't exist, it is read
> +from this property.

I echo what Thierry said about the MC register sharing.

> +- timings : Should contain 1 entry for each supported clock rate.

s/entry/node/ I think.

> +  Entries should be named "timing@n" where n is a 0-based increasing
> +  number. The timings must be listed in rate-ascending order.

That implies that #address-cells and #size-cells are required too.

> +Required properties for timings :

... and a reg property is needed here.

> +- clock-frequency : Should contain the memory clock rate.
> +- nvidia,parent-clock-frequency : Should contain the rate of the EMC
> +  clock's parent clock.

> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names : Must include the following entries:
> +  - emc-parent : EMC's parent clock.

Shouldn't clocks/clock-names be part of the main node, not the
per-timing node?

> +- The following properties contain EMC timing characterization values:
> +  - nvidia,emc-zcal-cnt-long
> +  - nvidia,emc-auto-cal-interval
> +  - nvidia,emc-ctt-term-ctrl
> +  - nvidia,emc-cfg
> +  - nvidia,emc-cfg-2
> +  - nvidia,emc-sel-dpd-ctrl
> +  - nvidia,emc-cfg-dig-dll
> +  - nvidia,emc-bgbias-ctl0
> +  - nvidia,emc-auto-cal-config
> +  - nvidia,emc-auto-cal-config2
> +  - nvidia,emc-auto-cal-config3
> +  - nvidia,emc-mode-reset
> +  - nvidia,emc-mode-1
> +  - nvidia,emc-mode-2
> +  - nvidia,emc-mode-4
> +- nvidia,emc-burst-data : EMC timing characterization data written to
> +  EMC registers.
> +- nvidia,mc-burst-data : EMC timing characterization data written to
> + MC registers.
> 

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

Re: [PATCH 2/8] ARM: tegra: Remove TEGRA124_CLK_EMC from tegra124-car.h

[Stephen Warren]

On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> Remove the TEGRA124_CLK_EMC cell value define for tegra124-car
> from the binding headers. This clock has never been able to do
> anything and is being replaced with a separate EMC driver with
> its own device tree node. Removing the define ensures that any
> user will not mistakenly refer to <&tegra_car TEGRA124_CLK_EMC>
> instead of <&emc> or similar.

If the clock physically exists in HW, I see no reason to remove it from
the binding definition, even if we don't implement it (or remove the
implementation of it) in the Linux clock driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] clk: tegra: make tegra_clocks_apply_init_table arch_initcall

[Stephen Warren]

On 07/16/2014 02:27 AM, Peter De Schrijver wrote:
> On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote:
>> [...]
>>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
>>> index d081732..65cde4e 100644
>>> --- a/drivers/clk/tegra/clk.c
>>> +++ b/drivers/clk/tegra/clk.c
>>> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
>>>  
>>>  tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
>>>  
>>> -void __init tegra_clocks_apply_init_table(void)
>>> +static int __init tegra_clocks_apply_init_table(void)
>>>  {
>>> if (!tegra_clk_apply_init_table)
>>> -   return;
>>> +   return 0;
>>
>> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets
> 
> An arch_initcall will be called for every ARM platform I think? In case
> this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not
> be set and therefore a silent return 0; seems the most appropriate thing to do
> to me?

This is one reason that doing all the initialization from separate
initcalls sucks. Much better to have a single top-level initialization
function that calls exactly what is needed, only what is needed, and
only runs on the correct SoCs.

But failing that, I guess you need to say something like
of_is_compatible(root node, "nvidia Tegra"), but of course the
definition of "nvidia Tegra" is an ever-growing list of possible values
that needs to be used from each separate initcall...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] of: Add Tegra124 EMC bindings

[Stephen Warren]

On 07/11/2014 10:43 AM, Andrew Bresticker wrote:
> On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen  
> wrote:
>> Add binding documentation for the nvidia,tegra124-emc device tree
>> node.
> 
>> diff --git 
>> a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt 
>> b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
> 
>> +Required properties :
>> +- compatible : "nvidia,tegra124-emc".
>> +- reg : Should contain 1 or 2 entries:
>> +  - EMC register set
>> +  - MC register set : Required only if no node with
>> +'compatible = "nvidia,tegra124-mc"' exists. The MC register set
>> +is first read from the MC node. If it doesn't exist, it is read
>> +from this property.
>> +- timings : Should contain 1 entry for each supported clock rate.
>> +  Entries should be named "timing@n" where n is a 0-based increasing
>> +  number. The timings must be listed in rate-ascending order.
> 
> There are upcoming boards which support multiple DRAM configurations
> and require a separate set of timings for each configuration.  Could
> we instead have multiple sets of timings with the proper one selected
> at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0?
> Something like:
> 
> emc {
> emc-table@0 {
> nvidia,ram-code = <0>;
> timing@0 {
> ...
> };
> ...
> };

Until recently, there was a binding for Tegra20 EMC in mainline. We
should make sure the Tegra124 driver (or rather binding...) is at least
as feature-ful as that was.

Furthermore, I thought that with some boards, there were more RAM
options that there were available RAM code strap bits. I assume that in
mainline, we'll simply have different base DT files for the different
sets of configurations? Or, will we want to add another level to the DT
to represent different sets of RAM code values? I'm not sure what data
source the bootloader uses to determine which set of RAM configuration
to use when there aren't enough entries in the BCT for the boot ROM to
do this automatically, and whether we have any way to get that value
into the kernel, so it could use it for the extra DT level lookup?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: a case for a common efuse API?

[Stephen Warren]

On 07/09/2014 05:49 AM, Peter De Schrijver wrote:
> On Tue, Jul 08, 2014 at 10:00:23PM +0200, Stephen Boyd wrote:
>>
>> I added Tegra folks because I see that on Tegra this hardware is exposed
>> via an SoC specific API, tegra_fuse_readl(), and an associated driver in
>> drivers/misc/fuse/tegra/. Unfortunately I don't see any users of the API
>> outside of the speedo code in the same directory and the sysfs bin
>> attribute that may or may not be in use by some userspace code.
>>
> 
> The SATA driver by Mikko Perttunen uses it. The driver hasn't been merged
> though. There's some more upcoming work which makes use of it.
> I don't think we can standardize much of an API for this. The data is
> SoC specific, so the user will always need to have some SoC specific
> knowledge on how to use it.

I think we could have a generic "read fuse X" API. The only thing that'd
be chip-specific is the value of "X". That could be hard-coded into the
client drivers, or perhaps represented in a DT propery e.g.
#fuse-cells/xxx-fuses. That said, I wonder what benefit we'd get from
such a common API. I suppose if any IP block was to be re-used in
completely different SoC families, it'd isolate the driver from having
to call different functions to read fuses on different SoC families, but
that doesn't seem to happen in practice yet, and the issue could be
solved later if needed.

It'd certainly be hard to create any higher-layer semantic API here,
since different SoCs store completely different sets of data in fuses,
so there would be little point in a common "read the MAC address from
the fuses" API, since that data wouldn't exist in fuses on many SoCs.

> In some cases we need it very early (eg. to
> determine the correct Tegra20 revision). On Tegra20 we can't use the CPU
> to read the fuses due to a hw bug, we have to use DMA, which means the
> transaction becomes blocking and could fail due to lack of resources.

Yes, any such API would become "least common-denominator" or similar;
i.e. any restriction imposed by any SoC would then affect how the API
works on any other SoC.


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

Re: [PATCH 4/4] ARM: tegra: roth: add display DT node

[Stephen Warren]

On 07/02/2014 09:10 PM, Alexandre Courbot wrote:
> On 07/03/2014 12:55 AM, Stephen Warren wrote:
>> On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
>>> DSI support has been fixed to support continuous clock behavior that the
>>> panel used on SHIELD requires, so finally add its device tree node since
>>> it is functional.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-roth.dts
>>> b/arch/arm/boot/dts/tegra114-roth.dts
>>
>>> +host1x@5000 {
>>> +dsi@5430 {
>>
>> That node looks fine, but I wonder why we need to mark a bunch of
>> regulators as always-on? shouldn't the references to vdd-supply and
>> power-supply from this node be enough? If not, perhaps the tree
>> structure of the regulators isn't correct, or the DSI or panel bindings
>> don't allow enough regulators to be referenced?
> 
> regulator-always-on is indeed not needed for vdd_lcd. I actually had a
> patch in my tree removing this line that I forgot to squash. Will post a
> v2 for this patch that fixes this, thanks.
> 
> vdd-2v8-display needs to remain always-on however. Here we may hit a
> limitation of the simple-panel driver, where only one power supply can
> be provided.

Can't we fix the simple-panel driver to allow a list of regulators in
the property?

I suppose that this technique is OK though; we can always simplify the
DT later if the simple-panel binding gets enhanced.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 0/3] drm/nouveau: support for probing platform devices

[Stephen Warren]

On 07/02/2014 03:09 AM, Alexandre Courbot wrote:
> On Thu, Jun 26, 2014 at 2:33 PM, Alexandre Courbot  
> wrote:
>> This series adds support for probing platform devices on Nouveau, as well as
>> the DT bindings for GK20A. It doesn't enable the GPU yet on Tegra boards 
>> since
>> a few extra things need to be supported before that.
>>
>> This version is mostly identical to v2 but fixes an important issue: the 
>> drvdata
>> must be set to the drm_device for sysfs to work, so the platform device
>> structure now includes the nouveau_device flattened into it to let us compute
>> the address of one from the other. Since the platform device resources 
>> (clocks,
>> regulators, ...) need to live longer than the nouveau_device, they are stored
>> into their own structure which is allocated separately.
...
> Stephen, Thierry,
> 
> Ben has merged the first patch of this series into the Nouveau tree.
> Could you get patches 2 and 3 through the Tegra tree? Thanks!

I've applied patches 2 and 3 to Tegra's for-3.17/dt branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 10:09 AM, Alan Stern wrote:
> On Wed, 2 Jul 2014, Stephen Warren wrote:
> 
>> On 07/02/2014 08:02 AM, Alan Stern wrote:
>>> On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
>>>
>>>> Hi all,
>>>>
>>>> This series fixes a probe order issue with the Tegra EHCI driver.
>>>> Basically, the register area of the 1st USB controller contains some
>>>> registers that are global to all of the controllers, but that are also
>>>> cleared when reset is asserted to the 1st controller. So if (say) the
>>>> 3rd controller would be the first one to finish probing successfully,
>>>> then the reset that happens during the 1st controller's probe would
>>>> result in broken USB. So the before doing anything with the USB HW,
>>>> we should reset the 1st controller once, and then never ever reset
>>>> it again.
>>>
>>> This sounds very much like the sort of thing that ought to be described 
>>> in DT.  It is a hardware dependence, and DT exists for the purpose of 
>>> describing the hardware.
>>
>> DT is more about describing the HW, not how SW has to use the HW.
> 
> Tuomas wrote: "the register area of the 1st USB controller contains
> some registers that are global to all of the controllers, but that are
> also cleared when reset is asserted to the 1st controller."  That is
> very much an attribute of the hardware and so DT should describe it.

So you want to add a Boolean DT property to the first USB controller
node indicating that it has the "shared" reset? That would be fine, but
would only replace the content of tegra_find_usb1_node(); much of the
rest of the patch would remain the same.

I guess in this case, it's fine to require DT changes to enable the new
feature of fixing this issue, so long as the code continues to work the
same as it currently does with old DTs. Due to the backwards
compatibility requirement, the patch will end up slightly more complex,
but hopefully not too bad.

>> probe() ordering is a SW issue, not a HW description. It's driver
>> knowledge that the HW resets have to run in a certain order, and if the
>> driver didn't actually reset the HW ever (but rather, re-programmed all
>> registers so reset was never needed), then order wouldn't be relevant
>>
>> DT certainly doesn't have any mechanism for describing probe order or
>> anything like that, although you can fake it out by adding phandles
>> between nodes, and having SW wait for the driver for the referenced node
>> to probe first. That won't work here though, since there's no guarantee
>> that the USB1 node will actually be enabled (that USB port might not be
>> hooked up on the board, hence the DT node will be disabled), so we can't
>> rely on a driver for it ever appearing.
> 
> I wasn't talking about probe order; I was talking about the fact that 
> registers pertinent to the later controllers are in the reset domain of 
> the first controller.
> 
> Alan Stern

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

Re: [PATCH 4/4] ARM: tegra: roth: add display DT node

[Stephen Warren]

On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
> DSI support has been fixed to support continuous clock behavior that the
> panel used on SHIELD requires, so finally add its device tree node since
> it is functional.

> diff --git a/arch/arm/boot/dts/tegra114-roth.dts 
> b/arch/arm/boot/dts/tegra114-roth.dts

> + host1x@5000 {
> + dsi@5430 {

That node looks fine, but I wonder why we need to mark a bunch of
regulators as always-on? shouldn't the references to vdd-supply and
power-supply from this node be enough? If not, perhaps the tree
structure of the regulators isn't correct, or the DSI or panel bindings
don't allow enough regulators to be referenced?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] Tegra USB probe order issue fix

[Stephen Warren]

On 07/02/2014 08:02 AM, Alan Stern wrote:
> On Wed, 2 Jul 2014, Tuomas Tynkkynen wrote:
> 
>> Hi all,
>>
>> This series fixes a probe order issue with the Tegra EHCI driver.
>> Basically, the register area of the 1st USB controller contains some
>> registers that are global to all of the controllers, but that are also
>> cleared when reset is asserted to the 1st controller. So if (say) the
>> 3rd controller would be the first one to finish probing successfully,
>> then the reset that happens during the 1st controller's probe would
>> result in broken USB. So the before doing anything with the USB HW,
>> we should reset the 1st controller once, and then never ever reset
>> it again.
> 
> This sounds very much like the sort of thing that ought to be described 
> in DT.  It is a hardware dependence, and DT exists for the purpose of 
> describing the hardware.

DT is more about describing the HW, not how SW has to use the HW.
probe() ordering is a SW issue, not a HW description. It's driver
knowledge that the HW resets have to run in a certain order, and if the
driver didn't actually reset the HW ever (but rather, re-programmed all
registers so reset was never needed), then order wouldn't be relevant

DT certainly doesn't have any mechanism for describing probe order or
anything like that, although you can fake it out by adding phandles
between nodes, and having SW wait for the driver for the referenced node
to probe first. That won't work here though, since there's no guarantee
that the USB1 node will actually be enabled (that USB port might not be
hooked up on the board, hence the DT node will be disabled), so we can't
rely on a driver for it ever appearing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] USB: PHY: tegra: Call tegra_usb_phy_close only on device removal

[Stephen Warren]

On 07/01/2014 03:08 PM, Tuomas Tynkkynen wrote:
> tegra_usb_phy_close() is supposed to undo the effects of
> tegra_usb_phy_init(). It is also currently added as the USB PHY shutdown
> callback, which is wrong, since tegra_usb_phy_init() is only called
> during probing wheras the shutdown callback can get called multiple
> times. This then leads to warnings about unbalanced regulator_disable if
> the EHCI driver is unbound and bound again at runtime.

The series,
Reviewed-by: Stephen Warren 

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c

> -static void tegra_usb_phy_close(struct usb_phy *x)
> +static void tegra_usb_phy_close(struct tegra_usb_phy *phy)

If this function undoes what _init does, it seems it should be called
_fini not _close. But that's bike-shedding perhaps.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/25/2014 10:23 AM, Stephen Warren wrote:
> On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
>> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>>> irq_work_cpu_notify() altogether?
>>
>> Just so...
>>
>> getting up at 6am and sitting in an airport terminal doesn't seem to
>> agree with me; any more silly fail here?
>>
>> ---
>> Subject: irq_work: Remove BUG_ON in irq_work_run()
>> From: Peter Zijlstra 
>> Date: Wed Jun 25 07:13:07 CEST 2014
>>
>> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
>> pending IPI callbacks before CPU offline"), which ends up calling
>> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
>> is not from IRQ context.
>>
>> And since that already calls irq_work_run() from the hotplug path,
>> remove our entire hotplug handling.
> 
> Tested-by: Stephen Warren 
> 
> [with the s/static// already mentioned in this thread, obviously:-)]

next-20140701 still seems to fail CPU hotplug. I assume this patch
hasn't yet been applied for some reason?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver

[Stephen Warren]

On 07/01/2014 02:06 AM, Mikko Perttunen wrote:
> Inline.
> 
> On 01/07/14 00:23, Stephen Warren wrote:
>> On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
>>> This adds support for the Tegra SOCTHERM thermal sensing and management
>>> system found in the Tegra124 system-on-chip. This initial driver
>>> supports
>>> the four thermal zones with hardware-tracked trip points.
>>
>>> diff --git a/drivers/thermal/tegra_soctherm.c
>>> b/drivers/thermal/tegra_soctherm.c
>>
>>> +static struct tegra_tsensor t124_tsensors[] = {
>>> +{
>>> +.base = 0xc0,
>>> +.name = "cpu0",
>>> +.config = &t124_tsensor_config,
>>> +.calib_fuse_offset = 0x098,
>>> +.fuse_corr_alpha = 1135400,
>>> +.fuse_corr_beta = -6266900,
>>> +},
>>
>> I wonder why some of those fields are named "fuse_xxx" when the values
>> are hard-coded in these tables rather than read from fuses? These values
>> don't seem to be used to adjust values read from fuses.
> 
> They are used to when calculating the thermal calibration in
> calculate_tsensor_calibration, which is based on the value read from the
> fuse. Downstream calls them fuse correction values, so I kept that. (I
> guess the meaning of corr might not be obvious..) On downstream there is
> another set of these correction values used depending on the fuse
> revision, but I believe the older revision is only found internally.

Ah, so there's some manufacturing calibration process that sets some
fuse value, and the HW uses a combination of that fuse value, and some
parameters of the manufacturing process as represented by the
SENSOR_CONFIG2 register, to apply the calibration? I wonder why
SENSOR_CONFIG2 is a register not a fuse in that case, but anyway...

Perhaps some comments or kerneldoc in the definition of struct
tegra_tsensor would be useful?

I did notice some inconsistency in bracketing at:

> + *calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |
> +  ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);

>>> +err = devm_request_threaded_irq(&pdev->dev, irq, soctherm_isr,
>>> +soctherm_isr_thread,
>>> +IRQF_SHARED, "tegra_soctherm",
>>> +zone);
>>
>> Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
>> requested once, and the ISR simply loop over the status register (or
>> whatever there are 4 of)?
> 
> I had that variant as well, but since we need to pass the list of
> tripped sensors to soctherm_isr_thread somehow, I guess some kind of
> locking or atomic is needed. This version doesn't need that, so I went
> with it.

Why not read THERMCTL_INTR_STATUS inside the IRQ thread. IIRC, if the
ISR wakes an IRQ thread, the interrupt remains disable until the thread
has run its course, so there's no issue deferring the register read
until the thread runs, at which point, the thread can simply loop over
all the sensors.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] thermal: of: Add support for hardware-tracked trip points

[Stephen Warren]

On 07/01/2014 01:27 AM, Mikko Perttunen wrote:
> Inline.
> 
> On 01/07/14 00:08, Stephen Warren wrote:
>> On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
>>> This adds support for hardware-tracked trip points to the device tree
>>> thermal sensor framework.
>>>
>>> The framework supports an arbitrary number of trip points. Whenever
>>> the current temperature is updated, the trip points immediately
>>> below and above the current temperature are found. A sensor driver
>>> callback `set_trips' is then called with the temperatures.
>>> If there is no trip point above or below the current temperature,
>>> the passed trip temperature will be LONG_MAX or LONG_MIN respectively.
>>> In this callback, the driver should program the hardware such that
>>> it is notified when either of these trip points are triggered.
>>> When a trip point is triggered, the driver should call
>>> `thermal_zone_device_update' for the respective thermal zone. This
>>> will cause the trip points to be updated again.
>>>
>>> If the `set_trips' callback is not implemented (is NULL), the framework
>>> behaves as before.
>>
>> Is there no "core thermal" code? I would have expected this new feature
>> to be implemented in "core" code rather than of/dt "support" code.
>> Perhaps there would also be some additions to the of/dt code, but I'd
>> still expect the bulk of the feature to be complete independant of
>> of/dt. Systems still using board files or ACPI or ... would surely
>> benefit from this too?
> 
> The thermal core only supports a fixed number of trip points for each
> driver and the core informs the driver of any changes to those, so
> drivers using the core framework can already have hardware trip points,
> but just a fixed number of them.
> 
> The way of-thermal works, is it reads all the trip points from the
> device tree, registers a new thermal_zone_device with that number of
> trip points and then handles the trip points completely independently.
> Of course, if we're just polling, this is fine, since the thermal core
> also knows about those trip points and will trigger cooling when polling
> the each zone. However, the driver doesn't, so it cannot setup any
> interrupts to call thermal_zone_device_update.

Is there any possibility of cleaning that up? It's obviously horribly
inconsistent if core driver functionality works completely differently
simply because the list of trip-points comes from DT rather than a
static table in the driver. of_thermal should be limited to DT parsing
and related device instantiation/lookup, not introducing a completely
different functionality model.

>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c

>>> +for (i = 0; i < data->ntrips; ++i) {
>>> +struct __thermal_trip *trip = data->trips + i;
>>> +long trip_low = trip->temperature - trip->hysteresis;
>>> +
>>> +if (trip_low < temp && trip_low > low)
>>> +low = trip_low;
>>> +
>>> +if (trip->temperature > temp && trip->temperature < high)
>>> +high = trip->temperature;
>>> +}
>>
>> That seems to always apply hysteresis to the low end of a trip object.
>> Don't you need to apply the hysteresis to either the low or high end of
>> the range, depending on whether the temperature is currently below/above
>> the range, and hence which direction the edge will be crossed?
> 
> I believe applying only to the low end is correct. Say that we have a
> trip point at 40C and hysteresis of 2C. When we exceed 40C cooling will
> start immediately, but it will only be stopped when we cool down to 38C.
> At that point there is again a 2C gap between the current temperature
> and the trip point. It would seem that this is the interpretation used
> by our downstream kernel and also some people on the Internet (however
> trustworthy they may be..)
> 
> If you don't feel this is right, please elaborate.

Ah, the point I was missing is that each trip point is a single
temperature, not a temperature range. As such, the code in your patch is
correct.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE mode

[Stephen Warren]

On 07/01/2014 01:16 AM, Vidya Sagar wrote:
> commit 1a680b7c moved pcie_aspm_powersave_config_link() out of
> pci_raw_set_power_state() to pci_set_power_state() which would enable
> ASPM. But, with commit db288c9c, which re-introduced the following check
> ./drivers/pci/pci.c: pci_set_power_state()
> +   /* Check if we're already there */
> +   if (dev->current_state == state)
> +   return 0;
> in pci_set_power_state(), call to pcie_aspm_powersave_config_link() is never
> made leaving ASPM broken.
> Fix it by not returning from when the above condition is true, rather, jump to
> ASPM configuration code and exit from there eventually.

Out of curiosity, was this patch tested by running an umodified mainline
kernel on a Tegra device, or is this simply a port from our downstream
kernel, without any additional upstream testing?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 04/10] memory: Add Tegra124 memory controller support

[Stephen Warren]

On 06/26/2014 02:49 PM, Thierry Reding wrote:
> From: Thierry Reding 
> 
> The memory controller on NVIDIA Tegra124 exposes various knobs that can
> be used to tune the behaviour of the clients attached to it.
> 
> Currently this driver sets up the latency allowance registers to the HW
> defaults. Eventually an API should be exported by this driver (via a
> custom API or a generic subsystem) to allow clients to register latency
> requirements.
> 
> This driver also registers an IOMMU (SMMU) that's implemented by the
> memory controller.

> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig

> +config TEGRA124_MC
> + bool "Tegra124 Memory Controller driver"
> + depends on ARCH_TEGRA

Does it make sense to default to y for system-level drivers like this?

> diff --git a/drivers/memory/tegra124-mc.c b/drivers/memory/tegra124-mc.c

As a general comment, I wonder why the Tegra124 code/data here is
ifdef'd based on CONFIG_ARCH_TEGRA_124_SOC but the Tegra132 code isn't
ifdef'd at all. I'd assert that the Tegra124 code is small enough it's
hardly worth worrying about ifdefs.

> +static inline void smmu_flush_ptc(struct tegra_smmu *smmu, struct page *page,
> +   unsigned long offset)
> +{
> + phys_addr_t phys = page ? page_to_phys(page) : 0;
> + u32 value;
> +
> + if (page) {
> + offset &= ~(smmu->soc->atom_size - 1);
> +
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> + value = (phys >> 32) & SMMU_PTC_FLUSH_HI_MASK;
> +#else
> + value = 0;
> +#endif

Shouldn't Tegra124 have CONFIG_PHYS_ADDR_T_64BIT defined, such that
there's no need for this ifdef? Certainly Tegra124 {has,can have} RAM
above 4GB physical, for some memory map layouts (i.e. non swiss cheese).

(I assume most of this code matches the existing Tegra30 SMMU driver, so
I didn't look at all of it that closely).

> +static int tegra_smmu_attach(struct iommu *iommu, struct device *dev)
...
> +#ifndef CONFIG_ARM64
> + return arm_iommu_attach_device(dev, group->mapping);
> +#else
> + return 0;
> +#endif

Hmm. Why must an SMMU driver for the exact same HW operate differently
depending on the CPU that's attached to the SoC? Surely the requirements
for how IOMMU drives should work should be the same for all architectures?

> +static int tegra_mc_probe(struct platform_device *pdev)

> + err = devm_request_irq(&pdev->dev, mc->irq, tegra124_mc_irq,
> +IRQF_SHARED, dev_name(&pdev->dev), mc);

I don't see any code in tegra_mc_remove() that guarantees that the IRQ
won't fire between tegra_mc_remove() returning, and the devm cleanup
code running to unhook that IRQ handler.

> diff --git a/include/dt-bindings/memory/tegra124-mc.h 
> b/include/dt-bindings/memory/tegra124-mc.h

This file is part of the DT binding, so should be added in the patch
that adds the binding.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 02/10] devicetree: Add generic IOMMU device tree bindings

[Stephen Warren]

On 06/26/2014 02:49 PM, Thierry Reding wrote:
> From: Thierry Reding 
> 
> This commit introduces a generic device tree binding for IOMMU devices.
> Only a very minimal subset is described here, but it is enough to cover
> the requirements of both the Exynos System MMU and Tegra SMMU as
> discussed here:
> 
> https://lkml.org/lkml/2014/4/27/346

> diff --git a/Documentation/devicetree/bindings/iommu/iommu.txt 
> b/Documentation/devicetree/bindings/iommu/iommu.txt

> +When an "iommus" property is specified in a device tree node, the IOMMU will
> +be used for address translation. If a "dma-ranges" property exists in the
> +device's parent node it will be ignored. An exception to this rule is if the
> +referenced IOMMU is disabled, in which case the "dma-ranges" property of the
> +parent shall take effect.

I wonder how useful that paragraph is. The fact that someone disabled a
particular IOMMU's node doesn't necessarily mean that the HW can
actually do that; an IOMMU might always be active in HW and always
translate accesses by some master. In that case, the fallback to
dma-ranges wouldn't correlate with what the HW actually does. Perhaps
all we need is to add a note to that effect here?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] thermal: Add Tegra SOCTHERM thermal management driver

[Stephen Warren]

On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds support for the Tegra SOCTHERM thermal sensing and management
> system found in the Tegra124 system-on-chip. This initial driver supports
> the four thermal zones with hardware-tracked trip points.

> diff --git a/drivers/thermal/tegra_soctherm.c 
> b/drivers/thermal/tegra_soctherm.c

> +static struct tegra_tsensor t124_tsensors[] = {
> + {
> + .base = 0xc0,
> + .name = "cpu0",
> + .config = &t124_tsensor_config,
> + .calib_fuse_offset = 0x098,
> + .fuse_corr_alpha = 1135400,
> + .fuse_corr_beta = -6266900,
> + },

I wonder why some of those fields are named "fuse_xxx" when the values
are hard-coded in these tables rather than read from fuses? These values
don't seem to be used to adjust values read from fuses.

> +static int tegra_thermctl_get_temp(void *data, long *out_temp)

> + switch (zone->sensor) {
> + case 0:
> + val = readl(zone->tegra->regs + SENSOR_TEMP1)
> + >> SENSOR_TEMP1_CPU_TEMP_SHIFT;

Can't the register offset and shift be stored in *zone, so that this
whole switch can be replaced with something generic:

val = readl(zone->tegra->regs + zone->reg_offset) >> zone->value_shift;

> +static int tegra_soctherm_probe(struct platform_device *pdev)

> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0) {
> + dev_err(&pdev->dev, "can't get interrupt\n");
> + return -EINVAL;
> + }

irq is assigned once here ... (see later)

> + for (i = 0; i < 4; ++i) {

Why "4"? Should the loop count be the ARRAY_SIZE(some array)? At the
very least, a named constant that describes the value would be useful...

> + err = devm_request_threaded_irq(&pdev->dev, irq, soctherm_isr,
> + soctherm_isr_thread,
> + IRQF_SHARED, "tegra_soctherm",
> + zone);

Why request the same IRQ 4 times here. Rather, shouldn't the IRQ be
requested once, and the ISR simply loop over the status register (or
whatever there are 4 of)?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] thermal: of: Add support for hardware-tracked trip points

[Stephen Warren]

On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds support for hardware-tracked trip points to the device tree
> thermal sensor framework.
> 
> The framework supports an arbitrary number of trip points. Whenever
> the current temperature is updated, the trip points immediately
> below and above the current temperature are found. A sensor driver
> callback `set_trips' is then called with the temperatures.
> If there is no trip point above or below the current temperature,
> the passed trip temperature will be LONG_MAX or LONG_MIN respectively.
> In this callback, the driver should program the hardware such that
> it is notified when either of these trip points are triggered.
> When a trip point is triggered, the driver should call
> `thermal_zone_device_update' for the respective thermal zone. This
> will cause the trip points to be updated again.
> 
> If the `set_trips' callback is not implemented (is NULL), the framework
> behaves as before.

Is there no "core thermal" code? I would have expected this new feature
to be implemented in "core" code rather than of/dt "support" code.
Perhaps there would also be some additions to the of/dt code, but I'd
still expect the bulk of the feature to be complete independant of
of/dt. Systems still using board files or ACPI or ... would surely
benefit from this too?

> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c

> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)

s/tz/tzd/ or s/tz/tzdev/ ? Since "tz" says "thermal zone" to me, but
it's actually a "thermal zone device".

> + struct __thermal_zone *data = tz->devdata;

s/data/tz/ ? "data" is a rather generic term, and "tz" seems like a good
abbreviation for a __thermal_zone.

> + for (i = 0; i < data->ntrips; ++i) {
> + struct __thermal_trip *trip = data->trips + i;
> + long trip_low = trip->temperature - trip->hysteresis;
> +
> + if (trip_low < temp && trip_low > low)
> + low = trip_low;
> +
> + if (trip->temperature > temp && trip->temperature < high)
> + high = trip->temperature;
> + }

That seems to always apply hysteresis to the low end of a trip object.
Don't you need to apply the hysteresis to either the low or high end of
the range, depending on whether the temperature is currently below/above
the range, and hence which direction the edge will be crossed?

Similar comments elsewhere. Perhaps the patch is consistent with some
existing confusing naming style though?

> +static int of_thermal_update_trips(struct thermal_zone_device *tz)
> +{
> + long temp;
> + int err;
> +
> + err = of_thermal_get_temp(tz, &temp);
> + if (err)
> + return err;
> +
> + err = of_thermal_set_trips(tz, temp);

Doesn't this patch modify of_thermal_get_temp() to call
of_thermal_set_trips() itself?

> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  struct thermal_zone_device *
>  thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>   void *data, int (*get_temp)(void *, long *),
> - int (*get_trend)(void *, long *))
> + int (*get_trend)(void *, long *),
> + int (*set_trips)(void *, long, long))

Passing in a struct containing fields for all the ops seem better than
forever extending this function with more and more function pointers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/6] ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree

[Stephen Warren]

On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds the soctherm thermal sensing and management unit to the
> Tegra124 device tree along with the four thermal zones it exports.

> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi

> + thermal-zones {
> + cpu {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;

I think we should still define a polling delay so that if there's SW
that doesn't support HW trip points/interrupts, it still knows how often
it should reasonably check the sensor.

Perhaps a delay of 0 is used to determine whether to use HW trip points
vs polling (I haven't read patch 1 yet)? If so, I'd prefer not to do
that. Rather, the driver should advertize its ability to provide HW trip
points, and it would be up to the core to then make use of them. The DT
should just describe the HW, not assume it can influence SW's choice of
whether to use HW trip points.

> + soctherm: soctherm@0,700e2000 {
...
> + reset-names = "soctherm";
> +
> + #thermal-sensor-cells = <1>;

I don't see any real need for that blank line. If there was, there would
probably be more blank lines in the big list of properties above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/6] ARM: tegra: Add thermal trip points for Jetson TK1

[Stephen Warren]

On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds critical trip points to the Jetson TK1 device tree.
> The device will do a controlled shutdown when either the CPU, GPU
> or MEM thermal zone reaches 101 degrees Celsius.

It would be more typical to order the patches with changes to
tegra124.dtsi first, then changes to board files (that build on the core
SoC support) second.

> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts 
> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts

> + thermal-zones {
> + cpu {
> + trips {
> + cpu-critical {

Can we name that simply "critical"? DT node names are supposed to
represent type of object more than identity of object. Even "critical"
is a bit of an identity, and "trip@0" would be better, but I imagine the
core thermal DT bindings precluded that?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] of: Add bindings for nvidia,tegra124-soctherm

[Stephen Warren]

On 06/27/2014 02:11 AM, Mikko Perttunen wrote:
> This adds binding documentation and headers for the Tegra124
> SOCTHERM device tree node.

> diff --git a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt 
> b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt

> +Required properties :
...
> +- #thermal-sensor-cells : For thermctl sensors. Should be 1.

I'd prefer a tiny bit more information here: A link to whatever other
document defines the meaning of #thermal-sensor-cells, and a link to the
header that defines the meaning of the data in the cell. Perhaps:

#thermal-sensor-cells : Should be 1. See ./thermal.txt for a description
of this property. See  for a
list of valid values.

> diff --git a/include/dt-bindings/thermal/tegra124-soctherm.h 
> b/include/dt-bindings/thermal/tegra124-soctherm.h
> +#endif
> +
> +

There are a couple of blank lines at the end of that file.

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

Re: [PATCH V2 0/6] regulator: palmas: cleanup and fixes

[Stephen Warren]

On 06/30/2014 09:57 AM, Nishanth Menon wrote:
> Hi,
>   Original thread (v1): http://marc.info/?t=14038076654&r=1&w=2
> This series is based on: 
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
> branch: topic/palmas (4c0c9ca Merge remote-tracking branch 
> 'regulator/fix/palmas' into regulator-palmas)
> 
> This series does a few cleanups to help ensure we dont make typo mistakes and 
> finally provides a fix as attempted by (v1).
> 
> Basic cpufreq tests performed on OMAP5uEVM, DRA7-evm, DRA72-evm all of which 
> are palmas variants.
> 
> The patches are also available here: 
> https://github.com/nmenon/linux-2.6-playground/commits/broonie-topic-palmas-fixes

The series,
Tested-by: Stephen Warren 

(An NVIDIA Dalmore board boots and shuts down OK with these applied on
top of next-20140630, and various peripherals such as LCD panel, audio,
USB, and SPI flash all work)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] ARM: tegra: roth: pinmux fixes

[Stephen Warren]

On 06/28/2014 10:27 PM, Alexandre Courbot wrote:
> On Sat, Jun 28, 2014 at 7:08 AM, Stephen Warren  wrote:
>> On 06/23/2014 01:32 AM, Alexandre Courbot wrote:
>>> Two small but important fixes to SHIELD's pinmux configuration.
>>> The use of invalid properties caused the pinmux to not be applied
>>> at all. Also the setting for sdmmc clock lines resulted in random
>>> errors or even the impossibility to probe attached devices.
>>>
>>> Alexandre Courbot (2):
>>>   ARM: tegra: roth: fix unsupported pinmux properties
>>>   ARM: tegra: roth: enable input on mmc clock pins
>>
>> The series, applied to Tegra's for-3.17/dt branch.
>>
>> Sorry for the delay; I'd forgotten that our internal discussion resolved
>> my questions about patch 2.
> 
> Thanks! And sorry for not mirroring the discussion on the public list.
> 
>> Still looking forward to internal bugs files against the Jetson TK1 and
>> Venice2 board pinmux spreadsheets for the same issue:-)
> 
> Are the spreadsheets you are talking about the following documents?
> 
> https://github.com/NVIDIA/tegra-pinmux-scripts/blob/master/configs/jetson-tk1.board
> 
> Or do we have internal structures we need to update as well?

That's data that is extracted from the internal spreadsheets. I'll try
and remember to send you the links internally (since they wouldn't be
useful to anyone on-list).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 5/5] clk: Add floor and ceiling constraints to clock rates

[Stephen Warren]

On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
> Adds a way for clock consumers to set maximum and minimum rates. This can be
> used for thermal drivers to set ceiling rates, or by misc. drivers to set
> floor rates to assure a minimum performance level.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c

> +static struct rate_constraint *__ensure_constraint(struct clk *clk_user,
> +enum constraint_type type)

> + if (!found) {
> + constraint = kzalloc(sizeof(*constraint), GFP_KERNEL);
> + if (!constraint) {
> + pr_err("%s: could not allocate constraint\n", __func__);

Doesn't kzalloc print an error itself if the allocation fails? I've
certainly seen quite a few patches ripping out custom "allocation
failed" errors in code.

> +void __clk_free_clk(struct clk *clk_user)
> +{
> + struct clk_core *clk = clk_to_clk_core(clk_user);
> + struct rate_constraint *constraint;
> + struct hlist_node *tmp;
> +
> + hlist_for_each_entry_safe(constraint, tmp, &clk->rate_constraints, 
> node) {
> + if (constraint->dev_id == clk_user->dev_id &&
> + constraint->con_id == clk_user->con_id) {
> + hlist_del(&constraint->node);
> + kfree(constraint);

Perhaps the list of constraints should be indexed by the client clk
structure, so that test should be:

if (constraint->clk_user == clk_user)

It might be a bit more work, but perhaps the constraints should simply
be stored directly in the struct clk rater than the struct clk_core.
That would require a nested loop to apply constraints though; first over
each struct clk associated with a struct clk_core, then over each
constraints in that struct clk. It would slightly simplify
adding/removing constraints though, and store the constraints at their
"source".

> diff --git a/include/linux/clk.h b/include/linux/clk.h

> +int clk_set_floor_rate(struct clk *clk, unsigned long rate);

> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);

Additions functions to explicitly remove any previously requested
floor/ceiling rate might be useful. The same effect could be achieved by
a floor of 0 or a very high ceiling, but it feels cleaner to remove them.

Overall, this series seems to implement the right kind of concept to me.
It'll certainly stop us (NVIDIA at least) wanting to create all kinds of
"virtual" clock objects (and associated clock IDs and device tree clock
IDs) to achieve a similar effect.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 4/5] clk: per-user clock accounting for debug

[Stephen Warren]

On 06/27/2014 04:44 PM, Stephen Warren wrote:
> On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
>> From: Rabin Vincent 
>>
>> When a clock has multiple users, the WARNING on imbalance of
>> enable/disable may not show the guilty party since although they may
>> have commited the error earlier, the warning is emitted later when some
>> other user, presumably innocent, disables the clock.
>>
>> Provide per-user clock enable/disable accounting and disabler tracking
>> in order to help debug these problems.

>> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
>> index 91659b2..9657fc8 100644
>> --- a/include/linux/clk-private.h
>> +++ b/include/linux/clk-private.h
>> @@ -56,7 +56,11 @@ struct clk_core {
>>  };
>>  
>>  struct clk {
>> -struct clk_core clk;
>> +struct clk_core *core;
>> +unsigned intenable_count;
>> +const char  *dev_id;
>> +const char  *con_id;
> 
> Why not just store the "struct device *" there instead of pulling the
> name out of it, so ...

Oh, perhaps I'm confused here. These are the names that are used to look
up the clock, not the name of the device that looked it up.

It might still be nice to store the "struct device *" as well as the
clock name parameters.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 4/5] clk: per-user clock accounting for debug

[Stephen Warren]

On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
> From: Rabin Vincent 
> 
> When a clock has multiple users, the WARNING on imbalance of
> enable/disable may not show the guilty party since although they may
> have commited the error earlier, the warning is emitted later when some
> other user, presumably innocent, disables the clock.
> 
> Provide per-user clock enable/disable accounting and disabler tracking
> in order to help debug these problems.
> 
> NOTE: with this patch, clk_get_parent() behaves like clk_get(), i.e. it
> needs to be matched with a clk_put().  Otherwise, memory will leak.

> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index 91659b2..9657fc8 100644
> --- a/include/linux/clk-private.h
> +++ b/include/linux/clk-private.h
> @@ -56,7 +56,11 @@ struct clk_core {
>  };
>  
>  struct clk {
> - struct clk_core clk;
> + struct clk_core *core;
> + unsigned intenable_count;
> + const char  *dev_id;
> + const char  *con_id;

Why not just store the "struct device *" there instead of pulling the
name out of it, so ...

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c

>  void clk_disable(struct clk *clk_user)
>  {
> - __clk_disable_internal(clk_to_clk_core(clk_user));
> + struct clk_core *clk = clk_to_clk_core(clk_user);
> + unsigned long flags;
> +
> + flags = clk_enable_lock();
> + if (!WARN(clk_user->enable_count == 0,
> +   "incorrect disable clk dev %s con %s last disabler %pF\n",
> +   clk_user->dev_id, clk_user->con_id, clk_user->last_disable)) {

Here, you could do something like:

if (!clk_user->enable_count) {
dev_err(clk_user->dev, "", ...);
goto out;
}
...
out:
clk_enable_unlock(flags);
}

I suppose that has the disadvantage of not using WARN() so not
generating a full back-trace. Still, you could keep the use of WARN()
and pass as a parameter to the printf parameters dev_name(clk_user->dev)
rather than manually saving the fields separately.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 3/5] clk: use struct clk only for external API

[Stephen Warren]

On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
> From: Rabin Vincent 
> 
> In order to provide per-user accounting, this separates the struct clk
> used in the common clock framework into two structures 'struct clk_core'
> and 'struct clk'.  struct clk_core will be used for internal
> manipulation and struct clk will be used in the clock API
> implementation.
> 
> In this patch, struct clk is simply renamed to struct clk_core and a new
> struct clk is implemented which simply wraps it.  In the next patch, the
> new struct clk will be used to implement per-user clock enable
> accounting.

> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h

> -struct clk {
> +struct clk_core {
>   const char  *name;
>   const struct clk_ops*ops;
>   struct clk_hw   *hw;
>   struct module   *owner;
> - struct clk  *parent;
> + struct clk_core *parent;
>   const char  **parent_names;
> - struct clk  **parents;
> + struct clk_core **parents;
>   u8  num_parents;
>   u8  new_parent_index;
>   unsigned long   rate;
>   unsigned long   new_rate;
> - struct clk  *new_parent;
> - struct clk  *new_child;
> + struct clk_core *new_parent;
> + struct clk_core *new_child;
>   unsigned long   flags;
>   unsigned intenable_count;
>   unsigned intprepare_count;
> @@ -55,6 +55,10 @@ struct clk {
>   struct kref ref;
>  };
>  
> +struct clk {
> + struct clk_core clk;
> +};

I'm confused why that field isn't a pointer instead. Don't we want to
end up with the following data structure:

(dev a's) struct clk v
   struct clk_core -> struct clk_hw
(dev b's) struct clk ^

Where all 3 arrows are pointers? (and struct clk_core probably contains
a list of the struct clk that point at it).

Otherwise, we end up creating a whole struct clk_core for each client.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 0/5] Per-user clock constraints

[Stephen Warren]

On 06/27/2014 01:57 AM, Tomeu Vizoso wrote:
> Hi,
> 
> I'm retaking Rabin's patches [0] for splitting the clk API in two: one API for
> clk consumers and another for providers. The consumer API uses a clk structure
> that just keeps track of the consumer and has a reference to the actual
> clk_core struct, which is used internally.
> 
> I have kept a patch from Rabin that aims to aid in debugging nested
> enable/disable calls, though my personal aim is to allow more than one 
> consumer
> to influence the final, effective rate. For now this is limited to setting
> floor and ceiling constraints.
> 
> For those functions in the consumer clk API that were called from providers, I
> have added variants to clk-provider.h that are the same only that accept a
> clk_core instead. In this first version of the patchset, these functions are
> prepended with two underscores and have the _internal suffix at the end. Mike
> has stated his preference of not prefixing with underscores any public API and
> I agree with him, but we still need a way to distinguish e.g. clk_set_parent()
> in the provider API from that in the consumer API (and from the lock-less
> variant in clk-provider.h!).

The name clk_provider_set_rate would be a good hint that it's an API for
clock providers not consumers.

The name clk_core_set_rate would be a good hint that the function takes
a clk_core object rather than the clk (client) object.

Neither names see too unwieldy to me.

Anyway, that's the color of my bikeshed:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] ARM: tegra: roth: pinmux fixes

[Stephen Warren]

On 06/23/2014 01:32 AM, Alexandre Courbot wrote:
> Two small but important fixes to SHIELD's pinmux configuration.
> The use of invalid properties caused the pinmux to not be applied
> at all. Also the setting for sdmmc clock lines resulted in random
> errors or even the impossibility to probe attached devices.
> 
> Alexandre Courbot (2):
>   ARM: tegra: roth: fix unsupported pinmux properties
>   ARM: tegra: roth: enable input on mmc clock pins

The series, applied to Tegra's for-3.17/dt branch.

Sorry for the delay; I'd forgotten that our internal discussion resolved
my questions about patch 2.

Still looking forward to internal bugs files against the Jetson TK1 and
Venice2 board pinmux spreadsheets for the same issue:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/27/2014 03:19 PM, Andrew Bresticker wrote:
> On Thu, Jun 26, 2014 at 11:07 AM, Stephen Warren  
> wrote:
>> On 06/25/2014 06:06 PM, Andrew Bresticker wrote:
>>> On Wed, Jun 25, 2014 at 3:37 PM, Stephen Warren  
>>> wrote:
>>>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>>>> Add support for the on-chip XHCI host controller present on Tegra SoCs.
>>>>>
>>>>> The driver is currently very basic: it loads the controller with its
>>>>> firmware, starts the controller, and is able to service messages sent
>>>>> by the controller's firmware.  The hardware supports device mode as
>>>>> well as runtime power-gating, but support for these is not yet
>>>>> implemented here.
> 
>>>>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> 
>>>>> +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
>>>>> +  unsigned long rate)
>>>>
>>>>> + switch (rate) {
>>>>> + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
>>>>> + /* Reparent to PLLU_480M. Set div first to avoid 
>>>>> overclocking */
>>>>> + old_parent_rate = clk_get_rate(clk_get_parent(clk));
>>>>> + new_parent_rate = clk_get_rate(tegra->pll_u_480m);
>>>>> + div = new_parent_rate / rate;
>>>>> + ret = clk_set_rate(clk, old_parent_rate / div);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + ret = clk_set_parent(clk, tegra->pll_u_480m);
>>>>> + if (ret)
>>>>> + return ret;
>>>>
>>>> Don't you need to call clk_set_rate() again after reparenting, since the
>>>> divisor will be different, and the rounding too.
>>>
>>> Nope, the divider we set before remains in-tact after clk_set_parent().
>>
>> Oh I see, the clk_set_rate() call is setting up div so it's appropriate
>> after the new parent is selected.
>>
>> Wouldn't it be better to just stop the clock, assert reset, reparent the
>> clock, and then set the desired rate directly?
> 
> I'm not sure how that would be better than making it more obvious as
> to how we arrive at the final rate.  Keep in mind that the XHCI host
> is running at this point (we usually get the scale-up message as a
> USB3 device is being enumerated) and that disabling the clock and/or
> asserting reset to the SS partition clock may not be the best idea...

Oh, this happens while the device is running rather than when
initializing it? Applying reset is probably a bad idea then. Still,
perhaps stopping the clock for a short time is fine? What about:

clk_disable_unprepare(clk);
clk_set_parent(clk, tegra->pll_u_480m);
clk_set_rate(clk, rate);
clk_prepare_enable(clk);

That seems much more direct to me. The code above feels over-complex to me.

If the clock really can't be stopped, then I suppose the existing code
in the patch is fine. I'd like to see a final clk_get_rate(clk) call
added, and the value compared against the expected value, to make sure
no rounding/truncation of the divider happened though.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC 04/10] memory: Add Tegra124 memory controller support

[Stephen Warren]

On 06/27/2014 05:15 AM, Thierry Reding wrote:
> On Fri, Jun 27, 2014 at 01:07:04PM +0200, Arnd Bergmann wrote:
>> On Thursday 26 June 2014 22:49:44 Thierry Reding wrote:
>>> +static const struct tegra_mc_client tegra124_mc_clients[] = {
>>> +   {
>>> +   .id = 0x01,
>>> +   .name = "display0a",
>>> +   .swgroup = TEGRA_SWGROUP_DC,
>>> +   .smmu = {
>>> +   .reg = 0x228,
>>> +   .bit = 1,
>>> +   },
>>> +   .latency = {
>>> +   .reg = 0x2e8,
>>> +   .shift = 0,
>>> +   .mask = 0xff,
>>> +   .def = 0xc2,
>>> +   },
>>> +   }, {
>>
>> This is a rather long table that I assume would need to get duplicated
>> and modified for each specific SoC. Have you considered to put the 
>> information
>> into DT instead, as auxiliary data in the iommu specifier as provided by
>> the device?
> 
> Most of this data really is register information and I don't think that
> belongs in DT.

I agree. I think it's quite inappropriate to put information into DT
that could simply be put into a table in the driver. If the information
is put into DT, you have to define a fixed binding for it, munge the
table and data representation to fit DT's much less flexible (than C
structs/arrays) syntax, write a whole bunch of code to parse it back out
(at probably not do a good job with error-checking), all only to end up
with exactly the same C structs in the driver at the end of the process.
Oh, and if multiple SoCs use the same data values, you have to duplicate
those tables into at least the DTBs if not in the .dts files, whereas
with C you can just point at the same struct.

SoCs come out much less frequently than new boards (perhaps ignoring the
fact that we support a small subset of boards in mainline, so the
frequency isn't too dissimilar there). It makes good sense to put
board-to-board differences in DT, but I see little point in putting
static SoC information into DT.



signature.asc
Description: OpenPGP digital signature

Re: [RFC 04/10] memory: Add Tegra124 memory controller support

[Stephen Warren]

On 06/27/2014 05:08 AM, Thierry Reding wrote:
> On Fri, Jun 27, 2014 at 12:46:38PM +0300, Hiroshi DOyu wrote:
>>
>> Thierry Reding  writes:
>>
>>> From: Thierry Reding 
>>>
>>> The memory controller on NVIDIA Tegra124 exposes various knobs that can
>>> be used to tune the behaviour of the clients attached to it.
>>>
>>> Currently this driver sets up the latency allowance registers to the HW
>>> defaults. Eventually an API should be exported by this driver (via a
>>> custom API or a generic subsystem) to allow clients to register latency
>>> requirements.
>>>
>>> This driver also registers an IOMMU (SMMU) that's implemented by the
>>> memory controller.
>>>
>>> Signed-off-by: Thierry Reding 
>>> ---
>>>  drivers/memory/Kconfig   |9 +
>>>  drivers/memory/Makefile  |1 +
>>>  drivers/memory/tegra124-mc.c | 1945 
>>> ++
>>>  include/dt-bindings/memory/tegra124-mc.h |   30 +
>>>  4 files changed, 1985 insertions(+)
>>>  create mode 100644 drivers/memory/tegra124-mc.c
>>>  create mode 100644 include/dt-bindings/memory/tegra124-mc.h
>>
>> I prefer reusing the existing SMMU and having MC and SMMU separated
>> since most of SMMU code are not different from functionality POV, and
>> new MC features are quite independent of SMMU.
>>
>> If it's really convenient to combine MC and SMMU into one driver, we
>> could move "drivers/iomm/tegra-smmu.c" here first, and add MC features
>> on the top of it.
> 
> I'm not sure if we can do that, since the tegra-smmu driver is
> technically used by Tegra30 and Tegra114. We've never really made use of
> it, but there are device trees in mainline releases that contain the
> separate SMMU node.

The existing DT nodes do nothing more than instantiate the driver.
However, IIUC nothing actually uses the driver for any purpose, so if we
simply deleted those nodes or changed them incompatibly, there'd be no
functional difference. Perhaps this is stretching DT ABIness very
slightly, but I think it makes no practical difference.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/27/2014 09:00 AM, Felipe Balbi wrote:
> On Wed, Jun 25, 2014 at 04:30:48PM -0700, Andrew Bresticker wrote:
 +static int usb3_phy_power_on(struct phy *phy)
 +{
 + struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
 + int port = usb3_phy_to_port(phy);
 + int lane = padctl->usb3_ports[port].lane;
 + u32 value, offset;
 +
 + value = padctl_readl(padctl, XUSB_PADCTL_IOPHY_USB3_PADX_CTL2(port));
 + value &= ~((XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_MASK <<
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_MASK <<
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_SHIFT) |
 +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_MASK <<
 + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_SHIFT));
>>>
>>> Hmm. So there is a lot of "PHY" stuff here after all.
>>>
>>> However, the PHYs implemented here appear to implement very low-level
>>> I/O pad code, whereas the PHYs we have for our USB 2.0 controller are
>>> somewhat higher-level; they're more USB-oriented than just IO pad
>>> oriented. Do you know which level of abstraction a Linux PHY object is
>>> supposed to be? I could never get an answer when I asked before.
>>
>> The only other PHY driver I've worked with (Exynos USB2/3 PHYs) also
>> mainly only did low-level pad control stuff, but looking at a couple
>> of other USB PHYs (MSM, MV), there appear to be others that have
>> higher-level USB stuff in the PHY driver.  Perhaps Kishon or Felipe
>> could offer us some guidance?
> 
> well, if you're adding a new driver, I'd rather see folks moving over to
> the generic phy framework (drivers/phy) because we're trying really hard
> to get rid of drivers/usb/phy/. And I think, in your case, it's actually
> ok to use pinctrl because you actually are muxing pads to the USB3 PHY,
> you just do it lazyly.

What I'm looking for is a good definition of exactly what a PHY is
supposed to be in Linux.

Is it purely something that turns some IO pads/drivers off/on, and
nothing more?

Or, does the PHY concept encompass protocol-specific concepts such as
USB VBUS enable, USB VBUS detection, USB OTG switching,
UTMI-vs-ULPI-vs-HSIC selection... all of which are irrelevant of the PHY
(or at least IO pads) are used for SATA or PCIe instead.

This obviously affects which code goes in the PHY driver, and which in
the EHCI/XHCI controller driver.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] clk: tegra: export clock names for debugging

[Stephen Warren]

On 06/27/2014 06:19 AM, Peter De Schrijver wrote:
> On Thu, Jun 26, 2014 at 05:52:16PM +0200, Stephen Warren wrote:
>> On 06/26/2014 09:48 AM, Peter De Schrijver wrote:
>>> When writing a module for testing or debugging purposes, there is no way to
>>> get hold of clk handles. This patch solves this by exposing all valid clocks
>>> as clkdev's for the virtual device tegra-clk-debug.
>>
>> This is to support clk_get_sys()?
>>
> 
> Yes.
> 
>> I guess this seems fine, so feel free to apply it, but I slightly wonder
>> why not just include this change as part of the presumably local and
>> never-to-be-upstreamed test/dev module?
> 
> The idea is to allow local debug modules to get hold of the struct clk *. I
> don't think that's possible without using clk_get_sys()?

Well, for a debug hack you can always just add extra clock entries into
some DT node and so avoid clk_get_sys:-)

My point wasn't so much about not using clk_get_sys in a debug driver,
but more that if this patch is only required to support non-upstreamed
debug code, perhaps this patch should stay downstream along with that
non-upstreamed debug code. That said, I'm not really objecting to it,
just making a minor comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 06/25/2014 04:25 PM, Andrew Bresticker wrote:
> Thanks for the review Stephen!
> 
> On Wed, Jun 25, 2014 at 2:46 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> Add new bindings used for USB support by the Tegra XUSB pad controller.
>>> This includes additional PHY types, USB-specific pinconfig properties, etc.
>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt 
>>> b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
>>
>>> @@ -21,6 +21,12 @@ Required properties:
>>>- padctl
>>>  - #phy-cells: Should be 1. The specifier is the index of the PHY to 
>>> reference.
>>>See  for the list of valid 
>>> values.
>>> +- nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.
>>
>> Why does the padctrl code need access to the XUSB mailbox?
> 
> The XUSB firmware sends messages which make requests of the PHY (XUSB
> pad controller), such as idling/un-idling the HSIC PHYs or saving USB3
> PHY context.
> 
>> Isn't the padctrl HW module something that provides services to the XUSB
>> code. I would have expected the XUSB node to reference the padctrl node.
> 
> The XUSB padctrl HW does provide services to the XUSB host in the form
> of PHYs and it is through the PHY bindings that the host references
> the padctrl node.
> 
>> If notifications need to be sent back from XUSB padctrl to XUSB, then that
>> seems like an internal SW detail that doesn't need to be represented in DT.
> 
> I think you mean notifications need to be sent back from the XUSB host
> to the XUSB padctrl?  This is what the mailbox is for and I chose to
> have the padctrl refer to the mailbox since messages are sent from the
> mailbox which make requests to the PHY specifically and not the host
> (see above).

I've looked at the details of the mailbox messages a bit more now. It
seems that the firmware running on the XUSB controller sends a variety
of different messages, some of which are relevant to the XHCI controller
driver and some relevant to the PHY/PAD driver. It's a pity these
different message streams are intermixed, but I guess that's not changing.

As such, I think at this stage it does make sense for the mailbox to be
represented as a separate node, with each of the XHCI controller and USB
PADCTL nodes referring to the mailbox node by phandle.

I'm still not 100% sure about whether the PHY driver is the same level
of abstraction intended by the Linux kernel's PHY layer. Pending that
discussion's results, the "PHY" message from the firmware may not go to
a Linux kernel PHY but some layer above which might get subsumed into
the overall XHCI controller driver, which would change my argument above
a bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/25/2014 05:30 PM, Andrew Bresticker wrote:
> On Wed, Jun 25, 2014 at 3:12 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> In addition to the PCIe and SATA PHYs, the XUSB pad controller also
>>> supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
>>> PCIe or SATA lane and is mapped to one of the three UTMI ports.
>>>
>>
>>> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c 
>>> b/drivers/pinctrl/pinctrl-tegra-xusb.c
>>
>>> @@ -372,6 +720,193 @@ static int tegra_xusb_padctl_pinconf_group_set(struct 
>>> pinctrl_dev *pinctrl,
>>>   padctl_writel(padctl, regval, lane->offset);
>>>   break;
>>>
>>> + case TEGRA_XUSB_PADCTL_USB3_PORT_NUM:
>>> + if (value >= TEGRA_XUSB_PADCTL_USB3_PORTS) {
>>> + dev_err(padctl->dev, "Invalid USB3 port: 
>>> %lu\n",
>>> + value);
>>> + return -EINVAL;
>>> + }
>>> + if (!is_pcie_sata_lane(group)) {
>>> + dev_err(padctl->dev,
>>> + "USB3 port not applicable for pin 
>>> %d\n",
>>> + group);
>>> + return -EINVAL;
>>> + }
>>> + padctl->usb3_ports[value].lane = group;
>>> + break;
>>
>> It feels odd to use pinctrl for a SW-only purpose. In other words, that
>> chunk of code isn't writing the pinconf data to HW, but rather some
>> internal variable.
> 
> Well the mapping of lanes to USB3 ports is a hardware property and we
> do use it when programming the hardware later to choose which set of
> lane registers to program given a USB3 port, but it's true that it's
> not some value we program into HW directly.
> 
>> Perhaps it would make more sense for the DT binding to represent this
>> data directly in a custom property that's parsed at probe() time. That
>> way, pinctrl only touches "real" HW stuff.
> 
> I'm on the fence about this.  If you or others feel strongly about
> this then I can make it a separate DT property and move it out of the
> pinctrl properties.

I'd certainly prefer to use pinctrl bindings only for things that get
directly written into HW. Other configuration data should be easy to
retrieve directly from properties.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/25/2014 06:06 PM, Andrew Bresticker wrote:
> On Wed, Jun 25, 2014 at 3:37 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> Add support for the on-chip XHCI host controller present on Tegra SoCs.
>>>
>>> The driver is currently very basic: it loads the controller with its
>>> firmware, starts the controller, and is able to service messages sent
>>> by the controller's firmware.  The hardware supports device mode as
>>> well as runtime power-gating, but support for these is not yet
>>> implemented here.

>>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c

>>> +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
>>> +  unsigned long rate)
>>
>>> + switch (rate) {
>>> + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
>>> + /* Reparent to PLLU_480M. Set div first to avoid overclocking 
>>> */
>>> + old_parent_rate = clk_get_rate(clk_get_parent(clk));
>>> + new_parent_rate = clk_get_rate(tegra->pll_u_480m);
>>> + div = new_parent_rate / rate;
>>> + ret = clk_set_rate(clk, old_parent_rate / div);
>>> + if (ret)
>>> + return ret;
>>> + ret = clk_set_parent(clk, tegra->pll_u_480m);
>>> + if (ret)
>>> + return ret;
>>
>> Don't you need to call clk_set_rate() again after reparenting, since the
>> divisor will be different, and the rounding too.
> 
> Nope, the divider we set before remains in-tact after clk_set_parent().

Oh I see, the clk_set_rate() call is setting up div so it's appropriate
after the new parent is selected.

Wouldn't it be better to just stop the clock, assert reset, reparent the
clock, and then set the desired rate directly?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] clk: tegra: export clock names for debugging

[Stephen Warren]

On 06/26/2014 09:48 AM, Peter De Schrijver wrote:
> When writing a module for testing or debugging purposes, there is no way to
> get hold of clk handles. This patch solves this by exposing all valid clocks
> as clkdev's for the virtual device tegra-clk-debug.

This is to support clk_get_sys()?

I guess this seems fine, so feel free to apply it, but I slightly wonder
why not just include this change as part of the presumably local and
never-to-be-upstreamed test/dev module?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] clk: tegra124: init table updates

[Stephen Warren]

On 06/26/2014 09:26 AM, Peter De Schrijver wrote:
> Ensure some clocks critical for system operation are always on. Also enable
> csite for JTAG debugging and set the tsensor clock frequency for the upcoming
> soctherm driver.

Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/25/2014 05:02 PM, Andrew Bresticker wrote:
> On Wed, Jun 25, 2014 at 2:54 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> Add device-tree binding documentation for the XHCI controller present
>>> on Tegra124 and later SoCs.
>>
>>> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
>>> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt
>>
>>> + - clock-names: Must include the following entries:
>>> +- xusb_host
>>> +- xusb_falcon_src
>>> +- xusb_ss
>>> +- xusb_ss_src
>>> +- xusb_hs_src
>>> +- xusb_fs_src
>>
>> Looking at include/dt-bindings/clock/tegra124-car.h I see a few entries
>> potentially missing here:
>>
>> #define TEGRA124_CLK_XUSB_HOST_SRC 252
>> #define TEGRA124_CLK_XUSB_DEV_SRC 256
>> #define TEGRA124_CLK_XUSB_DEV 257
>> #define TEGRA124_CLK_XUSB_SS_DIV2 312
> 
> The driver doesn't use them, so I didn't put them in the binding.

I think we should add them in case we need them later. Best to fully
describe the HW rather than the parts of the HW that SW currently uses.

>>> +- pll_u_480m
>>
>> Not just pll_u?
> 
> We specifically want pll_u_480M as that's what we use as the parent of
> xusb_ss_src when scaling it to 120Mhz.

OK. I recall text in the TRM implying that SW should just leave PLL_U
alone and not fiddle with the separate output clocks. Still, if we have
a clock ID for each output, and it's the correct parent for the clock,
then it does make sense to use that ID.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/25/2014 05:01 PM, Andrew Bresticker wrote:
> On Wed, Jun 25, 2014 at 2:52 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> Add device-tree binding documentation for the XHCI controller present
>>> on Tegra124 and later SoCs.
>>
>>> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
>>> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt
>>
>>> +Required properties:
>>> +
>>
>>> + - clock-names: Must include the following entries:
>>> +- xusb_host
>>> +- xusb_falcon_src
>>> +- xusb_ss
>>> +- xusb_ss_src
>>> +- xusb_hs_src
>>> +- xusb_fs_src
>>> +- pll_u_480m
>>> +- clk_m
>>> +- pll_e
>>
>>> + - reset-names: Must include the following entries:
>>> +   - xusb_host
>>> +   - xusb_ss
>>
>> Usually the CAR has a reset control for each clock. So, I would expect
>> as many entries in reset-names as in clock-names. Even if the SW doesn't
>> currently touch all the reset lines, we should make sure the binding
>> requires them to be present so that any DT will contain the entries if
>> they're ever needed in the future.
> 
> The xusb_{falcon,host,hs,fs,ss}_src clocks all share the same reset
> bit (143), so I can add a single entry for those.
> 
>> In the CAR documentation, I see "XUSB_DEV" as a clock/reset bit. Is that
>> missing from the list above?
> 
> This is used when XUSB is in device mode, which the driver does not
> support.  I can add those clocks here though if you want.

That'd be a good idea. That way, the DT doesn't have to change later.

>>> +Optional properties:
>>
>>> + - s1p05v-supply: 1.05V supply regulator.
>>> + - s1p8v-supply: 1.8V supply regulator.
>>> + - s3p3v-supply: 3.3V supply regulator.
>>
>> What are those supplies for? I would have expected any input to the SoC
>> to have a name that described its purpose, and the pins and DT
>> properties would be named to match.
> 
> I *think* this what they are from looking at the schematic, but I'll
> have to ask around:
>  - s1p05v: avddio_pex, dvddio_pex, and maybe avdd_pll_erefe
>  - s1p8v: avdd_pll_utmip
>  - s3p3v: avdd_usb, hvdd_pex, hvdd_pex_pll_e
> Should these be separated out as they are for PCIe?

Yes, I think they should be separated. I wonder if the supplies for PCIe
shouldn't have been added to the XUSB padctrl rather than PCIe node
though? It probably doesn't matter much either way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 06/25/2014 04:37 PM, Andrew Bresticker wrote:
> On Wed, Jun 25, 2014 at 2:42 PM, Stephen Warren  wrote:
>> On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
>>> Add device-tree bindings for the Tegra XUSB mailbox which will be used
>>> for communication between the Tegra XHCI controller and the host.
>>
>> Sorry for the slow review.
>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt 
>>> b/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt
>>
>>> +NVIDIA Tegra XUSB mailbox
>>> +=
>>> +
>>> +The Tegra XUSB mailbox is used by the Tegra XHCI controller's firmware to
>>> +communicate with the host.
>>
>> Isn't the mailbox an internal implementation detail of the XUSB controller.
>>
>> In other words, I'd naively think that there isn't a standalone generic
>> mailbox that can be used by anything, but we just happen to want to use
>> for XUSB. Rather, there's an XUSB controller, and part of the interface
>> to that controller is a mailbox.
> 
> Yes, the mailbox isn't an actual piece of hardware but rather the
> interface through which the XUSB host and AP communicate.
> 
>> As such, I don't think we want a standalone mailbox node in DT. Rather,
>> we should add the required reg and interrupt values into the XUSB DT node.
>>
>> The driver for that XUSB HW module can either:
>>
>> a) Register as both a mailbox driver and an EHCI driver.
>>
>> b) Spawn a child device to instantiate the mailbox driver.
>>
>> Perhaps (b) could be assisted by using the MFD framework?
> 
> So in the RFC series I did something like (a) where the XUSB host
> handled the mailbox interrupt with both the PHY and host could
> registering notifiers to handle the messages.  It was suggested by
> Arnd though that I make a separate mailbox driver.  Instead of having
> a both a host and mailbox node, I could have a single XUSB host node
> and have the mailbox driver bind to that - thoughts?

Yes, that sounds like what I meant by (b) above. I don't think you can
actually have 2 drivers bind to the same DT node though, so it'd have to
work something like:

* XUSB host node causes a platform device to be instantiated
* XUSB host driver probe()s against that
* XUSB host driver's probe() creates a platform device for the mailbox
* XUSB mailbox driver probe()s against that.

Or, perhaps go completely MFD, and have 2 child devices (XUSB host and
XUSB mailbox) instantiated by the MFD parent, which is what is in the DT.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> Add support for the on-chip XHCI host controller present on Tegra SoCs.
> 
> The driver is currently very basic: it loads the controller with its
> firmware, starts the controller, and is able to service messages sent
> by the controller's firmware.  The hardware supports device mode as
> well as runtime power-gating, but support for these is not yet
> implemented here.
> 
> Based on work by:
>   Ajay Gupta 
>   Bharath Yadav 

> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig

> +config USB_XHCI_TEGRA
> + tristate "NVIDIA Tegra XHCI support"
> + depends on ARCH_TEGRA
> + select PINCTRL_TEGRA_XUSB
> + select TEGRA_XUSB_MBOX
> + select FW_LOADER

I think at least some of those should be depends. In particular, the
mbox driver patch said:

+config TEGRA_XUSB_MBOX
+   bool "NVIDIA Tegra XUSB mailbox support"

which means the option is user-selectable. Either MBOX should be
invisible and selected here, or it should be visible with USB_XHCI_TEGRA
depending on it.

> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c

> +#define TEGRA_XHCI_UTMI_PHYS 3
> +#define TEGRA_XHCI_HSIC_PHYS 2
> +#define TEGRA_XHCI_USB3_PHYS 2
> +#define TEGRA_XHCI_MAX_PHYS (TEGRA_XHCI_UTMI_PHYS + TEGRA_XHCI_HSIC_PHYS + \
> +  TEGRA_XHCI_USB3_PHYS)

Do those numbers need to be synchronized with the XUSB padctrl driver at
all?

> +static u32 csb_readl(struct tegra_xhci_hcd *tegra, u32 addr)
> +{
> + u32 page, offset;
> +
> + page = CSB_PAGE_SELECT(addr);
> + offset = CSB_PAGE_OFFSET(addr);
> + fpci_writel(tegra, page, XUSB_CFG_ARU_C11_CSBRANGE);
> + return fpci_readl(tegra, XUSB_CFG_CSB_BASE_ADDR + offset);
> +}

I assume some higher level has the required locking or single-threading
so that the keyhole register accesses don't get interleaved?

> +static void tegra_xhci_cfg(struct tegra_xhci_hcd *tegra)
> +{
> + u32 reg;
> +
> + reg = ipfs_readl(tegra, IPFS_XUSB_HOST_CONFIGURATION_0);
> + reg |= IPFS_EN_FPCI;
> + ipfs_writel(tegra, reg, IPFS_XUSB_HOST_CONFIGURATION_0);
> + udelay(10);
> +
> + /* Program Bar0 Space */
> + reg = fpci_readl(tegra, XUSB_CFG_4);
> + reg |= tegra->hcd->rsrc_start;

Don't you need to mask out the original value here? I guess whatever is
being written is probably always the same, but it seems scary to assume
that a bootloader, or previous version of a module during development,
didn't write something unexpected there. Perhaps if the HW module's
reset is pulsed we don't need to worry though.

> +static int tegra_xhci_load_firmware(struct tegra_xhci_hcd *tegra)
> +{
> + struct device *dev = tegra->dev;
> + struct tegra_xhci_fw_cfgtbl *cfg_tbl;
> + u64 fw_base;
> + u32 val;
> + time_t fw_time;
> + struct tm fw_tm;
> +
> + if (csb_readl(tegra, XUSB_CSB_MP_ILOAD_BASE_LO) != 0) {
> + dev_info(dev, "Firmware already loaded, Falcon state 0x%x\n",
> +  csb_readl(tegra, XUSB_FALC_CPUCTL));
> + return 0;
> + }
> +
> + cfg_tbl = (struct tegra_xhci_fw_cfgtbl *)tegra->fw_data;

Are there endianness or CPU word size (e.g. ARMv8) issues here; this is
casting the content of a data file to a CPU data structure.

> +static int tegra_xhci_set_ss_clk(struct tegra_xhci_hcd *tegra,
> +  unsigned long rate)

> + switch (rate) {
> + case TEGRA_XHCI_SS_CLK_HIGH_SPEED:
> + /* Reparent to PLLU_480M. Set div first to avoid overclocking */
> + old_parent_rate = clk_get_rate(clk_get_parent(clk));
> + new_parent_rate = clk_get_rate(tegra->pll_u_480m);
> + div = new_parent_rate / rate;
> + ret = clk_set_rate(clk, old_parent_rate / div);
> + if (ret)
> + return ret;
> + ret = clk_set_parent(clk, tegra->pll_u_480m);
> + if (ret)
> + return ret;

Don't you need to call clk_set_rate() again after reparenting, since the
divisor will be different, and the rounding too.

> +static int tegra_xhci_regulator_enable(struct tegra_xhci_hcd *tegra)
> +{
> + int ret;
> +
> + ret = regulator_enable(tegra->s3p3v_reg);
> + if (ret < 0)
> + return ret;
> + ret = regulator_enable(tegra->s1p8v_reg);
> + if (ret < 0)
> + goto disable_s3p3v;
> + ret = regulator_enable(tegra->s1p05v_reg);
> + if (ret < 0)
> + goto disable_s1p8v;

Would regulator_bulk_enable() save any code here? Similar in _disable().

> +static const struct tegra_xhci_soc_config tegra124_soc_config = {
> + .firmware_file = "tegra12x/tegra_xusb_firmware",
> +};

I would prefer an "nvidia/" prefix so everything gets namespaced by vendor.

"tegra12x" isn't the name of the chip, but rather "Tegra124".

"tegra_" and "_firmware" seem redundant, since they're implied by parent
directories.

So, how about "

Re: [PATCH v1 4/9] pinctrl: tegra-xusb: Add USB PHY support

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> In addition to the PCIe and SATA PHYs, the XUSB pad controller also
> supports 3 UTMI, 2 HSIC, and 2 USB3 PHYs.  Each USB3 PHY uses a single
> PCIe or SATA lane and is mapped to one of the three UTMI ports.
> 

> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c 
> b/drivers/pinctrl/pinctrl-tegra-xusb.c

> @@ -372,6 +720,193 @@ static int tegra_xusb_padctl_pinconf_group_set(struct 
> pinctrl_dev *pinctrl,
>   padctl_writel(padctl, regval, lane->offset);
>   break;
>  
> + case TEGRA_XUSB_PADCTL_USB3_PORT_NUM:
> + if (value >= TEGRA_XUSB_PADCTL_USB3_PORTS) {
> + dev_err(padctl->dev, "Invalid USB3 port: %lu\n",
> + value);
> + return -EINVAL;
> + }
> + if (!is_pcie_sata_lane(group)) {
> + dev_err(padctl->dev,
> + "USB3 port not applicable for pin %d\n",
> + group);
> + return -EINVAL;
> + }
> + padctl->usb3_ports[value].lane = group;
> + break;

It feels odd to use pinctrl for a SW-only purpose. In other words, that
chunk of code isn't writing the pinconf data to HW, but rather some
internal variable.

Perhaps it would make more sense for the DT binding to represent this
data directly in a custom property that's parsed at probe() time. That
way, pinctrl only touches "real" HW stuff.

> +static int usb3_phy_power_on(struct phy *phy)
> +{
> + struct tegra_xusb_padctl *padctl = phy_get_drvdata(phy);
> + int port = usb3_phy_to_port(phy);
> + int lane = padctl->usb3_ports[port].lane;
> + u32 value, offset;
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_IOPHY_USB3_PADX_CTL2(port));
> + value &= ~((XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_MASK <<
> + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_WANDER_SHIFT) |
> +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_MASK <<
> + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_RX_EQ_SHIFT) |
> +(XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_MASK <<
> + XUSB_PADCTL_IOPHY_USB3_PAD_CTL2_CDR_CNTL_SHIFT));

Hmm. So there is a lot of "PHY" stuff here after all.

However, the PHYs implemented here appear to implement very low-level
I/O pad code, whereas the PHYs we have for our USB 2.0 controller are
somewhat higher-level; they're more USB-oriented than just IO pad
oriented. Do you know which level of abstraction a Linux PHY object is
supposed to be? I could never get an answer when I asked before.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> The Tegra XHCI controller communicates requests to the host through
> a mailbox interface.  Host drivers which can handle these requests,
> such as the Tegra XUSB pad controller driver and upcoming Tegra XHCI
> host controller driver, can send messages and register to be notified
> of incoming messages.

> diff --git a/include/linux/tegra-xusb-mbox.h b/include/linux/tegra-xusb-mbox.h

> +extern int tegra_xusb_mbox_register_notifier(struct tegra_xusb_mbox *mbox,
> +  struct notifier_block *nb);
> +extern void tegra_xusb_mbox_unregister_notifier(struct tegra_xusb_mbox *mbox,
> + struct notifier_block *nb);
> +extern int tegra_xusb_mbox_send(struct tegra_xusb_mbox *mbox,
> + enum tegra_xusb_mbox_cmd cmd, u32 data);
> +extern struct tegra_xusb_mbox *
> +tegra_xusb_mbox_lookup_by_phandle(struct device_node *np, const char *prop);

This seems to use a custom API. I've seen mention of a mailbox
subsystem, and I assume that has a standardized API. Should this driver
implement that instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> Add device-tree binding documentation for the XHCI controller present
> on Tegra124 and later SoCs.

> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

> + - clock-names: Must include the following entries:
> +- xusb_host
> +- xusb_falcon_src
> +- xusb_ss
> +- xusb_ss_src
> +- xusb_hs_src
> +- xusb_fs_src

Looking at include/dt-bindings/clock/tegra124-car.h I see a few entries
potentially missing here:

#define TEGRA124_CLK_XUSB_HOST_SRC 252
#define TEGRA124_CLK_XUSB_DEV_SRC 256
#define TEGRA124_CLK_XUSB_DEV 257
#define TEGRA124_CLK_XUSB_SS_DIV2 312

> +- pll_u_480m

Not just pll_u?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 5/9] of: Add NVIDIA Tegra XHCI controller binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> Add device-tree binding documentation for the XHCI controller present
> on Tegra124 and later SoCs.

> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt 
> b/Documentation/devicetree/bindings/usb/nvidia,tegra124-xhci.txt

> +Required properties:
> +

> + - clock-names: Must include the following entries:
> +- xusb_host
> +- xusb_falcon_src
> +- xusb_ss
> +- xusb_ss_src
> +- xusb_hs_src
> +- xusb_fs_src
> +- pll_u_480m
> +- clk_m
> +- pll_e

> + - reset-names: Must include the following entries:
> +   - xusb_host
> +   - xusb_ss

Usually the CAR has a reset control for each clock. So, I would expect
as many entries in reset-names as in clock-names. Even if the SW doesn't
currently touch all the reset lines, we should make sure the binding
requires them to be present so that any DT will contain the entries if
they're ever needed in the future.

In the CAR documentation, I see "XUSB_DEV" as a clock/reset bit. Is that
missing from the list above?

> + - nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.

As mentioned earlier, I think that's an internal implementation detail.
Shouldn't the two nodes be squashed together?

> +Optional properties:

> + - s1p05v-supply: 1.05V supply regulator.
> + - s1p8v-supply: 1.8V supply regulator.
> + - s3p3v-supply: 3.3V supply regulator.

What are those supplies for? I would have expected any input to the SoC
to have a name that described its purpose, and the pins and DT
properties would be named to match.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/9] of: Add NVIDIA Tegra XUSB mailbox binding

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> Add device-tree bindings for the Tegra XUSB mailbox which will be used
> for communication between the Tegra XHCI controller and the host.

Sorry for the slow review.

> diff --git 
> a/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt 
> b/Documentation/devicetree/bindings/mailbox/nvidia,tegra124-xusb-mbox.txt

> +NVIDIA Tegra XUSB mailbox
> +=
> +
> +The Tegra XUSB mailbox is used by the Tegra XHCI controller's firmware to
> +communicate with the host.

Isn't the mailbox an internal implementation detail of the XUSB controller.

In other words, I'd naively think that there isn't a standalone generic
mailbox that can be used by anything, but we just happen to want to use
for XUSB. Rather, there's an XUSB controller, and part of the interface
to that controller is a mailbox.

As such, I don't think we want a standalone mailbox node in DT. Rather,
we should add the required reg and interrupt values into the XUSB DT node.

The driver for that XUSB HW module can either:

a) Register as both a mailbox driver and an EHCI driver.

b) Spawn a child device to instantiate the mailbox driver.

Perhaps (b) could be assisted by using the MFD framework?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 3/9] of: Update Tegra XUSB pad controller binding for USB

[Stephen Warren]

On 06/18/2014 12:16 AM, Andrew Bresticker wrote:
> Add new bindings used for USB support by the Tegra XUSB pad controller.
> This includes additional PHY types, USB-specific pinconfig properties, etc.

> diff --git 
> a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt 
> b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt

> @@ -21,6 +21,12 @@ Required properties:
>- padctl
>  - #phy-cells: Should be 1. The specifier is the index of the PHY to 
> reference.
>See  for the list of valid 
> values.
> +- nvidia,xusb-mbox: Handle to the Tegra XUSB mailbox node.

Why does the padctrl code need access to the XUSB mailbox? Isn't the
padctrl HW module something that provides services to the XUSB code. I
would have expected the XUSB node to reference the padctrl node. If
notifications need to be sent back from XUSB padctrl to XUSB, then that
seems like an internal SW detail that doesn't need to be represented in DT.

> +Optional properties:
> +---
> +- vbus-otg-{0,1,2}-supply: VBUS regulator for the corresponding UTMI pad.

Isn't VBUS something that's controlled by the USB PHY? I think the PHYs
are part of the XUSB controller, whereas the XUSB pad control is just
the low level IO pad HW.

> +- nvidia,usb3-port-num: USB3 port (0 or 1) to which the lane is mapped.
> +- nvidia,usb2-port-num: USB2 port (0, 1, or 2) to which the lane is mapped.
> +- nvidia,hsic-strobe-trim: HSIC strobe trimmer value.
> +- nvidia,hsic-rx-strobe-trim: HSIC RX strobe trimmer value.
> +- nvidia,hsic-rx-data-trim: HSIC RX data trimmer value.
> +- nvidia,hsic-tx-rtune-n: HSIC TX RTUNEN value.
> +- nvidia,hsic-tx-rtune-p: HSIC TX RTUNEP value.
> +- nvidia,hsic-tx-slew-n: HSIC TX SLEWN value.
> +- nvidia,hsic-tx-slew-p: HSIC TX SLEWP value.
> +- nvidia,hsic-auto-term: Enables HSIC AUTO_TERM. (0: no, 1: yes)

I wonder if some of that isn't part of the PHY not the pads?

>  Valid functions for this group are: "snps", "xusb", "uart", "rsvd".
>  
> -The nvidia,iddq property does not apply to this group.
> +The nvidia,iddq, nvidia,usb3-port-num, nvidia,usb2-port-num, and
> +nvidia,hsic-* properties do not apply to this group.

Given the increased number of properties we now have, it seems better to
list the properties that *do* apply to each group, rather than those
that don't.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] clk: tegra: fix vi_sensor clocks on Tegra124

[Stephen Warren]

On 06/25/2014 10:10 AM, Peter De Schrijver wrote:
> vi_sensor and vi_sensor2 have a wrong hw clkid on Tegra124. Fix this by
> correcting the hw clkid for Tegra124 and creating the Tegra114 vi_sensor clock
> from its own data. Tegra124 was also using the wrong internal clock id.

> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c

> - MUX("vi_sensor2", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI_SENSOR2, 
> 20, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor2),
> + MUX("vi_sensor2", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI_SENSOR2, 
> 165, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor2),
...
> - MUX8("vi_sensor", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI_SENSOR, 
> 20, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor_8),
> + MUX8("vi_sensor", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI_SENSOR, 
> 164, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor_8),

If I'm reading the TRM right, these are CAM_MCLK/CAM_MCLK2 in the
RST_DEV_X register. Is the TRM simply inconsistent in the naming of
these clocks, or is there some other inconsistency?

> diff --git a/drivers/clk/tegra/clk-tegra114.c 
> b/drivers/clk/tegra/clk-tegra114.c

> @@ -777,7 +784,6 @@ static struct tegra_clk tegra114_clks[tegra_clk_max] 
> __initdata = {
>   [tegra_clk_spdif_in] = { .dt_id = TEGRA114_CLK_SPDIF_IN, .present = 
> true },
>   [tegra_clk_spdif_out] = { .dt_id = TEGRA114_CLK_SPDIF_OUT, .present = 
> true },
>   [tegra_clk_vi_8] = { .dt_id = TEGRA114_CLK_VI, .present = true },
> - [tegra_clk_vi_sensor_8] = { .dt_id = TEGRA114_CLK_VI_SENSOR, .present = 
> true },

Does it make any sense to
s/tegra_clk_vi_sensor_8/tegra_clk_vi_sensor_114/ and put the definition
into the table in clk-tegra-periph.c instead? I suppose if this clock
definition is specific to Tegra114 there's not much point, so this is
probably fine. Hopefully the new tegra_clk_vi_sensor_8 lasts longer than
just Tegra124...

So overall, I think:
Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: tegra: Migrate Apalis T30 PCIe power supply scheme

[Stephen Warren]

On 06/25/2014 02:10 PM, Marcel Ziswiler wrote:
> This migration is required for continued PCIe operation after commit:
> 
> d3c7e24b84fcb62f96fa9a585a41f1829f326878

Thanks. I've applied this to Tegra's for-3.17/dt branch, where this file
was added. I also shortened the commit hash and added the commit subject
in the description, so it's more human-readable.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>> irq_work_cpu_notify() altogether?
> 
> Just so...
> 
> getting up at 6am and sitting in an airport terminal doesn't seem to
> agree with me; any more silly fail here?
> 
> ---
> Subject: irq_work: Remove BUG_ON in irq_work_run()
> From: Peter Zijlstra 
> Date: Wed Jun 25 07:13:07 CEST 2014
> 
> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> pending IPI callbacks before CPU offline"), which ends up calling
> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> is not from IRQ context.
> 
> And since that already calls irq_work_run() from the hotplug path,
> remove our entire hotplug handling.

Tested-by: Stephen Warren 

[with the s/static// already mentioned in this thread, obviously:-)]

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

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
> irq work currently only supports local callbacks. However its code
> is mostly ready to run remote callbacks and we have some potential user.
> 
> The full nohz subsystem currently open codes its own remote irq work
> on top of the scheduler ipi when it wants a CPU to reevaluate its next
> tick. However this ad hoc solution bloats the scheduler IPI.
> 
> Lets just extend the irq work subsystem to support remote queuing on top
> of the generic SMP IPI to handle this kind of user. This shouldn't add
> noticeable overhead.

I'm running next-20140624 on an ARM system, and this patch causes CPU
hot(un)plug to Oops for me; the following fires:

void irq_work_run(void)
{
BUG_ON(!in_irq());

I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
works fine. I found that this commit inside the tip(?) tree works fine
(478850160636 "irq_work: Implement remote queueing"). However, if I
merge the two together, I hit that BUG_ON.

I think the issue is:

This commit adds a call from
generic_smp_call_function_single_interrupt() to irq_work_run().

Srivatsa's patch adds a call from hotplug_cfd() to
flush_smp_call_function_queue() to, which I imagine happens in
non-interrupt context. Note that this patch moves most of the body of
generic_smp_call_function_single_interrupt() into
flush_smp_call_function_queue().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/24/2014 02:33 PM, Stephen Warren wrote:
> On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
>> irq work currently only supports local callbacks. However its code
>> is mostly ready to run remote callbacks and we have some potential user.
>>
>> The full nohz subsystem currently open codes its own remote irq work
>> on top of the scheduler ipi when it wants a CPU to reevaluate its next
>> tick. However this ad hoc solution bloats the scheduler IPI.
>>
>> Lets just extend the irq work subsystem to support remote queuing on top
>> of the generic SMP IPI to handle this kind of user. This shouldn't add
>> noticeable overhead.
> 
> I'm running next-20140624 on an ARM system, and this patch causes CPU
> hot(un)plug to Oops for me; the following fires:
> 
> void irq_work_run(void)
> {
>   BUG_ON(!in_irq());
> 
> I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
> of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
> works fine. I found that this commit inside the tip(?) tree works fine
> (478850160636 "irq_work: Implement remote queueing"). However, if I
> merge the two together, I hit that BUG_ON.

I forgot to mention that the conflicting commit from Linus' tree is
8d056c48e486 "CPU hotplug, smp: flush any pending IPI callbacks before
CPU offline".

> I think the issue is:
> 
> This commit adds a call from
> generic_smp_call_function_single_interrupt() to irq_work_run().
> 
> Srivatsa's patch adds a call from hotplug_cfd() to
> flush_smp_call_function_queue() to, which I imagine happens in
> non-interrupt context. Note that this patch moves most of the body of
> generic_smp_call_function_single_interrupt() into
> flush_smp_call_function_queue().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 6/7] ata: Add support for the Tegra124 SATA controller

[Stephen Warren]

On 06/18/2014 08:23 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.

At a quick glance, this looks fine to me now. I'll wait for an ack to
take this patch through the Tegra tree, for the reasons I mentioned in
response to v1.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 5/7] clk: tegra: Add SATA clocks to Tegra124 initialization table

[Stephen Warren]

On 06/18/2014 08:23 AM, Mikko Perttunen wrote:
> This adds two clocks, SATA and SATA_OOB, to the Tegra124 clock initialization
> table. The clocks are needed for working SATA support.

Acked-by: Stephen Warren 
(When I wrote that for v1, it applied to both patches 4 and 5, not just
patch 4)

Peter, can you please pick up patches 4 and 5, and get them into
linux-next. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/6] include/linux/platform_data/tegra_emc.h: Remove unused header

[Stephen Warren]

On 06/24/2014 11:27 AM, Rasmus Villemoes wrote:
> The header file include/linux/platform_data/tegra_emc.h does not seem
> to be used anywhere. It was orphaned by a7cbe92c "ARM: tegra: remove
> tegra EMC scaling driver". Remove it.

Acked-by: Stephen Warren 

Perhaps this should go through the arm-soc tree like the original
commit, but it doesn't really matter either way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ARM: tegra: roth: enable input on mmc clock pins

[Stephen Warren]

On 06/23/2014 11:44 PM, Alexandre Courbot wrote:
> On 06/24/2014 04:01 AM, Stephen Warren wrote:
>> On 06/23/2014 01:32 AM, Alexandre Courbot wrote:
>>> Input had been disabled by mistake on these pins, leading to issues with
>>> SDIO devices like the Wifi module not being probed or random errors
>>> occuring on the SD card.
>>
>> I thought the host controller always drove the clock, so there should be
>> no need for the pin's input path to be enabled. Perhaps it depends on
>> the transfer mode (e.g. UHS)?
> 
> That's what I thought too, so I went against what was done downstream
> and disabled input mode. Eventually noticed various issues with MMC
> devices, reverted to the downstream settings and noticed my problems
> were solved by this single change.

Hmm. That's odd. Can you talk to one of the HW engineers behind the
SDHCI controller and get a definitive answer. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/23/2014 02:20 PM, Stephen Warren wrote:
> On 06/23/2014 02:11 PM, Nishanth Menon wrote:
>> On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren  
>> wrote:
>>> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>>>> We use regmap regulator ops to enable/disable and check if regulator
>>>> is enabled for various SMPS. However, these depend on valid
>>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>>
>>>> Currently we do not populate these for SMPS other than SMPS10, this
>>>> results in spurious results as regmap assumes that the values are
>>>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>>>> variants that do have RTC! To fix this, we update proper parameters
>>>> for the descriptor fields.
>>>>
>>>> Further, we want to ensure the behavior consistent with logic
>>>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>>>> enable/disable ensure the logic remains consistent and configures
>>>> Palmas to the configuration that we set with set_mode (since the
>>>> configuration register is common). To do this, we can rely on the
>>>> regulator core's regulator_register behavior where the regulator
>>>> descriptor pointer provided by the regulator driver is stored. (no
>>>> reallocation and copy is done). This lets us update the enable_value
>>>> post registration, to remain consistent with the mode we configure as
>>>> part of set_mode.
>>>>
>>>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions 
>>>> with helper functions")
>>>> Reported-by: Alexandre Courbot 
>>>> Signed-off-by: Nishanth Menon 
>>>
>>> Unfortunately, there is still some lingering problem in the original
>>> commit. In next-20130623 (and indeed since at least next-20140611),
>>> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
>>> Reverting this commit (just for conflicts) and the original problematic
>>> commit "regulator: palmas: Reemove open coded functions with helper
>>> functions" solves this. I see the following on boot:
>>>
>>>> [3.558776] tegra-dsi 5430.dsi: cannot get VDD supply
>>>> [3.564272] platform 5430.dsi: Driver tegra-dsi requests probe 
>>>> deferral
>>>> [3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator
>>>> [3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe 
>>>> deferral
>>>
>>> ... but probe deferral never completes, yet with your "remove open coded
>>> ..." patch reverted, it all works fine.
>>>
>>> Can you please take another look at the original patch?
>>
>> Will let keerthy (original patch author) comment on it.
>>
>> arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
>> suppose is the path in question?
>>
>> Seems to use drivers/mfd/tps65090.c and
>> drivers/regulator/tps65090-regulator.c and not palmas?
>>
>> Am I looking at the right dts?
> 
> Yes, that's the right DTS.
> 
> There's both a 65090 and a Palmas on the board. It's probably simpler to
> look at the HDMI PLL regulator, since the path to the Palmas is more
> obvious:
...

I think I found the problem; I sent "[PATCH] regulator: palmas: fix typo
in enable_reg calculation" with what's probably the fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] regulator: palmas: fix typo in enable_reg calculation

[Stephen Warren]

From: Stephen Warren 

When setting up .enable_reg for an SMPS regulator, presumably we should
call PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, ...) rather than using
LDO_BASE. This change makes the LCD panel and HDMI work again on the
NVIDIA Dalmore board anyway.

Cc: Alex Courbot 
Cc: Keerthy 
Cc: Nishanth Menon 
Cc: linux-o...@vger.kernel.org
Cc: linux-te...@vger.kernel.org
Fixes: 318dbb02b50c ("regulator: palmas: Fix SMPS enable/disable/is_enabled")
Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions with 
helper functions")
Signed-off-by: Stephen Warren 
---
 drivers/regulator/palmas-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/palmas-regulator.c 
b/drivers/regulator/palmas-regulator.c
index 3c861d5f9245..93b4ad842901 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -970,7 +970,7 @@ static int palmas_regulators_probe(struct platform_device 
*pdev)
PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
 
pmic->desc[id].enable_reg =
-   PALMAS_BASE_TO_REG(PALMAS_LDO_BASE,
+   PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE,
palmas_regs_info[id].ctrl_addr);
pmic->desc[id].enable_mask =
PALMAS_SMPS12_CTRL_MODE_ACTIVE_MASK;
-- 
1.8.1.5

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

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/23/2014 02:11 PM, Nishanth Menon wrote:
> On Mon, Jun 23, 2014 at 2:50 PM, Stephen Warren  wrote:
>> On 06/20/2014 11:26 AM, Nishanth Menon wrote:
>>> We use regmap regulator ops to enable/disable and check if regulator
>>> is enabled for various SMPS. However, these depend on valid
>>> enable_reg, enable_mask and enable_value in regulator descriptor.
>>>
>>> Currently we do not populate these for SMPS other than SMPS10, this
>>> results in spurious results as regmap assumes that the values are
>>> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
>>> variants that do have RTC! To fix this, we update proper parameters
>>> for the descriptor fields.
>>>
>>> Further, we want to ensure the behavior consistent with logic
>>> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
>>> enable/disable ensure the logic remains consistent and configures
>>> Palmas to the configuration that we set with set_mode (since the
>>> configuration register is common). To do this, we can rely on the
>>> regulator core's regulator_register behavior where the regulator
>>> descriptor pointer provided by the regulator driver is stored. (no
>>> reallocation and copy is done). This lets us update the enable_value
>>> post registration, to remain consistent with the mode we configure as
>>> part of set_mode.
>>>
>>> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions 
>>> with helper functions")
>>> Reported-by: Alexandre Courbot 
>>> Signed-off-by: Nishanth Menon 
>>
>> Unfortunately, there is still some lingering problem in the original
>> commit. In next-20130623 (and indeed since at least next-20140611),
>> neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
>> Reverting this commit (just for conflicts) and the original problematic
>> commit "regulator: palmas: Reemove open coded functions with helper
>> functions" solves this. I see the following on boot:
>>
>>> [3.558776] tegra-dsi 5430.dsi: cannot get VDD supply
>>> [3.564272] platform 5430.dsi: Driver tegra-dsi requests probe 
>>> deferral
>>> [3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator
>>> [3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe 
>>> deferral
>>
>> ... but probe deferral never completes, yet with your "remove open coded
>> ..." patch reverted, it all works fine.
>>
>> Can you please take another look at the original patch?
> 
> Will let keerthy (original patch author) comment on it.
> 
> arch/arm/boot/dts/tegra114-dalmore.dts tps65090, avdd_lcd_reg I
> suppose is the path in question?
> 
> Seems to use drivers/mfd/tps65090.c and
> drivers/regulator/tps65090-regulator.c and not palmas?
> 
> Am I looking at the right dts?

Yes, that's the right DTS.

There's both a 65090 and a Palmas on the board. It's probably simpler to
look at the HDMI PLL regulator, since the path to the Palmas is more
obvious:

/ {
host1x@5000 {
hdmi@5428 {
pll-supply = <&palmas_smps3_reg>;
...
i2c@7000d000 {
palmas: tps65913@58 {
compatible = "ti,palmas";
pmic {
compatible = "ti,tps65913-pmic",
"ti,palmas-pmic";
regulators {
palmas_smps3_reg: smps3 {

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

Re: [PATCH] regulator: palmas: Fix SMPS enable/disable/is_enabled

[Stephen Warren]

On 06/20/2014 11:26 AM, Nishanth Menon wrote:
> We use regmap regulator ops to enable/disable and check if regulator
> is enabled for various SMPS. However, these depend on valid
> enable_reg, enable_mask and enable_value in regulator descriptor.
> 
> Currently we do not populate these for SMPS other than SMPS10, this
> results in spurious results as regmap assumes that the values are
> valid and ends up reading register 0x0 RTC:SECONDS_REG on Palmas
> variants that do have RTC! To fix this, we update proper parameters
> for the descriptor fields.
> 
> Further, we want to ensure the behavior consistent with logic
> prior to commit dbabd624d4eec50b6, where, once you do a set_mode,
> enable/disable ensure the logic remains consistent and configures
> Palmas to the configuration that we set with set_mode (since the
> configuration register is common). To do this, we can rely on the
> regulator core's regulator_register behavior where the regulator
> descriptor pointer provided by the regulator driver is stored. (no
> reallocation and copy is done). This lets us update the enable_value
> post registration, to remain consistent with the mode we configure as
> part of set_mode.
> 
> Fixes: dbabd624d4eec50b6 ("regulator: palmas: Reemove open coded functions 
> with helper functions")
> Reported-by: Alexandre Courbot 
> Signed-off-by: Nishanth Menon 

Unfortunately, there is still some lingering problem in the original
commit. In next-20130623 (and indeed since at least next-20140611),
neither the LCD panel or HDMI work on the NVIDIA Dalmore board.
Reverting this commit (just for conflicts) and the original problematic
commit "regulator: palmas: Reemove open coded functions with helper
functions" solves this. I see the following on boot:

> [3.558776] tegra-dsi 5430.dsi: cannot get VDD supply
> [3.564272] platform 5430.dsi: Driver tegra-dsi requests probe deferral
> [3.571990] tegra-hdmi 5428.hdmi: failed to get PLL regulator
> [3.578377] platform 5428.hdmi: Driver tegra-hdmi requests probe 
> deferral

... but probe deferral never completes, yet with your "remove open coded
..." patch reverted, it all works fine.

Can you please take another look at the original patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ARM: tegra: roth: enable input on mmc clock pins

[Stephen Warren]

On 06/23/2014 01:32 AM, Alexandre Courbot wrote:
> Input had been disabled by mistake on these pins, leading to issues with
> SDIO devices like the Wifi module not being probed or random errors
> occuring on the SD card.

I thought the host controller always drove the clock, so there should be
no need for the pin's input path to be enabled. Perhaps it depends on
the transfer mode (e.g. UHS)?

If this fix is valid, perhaps Jetson TK1's sdmmc3_clk and Venice2's
sdmmc1_clk need the same fix, although we'll need to file bugs against
their pinmux spreadsheets first if that's the case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] ARM: tegra: roth: pinmux fixes

[Stephen Warren]

On 06/23/2014 01:32 AM, Alexandre Courbot wrote:
> Two small but important fixes to SHIELD's pinmux configuration.
> The use of invalid properties caused the pinmux to not be applied
> at all. Also the setting for sdmmc clock lines resulted in random
> errors or even the impossibility to probe attached devices.

It might be nice to add Roth's pinmux to:
https://github.com/NVIDIA/tegra-pinmux-scripts
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 1/2] ARM: tegra: tamonten: add the base board regulators

[Stephen Warren]

On 06/19/2014 07:25 AM, Alban Bedel wrote:
> Currently the Tamonten DTS define a fixed regulator for the 5V supply.
> However this regulator is in fact on the base board. Fix this by
> properly defining the regulators found on the base boards.

I've applied the series to Tegra's for-3.17/dt branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
>> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>>>> * PGP Signed by an unknown key
>>>>
>>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>>>> Old Signed by an unknown key
>>>>>>
>>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>>>> This symbol needs to be exported to power on rails without using
>>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>>>> by itself.
>>>>>>>
>>>>>>> Thierry, are you OK with this change?
>>>>>>
>>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>>>
>>>>> I don't think the current tegra_powergate_sequence_power_up() API is very 
>>>>> well
>>>>> defined though. I don't think the clocks and resets required by the 
>>>>> sequence
>>>>> should be provided by the driver. For one, there can be several clocks and
>>>>> resets that need to be controlled for a single domain.
>>>>
>>>> Do you have any suggestions for what the API should look like? Even if
>>>> we plan to move to some different API, I think there's some advantage in
>>>> using it consistently if for no other reason than to make it easier to
>>>> replace occurrences later on.
>>>>
>>>
>>> I think the API should only have the domain ID as input so:
>>>
>>> int tegra_powerdomain_on(int id) 
>>>
>>> /*
>>>  * Prerequisites: domain is off
>>>  * Result: domain is on, clocks of the modules in the domain are off, 
>>> modules are in reset
>>>  */
>>>
>>> int tegra_powerdomain_off(int id)
>>>
>>> /*
>>>  * Prerequisites: all clocks of the modules in the domain are off
>>>  * result: domain is off
>>>  */
>>
>> That doesn't make sense; the PMC doesn't have access to the clock and
>> reset IDs - that's why the API requires them to be passed in.
>>
> 
> We should make driver look them up by name then. It doesn't make sense to
> move this knowledge to the drivers as there can be several modules in 1
> powerdomain. So every driver would then need to have access to all clock
> and reset IDs of the modules of the domain?

Having the drivers know the clocks, resets, and power domains that
affect their HW module seems perfectly reasonable.

Do we actually have any case where unrelated modules are in the same
power domain /and/ there's some need for those drivers to manipulate the
power domain?

BTW, any objections here should have been raised when the initial API
design for powergating was created. Now that we have the current API,
which in turn has driven the DT bindings for at least some HW modules,
we're stuck with it due to DT ABIness. Well, perhaps we can introduce
something new, but we'll always have to support the old way, so there's
little point.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: tegra: TN7: relax some regulators

[Stephen Warren]

On 06/19/2014 01:49 AM, Alexandre Courbot wrote:
> Remove the regulator-always-on property from some regulators that do not
> need it. On recent kernels fixed regulators which supply is always on
> fail registration.

That sounds like a bug in the regulator core, which should be fixed there.

After all, we can fix *new* DTs to work around that behaviour change,
but old DTs are supposed to work with new kernels. Fixing this in the DT
doesn't help that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 05:14 PM, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 04:09:06PM -0600, Stephen Warren wrote:
>> On 06/18/2014 04:03 PM, Thierry Reding wrote:
...
>>> From what I remember, Mike was fairly strongly opposing the idea of
>>> virtual clocks, but what you're proposing here sounds like it would
>>> assume the existence of virtual clocks. clk_set_rate() per client
>>> doesn't work with the current API as I understand it.
>>>
>>> Or perhaps what you're proposing isn't about the individual clocks at
>>> all but rather about a mechanism to express constraints for a set of
>>> clocks?
>>
>> This doesn't have anything to do with virtual clocks. As you mention,
>> it's just about constraints.
>>
>> One user of clock "cpu" wants min rate 216MHz. Another wants max rate
>> 1GHz. cpufreq will request some rate between the 2, or be capped to
>> those limits. These set of imposed constraints would need to be stored
>> per client of the clock, not per HW clock, since many clients could set
>> different max rates (e.g. thermal throttle 1.5GHz due to temperature,
>> CPU policy 1GHz due to the user selecting low CPU power, etc.)
>>
>> Similarly for audio, of there are N clients of 1 clock/PLL, and they
>> each want the PLL to run at a different rate, something needs to detect
>> that and deny it.
> 
> I'm wondering how this should work with the current API. Could the clock
> core be modified to return a per-client struct clk * that references the
> hardware clock internally? Or do we need to add a new API?

I would assume the we can just change struct clk and hide the details
from any driver. Hopefully only clock-core and clock-drivers would need
any changes.



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 04:19 PM, Stéphane Marchesin wrote:
> On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
>  wrote:
>> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what you
>>> have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>>
>>> * an EMC driver would collect bandwidth and latency requests from consumers
>>> and call clk_set_floor on the EMC clock.
>>>
>>> * the EMC driver would also register for rate change notifications in the
>>> EMC clock and would update the latency allowance registers at that point.
>>
>> Latency allowance registers are part of the MC rather than the EMC. So I
>> think we have two options: a) have a unified driver for MC and EMC or b)
>> provide two parts of the API in two drivers.
>>
>> Or perhaps c), create a generic framework that both MC and EMC can
>> register with (bandwidth for EMC, latency for MC).
> 
> Is there any motivation for keeping MC and EMC separate? In my mind,
> the solution was always to handle those together.

Well, they are documented as being separate HW modules in the TRM.

I know there's an interlock in HW so that when the EMC clock is changed,
the EMC registers can flip atomically to a new configuration.

I'm not aware of any similar HW interlock between MC and EMC registers.
That would imply that very tight co-ordination shouldn't be required.

Do the MC latency allowance registers /really/ need to be *very tightly*
managed whenever the EMC clock is changed, or is it just a matter of it
being a good idea to update EMC clock and MC latency allowance registers
at roughly the same time? Even if there's some co-ordination required,
maybe it can be handled rather like cpufreq notifications; use clock
pre-rate change notifications to set MC up in a way that'll work at both
old/new EMC clocks, and then clock post-rate notifications to the final
MC configuration?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 04:03 PM, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 11:46:49AM -0600, Stephen Warren wrote:
>> On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>>>> better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints
>>>>>> besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what
>>> you have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>
>> Yes. I'd expect those to be maintained per-client, and so the clock core
>> (or whatever higher level code implements clk_set_floor/ceiling)
>> performs the logic that "blends" together all the different requests
>> from different clients.
>>
>> As an aside, for audio usage, I would expect clk_set_rate to be a
>> per-client (rather than per HW clock) operation too, and to error out if
>> one client says it wants to set pll_a to the rate needed for
>> 44.1KHz-based audio and a different client wants the rate for
>> 48KHz-based audio.
> 
> From what I remember, Mike was fairly strongly opposing the idea of
> virtual clocks, but what you're proposing here sounds like it would
> assume the existence of virtual clocks. clk_set_rate() per client
> doesn't work with the current API as I understand it.
> 
> Or perhaps what you're proposing isn't about the individual clocks at
> all but rather about a mechanism to express constraints for a set of
> clocks?

This doesn't have anything to do with virtual clocks. As you mention,
it's just about constraints.

One user of clock "cpu" wants min rate 216MHz. Another wants max rate
1GHz. cpufreq will request some rate between the 2, or be capped to
those limits. These set of imposed constraints would need to be stored
per client of the clock, not per HW clock, since many clients could set
different max rates (e.g. thermal throttle 1.5GHz due to temperature,
CPU policy 1GHz due to the user selecting low CPU power, etc.)

Similarly for audio, of there are N clients of 1 clock/PLL, and they
each want the PLL to run at a different rate, something needs to detect
that and deny it.




signature.asc
Description: OpenPGP digital signature

Re: [RFT 1/2] printk: make dynamic kernel ring buffer alignemnt explicit

[Stephen Warren]

On 06/18/2014 01:33 PM, Luis R. Rodriguez wrote:
> On Wed, Jun 18, 2014 at 09:56:03AM -0600, Stephen Warren wrote:
>> On 06/18/2014 05:14 AM, Luis R. Rodriguez wrote:
>>> From: "Luis R. Rodriguez" 
>>>
>>> We have to consider alignment for the ring buffer both for the
>>> default static size, and then also for when an dynamic allocation
>>> is made when the log_buf_len=n kernel parameter is passed to set
>>> the size specifically to a size larger than the default size set
>>> by the architecture through CONFIG_LOG_BUF_SHIFT.
>>>
>>> The default static kernel ring buffer can be aligned properly if
>>> architectures set CONFIG_LOG_BUF_SHIFT properly, we provide ranges
>>> for the size though so even if CONFIG_LOG_BUF_SHIFT has a sensible
>>> aligned value it can be reduced to a non aligned value. Commit
>>> 6ebb017de9 by Andrew ensures the static buffer is always aligned
>>> and the decision of alignment is done by the compiler by using
>>> __alignof__(struct log) (curious what value caused the crash?).
>>
>> IIRC the issue was that __log_buf's type is char[] so without the
>> __aligned it could have any alignment at all, e.g. 1 or 2. However,
>> struct printk_log is stored in the buffer rather than just char*, and so
>> if __log_buf isn't aligned to the required alignment for that structure,
>> that can caused unaligned accesses to fields in the structure, which
>> isn't supported on ARM in at least some cases.
>>
>> As such, I think the change to setup_log_buf() in this patch makes sense
>> (although I suppose in practice memblock_virt_alloc() probably has some
>> minimum internal alignment that dwards LOG_ALIGN, but that's an
>> implementation detail we shouldn't rely on).
> 
> Thanks for the feedback.
> 
> memblock_virt_alloc() will by default align to L1 cache, so if that satisfies
> the architecture alignment it should be safe, but perhaps not optimal for
> saving a few bytes. Still curious if without this patch a crash can be
> triggered somehow with some log_buf_len=n, if so this can go to stable.

If memblock_virt_alloc() aligns to L1 cache, then I believe that the
crash would never trigger.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate);
>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>> +#else
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate)
>>>>> +{ return -ENODEV; }
>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>> +{ return; }
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>> +{ return; }
>>>>> +#endif
>>>>
>>>> I'll repeat what I said off-list so that we can have the whole
>>>> conversation on the list:
>>>>
>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>> better
>>>> to integrate this into the common clock framework as a standard clock
>>>> constraints API. There are other use-cases for clock constraints
>>>> besides
>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>> SoCs too).
>>>
>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>> they map to the CCF. Could you please comment on that?
>>
>> My comments remain the same. I believe this is something that belongs in
>> the clock driver, or at the least, some API that takes a struct clock as
>> its parameter, so that drivers can use the existing DT clock lookup
>> mechanism.
> 
> Ok, let me put this strawman here to see if I have gotten close to what
> you have in mind:
> 
> * add per-client accounting (Rabin's patches referenced before)
> 
> * add clk_set_floor, to be used by cpufreq, load stats, etc.
> 
> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.

Yes. I'd expect those to be maintained per-client, and so the clock core
(or whatever higher level code implements clk_set_floor/ceiling)
performs the logic that "blends" together all the different requests
from different clients.

As an aside, for audio usage, I would expect clk_set_rate to be a
per-client (rather than per HW clock) operation too, and to error out if
one client says it wants to set pll_a to the rate needed for
44.1KHz-based audio and a different client wants the rate for
48KHz-based audio.

> * an EMC driver would collect bandwidth and latency requests from
> consumers and call clk_set_floor on the EMC clock.
> 
> * the EMC driver would also register for rate change notifications in
> the EMC clock and would update the latency allowance registers at that
> point.
> 
> How does it sound?

At a high level, yes this sounds about right to me.

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

Re: [RFT 1/2] printk: make dynamic kernel ring buffer alignemnt explicit

[Stephen Warren]

On 06/18/2014 05:14 AM, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" 
> 
> We have to consider alignment for the ring buffer both for the
> default static size, and then also for when an dynamic allocation
> is made when the log_buf_len=n kernel parameter is passed to set
> the size specifically to a size larger than the default size set
> by the architecture through CONFIG_LOG_BUF_SHIFT.
> 
> The default static kernel ring buffer can be aligned properly if
> architectures set CONFIG_LOG_BUF_SHIFT properly, we provide ranges
> for the size though so even if CONFIG_LOG_BUF_SHIFT has a sensible
> aligned value it can be reduced to a non aligned value. Commit
> 6ebb017de9 by Andrew ensures the static buffer is always aligned
> and the decision of alignment is done by the compiler by using
> __alignof__(struct log) (curious what value caused the crash?).

IIRC the issue was that __log_buf's type is char[] so without the
__aligned it could have any alignment at all, e.g. 1 or 2. However,
struct printk_log is stored in the buffer rather than just char*, and so
if __log_buf isn't aligned to the required alignment for that structure,
that can caused unaligned accesses to fields in the structure, which
isn't supported on ARM in at least some cases.

As such, I think the change to setup_log_buf() in this patch makes sense
(although I suppose in practice memblock_virt_alloc() probably has some
minimum internal alignment that dwards LOG_ALIGN, but that's an
implementation detail we shouldn't rely on).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>> This symbol needs to be exported to power on rails without using
>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>> by itself.
>>>>>
>>>>> Thierry, are you OK with this change?
>>>>
>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>
>>> I don't think the current tegra_powergate_sequence_power_up() API is very 
>>> well
>>> defined though. I don't think the clocks and resets required by the sequence
>>> should be provided by the driver. For one, there can be several clocks and
>>> resets that need to be controlled for a single domain.
>>
>> Do you have any suggestions for what the API should look like? Even if
>> we plan to move to some different API, I think there's some advantage in
>> using it consistently if for no other reason than to make it easier to
>> replace occurrences later on.
>>
> 
> I think the API should only have the domain ID as input so:
> 
> int tegra_powerdomain_on(int id) 
> 
> /*
>  * Prerequisites: domain is off
>  * Result: domain is on, clocks of the modules in the domain are off, modules 
> are in reset
>  */
> 
> int tegra_powerdomain_off(int id)
> 
> /*
>  * Prerequisites: all clocks of the modules in the domain are off
>  * result: domain is off
>  */

That doesn't make sense; the PMC doesn't have access to the clock and
reset IDs - that's why the API requires them to be passed in.

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

Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller

[Stephen Warren]

On 06/17/2014 11:36 AM, Mikko Perttunen wrote:
> On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
>>> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> [...]
>>
>>>>> +static struct platform_driver tegra_ahci_driver = {
>>>>> +.probe = tegra_ahci_probe,
>>>>> +.remove = ata_platform_remove_one,
>>>>> +.driver = {
>>>>> +.name = "tegra-ahci",
>>>>> +.owner = THIS_MODULE,
>>>>> +.of_match_table = tegra_ahci_of_match,
>>>>> +},
>>>>
>>>> This driver lacks PM suspend/resume support.  I assume that
>>>> the Tegra124 platform also doesn't support suspend/resume yet
>>>> (if so a comment in the driver code about this would be useful),
>>>> otherwise the driver should be fixed.
>>>
>>> We do have basic system suspend/resume support. However, I don't think
>>> missing suspend/resume in an individual driver should stop it from being
>>> merged. It can certainly be added later.
>>
>> There should be at least FIXME in the driver explaining the situation and
>> I would really prefer to have PM support added when the driver is still
>> "hot" (meaning there are people actively working on it) instead of
>> possibly
>> having to chase people months/years later when they have already moved on
>> and are working on something else.  Please also note that adding PM
>> support
>> should be quite simple if the driver is designed correctly.
>
> AFAIK, the deepest level of suspend currently supported on upstream is
> LP1, for which the driver doesn't need to do anything. Only when we go
> to LP0 the driver will need to save/reload registers and stuff.

Yes, it's generally true that no SoC register state is lost during
suspend, only CPU state.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller

[Stephen Warren]

On 06/17/2014 10:14 AM, Tejun Heo wrote:
> On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote:
>> On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote:
>>>> sata_writel() and sata_read() static inlines don't seem to add any value.
>>>>
>>>> Can they be removed?
>>>
>>> Such functions are quite idiomatic in drivers, and serve to simplify all
>>> the call-sites by removing the need to write out the addition of the
>>> base address everywhere.
>>
>> I think it obfuscates more than helps.  If you absoluately have to
>> keep it, please at least name it so that it's clear that it's
>> something specific to this driver.
> 
> Please also drop inline.  It isn't a difficult optimization to perform
> for the compiler.

There are probably hundreds of drivers marking those functions inline.
If they aren't marked inline, it'll just stick out as unusual when
people read them all, and then they'll send patches to fix it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources

[Stephen Warren]

On 06/17/2014 02:53 AM, Paul Bolle wrote:
> Doug,
> 
> On Fri, 2014-06-13 at 08:22 -0700, Doug Anderson wrote:
>> On Fri, Jun 13, 2014 at 1:08 AM, Paul Bolle  wrote:
>>> On Wed, 2014-06-11 at 08:11 -0700, Doug Anderson wrote:
 This is a config option on the ChromeOS EC
 .  Doing a
 grep there:

 board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
 common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
 common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
 common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
 driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
 driver/battery/samus.c:#endif   /* CONFIG_CHARGER_PROFILE_OVERRIDE */
 include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE
 include/ec_commands.h:  /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE 
 params */
 test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
>>>
>>> I see. So this is not a Kconfig macro but a general macro with a CONFIG_
>>> prefix. There are quite a bit of those in the tree already, but still,
>>> would another prefix also do?
>>
>> Given that it's an entirely separate project and this is a valid
>> CONFIG option in that project, it seems a lot to ask them not to use
>> the CONFIG_ prefix.  Also: the part you are objecting to is only a
>> comment, right?
> 
> So all the hits you quoted above are actually from code that's never
> going to be included in the kernel tree, right? If so, then yes, we're
> only discussing a single comment.
> 
>> We could certainly add extra wording in the comment to make it obvious
>> that this is a CONFIG option for the EC and not the kernel.  Would
>> that be enough?  ...or are you trying to use some scripts to
>> automatically process files to look for CONFIG options?
> 
> Yes, I'm using a script to check for Kconfig macros, among other things.
> It doesn't care about comments (because every now and then mistakes are
> made in comments too, and some of those can get surprisingly confusing).
> 
> Anyhow, the CONFIG_ prefix used in the kernel tree is quite generic, but
> we're stuck with it. Would it be bothersome to drop it in that comment?
> Mentioning a preprocessor macro from a separate project is a bit
> confusing to begin with. How is one supposed to know that this is a
> reference to something out of tree?
> 
> So, in summary, while we're apparently only discussing a single comment,
> I would appreciate it if it could be reworded, preferably by dropping
> that the CONFIG_ prefix. But other people might care very little, as
> they don't share this particular pet peeve.

Can't your tool maintain a whitelist or ignore list? There are many
cases where the kernel can pull in headers/data from other projects
(Firmware interfaces to an arbitrarily large set of HW, Device trees,
IO/network protocools, perhaps more). It feels quite unreasonable for
the kernel to decide that it exclusively owns the CONFIG_* namespace
even in comments, and that every other project it interacts with must
not use that namespace.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:

>> This binding looks quite anaemic vs.
>> Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
>> would expect that this binding needs all the EMC register data from the
>> tegra20-emc binding too. Can the two bindings be identical?
> 
> There's even less stuff needed right now, as all what ultimately the EMC
> driver does is call clk_set_rate on the EMC clock. As the T124 EMC
> driver gains more features, they should get more similar.

IIRC, even changing the EMC clock rate requires modifying the memory
controller's programming (e.g. delays/taps/tuning etc.). That's exactly
what the more complex stuff in the nvidia,tegra20-emc.txt is all about.
I not convinced that a driver that just modifies the clock rate without
adjusting the EMC programming will work reliably.

>>> +#ifdef CONFIG_TEGRA124_EMC
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate);
>>> +void tegra124_emc_set_floor(unsigned long freq);
>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>> +#else
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate)
>>> +{ return -ENODEV; }
>>> +void tegra124_emc_set_floor(unsigned long freq)
>>> +{ return; }
>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>> +{ return; }
>>> +#endif
>>
>> I'll repeat what I said off-list so that we can have the whole
>> conversation on the list:
>>
>> That looks like a custom Tegra-specific API. I think it'd be much better
>> to integrate this into the common clock framework as a standard clock
>> constraints API. There are other use-cases for clock constraints besides
>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>> SoCs too).
> 
> Yes, I wrote a bit in the cover letter about our requirements and how
> they map to the CCF. Could you please comment on that?

My comments remain the same. I believe this is something that belongs in
the clock driver, or at the least, some API that takes a struct clock as
its parameter, so that drivers can use the existing DT clock lookup
mechanism.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller

[Stephen Warren]

On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.

>> +static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
>> + unsigned long offset)
>> +{
>> +writel(value, tegra->sata_regs + offset);
>> +}
>> +
>> +static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
>> +unsigned long offset)
>> +{
>> +return readl(tegra->sata_regs + offset);
>> +}
> 
> sata_writel() and sata_read() static inlines don't seem to add any value.
> 
> Can they be removed?

Such functions are quite idiomatic in drivers, and serve to simplify all
the call-sites by removing the need to write out the addition of the
base address everywhere.

>> +static struct platform_driver tegra_ahci_driver = {
>> +.probe = tegra_ahci_probe,
>> +.remove = ata_platform_remove_one,
>> +.driver = {
>> +.name = "tegra-ahci",
>> +.owner = THIS_MODULE,
>> +.of_match_table = tegra_ahci_of_match,
>> +},
> 
> This driver lacks PM suspend/resume support.  I assume that
> the Tegra124 platform also doesn't support suspend/resume yet
> (if so a comment in the driver code about this would be useful),
> otherwise the driver should be fixed.

We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] USB: EHCI: tegra: Fix use-after-free in .remove()

[Stephen Warren]

On 06/17/2014 08:17 AM, Tuomas Tynkkynen wrote:
> The tegra_ehci_hcd structure is located in the private space allocated
> by the core USB code so it must not be accessed after the HCD is
> freed.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

This seems to be a persistent problem. Witness commit ecc8a0cdca18 "usb:
host: tegra: fix warning messages in ehci_remove". So, I wonder if ...

> @@ -479,10 +479,11 @@ static int tegra_ehci_remove(struct platform_device 
> *pdev)
>  
>   usb_phy_shutdown(hcd->phy);
>   usb_remove_hcd(hcd);
> - usb_put_hcd(hcd);
>  
>   clk_disable_unprepare(tegra->clk);
>  

It's worth putting a comment here:

/* This must be last, since *tegra is part of *hcd */
> + usb_put_hcd(hcd);
> +
>   return 0;
>  }

Of course, that could be a separate commit if you want.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.

> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c

> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)

> + /* Pad calibration */
> +
> + ret = tegra_fuse_readl(0x224, &val);
> + if (ret) {
> + dev_err(&tegra->pdev->dev,
> + "failed to read sata calibration fuse: %d\n", ret);
> + return ret;
> + }
> +
> + calib = tegra124_pad_calibration[val];

Shouldn't val be range-checked before blindly using it?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/9] ahci: Increase AHCI_MAX_CLKS to 4

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> The Tegra AHCI device requires four clocks, so increase the maximum
> amount of handled clocks from three to four.

Tejun,

The SATA driver in patch 8/9 in this series would usually be applied in
your ATA tree. However, it has a lot of compile-time dependencies on
things outside the ATA tree that I expect you don't want to deal with.

If you're OK with patches 7 and 8, could you ack them so that I can
apply them to the Tegra tree?

I assume the patches won't cause any significant conflicts, but if you
need, I can certainly give you a signed tag back to allow you to merge
the driver patch (plus the compile-time dependencies) into your tree
later if needed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This symbol needs to be exported to power on rails without using
> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> cannot be used in situations where the driver wants to handle clocking
> by itself.

Thierry, are you OK with this change?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/9] ARM: tegra: Add SATA and SATA power to Jetson TK1 device tree

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This enables the integrated SATA controller on the Tegra124 system-on-chip
> on the Jetson TK1 board and adds regulators for the onboard Molex connector
> commonly used to power SATA devices. The regulators are marked always-on
> since they can be used for other purposes than powering SATA devices.

> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts 
> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts

> + sata@0,7002 {

> + target-supply = <&vdd_5v0_sata>;

regulators {

> + /* Molex power connector */
> + vdd_5v0_sata: regulator@13 {
> + compatible = "regulator-fixed";
> + reg = <13>;
> + regulator-name = "+5V_SATA";
> + regulator-min-microvolt = <500>;
> + regulator-max-microvolt = <500>;
> + gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + vin-supply = <&vdd_5v0_sys>;
> + regulator-always-on;
> + };

Why is this always-on, considering that the SATA node references this,
and presumably the driver explicitly enables this regulator?

> +
> + vdd_12v0_sata: regulator@14 {
> + compatible = "regulator-fixed";
> + reg = <14>;
> + regulator-name = "+12V_SATA";
> + regulator-min-microvolt = <1200>;
> + regulator-max-microvolt = <1200>;
> + gpio = <&gpio TEGRA_GPIO(EE, 2) GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + vin-supply = <&vdd_mux>;
> + regulator-always-on;
> + };
>   };

If there are two regulators required for the SATA devices, shouldn't the
SATA node's target-supply property reference them both? Well, I suppose
there'd need to be target-5v-supply and target-12v-supply properties,
since each regulator property can only reference a single supply, IIRC.
I think the DT binding needs to be updated for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/9] of: Add NVIDIA Tegra SATA controller binding

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This patch adds device tree binding documentation for the SATA
> controller found on NVIDIA Tegra SoCs.

Just one nit below:

> diff --git a/Documentation/devicetree/bindings/ata/tegra-sata.txt 
> b/Documentation/devicetree/bindings/ata/tegra-sata.txt

> + - clocks : Defines the clocks listed in the clock-names property.
> + - clock-names : The following clock names must be present:
> +   - sata
...

It would be nice if the binding could use the exact same wording as all
the other Tegra bindings, i.e.:

==
- clocks : Must contain an entry for each entry in clock-names.
  See ../clocks/clock-bindings.txt for details.
- clock-names : Must include the following entries:
  - pll_a
  - pll_a_out0
  - mclk (The Tegra cdev1/extern1 clock, which feeds the CODEC's mclk)
==

Same for the resets property.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/9] clk: tegra: Enable hardware control of SATA PLL

[Stephen Warren]

On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This makes the SATA PLL be controlled by hardware instead of software.
> This is required for working SATA support.

Peter, could you please take patches 4 and 5 through the clock tree. As
far as I can tell, there's no compile-time dependency in the clock
patches, so they can go through a different tree to the rest of the
series without issue. These 2 patches look fine to me, so consider them:

Acked-by: Stephen Warren 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] firmware: Add device tree binding for coreboot

[Stephen Warren]

On 06/16/2014 07:30 AM, Rob Herring wrote:
> On Fri, Jun 13, 2014 at 4:58 PM, Julius Werner  wrote:
...
>> Rob Herring wrote:
>>> Don't you need need to keep the kernel from allocating this memory by
>>> using one of the reserved memory mechanisms? The recently added one
>>> should be able to specific what the memory is reserved for IIRC.
>>
>> Our bootloader is carving the location out of the /memory node and
>> adding it to the device tree reserve map. As far as I know, that only
>> contains a list of raw start and size entries. At any rate, I think
>> it's useful (and in line with other bindings) to add a more explicit
>> node like this (if only to make it easier accessible through
>> /proc/device-tree).
> 
> Understand there are 3 different memory reservation bindings. The
> original "/memreserve/" method is indeed limited. What I think you
> should use is the binding documented in
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
> So you could do something like this:
> 
> reserved-memory {
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> 
> /* global autoconfigured region for contiguous allocations */
> linux,cma {
> compatible = "shared-dma-pool";
> reusable;
> size = <0x400>;
> alignment = <0x2000>;
> linux,cma-default;
> };
> 
> coreboot_reserved: coreboot@fdfea000 {
>   compatible = "coreboot";
>   reg = <0xfdfea000 0x264>,
>   <0xfdfea000 0x16000>;
> };
> 

I thought that the /reserved-memory node was more so that the
(preferred?) location of firmware images or data buffers used by HW
accelerators could be communicated to the kernel. This feels like pure data.

The coreboot binding seems to be more about defining an interface to a
particular firmware (this feels like semantics), which as a side-effect
needs to communicate the location of certain data.

If /reserved-memory is used to communicate the address of the memory
regions, I think we still need a /firmware/coreboot node to indicate the
semantics of the reserved memory region, and point at the phandle of the
region. As such, it seems simpler just to put the addresses in the
coreboot node's reg property. The only exception I see to that argument
is if putting the region in /reserved-memory automatically carves that
region out of the memory the kernel will allocate from. That would
simplify the bootloader, since it wouldn't have to fiddle with the
/memory node and do the carveout itself.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 3/4] drm/tegra: Request memory bandwidth for the display controller

[Stephen Warren]

On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> Request it based solely on the current mode's refresh rate. More
> accurate requirements can be requested in future patches.

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c

> + bandwidth = mode->clock * window.bits_per_pixel / 8;
> + err = tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, 
> bandwidth);

DISP1 shouldn't be hard-coded here; the code should use DISP1 or DISP2
based on head or DC identity. We certainly have some boards capable of
dual-head operation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> Adds functionality for registering memory bandwidth needs and setting
> the EMC clock rate based on that.
> 
> Also adds API for setting floor and ceiling frequency rates.

> diff --git 
> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt 
> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> new file mode 100644
> index 000..88e6a55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> @@ -0,0 +1,26 @@
> +Tegra124 External Memory Controller
> +
> +Properties:
> +- compatible : Should contain "nvidia,tegra124-emc".
> +- reg : Should contain the register range of the device
> +- #address-cells : Should be 1
> +- #size-cells : Should be 0
> +- nvidia,mc : phandle to the mc bus connected to EMC.
> +- clocks : phandle to EMC, EMC shared bus override, and all parent clocks.
> +- clock-names : name of each clock.
> +- nvidia,pmc : phandle to the PMC syscon node.
> +- max-clock-frequency : optional, specifies the maximum EMC rate in kHz.
> +
> +Child device nodes describe the memory settings for different configurations 
> and
> +clock rates.

How do the child nodes do that? The binding needs to specify the format
of the child node. This binding looks quite anaemic vs.
Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
would expect that this binding needs all the EMC register data from the
tegra20-emc binding too. Can the two bindings be identical?

Can you explain what the nvidia,mc and nvidia,pmc references are needed
for? Hopefully, this driver isn't going to reach into those devices and
touch their registers directly.

> diff --git a/include/linux/platform_data/tegra_emc.h 
> b/include/linux/platform_data/tegra_emc.h

A header file that defines platform data format isn't the correct place
to put the definitions of public APIs. I'd expect something more like
.

> +#ifdef CONFIG_TEGRA124_EMC
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long 
> rate);
> +void tegra124_emc_set_floor(unsigned long freq);
> +void tegra124_emc_set_ceiling(unsigned long freq);
> +#else
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
> +{ return -ENODEV; }
> +void tegra124_emc_set_floor(unsigned long freq)
> +{ return; }
> +void tegra124_emc_set_ceiling(unsigned long freq)
> +{ return; }
> +#endif

I'll repeat what I said off-list so that we can have the whole
conversation on the list:

That looks like a custom Tegra-specific API. I think it'd be much better
to integrate this into the common clock framework as a standard clock
constraints API. There are other use-cases for clock constraints besides
EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
SoCs too).

See https://lkml.org/lkml/2014/5/16/569 for some previous discussion on
this topic.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/3] arm: tegra: initial support for apalis t30

[Stephen Warren]

On 06/09/2014 04:52 PM, Marcel Ziswiler wrote:
> This patch adds the device tree to support Toradex Apalis T30, a
> computer on module which can be used on different carrier boards.
> 
> The module consists of a Tegra 3 SoC, two PMICs, 1 or 2 GB of DDR3L
> RAM, eMMC, an LM95245 temperature sensor chip, an i210 resp. i211
> gigabit Ethernet controller, an STMPE811 ADC/touch controller as well
> as two MCP2515 CAN controllers. Furthermore, there is an SGTL5000 audio
> codec which is not yet supported. Anything that is not self contained
> on the module is disabled by default.
> 
> The device tree for the Evaluation Board includes the modules device
> tree and enables the supported peripherals of the carrier board (the
> Evaluation Board supports almost all of them).
> 
> While at it also add the device tree binding documentation for Apalis
> T30.

Applied to Tegra's for-3.17/dt branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 7/7] ARM: tegra: Add the EC i2c tunnel to tegra124-venice2

[Stephen Warren]

On 04/30/2014 11:44 AM, Doug Anderson wrote:
> This adds the EC i2c tunnel (and devices under it) to the
> tegra124-venice2 device tree.

I've applied this to Tegra's for-3.17/dt branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] arm: tegra: enable igb, stmpe, i2c chardev, lm95245, pwm leds

[Stephen Warren]

On 06/09/2014 04:52 PM, Marcel Ziswiler wrote:
> The NVIDIA Tegra 3 based Apalis T30 module contains an Intel i210 resp.
> i211 gigabit Ethernet controller, an STMPE811 ADC/touch controller, I2C
> buses and PWM LEDs generically accessible from user space and an
> LM95245 temperature sensor chip. The later four can also be found on
> the Colibri T30 module.

I've applied this patch to Tegra's for-3.17/defconfig branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] Use Tegra's microsecond counter for udelay()

[Stephen Warren]

On 06/12/2014 09:58 AM, Peter De Schrijver wrote:
> This patchset introduces support for Tegra's microsecond counter as the
> udelay() timer. This is useful on Tegra SoCs which do not have an arch timer
> such as Tegra20 and Tegra30. Using the microsecond counter instead of a delay
> based loop avoids potential problems during cpu frequency changes.
> 
> The set consists of 3 patches:
> 
> Patch 1 introduces a new call which is used by the ARM architecture delay
> timer code to prevent changing the delay timer after calibration is finished
> and thus can be in use.
> 
> Patch 2 adds logic to choose the delay timer with the highest resolution. This
> allows the same registration code to be used on all Tegra SoCs and yet use the
> higher resolution arch timer when available (eg on Tegra114 or Tegra124).
> 
> Patch 3 adds the actual delay timer code.
> 
> Patch set has been verified on ventana (Tegra20), beaver (Tegra30),
> dalmore (Tegra114) and jetson TK1 (Tegra124).

I've applied this series to Tegra's for-3.17/delay-timer branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] ARM: dts: Create a cros-ec-keyboard fragment

[Stephen Warren]

On 06/04/2014 04:20 PM, Doug Anderson wrote:
> All ChromeOS ARM devices that have the standard "CrOS EC" have the
> same keyboard mapping.  It's silly to include this same definition
> everywhere.  Let's create a "dtsi" fragment that we can include from
> many different boards.
> 
> This fragment is based on what's currently in tegra124-venice2.dts

I've applied this series to Tegra's for-3.17/dt-cros-ec-kbd branch. I'll
send a pull request to arm-soc shortly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/