Re: [PATCH] of: Add vendor 2nd prefix for Asahi Kasei Corp
On 06/11/2014 06:53 PM, Kuninori Morimoto wrote: From: Kuninori Morimoto kuninori.morimoto...@renesas.com Current vendor-prefixes.txt already has ak prefix for Asahi Kasei Corp by ae8c4209af2cec065fef15d200a42a04130799f7 (of: Add vendor prefix for Asahi Kasei Corp.) It went through the appropriate review process, and is already in use. But, almost all Asahi Kasei chip driver is using asahi-kasei prefix today. I'm fine with this patch, but just wanted to comment on this part of the commit description: What is in the drivers isn't relevant; that's not what drives the ABI. The issue is that some *DTs* use ak and some use asahi-kasei, so both those need to be supported. That said, we should document which one of the two prefixes is preferred, and use this for any new AK binding. It would also be nice to ensure the preferred prefix is in all drivers' of_match_tables, and remove the deprecated prefix from all drivers' of_match_tables *except* where there's already a DT file that references the deprecated vendor prefix. -- 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()
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). Russell, Paul, do patches 1 and 2 look good to you? If so, if you can ack them, I'd be happy to queue this series in the Tegra git tree. If that doesn't work for you, please let me know who will apply these patches. Thanks. Changes since v1: * Address review comments That's not a very useful description of the changes, and there's no V2 in the subject... -- 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 1/3] arm: multi_v7: enable igb, stmpe, lm95245, pwm leds
On 06/09/2014 04:49 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, PWM LEDs generically accessible from user space and an LM95245 temperature sensor chip. The later three can also be found on the Colibri T30 module. While at it move the NEON entry down to its proper place to have it all nicely ordered again. Acked-by: Stephen Warren swar...@nvidia.com I assume this will be applied directly to the arm-soc tree. -- 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
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.
This looks OK; I'll apply once the merge window is done.
diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts
b/arch/arm/boot/dts/tegra30-apalis-eval.dts
+ pwmleds {
...
+ pwm3 {
+ pwm2 {
+ pwm1 {
Those still aren't sorted. I'll try and remember to fix that when I
apply 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 v2 2/4] pinctrl: Add NVIDIA Tegra XUSB pad controller support
On 06/10/2014 05:11 AM, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com The XUSB pad controller found on NVIDIA Tegra SoCs provides several pads that lanes can be assigned to in order to support a variety of interface options: USB 2.0, USB 3.0, PCIe and SATA. In addition to the pin controller used to assign lanes to pads two PHYs are exposed to allow the bricks for PCIe and SATA to be powered up and down by PCIe and SATA drivers. Aside from the issue Andrew pointed out, this series looks good to me. I'll apply once that one issue is fixed. Linus, Patch 2 (pinctrl driver) depends on patch 1 (binding header), and there are other patches that will also depend on the binding header in patch 1. I guess I should apply patch 1 in a topic branch and send you a pull request which you can merge before applying patch 2. Does that sound OK to you? -- 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/2] ARM: tegra: tamonten: add the base board regulators
On 06/12/2014 09:11 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.
diff --git a/arch/arm/boot/dts/tegra20-medcom-wide.dts
b/arch/arm/boot/dts/tegra20-medcom-wide.dts
+ board_regulators {
The name board_regulators encodes both type (regulators) and
identify (board: the regulators on the board). DT node names are
suposed to contain type but not identity information.
The way this is done in other Tegra DTs is to have a regulators node in
both the board and module DT files, and use different ranges of reg
values for the board and module; e.g. 0..99 for module and 100.. for the
board or similar.
Other than that, this patch looks fine.
--
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/4] pinctrl: Add NVIDIA Tegra XUSB pad controller support
On 06/11/2014 02:23 PM, Andrew Bresticker wrote:
> On Tue, Jun 10, 2014 at 4:11 AM, Thierry Reding
> wrote:
>> From: Thierry Reding
>>
>> The XUSB pad controller found on NVIDIA Tegra SoCs provides several pads
>> that lanes can be assigned to in order to support a variety of interface
>> options: USB 2.0, USB 3.0, PCIe and SATA.
>>
>> In addition to the pin controller used to assign lanes to pads two PHYs
>> are exposed to allow the bricks for PCIe and SATA to be powered up and
>> down by PCIe and SATA drivers.
>> diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c
>> b/drivers/pinctrl/pinctrl-tegra-xusb.c
>> +static int tegra_xusb_padctl_pinconf_group_set(struct pinctrl_dev *pinctrl,
>> + unsigned int group,
>> + unsigned long *configs,
>> + unsigned int num_configs)
>> + for (i = 0; i < num_configs; i++) {
>> + param = TEGRA_XUSB_PADCTL_UNPACK_PARAM(configs[i]);
>> + value = TEGRA_XUSB_PADCTL_UNPACK_VALUE(configs[i]);
>> +
>> + switch (param) {
>> + case TEGRA_XUSB_PADCTL_IDDQ:
>> + value = padctl_readl(padctl, lane->offset);
>
> This overwrites the configuration value - probably want to use a
> separate variable for the register value.
It'd be nice to trim what you quote so that people don't have to wade
through hundreds of lines of code to find a 2-line comment. It's easily
missed.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/11/2014 10:19 AM, Peter De Schrijver wrote:
> On Wed, Jun 11, 2014 at 05:58:28PM +0200, Stephen Warren wrote:
>> On 06/11/2014 09:25 AM, Peter De Schrijver wrote:
>>> On Wed, Jun 11, 2014 at 02:47:31PM +0200, Mikko Perttunen wrote:
>>>> On 05/06/14 16:09, Peter De Schrijver wrote:
>>>> ...
>>>>> +int tegra_fuse_readl(u32 offset, u32 *val)
>>>>> +{
>>>>> + if (!fuse_readl)
>>>>> + return -ENXIO;
>>>>> +
>>>>> + *val = fuse_readl(offset);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>
>>>> -EPROBE_DEFER would be a better error value, so that drivers can work
>>>
>>> Ok.
>>>
>>>> even if they are initially probed before the fuse driver. Of course, if
>>>> the fuse initialization is moved into machine init then this is a
>>>> non-issue.
>>>
>>> The exported function will always be initialized later because on Tegra20 it
>>> requires APB DMA to be available. If you read the fuses directly, the system
>>> sometimes hangs.
>>
>> That's not true in the current code. IIRC, the bug was that *if* an APB
>> DMA access to anything and a CPU access to the fuses happen at the same
>> time, then there can be a hang. As such, the current fuse code accesses
>> the fuses directly (without potential for a hang) if the APB DMA driver
>> is not available, but once the driver becomes available, it reads the
>> fuses through DMA instead. Does the new code not do that?
>>
>
> I'm not so sure about that. I have seen the hang when dumping all fuses using
> sysfs in an otherwise idle system booted from initrd. I don't think there
> should be any APB DMA activity going on then?
Hmm. Perhaps I'm misremembering the trigger for the bug then. Still, the
existing code works as I described. Perhaps that's dangerous and it
shouldn't though. Either way, I think we should have a standalone commit
that removes tegra_apb_readl_using_dma()'s fallback to
tegra_apb_readl_direct(), so any behaviour change that causes a problem
can be bisected easily.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/11/2014 09:25 AM, Peter De Schrijver wrote:
> On Wed, Jun 11, 2014 at 02:47:31PM +0200, Mikko Perttunen wrote:
>> On 05/06/14 16:09, Peter De Schrijver wrote:
>> ...
>>> +int tegra_fuse_readl(u32 offset, u32 *val)
>>> +{
>>> + if (!fuse_readl)
>>> + return -ENXIO;
>>> +
>>> + *val = fuse_readl(offset);
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> -EPROBE_DEFER would be a better error value, so that drivers can work
>
> Ok.
>
>> even if they are initially probed before the fuse driver. Of course, if
>> the fuse initialization is moved into machine init then this is a non-issue.
>
> The exported function will always be initialized later because on Tegra20 it
> requires APB DMA to be available. If you read the fuses directly, the system
> sometimes hangs.
That's not true in the current code. IIRC, the bug was that *if* an APB
DMA access to anything and a CPU access to the fuses happen at the same
time, then there can be a hang. As such, the current fuse code accesses
the fuses directly (without potential for a hang) if the APB DMA driver
is not available, but once the driver becomes available, it reads the
fuses through DMA instead. Does the new code not do 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] irqchip: gic: always mask interrupts during suspend
On 06/10/2014 05:48 PM, Brian Norris wrote: > On Wed, Jun 11, 2014 at 01:34:39AM +0200, Thomas Gleixner wrote: >> On Tue, 10 Jun 2014, Brian Norris wrote: >>> Other random thought: it seems like any irqchip driver which does lazy IRQ >>> masking ought to use IRQCHIP_MASK_ON_SUSPEND. So maybe the IRQ core should >>> just >>> do something like: >>> >>> if (!chip->irq_disable) >>> chip->flags |= IRQCHIP_MASK_ON_SUSPEND; >> >> No. Lazy irq disable and the suspend logic are different beasts. > > OK, fair enough. Drop that random thought then. It's not in the patch > content anyway. > >> That's up to the platform to decide this. Just for the record: there >> is a world outside of ARM... > > OK. But GIC is ARM-specific, so we can still constrain this patch and > related topics to the world of ARM. > >> But that brings me to a different question: >> >> Why are you not putting that customization into the device tree >> instead of trying to add this to some random arch/arm/mach-foo >> files? > > I'm not adding customization to arch/arm/mach-foo files. I'm trying to > remove it. > > This property could be added to device tree, if there was really a valid > use case for a GIC which leaves its interrupts unmasked for suspend. My > question in this patch is essentially: does such a use case exist? DT should genernally only contain data that's expected to vary between boards, or just possibly between SoCs. Anything that the kernel knows simply because it knows what HW model it's driving has no place in DT, since it just adds redundant work to parse the DT and end up with the same data. -- 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] irqchip: gic: always mask interrupts during suspend
On 06/10/2014 05:48 PM, Brian Norris wrote: On Wed, Jun 11, 2014 at 01:34:39AM +0200, Thomas Gleixner wrote: On Tue, 10 Jun 2014, Brian Norris wrote: Other random thought: it seems like any irqchip driver which does lazy IRQ masking ought to use IRQCHIP_MASK_ON_SUSPEND. So maybe the IRQ core should just do something like: if (!chip-irq_disable) chip-flags |= IRQCHIP_MASK_ON_SUSPEND; No. Lazy irq disable and the suspend logic are different beasts. OK, fair enough. Drop that random thought then. It's not in the patch content anyway. That's up to the platform to decide this. Just for the record: there is a world outside of ARM... OK. But GIC is ARM-specific, so we can still constrain this patch and related topics to the world of ARM. But that brings me to a different question: Why are you not putting that customization into the device tree instead of trying to add this to some random arch/arm/mach-foo files? I'm not adding customization to arch/arm/mach-foo files. I'm trying to remove it. This property could be added to device tree, if there was really a valid use case for a GIC which leaves its interrupts unmasked for suspend. My question in this patch is essentially: does such a use case exist? DT should genernally only contain data that's expected to vary between boards, or just possibly between SoCs. Anything that the kernel knows simply because it knows what HW model it's driving has no place in DT, since it just adds redundant work to parse the DT and end up with the same data. -- 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/11/2014 09:25 AM, Peter De Schrijver wrote:
On Wed, Jun 11, 2014 at 02:47:31PM +0200, Mikko Perttunen wrote:
On 05/06/14 16:09, Peter De Schrijver wrote:
...
+int tegra_fuse_readl(u32 offset, u32 *val)
+{
+ if (!fuse_readl)
+ return -ENXIO;
+
+ *val = fuse_readl(offset);
+
+ return 0;
+}
+
-EPROBE_DEFER would be a better error value, so that drivers can work
Ok.
even if they are initially probed before the fuse driver. Of course, if
the fuse initialization is moved into machine init then this is a non-issue.
The exported function will always be initialized later because on Tegra20 it
requires APB DMA to be available. If you read the fuses directly, the system
sometimes hangs.
That's not true in the current code. IIRC, the bug was that *if* an APB
DMA access to anything and a CPU access to the fuses happen at the same
time, then there can be a hang. As such, the current fuse code accesses
the fuses directly (without potential for a hang) if the APB DMA driver
is not available, but once the driver becomes available, it reads the
fuses through DMA instead. Does the new code not do 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/11/2014 10:19 AM, Peter De Schrijver wrote:
On Wed, Jun 11, 2014 at 05:58:28PM +0200, Stephen Warren wrote:
On 06/11/2014 09:25 AM, Peter De Schrijver wrote:
On Wed, Jun 11, 2014 at 02:47:31PM +0200, Mikko Perttunen wrote:
On 05/06/14 16:09, Peter De Schrijver wrote:
...
+int tegra_fuse_readl(u32 offset, u32 *val)
+{
+ if (!fuse_readl)
+ return -ENXIO;
+
+ *val = fuse_readl(offset);
+
+ return 0;
+}
+
-EPROBE_DEFER would be a better error value, so that drivers can work
Ok.
even if they are initially probed before the fuse driver. Of course, if
the fuse initialization is moved into machine init then this is a
non-issue.
The exported function will always be initialized later because on Tegra20 it
requires APB DMA to be available. If you read the fuses directly, the system
sometimes hangs.
That's not true in the current code. IIRC, the bug was that *if* an APB
DMA access to anything and a CPU access to the fuses happen at the same
time, then there can be a hang. As such, the current fuse code accesses
the fuses directly (without potential for a hang) if the APB DMA driver
is not available, but once the driver becomes available, it reads the
fuses through DMA instead. Does the new code not do that?
I'm not so sure about that. I have seen the hang when dumping all fuses using
sysfs in an otherwise idle system booted from initrd. I don't think there
should be any APB DMA activity going on then?
Hmm. Perhaps I'm misremembering the trigger for the bug then. Still, the
existing code works as I described. Perhaps that's dangerous and it
shouldn't though. Either way, I think we should have a standalone commit
that removes tegra_apb_readl_using_dma()'s fallback to
tegra_apb_readl_direct(), so any behaviour change that causes a problem
can be bisected easily.
--
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/4] pinctrl: Add NVIDIA Tegra XUSB pad controller support
On 06/11/2014 02:23 PM, Andrew Bresticker wrote:
On Tue, Jun 10, 2014 at 4:11 AM, Thierry Reding
thierry.red...@gmail.com wrote:
From: Thierry Reding tred...@nvidia.com
The XUSB pad controller found on NVIDIA Tegra SoCs provides several pads
that lanes can be assigned to in order to support a variety of interface
options: USB 2.0, USB 3.0, PCIe and SATA.
In addition to the pin controller used to assign lanes to pads two PHYs
are exposed to allow the bricks for PCIe and SATA to be powered up and
down by PCIe and SATA drivers.
diff --git a/drivers/pinctrl/pinctrl-tegra-xusb.c
b/drivers/pinctrl/pinctrl-tegra-xusb.c
+static int tegra_xusb_padctl_pinconf_group_set(struct pinctrl_dev *pinctrl,
+ unsigned int group,
+ unsigned long *configs,
+ unsigned int num_configs)
+ for (i = 0; i num_configs; i++) {
+ param = TEGRA_XUSB_PADCTL_UNPACK_PARAM(configs[i]);
+ value = TEGRA_XUSB_PADCTL_UNPACK_VALUE(configs[i]);
+
+ switch (param) {
+ case TEGRA_XUSB_PADCTL_IDDQ:
+ value = padctl_readl(padctl, lane-offset);
This overwrites the configuration value - probably want to use a
separate variable for the register value.
It'd be nice to trim what you quote so that people don't have to wade
through hundreds of lines of code to find a 2-line comment. It's easily
missed.
--
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] cpufreq: tegra: update comment for clarity
On 06/05/2014 10:38 PM, Viresh Kumar wrote: > Tegra's driver got updated a bit (00917dd cpufreq: Tegra: implement > intermediate > frequency callbacks) and implements new 'intermediate freq' infrastructure of > core. Above commit updated comments about when to call > clk_prepare_enable(pll_x_clk) and Doug wasn't satisfied with those comments > and > said this: > >> The "Though when target-freq is intermediate freq, we don't need to >> take this reference." makes me think that this function is actually >> called when target-freq is intermediate freq. I don't think it is, >> right? > > For better clarity just make that comment more explicit about when we call > tegra_target_intermediate(). Wasn't sure if we actually need a commit for > this, > but anyway lets other decide if its worth enough :) > diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c > index a5fbc0a..48bc89b 100644 > --- a/drivers/cpufreq/tegra-cpufreq.c > +++ b/drivers/cpufreq/tegra-cpufreq.c > @@ -73,7 +73,7 @@ static int tegra_target_intermediate(struct cpufreq_policy > *policy, >* off when we move the cpu off of it as enabling it again while we >* switch to it from tegra_target() would take additional time. Though >* when target-freq is intermediate freq, we don't need to take this > - * reference. > + * reference and so this routine isn't called at all. >*/ > clk_prepare_enable(pll_x_clk); I would remove the word "so" in the added line; the function is not called because we don't need to use an intermediate frequency, not because we don't need to take a reference to pll_x. With that change, Reviewed-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 0/9] Serial ATA support for NVIDIA Tegra124
On 06/06/2014 12:27 AM, Mikko Perttunen wrote: > The only compile-time dependencies here should be that: > - patch 8 of 9 which contains the actual driver depends on patch 6 of 9 > (though only when building as a module) and the efuse series > - patch 2 of 9 refers to the DT node called "padctl", so it requires the > xusb series. (in the submitted xusb series, the node isn't actually > named, though. I will fix this in v2) Isn't that dependency a run-time dependency? -- 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/06/2014 01:35 AM, Peter De Schrijver wrote: > On Fri, Jun 06, 2014 at 12:54:00AM +0200, Stephen Warren wrote: ... >>> No. It's only used to populate /sys/devices/soc0/revision. I don't think >>> that's particularly important. >> >> But it's a feature that works today. Why should we break it? > > I don't expect people to not update their DT actually... But that's not how DT works; old DTs must continue to work. >>> sdhci needs this for faster modes I guess which will also need extra DT >>> properties looking at the chromeos driver. The others definitely will need >>> an updated DT. For randomness I haven't seen any appreciable difference in >>> when >>> the 'random: nonblocking pool is initialized' message appears between having >>> the randomness addition or not. Most likely the bulk of the randomness comes >>> from serial interrupts rather than the fuse data. So I don't think the move >>> to >>> a driver probe will cause any problem. Nor do I think the lack of an updated >>> DT will cause problems. >> >> But what advantage do we have by actively changing it? > > We need to move the code anyway when we will have 64bit SoCs. Using DT also > allows us to reuse the code even when the base address changes in the future. > If it weren't for Tegra20 A03p, we could also drop the hack to enable the > clocks directly, but use CCF instead. Sure we need to move the code out of arch/arm so it can be shared with arm64. However, that doesn't imply that we need to change anything about the way the code works or is initialized; we can still do all the initialization in response to a function call from the arch/board support, and not in response to driver probe. -- 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/06/2014 01:35 AM, Peter De Schrijver wrote: On Fri, Jun 06, 2014 at 12:54:00AM +0200, Stephen Warren wrote: ... No. It's only used to populate /sys/devices/soc0/revision. I don't think that's particularly important. But it's a feature that works today. Why should we break it? I don't expect people to not update their DT actually... But that's not how DT works; old DTs must continue to work. sdhci needs this for faster modes I guess which will also need extra DT properties looking at the chromeos driver. The others definitely will need an updated DT. For randomness I haven't seen any appreciable difference in when the 'random: nonblocking pool is initialized' message appears between having the randomness addition or not. Most likely the bulk of the randomness comes from serial interrupts rather than the fuse data. So I don't think the move to a driver probe will cause any problem. Nor do I think the lack of an updated DT will cause problems. But what advantage do we have by actively changing it? We need to move the code anyway when we will have 64bit SoCs. Using DT also allows us to reuse the code even when the base address changes in the future. If it weren't for Tegra20 A03p, we could also drop the hack to enable the clocks directly, but use CCF instead. Sure we need to move the code out of arch/arm so it can be shared with arm64. However, that doesn't imply that we need to change anything about the way the code works or is initialized; we can still do all the initialization in response to a function call from the arch/board support, and not in response to driver probe. -- 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/9] Serial ATA support for NVIDIA Tegra124
On 06/06/2014 12:27 AM, Mikko Perttunen wrote: The only compile-time dependencies here should be that: - patch 8 of 9 which contains the actual driver depends on patch 6 of 9 (though only when building as a module) and the efuse series - patch 2 of 9 refers to the DT node called padctl, so it requires the xusb series. (in the submitted xusb series, the node isn't actually named, though. I will fix this in v2) Isn't that dependency a run-time dependency? -- 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] cpufreq: tegra: update comment for clarity
On 06/05/2014 10:38 PM, Viresh Kumar wrote: Tegra's driver got updated a bit (00917dd cpufreq: Tegra: implement intermediate frequency callbacks) and implements new 'intermediate freq' infrastructure of core. Above commit updated comments about when to call clk_prepare_enable(pll_x_clk) and Doug wasn't satisfied with those comments and said this: The Though when target-freq is intermediate freq, we don't need to take this reference. makes me think that this function is actually called when target-freq is intermediate freq. I don't think it is, right? For better clarity just make that comment more explicit about when we call tegra_target_intermediate(). Wasn't sure if we actually need a commit for this, but anyway lets other decide if its worth enough :) diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index a5fbc0a..48bc89b 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -73,7 +73,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy, * off when we move the cpu off of it as enabling it again while we * switch to it from tegra_target() would take additional time. Though * when target-freq is intermediate freq, we don't need to take this - * reference. + * reference and so this routine isn't called at all. */ clk_prepare_enable(pll_x_clk); I would remove the word so in the added line; the function is not called because we don't need to use an intermediate frequency, not because we don't need to take a reference to pll_x. With that change, Reviewed-by: Stephen Warren swar...@nvidia.com -- 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/4] of: Add NVIDIA Tegra XUSB pad controller binding
On 06/05/2014 04:08 PM, Thierry Reding wrote: > On Thu, Jun 05, 2014 at 10:47:45AM -0600, Stephen Warren wrote: >> On 06/04/2014 09:16 AM, Thierry Reding wrote: >>> From: Thierry Reding >>> >>> This patch adds the device tree binding documentation for the XUSB pad >>> controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY >>> capabilities. >> >>> diff --git >>> a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >>> b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt >> >>> +- #phy-cells: Should be 1. The specifier is the index of the PHY to >>> reference. >>> + Possible values are: >>> + - 0: PCIe >>> + - 1: SATA >> >> Those values are defined in >> include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider >> the header files to be part of the binding itself, rather >> than being derived from the binding. As such, I'd suggest the following >> changes: >> >> * Make this patch 1 not patch 2 >> * Move pinctrl-tegra-xusb.h into this patch. >> * Remove the list of values above, and replace it with the text "See >> for the set of valid values". > > I remember discussions where people explicitly said that relying on the > symbolic names in the DT bindings was a mistake because it would mean > that everyone would need to have access to a mechanism similar to what > we have in the Linux kernel (and that the header files would always need > to be shipped with the DT bindings). The entire point of the headers is so that the binding definitions and code using them can't get out of sync. In my opinion, the headers *are* part of the bindings, so of course anyone either reading DT bindings, or parsing DT, must have the headers, and hence the headers must be bundled with the DT files or with the bindings wherever they go. signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 06/05/2014 04:13 PM, Peter De Schrijver wrote:
> On Thu, Jun 05, 2014 at 08:41:55PM +0200, Stephen Warren wrote:
>> On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
>>> Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114.dtsi
>>> b/arch/arm/boot/dts/tegra114.dtsi
>>
>>> + apbmisc@7800 {
>>> + compatible = "nvidia,tegra114-apbmisc",
>>> "nvidia,tegra20-apbmisc";
>>
>> Is the Tegra114 APBMISC register layout 100% a backwards-compatible
>> superset of that in Tegra20? For both registers the code currently uses
>> *and* all possible registers the code could ever use? Since the APB MISC
>> is a bit of a dumping ground for random registers, that feels unlikely,
>> but perhaps it's possible.
>
> For all I can see it is. At least for the registers the kernel is likely to
> use.
But that's ("At least for the registers the kernel is likely to use")
not how compatible values are defined. We need to explicitly look at all
the registers and actively decide that it really is compatible in order
to mark it so. If we don't want to do that, it's best just to use a
separate compatible value for each SoC, and add a couple more entries
into the match table.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 04:09 PM, Peter De Schrijver wrote:
> On Thu, Jun 05, 2014 at 08:37:26PM +0200, Stephen Warren wrote:
>> On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
>>> Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124.
>>>
>>> Signed-off-by: Peter De Schrijver
>>> ---
>>> Documentation/ABI/testing/sysfs-driver-tegra-fuse | 11 +
>>> drivers/misc/fuse/Makefile|1 +
>>> drivers/misc/fuse/tegra/Makefile |7 +
>>> drivers/misc/fuse/tegra/fuse-tegra.c | 250 +
>>
>> I wonder if we shouldn't put this into drivers/soc/tegra?
>>
>>> diff --git a/drivers/misc/fuse/tegra/fuse-tegra.c
>>> b/drivers/misc/fuse/tegra/fuse-tegra.c
>>
>>> +void __init tegra_init_fuse(void)
>>> +{
>>> + struct device_node *np;
>>> + u32 id;
>>> + void __iomem *car_base;
>>> +
>>> + np = of_find_matching_node(NULL, apbmisc_match);
>>> + apbmisc_base = of_iomap(np, 0);
>>> + if (!apbmisc_base) {
>>> + pr_warn("ioremap tegra apbmisc failed. using %08x instead\n",
>>> + APBMISC_BASE);
>>> + apbmisc_base = ioremap(APBMISC_BASE, APBMISC_SIZE);
>>> + }
>>> +
>>> + id = tegra_read_chipid();
>>> + tegra_chip_id = (id >> 8) & 0xff;
>>
>> So there's a fallback using APBMIS_BASE above if the node is missing, so
>> those last 2 lines always happen. However, if any of the following fail:
>>
>>> + strapping_base = of_iomap(np, 0);
>> ...
>>> + np = of_find_matching_node(NULL, tegra_fuse_match);
>> ...
>>> + np = of_find_matching_node(NULL, car_match);
>>
>> Then this doesn't get executed:
>>
>>> + tegra_get_revision(id);
>>
>> Isn't that important?
>
> No. It's only used to populate /sys/devices/soc0/revision. I don't think
> that's particularly important.
But it's a feature that works today. Why should we break it?
>> I guess that can't run if the lookup for tegra_fuse_match isn't
>> successful, since that tegra_get_revision may call
>> tegra20_spare_fuse_early() which uses fuse_base which is set up in
>> response to succesfully calling on of those node lookups. Isn't a
>> fallback needed there too?
>
> tegra20_spare_fuse_early() will not be called if fuse_base is NULL.
Oh yes. We can at least call the function then even if the fuses can't
be mapped.
But to avoid regressions, we should simply make sure the fuses can be
mapped.
>> I'm also a bit concerned that the driver probes, rather than the early
>> function tegra_init_fuse(), are doing things like setting up the speedo
>> data initialization, randomness addition, etc. For one, those won't
>> happen any more unless the DT nodes are present, and secondly,
>> triggering all those from driver probe rather than a function that's
>> called from the machine descriptor makes guaranteeing the timing
>> problematic.
>
> Today there are no users of the speedo ID in upstream.
Well, except people reading kernel log messages. Accurate speedo-related
log messages have help pin-point a problem or two in the past.
> Looking at chromeos
> the users of the speedo ID are: CPU dvfs, GPU dvfs and sdhci. The last 2 also
> need regulators and therefor will need to support deferred probing anyway.
> CPU dvfs isn't critical at all, we don't care when it gets initialized. So
> deferred probe is fine.
What condition will those modules/drivers use to defer their probe?
> sdhci needs this for faster modes I guess which will also need extra DT
> properties looking at the chromeos driver. The others definitely will need
> an updated DT. For randomness I haven't seen any appreciable difference in
> when
> the 'random: nonblocking pool is initialized' message appears between having
> the randomness addition or not. Most likely the bulk of the randomness comes
> from serial interrupts rather than the fuse data. So I don't think the move to
> a driver probe will cause any problem. Nor do I think the lack of an updated
> DT will cause problems.
But what advantage do we have by actively changing 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 v7 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
> Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> + apbmisc@7800 {
> + compatible = "nvidia,tegra114-apbmisc",
> "nvidia,tegra20-apbmisc";
Is the Tegra114 APBMISC register layout 100% a backwards-compatible
superset of that in Tegra20? For both registers the code currently uses
*and* all possible registers the code could ever use? Since the APB MISC
is a bit of a dumping ground for random registers, that feels unlikely,
but perhaps it's possible.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
> Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124.
>
> Signed-off-by: Peter De Schrijver
> ---
> Documentation/ABI/testing/sysfs-driver-tegra-fuse | 11 +
> drivers/misc/fuse/Makefile|1 +
> drivers/misc/fuse/tegra/Makefile |7 +
> drivers/misc/fuse/tegra/fuse-tegra.c | 250 +
I wonder if we shouldn't put this into drivers/soc/tegra?
> diff --git a/drivers/misc/fuse/tegra/fuse-tegra.c
> b/drivers/misc/fuse/tegra/fuse-tegra.c
> +void __init tegra_init_fuse(void)
> +{
> + struct device_node *np;
> + u32 id;
> + void __iomem *car_base;
> +
> + np = of_find_matching_node(NULL, apbmisc_match);
> + apbmisc_base = of_iomap(np, 0);
> + if (!apbmisc_base) {
> + pr_warn("ioremap tegra apbmisc failed. using %08x instead\n",
> + APBMISC_BASE);
> + apbmisc_base = ioremap(APBMISC_BASE, APBMISC_SIZE);
> + }
> +
> + id = tegra_read_chipid();
> + tegra_chip_id = (id >> 8) & 0xff;
So there's a fallback using APBMIS_BASE above if the node is missing, so
those last 2 lines always happen. However, if any of the following fail:
> + strapping_base = of_iomap(np, 0);
...
> + np = of_find_matching_node(NULL, tegra_fuse_match);
...
> + np = of_find_matching_node(NULL, car_match);
Then this doesn't get executed:
> + tegra_get_revision(id);
Isn't that important?
I guess that can't run if the lookup for tegra_fuse_match isn't
successful, since that tegra_get_revision may call
tegra20_spare_fuse_early() which uses fuse_base which is set up in
response to succesfully calling on of those node lookups. Isn't a
fallback needed there too?
I'm also a bit concerned that the driver probes, rather than the early
function tegra_init_fuse(), are doing things like setting up the speedo
data initialization, randomness addition, etc. For one, those won't
happen any more unless the DT nodes are present, and secondly,
triggering all those from driver probe rather than a function that's
called from the machine descriptor makes guaranteeing the timing
problematic.
I'd prefer to see the driver probes *just* set up the sysfs, and have
the code initialization stay unchanged relative to the code currently in
arch/arm/mach-tegra/ if possible.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 07:09 AM, Peter De Schrijver wrote: > Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. This series isn't bisectable; building at either patch 3 or patch 4 yields: > In file included from arch/arm/mach-tegra/fuse.c:28:0: > arch/arm/mach-tegra/fuse.h:40:5: error: conflicting types for > ‘tegra_fuse_readl’ > In file included from arch/arm/mach-tegra/fuse.c:26:0: > include/linux/tegra-soc.h:43:5: note: previous declaration of > ‘tegra_fuse_readl’ was here > arch/arm/mach-tegra/fuse.c:98:5: error: conflicting types for > ‘tegra_fuse_readl’ > In file included from arch/arm/mach-tegra/fuse.c:26:0: > include/linux/tegra-soc.h:43:5: note: previous declaration of > ‘tegra_fuse_readl’ was here > make[1]: *** [arch/arm/mach-tegra/fuse.o] Error 1 > make[1]: *** Waiting for unfinished jobs > make: *** [arch/arm/mach-tegra] Error 2 > make: *** Waiting for unfinished jobs -- 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 v7 1/5] ARM: tegra: export apb dma readl/writel
On 06/05/2014 11:57 AM, Stephen Warren wrote: > On 06/05/2014 07:09 AM, Peter De Schrijver wrote: >> Export APB DMA readl and writel. These are needed because we can't access >> the fuses directly on Tegra20 without potentially causing a system hang. >> Also have the APB DMA readl and writel return an error in case of a read >> failure instead of just returning zero or ignore write failures. > >> diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h > >> +int tegra_apb_readl_using_dma(unsigned long offset, u32 *value); >> +int tegra_apb_writel_using_dma(u32 value, unsigned long offset); > > Hmm. I wonder why we're exporting those low-level internal functions > rather than the higher-level tegra_apb_readl()/writel() wrappers that > "clients" are supposed to be using. Oh, I suppose this is just for rivers/misc/fuse/tegra/fuse-tegra20.c to use it, not for arbitrary clients, so this is probably OK. Any reason not to move all the DMA-related fuse-reading code into fuse-tegra20.c instead? After this series, is there code left in arch/arm/mach-tegra which will still need to call into the fuse-reading code and hence requires tegra_apb_readl_using_dma() to exist? -- 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 v7 1/5] ARM: tegra: export apb dma readl/writel
On 06/05/2014 07:09 AM, Peter De Schrijver wrote: > Export APB DMA readl and writel. These are needed because we can't access > the fuses directly on Tegra20 without potentially causing a system hang. > Also have the APB DMA readl and writel return an error in case of a read > failure instead of just returning zero or ignore write failures. > diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h > +int tegra_apb_readl_using_dma(unsigned long offset, u32 *value); > +int tegra_apb_writel_using_dma(u32 value, unsigned long offset); Hmm. I wonder why we're exporting those low-level internal functions rather than the higher-level tegra_apb_readl()/writel() wrappers that "clients" are supposed to be using. -- 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/9] Serial ATA support for NVIDIA Tegra124
On 06/04/2014 05:32 AM, Mikko Perttunen wrote: > Hi, > > This series adds support for the onboard AHCI-compliant Serial ATA > controller found on Tegra124 systems-on-chip. The controller is > enabled on Jetson TK1. The series depends on Peter's efuse series > and Thierry's yet to be submitted XUSB pinctrl driver. This series includes patches to a lot of different subsystems. That will complicate applying it. Can you write a summary of the *compile time* dependencies, since that will influence how the patches get merged. You mentioned that this series depends on efuse and XUSB padctl. Can you point out which specific parts of this series depend on which of those two other series, and whether this is a compile-time or run-time dependency. I hope that the drivers/ata patches, drivers/clk patches, DT, and defconfig patches can each be applied to their normal tree and don't depend on each-other at compile-time at all. At run-time, obviously all the patches are needed to make the code work, but since this is a new feature, it's fine if this all only works once everything is merged together in linux-next or Linus's tree. 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 1/4] pinctrl: Add NVIDIA Tegra XUSB pad controller support
On 06/04/2014 09:16 AM, Thierry Reding wrote:
> From: Thierry Reding
>
> The XUSB pad controller found on NVIDIA Tegra SoCs provides several pads
> that lanes can be assigned to in order to support a variety of interface
> options: USB 2.0, USB 3.0, PCIe and SATA.
>
> In addition to the pin controller used to assign lanes to pads two PHYs
> are exposed to allow the bricks for PCIe and SATA to be powered up and
> down by PCIe and SATA drivers.
> +#define TEGRA124_GROUP(_funcs)
> \
> + { \
> + .num_funcs = ARRAY_SIZE(tegra124_##_funcs##_functions), \
> + .funcs = tegra124_##_funcs##_functions, \
> + }
> +
> +static const struct tegra_xusb_padctl_group tegra124_groups[] = {
> + TEGRA124_GROUP(otg),
> + TEGRA124_GROUP(usb),
> + TEGRA124_GROUP(pci),
> +};
I'm not sure what this set of groups is for.
pinctrl muxes functions onto groups, so given that each pin in padctl is
individually configurable, we need 1 group per pin. As far as I can
tell, tegra_xusb_padctl_get_groups_count()/name() implement this
correctly, and this array isn't used anywhere?
--
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/4] of: Add NVIDIA Tegra XUSB pad controller binding
On 06/04/2014 09:16 AM, Thierry Reding wrote:
> From: Thierry Reding
>
> This patch adds the device tree binding documentation for the XUSB pad
> controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY
> capabilities.
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
> +- #phy-cells: Should be 1. The specifier is the index of the PHY to
> reference.
> + Possible values are:
> + - 0: PCIe
> + - 1: SATA
Those values are defined in
include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider
the header files to be part of the binding itself, rather
than being derived from the binding. As such, I'd suggest the following
changes:
* Make this patch 1 not patch 2
* Move pinctrl-tegra-xusb.h into this patch.
* Remove the list of values above, and replace it with the text "See
for the set of valid values".
We'll need to make sure this patch is applied to a topic branch that can
be merged into the pinctrl tree and the Tegra tree for 3.17, assuming
that you'll be sending patches for 3.17 that use pinctrl-tegra-xusb.h.
> +Example:
> +
> +
> +SoC file extract:
> +-
> +
> + padctl@0,7009f000 {
> + compatible = "nvidia,tegra124-xusb-padctl";
> + reg = <0x0 0x7009f000 0x0 0x1000>;
> + resets = <_car 142>;
> + reset-names = "padctl";
> +
> + #address-cells = <0>;
> + #size-cells = <0>;
Why are those two properties required? Yes, this node has sub-nodes, but
those sub-nodes don't have a reg property or unit address. The main
Tegra pinctrl nodes don't have #address/size-cells.
> +Board file extract:
> +---
> + padctl: padctl@0,7009f000 {
> + pinmux {
> + pinctrl-0 = <_default>;
> + pinctrl-names = "default";
Isn't there one extra level of nodes here. In the DT patches later in
this series, pinctrl-0/pinctrl-names are directly inside the top-level
padctl node.
--
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
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 This series looks fine to me. Given it touches both Tegra and Exynos, should it be merged into a topic branch in arm-soc, so that it can be pulled into both Tegra/Exynos trees to resolve any conflicts. So, Acked-by: Stephen Warren If you want, I'm happy to apply this to a topic branch and send a pull request to arm-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 v5] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/05/2014 12:50 AM, f...@marvell.com wrote:
> From: Fan Wu
>
> What the patch did:
> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time
> of
> calling pinctrl_select_state
> 2.Remove the HW disable operation in in pinmux_disable_setting function.
> 3.Remove the disable ops in struct pinmux_ops
> 4.Remove all the disable ops users in current code base.
...
The overall concept, plus the bcm2835, and Tegra driver parts,
Acked-by: Stephen Warren
That said ...
> diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
> -static void tegra_pinctrl_disable(struct pinctrl_dev *pctldev,
> - unsigned function, unsigned group)
> -{
> - struct tegra_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> - const struct tegra_pingroup *g;
> - u32 val;
> -
> - g = >soc->groups[group];
> -
> - if (WARN_ON(g->mux_reg < 0))
> - return;
> -
> - val = pmx_readl(pmx, g->mux_bank, g->mux_reg);
> - val &= ~(0x3 << g->mux_bit);
> - val |= g->func_safe << g->mux_bit;
> - pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
Those last 5 lines don't exist in the latest code, so this patch won't
apply cleanly. You probably want to rebase this on top of LinusW's
latest pinctrl development 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 v5] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/05/2014 12:50 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time
of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
3.Remove the disable ops in struct pinmux_ops
4.Remove all the disable ops users in current code base.
...
The overall concept, plus the bcm2835, and Tegra driver parts,
Acked-by: Stephen Warren swar...@nvidia.com
That said ...
diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
-static void tegra_pinctrl_disable(struct pinctrl_dev *pctldev,
- unsigned function, unsigned group)
-{
- struct tegra_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
- const struct tegra_pingroup *g;
- u32 val;
-
- g = pmx-soc-groups[group];
-
- if (WARN_ON(g-mux_reg 0))
- return;
-
- val = pmx_readl(pmx, g-mux_bank, g-mux_reg);
- val = ~(0x3 g-mux_bit);
- val |= g-func_safe g-mux_bit;
- pmx_writel(pmx, val, g-mux_bank, g-mux_reg);
Those last 5 lines don't exist in the latest code, so this patch won't
apply cleanly. You probably want to rebase this on top of LinusW's
latest pinctrl development 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
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 This series looks fine to me. Given it touches both Tegra and Exynos, should it be merged into a topic branch in arm-soc, so that it can be pulled into both Tegra/Exynos trees to resolve any conflicts. So, Acked-by: Stephen Warren swar...@nvidia.com If you want, I'm happy to apply this to a topic branch and send a pull request to arm-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 2/4] of: Add NVIDIA Tegra XUSB pad controller binding
On 06/04/2014 09:16 AM, Thierry Reding wrote:
From: Thierry Reding tred...@nvidia.com
This patch adds the device tree binding documentation for the XUSB pad
controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY
capabilities.
diff --git
a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt
+- #phy-cells: Should be 1. The specifier is the index of the PHY to
reference.
+ Possible values are:
+ - 0: PCIe
+ - 1: SATA
Those values are defined in
include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider
the dt-bindings/ header files to be part of the binding itself, rather
than being derived from the binding. As such, I'd suggest the following
changes:
* Make this patch 1 not patch 2
* Move pinctrl-tegra-xusb.h into this patch.
* Remove the list of values above, and replace it with the text See
dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the set of valid values.
We'll need to make sure this patch is applied to a topic branch that can
be merged into the pinctrl tree and the Tegra tree for 3.17, assuming
that you'll be sending patches for 3.17 that use pinctrl-tegra-xusb.h.
+Example:
+
+
+SoC file extract:
+-
+
+ padctl@0,7009f000 {
+ compatible = nvidia,tegra124-xusb-padctl;
+ reg = 0x0 0x7009f000 0x0 0x1000;
+ resets = tegra_car 142;
+ reset-names = padctl;
+
+ #address-cells = 0;
+ #size-cells = 0;
Why are those two properties required? Yes, this node has sub-nodes, but
those sub-nodes don't have a reg property or unit address. The main
Tegra pinctrl nodes don't have #address/size-cells.
+Board file extract:
+---
+ padctl: padctl@0,7009f000 {
+ pinmux {
+ pinctrl-0 = padctl_default;
+ pinctrl-names = default;
Isn't there one extra level of nodes here. In the DT patches later in
this series, pinctrl-0/pinctrl-names are directly inside the top-level
padctl node.
--
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/4] pinctrl: Add NVIDIA Tegra XUSB pad controller support
On 06/04/2014 09:16 AM, Thierry Reding wrote:
From: Thierry Reding tred...@nvidia.com
The XUSB pad controller found on NVIDIA Tegra SoCs provides several pads
that lanes can be assigned to in order to support a variety of interface
options: USB 2.0, USB 3.0, PCIe and SATA.
In addition to the pin controller used to assign lanes to pads two PHYs
are exposed to allow the bricks for PCIe and SATA to be powered up and
down by PCIe and SATA drivers.
+#define TEGRA124_GROUP(_funcs)
\
+ { \
+ .num_funcs = ARRAY_SIZE(tegra124_##_funcs##_functions), \
+ .funcs = tegra124_##_funcs##_functions, \
+ }
+
+static const struct tegra_xusb_padctl_group tegra124_groups[] = {
+ TEGRA124_GROUP(otg),
+ TEGRA124_GROUP(usb),
+ TEGRA124_GROUP(pci),
+};
I'm not sure what this set of groups is for.
pinctrl muxes functions onto groups, so given that each pin in padctl is
individually configurable, we need 1 group per pin. As far as I can
tell, tegra_xusb_padctl_get_groups_count()/name() implement this
correctly, and this array isn't used anywhere?
--
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/9] Serial ATA support for NVIDIA Tegra124
On 06/04/2014 05:32 AM, Mikko Perttunen wrote: Hi, This series adds support for the onboard AHCI-compliant Serial ATA controller found on Tegra124 systems-on-chip. The controller is enabled on Jetson TK1. The series depends on Peter's efuse series and Thierry's yet to be submitted XUSB pinctrl driver. This series includes patches to a lot of different subsystems. That will complicate applying it. Can you write a summary of the *compile time* dependencies, since that will influence how the patches get merged. You mentioned that this series depends on efuse and XUSB padctl. Can you point out which specific parts of this series depend on which of those two other series, and whether this is a compile-time or run-time dependency. I hope that the drivers/ata patches, drivers/clk patches, DT, and defconfig patches can each be applied to their normal tree and don't depend on each-other at compile-time at all. At run-time, obviously all the patches are needed to make the code work, but since this is a new feature, it's fine if this all only works once everything is merged together in linux-next or Linus's tree. 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 v7 1/5] ARM: tegra: export apb dma readl/writel
On 06/05/2014 07:09 AM, Peter De Schrijver wrote: Export APB DMA readl and writel. These are needed because we can't access the fuses directly on Tegra20 without potentially causing a system hang. Also have the APB DMA readl and writel return an error in case of a read failure instead of just returning zero or ignore write failures. diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h +int tegra_apb_readl_using_dma(unsigned long offset, u32 *value); +int tegra_apb_writel_using_dma(u32 value, unsigned long offset); Hmm. I wonder why we're exporting those low-level internal functions rather than the higher-level tegra_apb_readl()/writel() wrappers that clients are supposed to be using. -- 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 v7 1/5] ARM: tegra: export apb dma readl/writel
On 06/05/2014 11:57 AM, Stephen Warren wrote: On 06/05/2014 07:09 AM, Peter De Schrijver wrote: Export APB DMA readl and writel. These are needed because we can't access the fuses directly on Tegra20 without potentially causing a system hang. Also have the APB DMA readl and writel return an error in case of a read failure instead of just returning zero or ignore write failures. diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h +int tegra_apb_readl_using_dma(unsigned long offset, u32 *value); +int tegra_apb_writel_using_dma(u32 value, unsigned long offset); Hmm. I wonder why we're exporting those low-level internal functions rather than the higher-level tegra_apb_readl()/writel() wrappers that clients are supposed to be using. Oh, I suppose this is just for rivers/misc/fuse/tegra/fuse-tegra20.c to use it, not for arbitrary clients, so this is probably OK. Any reason not to move all the DMA-related fuse-reading code into fuse-tegra20.c instead? After this series, is there code left in arch/arm/mach-tegra which will still need to call into the fuse-reading code and hence requires tegra_apb_readl_using_dma() to exist? -- 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 07:09 AM, Peter De Schrijver wrote: Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. This series isn't bisectable; building at either patch 3 or patch 4 yields: In file included from arch/arm/mach-tegra/fuse.c:28:0: arch/arm/mach-tegra/fuse.h:40:5: error: conflicting types for ‘tegra_fuse_readl’ In file included from arch/arm/mach-tegra/fuse.c:26:0: include/linux/tegra-soc.h:43:5: note: previous declaration of ‘tegra_fuse_readl’ was here arch/arm/mach-tegra/fuse.c:98:5: error: conflicting types for ‘tegra_fuse_readl’ In file included from arch/arm/mach-tegra/fuse.c:26:0: include/linux/tegra-soc.h:43:5: note: previous declaration of ‘tegra_fuse_readl’ was here make[1]: *** [arch/arm/mach-tegra/fuse.o] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [arch/arm/mach-tegra] Error 2 make: *** Waiting for unfinished jobs -- 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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124.
Signed-off-by: Peter De Schrijver pdeschrij...@nvidia.com
---
Documentation/ABI/testing/sysfs-driver-tegra-fuse | 11 +
drivers/misc/fuse/Makefile|1 +
drivers/misc/fuse/tegra/Makefile |7 +
drivers/misc/fuse/tegra/fuse-tegra.c | 250 +
I wonder if we shouldn't put this into drivers/soc/tegra?
diff --git a/drivers/misc/fuse/tegra/fuse-tegra.c
b/drivers/misc/fuse/tegra/fuse-tegra.c
+void __init tegra_init_fuse(void)
+{
+ struct device_node *np;
+ u32 id;
+ void __iomem *car_base;
+
+ np = of_find_matching_node(NULL, apbmisc_match);
+ apbmisc_base = of_iomap(np, 0);
+ if (!apbmisc_base) {
+ pr_warn(ioremap tegra apbmisc failed. using %08x instead\n,
+ APBMISC_BASE);
+ apbmisc_base = ioremap(APBMISC_BASE, APBMISC_SIZE);
+ }
+
+ id = tegra_read_chipid();
+ tegra_chip_id = (id 8) 0xff;
So there's a fallback using APBMIS_BASE above if the node is missing, so
those last 2 lines always happen. However, if any of the following fail:
+ strapping_base = of_iomap(np, 0);
...
+ np = of_find_matching_node(NULL, tegra_fuse_match);
...
+ np = of_find_matching_node(NULL, car_match);
Then this doesn't get executed:
+ tegra_get_revision(id);
Isn't that important?
I guess that can't run if the lookup for tegra_fuse_match isn't
successful, since that tegra_get_revision may call
tegra20_spare_fuse_early() which uses fuse_base which is set up in
response to succesfully calling on of those node lookups. Isn't a
fallback needed there too?
I'm also a bit concerned that the driver probes, rather than the early
function tegra_init_fuse(), are doing things like setting up the speedo
data initialization, randomness addition, etc. For one, those won't
happen any more unless the DT nodes are present, and secondly,
triggering all those from driver probe rather than a function that's
called from the machine descriptor makes guaranteeing the timing
problematic.
I'd prefer to see the driver probes *just* set up the sysfs, and have
the code initialization stay unchanged relative to the code currently in
arch/arm/mach-tegra/ if possible.
--
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 v7 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
+ apbmisc@7800 {
+ compatible = nvidia,tegra114-apbmisc,
nvidia,tegra20-apbmisc;
Is the Tegra114 APBMISC register layout 100% a backwards-compatible
superset of that in Tegra20? For both registers the code currently uses
*and* all possible registers the code could ever use? Since the APB MISC
is a bit of a dumping ground for random registers, that feels unlikely,
but perhaps it's possible.
--
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 v7 3/5] misc: fuse: Add efuse driver for Tegra
On 06/05/2014 04:09 PM, Peter De Schrijver wrote:
On Thu, Jun 05, 2014 at 08:37:26PM +0200, Stephen Warren wrote:
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124.
Signed-off-by: Peter De Schrijver pdeschrij...@nvidia.com
---
Documentation/ABI/testing/sysfs-driver-tegra-fuse | 11 +
drivers/misc/fuse/Makefile|1 +
drivers/misc/fuse/tegra/Makefile |7 +
drivers/misc/fuse/tegra/fuse-tegra.c | 250 +
I wonder if we shouldn't put this into drivers/soc/tegra?
diff --git a/drivers/misc/fuse/tegra/fuse-tegra.c
b/drivers/misc/fuse/tegra/fuse-tegra.c
+void __init tegra_init_fuse(void)
+{
+ struct device_node *np;
+ u32 id;
+ void __iomem *car_base;
+
+ np = of_find_matching_node(NULL, apbmisc_match);
+ apbmisc_base = of_iomap(np, 0);
+ if (!apbmisc_base) {
+ pr_warn(ioremap tegra apbmisc failed. using %08x instead\n,
+ APBMISC_BASE);
+ apbmisc_base = ioremap(APBMISC_BASE, APBMISC_SIZE);
+ }
+
+ id = tegra_read_chipid();
+ tegra_chip_id = (id 8) 0xff;
So there's a fallback using APBMIS_BASE above if the node is missing, so
those last 2 lines always happen. However, if any of the following fail:
+ strapping_base = of_iomap(np, 0);
...
+ np = of_find_matching_node(NULL, tegra_fuse_match);
...
+ np = of_find_matching_node(NULL, car_match);
Then this doesn't get executed:
+ tegra_get_revision(id);
Isn't that important?
No. It's only used to populate /sys/devices/soc0/revision. I don't think
that's particularly important.
But it's a feature that works today. Why should we break it?
I guess that can't run if the lookup for tegra_fuse_match isn't
successful, since that tegra_get_revision may call
tegra20_spare_fuse_early() which uses fuse_base which is set up in
response to succesfully calling on of those node lookups. Isn't a
fallback needed there too?
tegra20_spare_fuse_early() will not be called if fuse_base is NULL.
Oh yes. We can at least call the function then even if the fuses can't
be mapped.
But to avoid regressions, we should simply make sure the fuses can be
mapped.
I'm also a bit concerned that the driver probes, rather than the early
function tegra_init_fuse(), are doing things like setting up the speedo
data initialization, randomness addition, etc. For one, those won't
happen any more unless the DT nodes are present, and secondly,
triggering all those from driver probe rather than a function that's
called from the machine descriptor makes guaranteeing the timing
problematic.
Today there are no users of the speedo ID in upstream.
Well, except people reading kernel log messages. Accurate speedo-related
log messages have help pin-point a problem or two in the past.
Looking at chromeos
the users of the speedo ID are: CPU dvfs, GPU dvfs and sdhci. The last 2 also
need regulators and therefor will need to support deferred probing anyway.
CPU dvfs isn't critical at all, we don't care when it gets initialized. So
deferred probe is fine.
What condition will those modules/drivers use to defer their probe?
sdhci needs this for faster modes I guess which will also need extra DT
properties looking at the chromeos driver. The others definitely will need
an updated DT. For randomness I haven't seen any appreciable difference in
when
the 'random: nonblocking pool is initialized' message appears between having
the randomness addition or not. Most likely the bulk of the randomness comes
from serial interrupts rather than the fuse data. So I don't think the move to
a driver probe will cause any problem. Nor do I think the lack of an updated
DT will cause problems.
But what advantage do we have by actively changing 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 v7 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 06/05/2014 04:13 PM, Peter De Schrijver wrote:
On Thu, Jun 05, 2014 at 08:41:55PM +0200, Stephen Warren wrote:
On 06/05/2014 07:09 AM, Peter De Schrijver wrote:
Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
diff --git a/arch/arm/boot/dts/tegra114.dtsi
b/arch/arm/boot/dts/tegra114.dtsi
+ apbmisc@7800 {
+ compatible = nvidia,tegra114-apbmisc,
nvidia,tegra20-apbmisc;
Is the Tegra114 APBMISC register layout 100% a backwards-compatible
superset of that in Tegra20? For both registers the code currently uses
*and* all possible registers the code could ever use? Since the APB MISC
is a bit of a dumping ground for random registers, that feels unlikely,
but perhaps it's possible.
For all I can see it is. At least for the registers the kernel is likely to
use.
But that's (At least for the registers the kernel is likely to use)
not how compatible values are defined. We need to explicitly look at all
the registers and actively decide that it really is compatible in order
to mark it so. If we don't want to do that, it's best just to use a
separate compatible value for each SoC, and add a couple more entries
into the match table.
--
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/4] of: Add NVIDIA Tegra XUSB pad controller binding
On 06/05/2014 04:08 PM, Thierry Reding wrote: On Thu, Jun 05, 2014 at 10:47:45AM -0600, Stephen Warren wrote: On 06/04/2014 09:16 AM, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com This patch adds the device tree binding documentation for the XUSB pad controller found on NVIDIA Tegra SoCs. It exposes both pinmuxing and PHY capabilities. diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-xusb-padctl.txt +- #phy-cells: Should be 1. The specifier is the index of the PHY to reference. + Possible values are: + - 0: PCIe + - 1: SATA Those values are defined in include/dt-bindings/pinctrl/pinctrl-tegra-xusb.h. I personally consider the dt-bindings/ header files to be part of the binding itself, rather than being derived from the binding. As such, I'd suggest the following changes: * Make this patch 1 not patch 2 * Move pinctrl-tegra-xusb.h into this patch. * Remove the list of values above, and replace it with the text See dt-bindings/pinctrl/pinctrl-tegra-xusb.h for the set of valid values. I remember discussions where people explicitly said that relying on the symbolic names in the DT bindings was a mistake because it would mean that everyone would need to have access to a mechanism similar to what we have in the Linux kernel (and that the header files would always need to be shipped with the DT bindings). The entire point of the headers is so that the binding definitions and code using them can't get out of sync. In my opinion, the headers *are* part of the bindings, so of course anyone either reading DT bindings, or parsing DT, must have the headers, and hence the headers must be bundled with the DT files or with the bindings wherever they go. signature.asc Description: OpenPGP digital signature
Re: [PATCH] ARM: fix debug prints relevant to PCI devices
On 06/04/2014 01:17 PM, Vidya Sagar wrote:
> As per PCIe spec, fast back-to-back transactions feature
> is not applicable to PCIe devices. Hence, do not print
> that fast back-to-back trasactions are disabled when
> there is a PCIe device found on the bus
> @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
> list_for_each_entry(dev, >devices, bus_list) {
> u16 status;
>
> + if (!has_pcie_dev)
> + has_pcie_dev = pci_pcie_cap(dev);
This sets the flag if any PCIe device is detected, even if regular PCI
devices are also detected. I assume the two can be mixed on a bus if
there's a bridge (although perhaps that would be separate buses, and
child buses don't get traversed by this function?)
> /*
>* Report what we did for this bus
> + * (only if the bus doesn't have even one PCIe device)
>*/
> - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n",
> - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis");
> + if (!has_pcie_dev)
> + printk(KERN_INFO "PCI: bus%d: Fast back to back transfers
> %sabled\n",
> + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en"
> : "dis");
So, this skips printing the message if any PCIe device was found.
Given that the message is relevant to PCI devices, more than being not
relevant to PCIe devices, perhaps the logic should be inverted, so that
the message is only printed if a PCI/non-PCIe device /is/ found, i.e.:
...
has_pci_device |= !pci_pcie_cap(dev);
...
if (has_pci_device)
printk(...);
--
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: fix debug prints relevant to PCI devices
On 06/04/2014 01:17 PM, Vidya Sagar wrote:
As per PCIe spec, fast back-to-back transactions feature
is not applicable to PCIe devices. Hence, do not print
that fast back-to-back trasactions are disabled when
there is a PCIe device found on the bus
@@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus)
list_for_each_entry(dev, bus-devices, bus_list) {
u16 status;
+ if (!has_pcie_dev)
+ has_pcie_dev = pci_pcie_cap(dev);
This sets the flag if any PCIe device is detected, even if regular PCI
devices are also detected. I assume the two can be mixed on a bus if
there's a bridge (although perhaps that would be separate buses, and
child buses don't get traversed by this function?)
/*
* Report what we did for this bus
+ * (only if the bus doesn't have even one PCIe device)
*/
- printk(KERN_INFO PCI: bus%d: Fast back to back transfers %sabled\n,
- bus-number, (features PCI_COMMAND_FAST_BACK) ? en : dis);
+ if (!has_pcie_dev)
+ printk(KERN_INFO PCI: bus%d: Fast back to back transfers
%sabled\n,
+ bus-number, (features PCI_COMMAND_FAST_BACK) ? en
: dis);
So, this skips printing the message if any PCIe device was found.
Given that the message is relevant to PCI devices, more than being not
relevant to PCIe devices, perhaps the logic should be inverted, so that
the message is only printed if a PCI/non-PCIe device /is/ found, i.e.:
...
has_pci_device |= !pci_pcie_cap(dev);
...
if (has_pci_device)
printk(...);
--
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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 07:28 PM, FanWu wrote:
> On 06/04/2014 12:49 AM, Stephen Warren wrote:
>> On 06/03/2014 01:37 AM, f...@marvell.com wrote:
>>> From: Fan Wu
>>>
>>> What the patch did:
>>> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in
>>> each time of
>>>calling pinctrl_select_state
>>> 2.Remove the HW disable operation in in pinmux_disable_setting function.
>>> 3.Remove the disable ops in struct pinmux_ops
>> ...
>>> Signed-off-by: Fan Wu
>>> Signed-off-by: Stephen Warren
>>
>> As I mentioned in my previous email, I didn't sign this off. I made some
>> suggestions for a better alternative in that email.
>>
>> If I *had* written that s-o-b, then it should be before yours in the
>> patch description since you handled the patch last.
>>
>
> The Signed-off didn't bother me.
> I will Choose your option 2# and thanks for your suggestion about this :)
>
>
>>> diff --git a/include/linux/pinctrl/pinmux.h
>>> b/include/linux/pinctrl/pinmux.h
>>
>>> @@ -70,8 +70,6 @@ struct pinmux_ops {
>>> unsigned * const num_groups);
>>> int (*enable) (struct pinctrl_dev *pctldev, unsigned
>>> func_selector,
>>> unsigned group_selector);
>>> -void (*disable) (struct pinctrl_dev *pctldev, unsigned
>>> func_selector,
>>> - unsigned group_selector);
>>
>> This will cause a compile failure, since many drivers still set the
>> .disable function pointer. You need to update all the driver files to
>> remove those functions too. There's quite a bit of code in some of those
>> functions, so you'd need the relevant driver maintainers to confirm it's
>> OK to remove it. I think only the owners of pinctrl-egra and
>> pinctrl-single have ack'd this concept so far.
>>
>
> For this part, I think I mentioned this before, simply removing disable
> ops will introduce the compiling error.
> I think there are several ways to handle this:
> 1. Don't remove the disable ops in struct pinmux_ops in this patch but
> to remove the disable ops in struct pinmux_ops after the another patch
> is merged, which is used to remove all of the disable ops user in all
> drivers.
> 2. Just remove the disable ops in pinmux_ops in this patch, and make a
> another patch ASAP to remove all the disable ops user in all drivers.
> 3. Remove the disable ops in struct pinmux_ops and remove all the
> disable ops user in all drivers, all in this patch.
>
>
> For the solution 2, I just think it may be not a good way to include so
> much content in a patch, which are not in a same code level.
>
> I am just inclined to use solution 1# or 3#.
I would expect option 3.
--
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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 01:37 AM, f...@marvell.com wrote:
> From: Fan Wu
>
> What the patch did:
> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time
> of
> calling pinctrl_select_state
> 2.Remove the HW disable operation in in pinmux_disable_setting function.
> 3.Remove the disable ops in struct pinmux_ops
...
> Signed-off-by: Fan Wu
> Signed-off-by: Stephen Warren
As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.
If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.
> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
> @@ -70,8 +70,6 @@ struct pinmux_ops {
> unsigned * const num_groups);
> int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
> unsigned group_selector);
> - void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
> - unsigned group_selector);
This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.
--
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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 01:48 AM, FanWu wrote: > On 06/03/2014 03:37 PM, f...@marvell.com wrote: >> From: Fan Wu >> >> What the patch did: >> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in >> each time of >>calling pinctrl_select_state >> 2.Remove the HW disable operation in in pinmux_disable_setting function. >> 3.Remove the disable ops in struct pinmux_ops ... > For the "signed-off" part, I added the "Stephen" because I change the > code inline comments according to the suggestion from Stephen. The point is that you can NEVER write S-o-b for someone else, only for yourself. Given the small contribution from me in this patch, the following would be better ways to credit me: 1) Not bother since it's a tiny comment 2) In patch description: (includes comment fixes from Stephen Warren) 3) In patch description final paragraph, before your S-o-b: Based-on-work-by: Stephen Warren (although (3) isn't really appropriate here, since my contribution was tiny compared to the overall patch, and came after the original patch rather than being inspiration for 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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 01:48 AM, FanWu wrote: On 06/03/2014 03:37 PM, f...@marvell.com wrote: From: Fan Wu f...@marvell.com What the patch did: 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of calling pinctrl_select_state 2.Remove the HW disable operation in in pinmux_disable_setting function. 3.Remove the disable ops in struct pinmux_ops ... For the signed-off part, I added the Stephen because I change the code inline comments according to the suggestion from Stephen. The point is that you can NEVER write S-o-b for someone else, only for yourself. Given the small contribution from me in this patch, the following would be better ways to credit me: 1) Not bother since it's a tiny comment 2) In patch description: (includes comment fixes from Stephen Warren) 3) In patch description final paragraph, before your S-o-b: Based-on-work-by: Stephen Warren swar...@nvidia.com (although (3) isn't really appropriate here, since my contribution was tiny compared to the overall patch, and came after the original patch rather than being inspiration for 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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 01:37 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time
of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
3.Remove the disable ops in struct pinmux_ops
...
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.
If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
@@ -70,8 +70,6 @@ struct pinmux_ops {
unsigned * const num_groups);
int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
unsigned group_selector);
- void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
- unsigned group_selector);
This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.
--
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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 07:28 PM, FanWu wrote:
On 06/04/2014 12:49 AM, Stephen Warren wrote:
On 06/03/2014 01:37 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in
each time of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
3.Remove the disable ops in struct pinmux_ops
...
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.
If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.
The Signed-off didn't bother me.
I will Choose your option 2# and thanks for your suggestion about this :)
diff --git a/include/linux/pinctrl/pinmux.h
b/include/linux/pinctrl/pinmux.h
@@ -70,8 +70,6 @@ struct pinmux_ops {
unsigned * const num_groups);
int (*enable) (struct pinctrl_dev *pctldev, unsigned
func_selector,
unsigned group_selector);
-void (*disable) (struct pinctrl_dev *pctldev, unsigned
func_selector,
- unsigned group_selector);
This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.
For this part, I think I mentioned this before, simply removing disable
ops will introduce the compiling error.
I think there are several ways to handle this:
1. Don't remove the disable ops in struct pinmux_ops in this patch but
to remove the disable ops in struct pinmux_ops after the another patch
is merged, which is used to remove all of the disable ops user in all
drivers.
2. Just remove the disable ops in pinmux_ops in this patch, and make a
another patch ASAP to remove all the disable ops user in all drivers.
3. Remove the disable ops in struct pinmux_ops and remove all the
disable ops user in all drivers, all in this patch.
For the solution 2, I just think it may be not a good way to include so
much content in a patch, which are not in a same code level.
I am just inclined to use solution 1# or 3#.
I would expect option 3.
--
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] arm: tegra: initial support for apalis t30
On 06/02/2014 02:18 PM, Marcel Ziswiler wrote:
> On 06/02/2014 06:26 PM, Stephen Warren wrote:
>>> +pwmleds {
>>> +compatible = "pwm-leds";
>>> +
>>> +pwm3 {
>>> +label = "PWM3";
>>> +pwms = < 1 19600>;
>>> +max-brightness = <255>;
>>> +};
>>> +pwm2 {
>>> +label = "PWM2";
>>> +pwms = < 2 19600>;
>>> +max-brightness = <255>;
>>> +};
>>> +pwm1 {
>>> +label = "PWM1";
>>> +pwms = < 3 19600>;
>>> +max-brightness = <255>;
>>> +};
>>
>> Nit: Why not sort those nodes in numerical order?
>
> Sure, the only question is ordering based on what. I choose the actual
> Tegra PWM instance while you propose to use our Apalis instance
> numbering which in this particular case turns out to be the opposite but
> makes us even more happy to comply.
Sorting by reg value, or if there is no reg value then alphanumerically,
is what I've tried to (remember to) require for the Tegra DTs.
--
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: multi_v7: enable igb, stmpe, spidev, lm95245, pwm leds
On 06/02/2014 11:01 AM, Olof Johansson wrote: > On Mon, Jun 2, 2014 at 2:43 AM, 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, >> SPI bus 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. >> >> While at it move the NEON entry down to its proper place to have it all >> nicely ordered again. >> >> Signed-off-by: Marcel Ziswiler >> --- >> BTW: How about TEGRA_EMC_SCALING_ENABLE, MACH_UX500_DT, OMAP_USB3 and >> MMC_SUNXI which I haven't found any mentioning anywhere, MTD_M25P80 >> which depends on MTD_SPI_NOR or DOVE_THERMAL which depends on the >> legacy ARCH_DOVE (besides USE_OF of course) being not much of multi >> compatible? > > Can you resend towards the end of the merge window, please? We should > make a pass and update anything new that has been merged for 3.16 > around then. I think the content in this patch is for 3.17. The issues mentioned below --- would be worth fixing for 3.16. -- 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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 06/02/2014 04:06 AM, Viresh Kumar wrote: > On 30 May 2014 21:56, Stephen Warren wrote: >> ... [This patch causes issues on Tegra20] ... >> I believe the issue is this: ... > Okay, that was very helpful.. > > What about this ? (Attached for testing) : > > Author: Viresh Kumar > Date: Fri May 16 14:22:40 2014 +0530 > > cpufreq: Tegra: implement intermediate frequency callbacks > > Tegra had always been switching to intermediate frequency (pll_p_clk) > since > ever. CPUFreq core has better support for handling notifications for these > frequencies and so we can adapt Tegra's driver to it. > > Also do a WARN() if clk_set_parent() fails while moving back to pll_x as > we > should have atleast restored to earlier frequency on error. Tested-by: Stephen Warren I'd prefer a couple of changes though: a) Rename "pll_p_clk_count" to better describe what it represents. It represents the fact that pll_x has been prepare_enabled, so why not call it "pll_x_prepared"? b) I think it should be a Boolean not an integer; there should never be a case where the value is not 0 or 1. The only way that could happen is if the cpufreq core called tegra_target_intermediate() out of sequence too many times. -- 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] arm: tegra: initial support for apalis t30
On 06/01/2014 05:37 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 as well as the previously missing one for the recently added
> Colibri T30.
> diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts
> b/arch/arm/boot/dts/tegra30-apalis-eval.dts
> + /* SPI1: Apalis SPI1 */
> + spi@7000d400 {
> + status = "okay";
> + spi-max-frequency = <2500>;
> + spidev0: spidev@1 {
> + compatible = "spidev";
> + reg = <1>;
> + spi-max-frequency = <2500>;
> + };
> + };
I vaguely recall people speaking out against including "spidev" devices
in DT because they don't represent actual HW, but rather a way to
request that the SPI bus be exposed to user-space, which is a pure SW
issue. Wouldn't it be better if the spidev interface worked like
I2C_CHARDEV, where fake devices weren't actually required?
--
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] arm: tegra: initial support for apalis t30
On 06/01/2014 05:37 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 as well as the previously missing one for the recently added
> Colibri T30.
> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
> b/Documentation/devicetree/bindings/arm/tegra.txt
> + toradex,colibri_t30
> + toradex,colibri_t30-eval-v3
Those don't seem to be related to Apalis support.
> diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts
> b/arch/arm/boot/dts/tegra30-apalis-eval.dts
> + host1x@5000 {
> + dc@5420 {
> + rgb {
> + status = "okay";
> + nvidia,panel = <>;
> + };
> + };
> + hdmi@5428 {
Nit: Add a blank line between the nodes. Check elsewhere too.
> + serial@70006040 {
> + compatible = "nvidia,tegra30-hsuart";
> + status = "okay";
> + };
Nit: Put the status property first followed by new/overridden
properties, to be consistent with other Tegra DTs. Check elsewhere too.
> + /* SPI1: Apalis SPI1 */
> + spi@7000d400 {
> + status = "okay";
> + spi-max-frequency = <2500>;
> + spidev0: spidev@1 {
Nit: Add a blank line between properties and nodes. Check elsewhere too.
> + sd1: sdhci@7800 {
...
> + mmc1: sdhci@78000400 {
Do those nodes really need labels? Nothing appears to reference them,
and I can't see why anything would.
Should the mmc1 node be non-removable? It seems a bit odd for a
removable device to have an 8-bit data bus.
> + backlight: backlight {
> + compatible = "pwm-backlight";
> +
> + /* PWM0 */
Nit: No need for a blank line between a bunch of related properties.
Check elsewhere too.
> + pwmleds {
> + compatible = "pwm-leds";
> +
> + pwm3 {
> + label = "PWM3";
> + pwms = < 1 19600>;
> + max-brightness = <255>;
> + };
> + pwm2 {
> + label = "PWM2";
> + pwms = < 2 19600>;
> + max-brightness = <255>;
> + };
> + pwm1 {
> + label = "PWM1";
> + pwms = < 3 19600>;
> + max-brightness = <255>;
> + };
Nit: Why not sort those nodes in numerical order?
> + regulators {
> + sys_5v0_reg: regulator@1 {
Nit: Why not start the numbering at 0?
> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi
> b/arch/arm/boot/dts/tegra30-apalis.dtsi
> + pinmux@7868 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_default>;
> +
> + state_default: pinmux {
It might make sense to add all the pinmux data to
https://github.com/NVIDIA/tegra-pinmux-scripts, so that both the kernel
and U-Boot pinmux initialization tables can be auto-generated from a
single data-structure. I think that'll get a small amount of
error-/consistency-checking of the data too.
--
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/3] arm: tegra: enable igb, stmpe, i2c chardev, spidev, lm95245, pwm leds
On 06/01/2014 05:37 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 > as well as SPI buses and PWM LEDs generically accessible from user > space and an LM95245 temperature sensor chip. The later five can also > be found on the Colibri T30 module. > > While at it move the PCA953x GPIO entry down to its proper place to > have it all nicely ordered again. > > Signed-off-by: Marcel Ziswiler > --- > BTW: How about MTD_SPI_NOR, That might only exist in linux-next. > PROC_DEVICETREE and CRYPTO_DEV_TEGRA_AES > which I haven't found any mentioning anywhere? The TEGRA_AES driver has been removed, so the option should be removed from defconfig too. I don't know what happened to PROC_DEVICTREE - it doesn't seem to exist any more. Was it replaced by something else or deleted? Feel free to send patches for those. > diff --git a/arch/arm/configs/tegra_defconfig > b/arch/arm/configs/tegra_defconfig > +CONFIG_SPI_SPIDEV=y Is this useful with DT? I thought that unlike I2C_CHARDEV, spidev needed dummy devices to exist in DT for spidev to work? If so, there's not much point adding the option to defconfig, since people can add it when they put the dummy devices into 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 1/3] arm: multi_v7: enable igb, stmpe, spidev, lm95245, pwm leds
On 06/02/2014 03:43 AM, 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, > SPI bus 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. > > While at it move the NEON entry down to its proper place to have it all > nicely ordered again. > > Signed-off-by: Marcel Ziswiler > --- > BTW: How about TEGRA_EMC_SCALING_ENABLE I thought a patch was sent to remove that. Perhaps the patch was only sent to tegra_defconfig. > ... MTD_M25P80 which depends on MTD_SPI_NOR I've seen patches to fix that too. -- 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: multi_v7: enable igb, stmpe, spidev, lm95245, pwm leds
On 06/02/2014 03:43 AM, 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, SPI bus 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. While at it move the NEON entry down to its proper place to have it all nicely ordered again. Signed-off-by: Marcel Ziswiler mar...@ziswiler.com --- BTW: How about TEGRA_EMC_SCALING_ENABLE I thought a patch was sent to remove that. Perhaps the patch was only sent to tegra_defconfig. ... MTD_M25P80 which depends on MTD_SPI_NOR I've seen patches to fix that too. -- 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/3] arm: tegra: enable igb, stmpe, i2c chardev, spidev, lm95245, pwm leds
On 06/01/2014 05:37 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 as well as SPI buses and PWM LEDs generically accessible from user space and an LM95245 temperature sensor chip. The later five can also be found on the Colibri T30 module. While at it move the PCA953x GPIO entry down to its proper place to have it all nicely ordered again. Signed-off-by: Marcel Ziswiler mar...@ziswiler.com --- BTW: How about MTD_SPI_NOR, That might only exist in linux-next. PROC_DEVICETREE and CRYPTO_DEV_TEGRA_AES which I haven't found any mentioning anywhere? The TEGRA_AES driver has been removed, so the option should be removed from defconfig too. I don't know what happened to PROC_DEVICTREE - it doesn't seem to exist any more. Was it replaced by something else or deleted? Feel free to send patches for those. diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig +CONFIG_SPI_SPIDEV=y Is this useful with DT? I thought that unlike I2C_CHARDEV, spidev needed dummy devices to exist in DT for spidev to work? If so, there's not much point adding the option to defconfig, since people can add it when they put the dummy devices into 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 3/3] arm: tegra: initial support for apalis t30
On 06/01/2014 05:37 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 as well as the previously missing one for the recently added
Colibri T30.
diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
b/Documentation/devicetree/bindings/arm/tegra.txt
+ toradex,colibri_t30
+ toradex,colibri_t30-eval-v3
Those don't seem to be related to Apalis support.
diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts
b/arch/arm/boot/dts/tegra30-apalis-eval.dts
+ host1x@5000 {
+ dc@5420 {
+ rgb {
+ status = okay;
+ nvidia,panel = panel;
+ };
+ };
+ hdmi@5428 {
Nit: Add a blank line between the nodes. Check elsewhere too.
+ serial@70006040 {
+ compatible = nvidia,tegra30-hsuart;
+ status = okay;
+ };
Nit: Put the status property first followed by new/overridden
properties, to be consistent with other Tegra DTs. Check elsewhere too.
+ /* SPI1: Apalis SPI1 */
+ spi@7000d400 {
+ status = okay;
+ spi-max-frequency = 2500;
+ spidev0: spidev@1 {
Nit: Add a blank line between properties and nodes. Check elsewhere too.
+ sd1: sdhci@7800 {
...
+ mmc1: sdhci@78000400 {
Do those nodes really need labels? Nothing appears to reference them,
and I can't see why anything would.
Should the mmc1 node be non-removable? It seems a bit odd for a
removable device to have an 8-bit data bus.
+ backlight: backlight {
+ compatible = pwm-backlight;
+
+ /* PWM0 */
Nit: No need for a blank line between a bunch of related properties.
Check elsewhere too.
+ pwmleds {
+ compatible = pwm-leds;
+
+ pwm3 {
+ label = PWM3;
+ pwms = pwm 1 19600;
+ max-brightness = 255;
+ };
+ pwm2 {
+ label = PWM2;
+ pwms = pwm 2 19600;
+ max-brightness = 255;
+ };
+ pwm1 {
+ label = PWM1;
+ pwms = pwm 3 19600;
+ max-brightness = 255;
+ };
Nit: Why not sort those nodes in numerical order?
+ regulators {
+ sys_5v0_reg: regulator@1 {
Nit: Why not start the numbering at 0?
diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi
b/arch/arm/boot/dts/tegra30-apalis.dtsi
+ pinmux@7868 {
+ pinctrl-names = default;
+ pinctrl-0 = state_default;
+
+ state_default: pinmux {
It might make sense to add all the pinmux data to
https://github.com/NVIDIA/tegra-pinmux-scripts, so that both the kernel
and U-Boot pinmux initialization tables can be auto-generated from a
single data-structure. I think that'll get a small amount of
error-/consistency-checking of the data too.
--
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] arm: tegra: initial support for apalis t30
On 06/01/2014 05:37 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 as well as the previously missing one for the recently added
Colibri T30.
diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts
b/arch/arm/boot/dts/tegra30-apalis-eval.dts
+ /* SPI1: Apalis SPI1 */
+ spi@7000d400 {
+ status = okay;
+ spi-max-frequency = 2500;
+ spidev0: spidev@1 {
+ compatible = spidev;
+ reg = 1;
+ spi-max-frequency = 2500;
+ };
+ };
I vaguely recall people speaking out against including spidev devices
in DT because they don't represent actual HW, but rather a way to
request that the SPI bus be exposed to user-space, which is a pure SW
issue. Wouldn't it be better if the spidev interface worked like
I2C_CHARDEV, where fake devices weren't actually required?
--
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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 06/02/2014 04:06 AM, Viresh Kumar wrote: On 30 May 2014 21:56, Stephen Warren swar...@wwwdotorg.org wrote: ... [This patch causes issues on Tegra20] ... I believe the issue is this: ... Okay, that was very helpful.. What about this ? (Attached for testing) : Author: Viresh Kumar viresh.ku...@linaro.org Date: Fri May 16 14:22:40 2014 +0530 cpufreq: Tegra: implement intermediate frequency callbacks Tegra had always been switching to intermediate frequency (pll_p_clk) since ever. CPUFreq core has better support for handling notifications for these frequencies and so we can adapt Tegra's driver to it. Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we should have atleast restored to earlier frequency on error. Tested-by: Stephen Warren swar...@nvidia.com I'd prefer a couple of changes though: a) Rename pll_p_clk_count to better describe what it represents. It represents the fact that pll_x has been prepare_enabled, so why not call it pll_x_prepared? b) I think it should be a Boolean not an integer; there should never be a case where the value is not 0 or 1. The only way that could happen is if the cpufreq core called tegra_target_intermediate() out of sequence too many times. -- 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: multi_v7: enable igb, stmpe, spidev, lm95245, pwm leds
On 06/02/2014 11:01 AM, Olof Johansson wrote: On Mon, Jun 2, 2014 at 2:43 AM, Marcel Ziswiler mar...@ziswiler.com wrote: The NVIDIA Tegra 3 based Apalis T30 module contains an Intel i210 resp. i211 gigabit Ethernet controller, an STMPE811 ADC/touch controller, SPI bus 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. While at it move the NEON entry down to its proper place to have it all nicely ordered again. Signed-off-by: Marcel Ziswiler mar...@ziswiler.com --- BTW: How about TEGRA_EMC_SCALING_ENABLE, MACH_UX500_DT, OMAP_USB3 and MMC_SUNXI which I haven't found any mentioning anywhere, MTD_M25P80 which depends on MTD_SPI_NOR or DOVE_THERMAL which depends on the legacy ARCH_DOVE (besides USE_OF of course) being not much of multi compatible? Can you resend towards the end of the merge window, please? We should make a pass and update anything new that has been merged for 3.16 around then. I think the content in this patch is for 3.17. The issues mentioned below --- would be worth fixing for 3.16. -- 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] arm: tegra: initial support for apalis t30
On 06/02/2014 02:18 PM, Marcel Ziswiler wrote:
On 06/02/2014 06:26 PM, Stephen Warren wrote:
+pwmleds {
+compatible = pwm-leds;
+
+pwm3 {
+label = PWM3;
+pwms = pwm 1 19600;
+max-brightness = 255;
+};
+pwm2 {
+label = PWM2;
+pwms = pwm 2 19600;
+max-brightness = 255;
+};
+pwm1 {
+label = PWM1;
+pwms = pwm 3 19600;
+max-brightness = 255;
+};
Nit: Why not sort those nodes in numerical order?
Sure, the only question is ordering based on what. I choose the actual
Tegra PWM instance while you propose to use our Apalis instance
numbering which in this particular case turns out to be the opposite but
makes us even more happy to comply.
Sorting by reg value, or if there is no reg value then alphanumerically,
is what I've tried to (remember to) require for the Tegra DTs.
--
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] mm, compaction: properly signal and act upon lock and need_sched() contention
On 05/23/2014 02:34 AM, Vlastimil Babka wrote: > On 05/23/2014 04:48 AM, Shawn Guo wrote: >> On 23 May 2014 07:49, Kevin Hilman wrote: >>> On Fri, May 16, 2014 at 2:47 AM, Vlastimil Babka wrote: >>>> Compaction uses compact_checklock_irqsave() function to periodically check >>>> for >>>> lock contention and need_resched() to either abort async compaction, or to >>>> free the lock, schedule and retake the lock. When aborting, cc->contended >>>> is >>>> set to signal the contended state to the caller. Two problems have been >>>> identified in this mechanism. >>> >>> This patch (or later version) has hit next-20140522 (in the form >>> commit 645ceea9331bfd851bc21eea456dda27862a10f4) and according to my >>> bisect, appears to be the culprit of several boot failures on ARM >>> platforms. >> >> On i.MX6 where CMA is enabled, the commit causes the drivers calling >> dma_alloc_coherent() fail to probe. Tracing it a little bit, it seems >> dma_alloc_from_contiguous() always return page as NULL after this >> commit. >> >> Shawn >> > > Really sorry, guys :/ > > -8<- > From: Vlastimil Babka > Date: Fri, 23 May 2014 10:18:56 +0200 > Subject: > mm-compaction-properly-signal-and-act-upon-lock-and-need_sched-contention-fix2 > > Step 1: Change function name and comment between v1 and v2 so that the return > value signals the opposite thing. > Step 2: Change the call sites to reflect the opposite return value. > Step 3: ??? > Step 4: Make a complete fool of yourself. Tested-by: Stephen Warren This fix doesn't seem to be in linux-next 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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/29/2014 07:56 PM, Viresh Kumar wrote: > On 29 May 2014 23:10, Stephen Warren wrote: >> This patch breaks Tegra. The reason is below. ... >>> +disable_pll_x: >>> + clk_disable_unprepare(pll_x_clk); >> >> ... so this turns off pll_x even though we're running from it. > > Can you describe the role of the enable/disable of this pll_x_clk please? > Which all clocks depend on it, etc? So that I understand why its important > to enable it and for which clocks. And also if we need to disable it after > changing to any freq.. I believe the issue is this: We can't change the rate of pll_x while it's being used as a source for something. I'm not 100% sure why. I assume the PLL output simply isn't stable enough while changing rates; perhaps it can go out-of-range, or generate glitches. This means we must switch the CPU clock source to something else (we use pll_p) while changing the pll_x rate. Since the CPU is the only thing that uses pll_x, if we switch the CPU to some other clock source, pll_x will become unused, so it will be automatically disabled. Disabling the PLL, and then re-enabling it later when switching the CPU back to it, presumably takes some extra time (e.g. waiting for PLL lock when it starts back up), so the code takes an extra reference to the clock (prepare_enable) before switching the CPU away from it, and drops that (disable_unprepare) after switching the CPU back to it. The only case pll_x is disabled is when we use a cpufreq of 216MHz; that frequency is provided by pll_p itself, so we never switch back to pll_x in this case, and do want to shut down pll_x to save some power. Now, in your patch when switching from 216MHz to pll_x, the initial prepare_enable(pll_x) never happens, then the CPU is switched back to pll_x which turns it on, then the unpaired disable_unprepare(pll_x) happens which turns off pll_x even though the CPU is using it as clock source. Arguably the clock API has a bug in that it shouldn't allow these unpaired calls to break the reference counting, but that's the way the API is right now. -- 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 v5 0/5] efuse driver for Tegra
On 05/30/2014 02:23 AM, Peter De Schrijver wrote: > On Thu, May 29, 2014 at 09:01:27PM +0200, Stephen Warren wrote: >> On 05/28/2014 06:54 AM, Peter De Schrijver wrote: >>> This driver allows userspace to read the raw efuse data. Its userspace >>> interface is modelled after the sunxi_sid driver which provides similar >>> functionality for some Allwinner SoCs. It has been tested on >>> Tegra20 (ventana), Tegra30 (beaverboard), Tegra114 (dalmore) and >>> Tegra124 (jetson TK1). >> >>> Changes since v4: >>> >>> * Provide fallback to hardcoded 0x7800 in case the apbmisc DT node is >>> missing. This is exactly what the current code does and prevents a system >>> crash in that case due to an invalid memory access by tegra_read_chipid() >> >> Wouldn't it be better to simply return an error? > > This would mean you can't boot a system with these patches applied unless you > also update the device tree. The system would crash during boot because CCF > relies on tegra_read_chipid() as an APB barrier. Also tegra_boot_secondary() > relies on the chipid to select the correct method for booting secondary cores. Is this series really backwards-compatible anyway? tegra_init_fuse() contains a whole bunch of places where resources are pulled out of DT, and only one of those has a fallback to use APBMISC_BASE if the DT entry isn't present. -- 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 v5 3/5] misc: fuse: Add efuse driver for Tegra
On 05/30/2014 05:36 AM, Peter De Schrijver wrote: > On Thu, May 29, 2014 at 09:04:33PM +0200, Stephen Warren wrote: >> On 05/28/2014 06:54 AM, Peter De Schrijver wrote: >>> Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. >> >>> diff --git a/Documentation/ABI/testing/sysfs-driver-tegra-fuse >>> b/Documentation/ABI/testing/sysfs-driver-tegra-fuse >> >>> +Description: read-only access to the efuses on Tegra20, Tegra30, >>> Tegra114 >>> + and Tegra124 SoC's from NVIDIA. The efuses contain write once >>> + data programmed at the factory. The data is layed out in 32bit >>> + words in LSB first formnat. The number of valid bits depends >> >> s/formnat/format/ >> >>> + on the word and the SoC. The mapping is as follows: >>> + >>> + For Tegra20: >>> + Word 0 - 1: bit 0 >>> + Word 2: unused >>> + Word 3: bits 0 - 31 >>> + Word 4: bits 0 - 7 >> >> Do we really need these long tables that indicate which bits are used? >> As I mentioned before, when I asked for documentation of the format of >> these files, all I wanted was a brief not indicating that the data was >> binary, and that each bit potentially represents a fuse... Either we >> should leave it at that, or actually document what each bit represents, >> which would hopefully be a pointless duplication of the TRM. > > Some fuses are OEM defined, so there is no way to document all fuses there. > Would you be ok with just dropping the tables then? Yes. > So, the description would become: > > Description:read-only access to the efuses on Tegra20, Tegra30, Tegra114 > and Tegra124 SoC's from NVIDIA. The efuses contain write once > data programmed at the factory. The data is layed out in 32bit > words in LSB first format. The number of valid bits depends > on the word and the SoC. Almost. That's still missing the key information that the data format is one bit per fuse, and the ordering. Perhaps change from: The data is layed out in 32bit words in LSB first format. to: The data is laid out in 32bit words in LSB first format. Each bit represents a single fuse value. Bits order/assignment exactly matches the HW registers, including any unused bits. -- 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 v5 3/5] misc: fuse: Add efuse driver for Tegra
On 05/30/2014 05:36 AM, Peter De Schrijver wrote: On Thu, May 29, 2014 at 09:04:33PM +0200, Stephen Warren wrote: On 05/28/2014 06:54 AM, Peter De Schrijver wrote: Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. diff --git a/Documentation/ABI/testing/sysfs-driver-tegra-fuse b/Documentation/ABI/testing/sysfs-driver-tegra-fuse +Description: read-only access to the efuses on Tegra20, Tegra30, Tegra114 + and Tegra124 SoC's from NVIDIA. The efuses contain write once + data programmed at the factory. The data is layed out in 32bit + words in LSB first formnat. The number of valid bits depends s/formnat/format/ + on the word and the SoC. The mapping is as follows: + + For Tegra20: + Word 0 - 1: bit 0 + Word 2: unused + Word 3: bits 0 - 31 + Word 4: bits 0 - 7 Do we really need these long tables that indicate which bits are used? As I mentioned before, when I asked for documentation of the format of these files, all I wanted was a brief not indicating that the data was binary, and that each bit potentially represents a fuse... Either we should leave it at that, or actually document what each bit represents, which would hopefully be a pointless duplication of the TRM. Some fuses are OEM defined, so there is no way to document all fuses there. Would you be ok with just dropping the tables then? Yes. So, the description would become: Description:read-only access to the efuses on Tegra20, Tegra30, Tegra114 and Tegra124 SoC's from NVIDIA. The efuses contain write once data programmed at the factory. The data is layed out in 32bit words in LSB first format. The number of valid bits depends on the word and the SoC. Almost. That's still missing the key information that the data format is one bit per fuse, and the ordering. Perhaps change from: The data is layed out in 32bit words in LSB first format. to: The data is laid out in 32bit words in LSB first format. Each bit represents a single fuse value. Bits order/assignment exactly matches the HW registers, including any unused bits. -- 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 v5 0/5] efuse driver for Tegra
On 05/30/2014 02:23 AM, Peter De Schrijver wrote: On Thu, May 29, 2014 at 09:01:27PM +0200, Stephen Warren wrote: On 05/28/2014 06:54 AM, Peter De Schrijver wrote: This driver allows userspace to read the raw efuse data. Its userspace interface is modelled after the sunxi_sid driver which provides similar functionality for some Allwinner SoCs. It has been tested on Tegra20 (ventana), Tegra30 (beaverboard), Tegra114 (dalmore) and Tegra124 (jetson TK1). Changes since v4: * Provide fallback to hardcoded 0x7800 in case the apbmisc DT node is missing. This is exactly what the current code does and prevents a system crash in that case due to an invalid memory access by tegra_read_chipid() Wouldn't it be better to simply return an error? This would mean you can't boot a system with these patches applied unless you also update the device tree. The system would crash during boot because CCF relies on tegra_read_chipid() as an APB barrier. Also tegra_boot_secondary() relies on the chipid to select the correct method for booting secondary cores. Is this series really backwards-compatible anyway? tegra_init_fuse() contains a whole bunch of places where resources are pulled out of DT, and only one of those has a fallback to use APBMISC_BASE if the DT entry isn't present. -- 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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/29/2014 07:56 PM, Viresh Kumar wrote: On 29 May 2014 23:10, Stephen Warren swar...@wwwdotorg.org wrote: This patch breaks Tegra. The reason is below. ... +disable_pll_x: + clk_disable_unprepare(pll_x_clk); ... so this turns off pll_x even though we're running from it. Can you describe the role of the enable/disable of this pll_x_clk please? Which all clocks depend on it, etc? So that I understand why its important to enable it and for which clocks. And also if we need to disable it after changing to any freq.. I believe the issue is this: We can't change the rate of pll_x while it's being used as a source for something. I'm not 100% sure why. I assume the PLL output simply isn't stable enough while changing rates; perhaps it can go out-of-range, or generate glitches. This means we must switch the CPU clock source to something else (we use pll_p) while changing the pll_x rate. Since the CPU is the only thing that uses pll_x, if we switch the CPU to some other clock source, pll_x will become unused, so it will be automatically disabled. Disabling the PLL, and then re-enabling it later when switching the CPU back to it, presumably takes some extra time (e.g. waiting for PLL lock when it starts back up), so the code takes an extra reference to the clock (prepare_enable) before switching the CPU away from it, and drops that (disable_unprepare) after switching the CPU back to it. The only case pll_x is disabled is when we use a cpufreq of 216MHz; that frequency is provided by pll_p itself, so we never switch back to pll_x in this case, and do want to shut down pll_x to save some power. Now, in your patch when switching from 216MHz to pll_x, the initial prepare_enable(pll_x) never happens, then the CPU is switched back to pll_x which turns it on, then the unpaired disable_unprepare(pll_x) happens which turns off pll_x even though the CPU is using it as clock source. Arguably the clock API has a bug in that it shouldn't allow these unpaired calls to break the reference counting, but that's the way the API is right now. -- 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] mm, compaction: properly signal and act upon lock and need_sched() contention
On 05/23/2014 02:34 AM, Vlastimil Babka wrote: On 05/23/2014 04:48 AM, Shawn Guo wrote: On 23 May 2014 07:49, Kevin Hilman khil...@linaro.org wrote: On Fri, May 16, 2014 at 2:47 AM, Vlastimil Babka vba...@suse.cz wrote: Compaction uses compact_checklock_irqsave() function to periodically check for lock contention and need_resched() to either abort async compaction, or to free the lock, schedule and retake the lock. When aborting, cc-contended is set to signal the contended state to the caller. Two problems have been identified in this mechanism. This patch (or later version) has hit next-20140522 (in the form commit 645ceea9331bfd851bc21eea456dda27862a10f4) and according to my bisect, appears to be the culprit of several boot failures on ARM platforms. On i.MX6 where CMA is enabled, the commit causes the drivers calling dma_alloc_coherent() fail to probe. Tracing it a little bit, it seems dma_alloc_from_contiguous() always return page as NULL after this commit. Shawn Really sorry, guys :/ -8- From: Vlastimil Babka vba...@suse.cz Date: Fri, 23 May 2014 10:18:56 +0200 Subject: mm-compaction-properly-signal-and-act-upon-lock-and-need_sched-contention-fix2 Step 1: Change function name and comment between v1 and v2 so that the return value signals the opposite thing. Step 2: Change the call sites to reflect the opposite return value. Step 3: ??? Step 4: Make a complete fool of yourself. Tested-by: Stephen Warren swar...@nvidia.com This fix doesn't seem to be in linux-next 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 v5 0/5] efuse driver for Tegra
On 05/28/2014 07:16 AM, Andrew Morton wrote: > On Wed, 28 May 2014 15:54:32 +0300 Peter De Schrijver > wrote: > >> This driver allows userspace to read the raw efuse data. > > The patchset uses an inexplicable mixture of "fuse" and "efuse". > Given that the kernel already has a "fuse", it would be nice if > this code could use "efuse" everywhere. I didn't check every instance, but a mix of those two terms seems reasonable. "fuse" is the type of HW object; it's a generic HW term that exists outside the scope of this patch. While the conflict with Linux's filesystem is unfortunate, it's a name collision that I think we'll simply have to live with; it's not created by this patch. "efuse" is the name of the Tegra module that hosts Tegra's fuses. -- 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] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/25/2014 08:43 PM, f...@marvell.com wrote: > From: Fan Wu > > What the patch did: > 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time > of > calling pinctrl_select_state > 2.Remove the HW disable operation in in pinmux_disable_setting function. ... This commit description is way too long for such a simple change. A much shorter summary would be better. > Signed-off-by: Fan Wu > Signed-off-by: Stephen Warren I'm pretty sure I never signed off on this patch. How come my s-o-b line is there? This patch still doesn't remove ops->disable from the struct pinmux_ops, nor any of its implementations. Shouldn't 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 v5 5/5] ARM: tegra: build new fuse driver in drivers/misc
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: > The Tegra fuse code has moved to drivers/misc/fuse/tegra. Enable the > building of that code, and disable the building of the old fuse code in > arch/arm/mach-tegra/. > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 7eb4b69..994d02d 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -55,3 +55,5 @@ obj-$(CONFIG_SRAM) += sram.o > obj-y+= mic/ > obj-$(CONFIG_GENWQE) += genwqe/ > obj-$(CONFIG_ECHO) += echo/ > +obj-y+= fuse/ > + This adds an empty blank line to the end of the 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 v5 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 05/28/2014 06:54 AM, Peter De Schrijver wrote:
> Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
>
> Signed-off-by: Peter De Schrijver
> ---
> .../devicetree/bindings/fuse/fuse-tegra.txt| 30
>
> .../devicetree/bindings/misc/nvidia,apbmisc.txt| 13
Please name these files according to the compatible value, like all the
other Tegra bindings. So, nvidia,tegra20-efuse.txt and
nvidia,tegra20-apbmisc.txt.
> arch/arm/boot/dts/tegra114.dtsi| 12
> arch/arm/boot/dts/tegra124.dtsi| 12
> arch/arm/boot/dts/tegra20.dtsi | 12
> arch/arm/boot/dts/tegra30.dtsi | 12
It's a bit odd to mix in the .dtsi changes in the same patch as the
binding doc.
> diff --git a/Documentation/devicetree/bindings/fuse/fuse-tegra.txt
> b/Documentation/devicetree/bindings/fuse/fuse-tegra.txt
> +- compatible : should be:
> + "nvidia,tegra20-efuse"
> + "nvidia,tegra30-efuse"
> + "nvidia,tegra114-efuse"
> + "nvidia,tegra124-efuse"
> + Details:
> + nvidia,tegra20-efuse: Tegra20 requires using APB DMA to read the fuse data
> + due to a hardware bug. Tegra20 also lacks certain information which is
> + available in later generations such as fab code, lot code, wafer id,..
> + nvidia,tegra30-efuse, nvidia,tegra114-efuse and nvidia,tegra124-efuse:
> + The differences between these SoCs are the size of the efuse array,
> + the location of the spare (OEM programmable) bits and the location of
> + the speedo data.
I suppose it doesn't hurt, but I don't think we need any of the
"Details" section here.
> +- clocks: Should contain a pointer to the fuse clock.
Please require clock-names so we don't need a mix of index-/name-based
lookups in the future. Please follow the same wording as existing Tegra
binding docs that use clocks. For example:
- 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:
- d_audio
- apbif
Are there no PMC reset signals fed into the fuse block?
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> +apbmisc@0x7800 {
The unit address in the DT nod name should not contain "0x". Same for
other .dtsi files.
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> + apbmisc@0x7800 {
Tegra124 uses #address-cells=<2>, so the unit address in the node name
needs to follow that. Hence, apbmisc@0,7800.
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> @@ -236,6 +236,12 @@
> interrupt-controller;
> };
>
> +apbmisc@0x7800 {
> + compatible = "nvidia,tegra20-apbmisc";
Indentation looks odd here. Mix of TABs/spaces 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 v5 3/5] misc: fuse: Add efuse driver for Tegra
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: > Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. > diff --git a/Documentation/ABI/testing/sysfs-driver-tegra-fuse > b/Documentation/ABI/testing/sysfs-driver-tegra-fuse > +Description: read-only access to the efuses on Tegra20, Tegra30, Tegra114 > + and Tegra124 SoC's from NVIDIA. The efuses contain write once > + data programmed at the factory. The data is layed out in 32bit > + words in LSB first formnat. The number of valid bits depends s/formnat/format/ > + on the word and the SoC. The mapping is as follows: > + > + For Tegra20: > + Word 0 - 1: bit 0 > + Word 2: unused > + Word 3: bits 0 - 31 > + Word 4: bits 0 - 7 Do we really need these long tables that indicate which bits are used? As I mentioned before, when I asked for documentation of the format of these files, all I wanted was a brief not indicating that the data was binary, and that each bit potentially represents a fuse... Either we should leave it at that, or actually document what each bit represents, which would hopefully be a pointless duplication of the TRM. -- 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 v5 0/5] efuse driver for Tegra
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: > This driver allows userspace to read the raw efuse data. Its userspace > interface is modelled after the sunxi_sid driver which provides similar > functionality for some Allwinner SoCs. It has been tested on > Tegra20 (ventana), Tegra30 (beaverboard), Tegra114 (dalmore) and > Tegra124 (jetson TK1). > Changes since v4: > > * Provide fallback to hardcoded 0x7800 in case the apbmisc DT node is > missing. This is exactly what the current code does and prevents a system > crash in that case due to an invalid memory access by tegra_read_chipid() Wouldn't it be better to simply return an error? -- 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 v5 2/5] ARM: tegra: move fuse exports to tegra-soc.h
On 05/28/2014 06:54 AM, Peter De Schrijver wrote:
> All fuse related functionality will move to a driver in the following patches.
> To prepare for this, export all the required functionality in a global header
> file and move all users of fuse.h to tegra-soc.h. While we're at it, remove
> tegra_bct_strapping, as its only user was removed in Commit a7cbe92cef27
> ("ARM: tegra: remove tegra EMC scaling driver").
> diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h
> @@ -33,4 +54,8 @@ static inline int tegra_apb_writel_using_dma(u32 value,
> unsigned long offset)
> return -EINVAL;
> }
> #endif
> +
> +#endif /* __ASSEMBLY__ */
> +
> +
> #endif /* __LINUX_TEGRA_SOC_H_ */
Nit: 2 blank lines there. I'll try and remember to fix that up when I
apply this, if nothing requires a repost.
--
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] debugfs entries for Tegra clocks
On 05/28/2014 10:51 AM, Peter De Schrijver wrote: > This patch set introduces the 'regs' debugfs entry which shows the > contents of all registers related to a clock. Would it be better to create a regmap object for the entire clock module register space, and then get all the debugfs stuff for free? -- 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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/22/2014 10:05 PM, Viresh Kumar wrote:
> On 22 May 2014 22:09, Stephen Warren wrote:
>> I think the call to tegra_target_intermediate() is wrong here; shouldn't
>> the cpufreq core guarantee that tegra_target_intermediate() has always
>> been called before tegra_target(), so there's no need to repeat that
>> call here?
>> Also, tegra_target() doesn't seem to follow the rule documented by patch
>> 2/3 that states ->target() should restore the orignal frequency on
>> error. I'm not even sure if that's possible in general.
>
> I thought I took care of that. Can you please give some example when
> we aren't restoring original frequency on failure ?
>
> About the rule, that has to be the expectation from core as there is no
> way out that for core to know what happened at end of target_index()..
>
> It can call get_rate() but that would be over engineering it looks ..
E.g. in the following code after the series is applied:
> ret = clk_set_rate(pll_x_clk, rate * 1000);
> /* Restore to earlier frequency on error, i.e. pll_x */
> if (ret)
> pr_err("Failed to change pll_x to %lu\n", rate);
>
> ret = clk_set_parent(cpu_clk, pll_x_clk);
If the clk_set_rate() failed, we have no idea what the pll_x rate is; I
don't think the clock API guarantees that zero HW changes have been made
if the function fails. Yet the code switches the CPU clock back to pll_x
without attempting to fix the pll_x rate to be the old rate. Perhaps
there's not much that can be done here though, since if one
clk_set_rate() failed, perhaps the one to fix it will too.
I suspect there are other issues, like switching between pll_p/pllx
might not be restored on error, and the EMC frequency isn't switched
back, etc.
--
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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/21/2014 02:59 AM, Viresh Kumar wrote:
> Tegra had always been switching to intermediate frequency (pll_p_clk) since
> ever. CPUFreq core has better support for handling notifications for these
> frequencies and so we can adapt Tegra's driver to it.
>
> Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we
> should have atleast restored to earlier frequency on error.
This patch breaks Tegra. The reason is below.
> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
> -static int tegra_cpu_clk_set_rate(unsigned long rate)
> +static unsigned int
> +tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)
(BTW, can we please not put the return type on a separate line; it's
inconsistent with the rest of the code in this file)
> +{
> + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
> +
> + /*
> + * Don't switch to intermediate freq if:
> + * - we are already at it, i.e. policy->cur == ifreq
> + * - index corresponds to ifreq
> + */
> + if ((freq_table[index].frequency == ifreq) || (policy->cur == ifreq))
> + return 0;
If policy->cur == ifreq here, then tegra_target_intermediate() isn't
called by the cpufreq core, so ...
> +static int
> +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
> {
> int ret;
>
> /*
>* Take an extra reference to the main pll so it doesn't turn
>* off when we move the cpu off of it
>*/
> clk_prepare_enable(pll_x_clk);
... that reference isn't added...
> @@ -98,10 +96,23 @@ static int tegra_target(struct cpufreq_policy *policy,
> unsigned int index)
> else
> clk_set_rate(emc_clk, 1); /* emc 50Mhz */
>
> - ret = tegra_cpu_clk_set_rate(rate * 1000);
> + /* target freq == pll_p */
> + if (rate * 1000 == clk_get_rate(pll_p_clk)) {
> + ret = tegra_target_intermediate(policy, index);
> + goto disable_pll_x;
> + }
... and this code doesn't call it either, since we could be switching
from the pll_p rate to something faster ...
> +
> + ret = clk_set_rate(pll_x_clk, rate * 1000);
> + /* Restore to earlier frequency on error, i.e. pll_x */
> if (ret)
> - pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
> - rate);
> + pr_err("Failed to change pll_x to %lu\n", rate);
> +
> + ret = clk_set_parent(cpu_clk, pll_x_clk);
> + /* This shouldn't fail while changing or restoring */
> + WARN_ON(ret);
> +
> +disable_pll_x:
> + clk_disable_unprepare(pll_x_clk);
... so this turns off pll_x even though we're running from it.
It would be simpler if Tegra *always* used an intermediate frequency,
and hence the core *always* called tegra_target_intermediate().
Admittedly, this would result in tegra_target() sometimes (when
switching CPU clock rate to the pll_p rate) doing nothing other than
removing the extra reference on pll_x, but I think that the code would
be simpler to follow and more robust.
--
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] devicetree: Add generic IOMMU device tree bindings
On 05/23/2014 02:36 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 > > Signed-off-by: Thierry Reding > --- > Apologies for the noise, but apparently I mistyped one of the email > addresses, should be fixed now. > > Changes in v2: > - add notes about "dma-ranges" property (drop note from commit message) > - document priorities of "iommus" property vs. "dma-ranges" property > - drop #iommu-cells in favour of #address-cells and #size-cells I think this is a mistake. address-cells/size-cells are for transactions flowing down the bus (from the CPU to date). Describing a connection from a device to an IOMMU is something completely different, and should therefore simply use an iommu-cells property to describe any necessary information. If we start re-using properties for different things in different contexts, how is anyone going to know what they mean, and how will conflicts be resolved. For example, what if there's a single HW module that both acts as a regular register bus with children (where address-cells/size-cells defines how transactions reach the children from the parent), and is also an IOMMU (where according to this binding proposal, address-cells/size-cells represent some aspect of the IOMMU feature). Using different properties for different things is the only sane way to keep different concepts separate. Another alternative would be to represent the single HW module as separate nodes in DT, but I think that will only make our lives harder, and where I've done that in the past, I've regretted 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] scripts/dtc: pad DTBs to facilitate later modification
On 05/23/2014 05:41 PM, Kevin Hilman wrote: > By default, add some padding to the DT blobs to facilitate later > patching. > > An example need for DTB patching is the need to modifiy the command > line on platforms where ATAGS are not (or cannot) be used to pass the > commandline. For example, we do not support a big-endian kernel > reading ATAGS from a little-endian u-boot, so the only way to pass a > command line in the DT. > > Also, without ATAG support (or if u-boot was built without > CONFIG_INITRD_TAG) the only way to pass an initrd is by adding an > initrd= option to command line (in the DT). > > Therefore, to facilitate adding to the DT command line directly in the > DTB, add some padding. > > Cc: Nicolas Pitre > Cc: Stephen Warren > Cc: Thomas Petazzoni > Signed-off-by: Kevin Hilman > --- > I kinda pulled 64 bytes out of the air here since it's enough to add > some common things to the commandline like debug, earlyprink > initrd=,, etc., but I'm certainlly not opposed to more > padding. Conceptually, Acked-by: Stephen Warren But I would expect a pad of something like 4KB to be more future-proof. U-Boot appears to use 4KB on ARM at least: ./arch/arm/dts/Makefile:37:DTC_FLAGS += -R 4 -p 0x1000 -- 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] scripts/dtc: pad DTBs to facilitate later modification
On 05/23/2014 05:41 PM, Kevin Hilman wrote: By default, add some padding to the DT blobs to facilitate later patching. An example need for DTB patching is the need to modifiy the command line on platforms where ATAGS are not (or cannot) be used to pass the commandline. For example, we do not support a big-endian kernel reading ATAGS from a little-endian u-boot, so the only way to pass a command line in the DT. Also, without ATAG support (or if u-boot was built without CONFIG_INITRD_TAG) the only way to pass an initrd is by adding an initrd= option to command line (in the DT). Therefore, to facilitate adding to the DT command line directly in the DTB, add some padding. Cc: Nicolas Pitre n...@linaro.org Cc: Stephen Warren swar...@wwwdotorg.org Cc: Thomas Petazzoni thomas.petazz...@free-electrons.com Signed-off-by: Kevin Hilman khil...@linaro.org --- I kinda pulled 64 bytes out of the air here since it's enough to add some common things to the commandline like debug, earlyprink initrd=addr,size, etc., but I'm certainlly not opposed to more padding. Conceptually, Acked-by: Stephen Warren swar...@nvidia.com But I would expect a pad of something like 4KB to be more future-proof. U-Boot appears to use 4KB on ARM at least: ./arch/arm/dts/Makefile:37:DTC_FLAGS += -R 4 -p 0x1000 -- 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] devicetree: Add generic IOMMU device tree bindings
On 05/23/2014 02:36 PM, Thierry Reding wrote: From: Thierry Reding tred...@nvidia.com 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 Signed-off-by: Thierry Reding tred...@nvidia.com --- Apologies for the noise, but apparently I mistyped one of the email addresses, should be fixed now. Changes in v2: - add notes about dma-ranges property (drop note from commit message) - document priorities of iommus property vs. dma-ranges property - drop #iommu-cells in favour of #address-cells and #size-cells I think this is a mistake. address-cells/size-cells are for transactions flowing down the bus (from the CPU to date). Describing a connection from a device to an IOMMU is something completely different, and should therefore simply use an iommu-cells property to describe any necessary information. If we start re-using properties for different things in different contexts, how is anyone going to know what they mean, and how will conflicts be resolved. For example, what if there's a single HW module that both acts as a regular register bus with children (where address-cells/size-cells defines how transactions reach the children from the parent), and is also an IOMMU (where according to this binding proposal, address-cells/size-cells represent some aspect of the IOMMU feature). Using different properties for different things is the only sane way to keep different concepts separate. Another alternative would be to represent the single HW module as separate nodes in DT, but I think that will only make our lives harder, and where I've done that in the past, I've regretted 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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/21/2014 02:59 AM, Viresh Kumar wrote:
Tegra had always been switching to intermediate frequency (pll_p_clk) since
ever. CPUFreq core has better support for handling notifications for these
frequencies and so we can adapt Tegra's driver to it.
Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we
should have atleast restored to earlier frequency on error.
This patch breaks Tegra. The reason is below.
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
-static int tegra_cpu_clk_set_rate(unsigned long rate)
+static unsigned int
+tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)
(BTW, can we please not put the return type on a separate line; it's
inconsistent with the rest of the code in this file)
+{
+ unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+
+ /*
+ * Don't switch to intermediate freq if:
+ * - we are already at it, i.e. policy-cur == ifreq
+ * - index corresponds to ifreq
+ */
+ if ((freq_table[index].frequency == ifreq) || (policy-cur == ifreq))
+ return 0;
If policy-cur == ifreq here, then tegra_target_intermediate() isn't
called by the cpufreq core, so ...
+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
{
int ret;
/*
* Take an extra reference to the main pll so it doesn't turn
* off when we move the cpu off of it
*/
clk_prepare_enable(pll_x_clk);
... that reference isn't added...
@@ -98,10 +96,23 @@ static int tegra_target(struct cpufreq_policy *policy,
unsigned int index)
else
clk_set_rate(emc_clk, 1); /* emc 50Mhz */
- ret = tegra_cpu_clk_set_rate(rate * 1000);
+ /* target freq == pll_p */
+ if (rate * 1000 == clk_get_rate(pll_p_clk)) {
+ ret = tegra_target_intermediate(policy, index);
+ goto disable_pll_x;
+ }
... and this code doesn't call it either, since we could be switching
from the pll_p rate to something faster ...
+
+ ret = clk_set_rate(pll_x_clk, rate * 1000);
+ /* Restore to earlier frequency on error, i.e. pll_x */
if (ret)
- pr_err(cpu-tegra: Failed to set cpu frequency to %lu kHz\n,
- rate);
+ pr_err(Failed to change pll_x to %lu\n, rate);
+
+ ret = clk_set_parent(cpu_clk, pll_x_clk);
+ /* This shouldn't fail while changing or restoring */
+ WARN_ON(ret);
+
+disable_pll_x:
+ clk_disable_unprepare(pll_x_clk);
... so this turns off pll_x even though we're running from it.
It would be simpler if Tegra *always* used an intermediate frequency,
and hence the core *always* called tegra_target_intermediate().
Admittedly, this would result in tegra_target() sometimes (when
switching CPU clock rate to the pll_p rate) doing nothing other than
removing the extra reference on pll_x, but I think that the code would
be simpler to follow and more robust.
--
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 V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks
On 05/22/2014 10:05 PM, Viresh Kumar wrote: On 22 May 2014 22:09, Stephen Warren swar...@wwwdotorg.org wrote: I think the call to tegra_target_intermediate() is wrong here; shouldn't the cpufreq core guarantee that tegra_target_intermediate() has always been called before tegra_target(), so there's no need to repeat that call here? Also, tegra_target() doesn't seem to follow the rule documented by patch 2/3 that states -target() should restore the orignal frequency on error. I'm not even sure if that's possible in general. I thought I took care of that. Can you please give some example when we aren't restoring original frequency on failure ? About the rule, that has to be the expectation from core as there is no way out that for core to know what happened at end of target_index().. It can call get_rate() but that would be over engineering it looks .. E.g. in the following code after the series is applied: ret = clk_set_rate(pll_x_clk, rate * 1000); /* Restore to earlier frequency on error, i.e. pll_x */ if (ret) pr_err(Failed to change pll_x to %lu\n, rate); ret = clk_set_parent(cpu_clk, pll_x_clk); If the clk_set_rate() failed, we have no idea what the pll_x rate is; I don't think the clock API guarantees that zero HW changes have been made if the function fails. Yet the code switches the CPU clock back to pll_x without attempting to fix the pll_x rate to be the old rate. Perhaps there's not much that can be done here though, since if one clk_set_rate() failed, perhaps the one to fix it will too. I suspect there are other issues, like switching between pll_p/pllx might not be restored on error, and the EMC frequency isn't switched back, etc. -- 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] debugfs entries for Tegra clocks
On 05/28/2014 10:51 AM, Peter De Schrijver wrote: This patch set introduces the 'regs' debugfs entry which shows the contents of all registers related to a clock. Would it be better to create a regmap object for the entire clock module register space, and then get all the debugfs stuff for free? -- 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 v5 0/5] efuse driver for Tegra
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: This driver allows userspace to read the raw efuse data. Its userspace interface is modelled after the sunxi_sid driver which provides similar functionality for some Allwinner SoCs. It has been tested on Tegra20 (ventana), Tegra30 (beaverboard), Tegra114 (dalmore) and Tegra124 (jetson TK1). Changes since v4: * Provide fallback to hardcoded 0x7800 in case the apbmisc DT node is missing. This is exactly what the current code does and prevents a system crash in that case due to an invalid memory access by tegra_read_chipid() Wouldn't it be better to simply return an error? -- 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 v5 2/5] ARM: tegra: move fuse exports to tegra-soc.h
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: All fuse related functionality will move to a driver in the following patches. To prepare for this, export all the required functionality in a global header file and move all users of fuse.h to tegra-soc.h. While we're at it, remove tegra_bct_strapping, as its only user was removed in Commit a7cbe92cef27 (ARM: tegra: remove tegra EMC scaling driver). diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h @@ -33,4 +54,8 @@ static inline int tegra_apb_writel_using_dma(u32 value, unsigned long offset) return -EINVAL; } #endif + +#endif /* __ASSEMBLY__ */ + + #endif /* __LINUX_TEGRA_SOC_H_ */ Nit: 2 blank lines there. I'll try and remember to fix that up when I apply this, if nothing requires a repost. -- 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 v5 3/5] misc: fuse: Add efuse driver for Tegra
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: Implement fuse driver for Tegra20, Tegra30, Tegra114 and Tegra124. diff --git a/Documentation/ABI/testing/sysfs-driver-tegra-fuse b/Documentation/ABI/testing/sysfs-driver-tegra-fuse +Description: read-only access to the efuses on Tegra20, Tegra30, Tegra114 + and Tegra124 SoC's from NVIDIA. The efuses contain write once + data programmed at the factory. The data is layed out in 32bit + words in LSB first formnat. The number of valid bits depends s/formnat/format/ + on the word and the SoC. The mapping is as follows: + + For Tegra20: + Word 0 - 1: bit 0 + Word 2: unused + Word 3: bits 0 - 31 + Word 4: bits 0 - 7 Do we really need these long tables that indicate which bits are used? As I mentioned before, when I asked for documentation of the format of these files, all I wanted was a brief not indicating that the data was binary, and that each bit potentially represents a fuse... Either we should leave it at that, or actually document what each bit represents, which would hopefully be a pointless duplication of the TRM. -- 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 v5 4/5] ARM: tegra: Add efuse and apbmisc bindings
On 05/28/2014 06:54 AM, Peter De Schrijver wrote:
Add efuse and apbmisc bindings for Tegra20, Tegra30, Tegra114 and Tegra124.
Signed-off-by: Peter De Schrijver pdeschrij...@nvidia.com
---
.../devicetree/bindings/fuse/fuse-tegra.txt| 30
.../devicetree/bindings/misc/nvidia,apbmisc.txt| 13
Please name these files according to the compatible value, like all the
other Tegra bindings. So, nvidia,tegra20-efuse.txt and
nvidia,tegra20-apbmisc.txt.
arch/arm/boot/dts/tegra114.dtsi| 12
arch/arm/boot/dts/tegra124.dtsi| 12
arch/arm/boot/dts/tegra20.dtsi | 12
arch/arm/boot/dts/tegra30.dtsi | 12
It's a bit odd to mix in the .dtsi changes in the same patch as the
binding doc.
diff --git a/Documentation/devicetree/bindings/fuse/fuse-tegra.txt
b/Documentation/devicetree/bindings/fuse/fuse-tegra.txt
+- compatible : should be:
+ nvidia,tegra20-efuse
+ nvidia,tegra30-efuse
+ nvidia,tegra114-efuse
+ nvidia,tegra124-efuse
+ Details:
+ nvidia,tegra20-efuse: Tegra20 requires using APB DMA to read the fuse data
+ due to a hardware bug. Tegra20 also lacks certain information which is
+ available in later generations such as fab code, lot code, wafer id,..
+ nvidia,tegra30-efuse, nvidia,tegra114-efuse and nvidia,tegra124-efuse:
+ The differences between these SoCs are the size of the efuse array,
+ the location of the spare (OEM programmable) bits and the location of
+ the speedo data.
I suppose it doesn't hurt, but I don't think we need any of the
Details section here.
+- clocks: Should contain a pointer to the fuse clock.
Please require clock-names so we don't need a mix of index-/name-based
lookups in the future. Please follow the same wording as existing Tegra
binding docs that use clocks. For example:
- 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:
- d_audio
- apbif
Are there no PMC reset signals fed into the fuse block?
diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
+apbmisc@0x7800 {
The unit address in the DT nod name should not contain 0x. Same for
other .dtsi files.
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
+ apbmisc@0x7800 {
Tegra124 uses #address-cells=2, so the unit address in the node name
needs to follow that. Hence, apbmisc@0,7800.
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
@@ -236,6 +236,12 @@
interrupt-controller;
};
+apbmisc@0x7800 {
+ compatible = nvidia,tegra20-apbmisc;
Indentation looks odd here. Mix of TABs/spaces 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 v5 5/5] ARM: tegra: build new fuse driver in drivers/misc
On 05/28/2014 06:54 AM, Peter De Schrijver wrote: The Tegra fuse code has moved to drivers/misc/fuse/tegra. Enable the building of that code, and disable the building of the old fuse code in arch/arm/mach-tegra/. diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 7eb4b69..994d02d 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -55,3 +55,5 @@ obj-$(CONFIG_SRAM) += sram.o obj-y+= mic/ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ +obj-y+= fuse/ + This adds an empty blank line to the end of the 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 v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/25/2014 08:43 PM, f...@marvell.com wrote: From: Fan Wu f...@marvell.com What the patch did: 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of calling pinctrl_select_state 2.Remove the HW disable operation in in pinmux_disable_setting function. ... This commit description is way too long for such a simple change. A much shorter summary would be better. Signed-off-by: Fan Wu f...@marvell.com Signed-off-by: Stephen Warren swar...@nvidia.com I'm pretty sure I never signed off on this patch. How come my s-o-b line is there? This patch still doesn't remove ops-disable from the struct pinmux_ops, nor any of its implementations. Shouldn't 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 v5 0/5] efuse driver for Tegra
On 05/28/2014 07:16 AM, Andrew Morton wrote: On Wed, 28 May 2014 15:54:32 +0300 Peter De Schrijver pdeschrij...@nvidia.com wrote: This driver allows userspace to read the raw efuse data. The patchset uses an inexplicable mixture of fuse and efuse. Given that the kernel already has a fuse, it would be nice if this code could use efuse everywhere. I didn't check every instance, but a mix of those two terms seems reasonable. fuse is the type of HW object; it's a generic HW term that exists outside the scope of this patch. While the conflict with Linux's filesystem is unfortunate, it's a name collision that I think we'll simply have to live with; it's not created by this patch. efuse is the name of the Tegra module that hosts Tegra's fuses. -- 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/
