* [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks
From: William Towle @ 2014-02-04 18:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391537858-28593-1-git-send-email-william.towle@codethink.co.uk>
The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
extal_clk reference for the parent of all the clocks that
it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
are all parented to either pll1 or pll1_div2 which means
that the clock rates are incorrect.
This is part of the fix that corrects the SDHI0 clock
rate error where it reports 1MHz instead of 97.5:
sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
Notes:
- May require cross-merge with clk-rcar-gen2.c fix
- Also not clear which clock "z" is to fix it.
[ben.dooks at codethink.co.uk: updated patch description]
Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
arch/arm/boot/dts/r8a7790.dtsi | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index ff55c6e..242e6e2 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -446,7 +446,13 @@
compatible = "renesas,r8a7790-cpg-clocks",
"renesas,rcar-gen2-cpg-clocks";
reg = <0 0xe6150000 0 0x1000>;
- clocks = <&extal_clk>;
+ clocks = <&extal_clk>, <&extal_clk>, <&extal_clk>, <&extal_clk>,
+ <&cpg_clocks R8A7790_CLK_PLL1>,
+ <&pll1_div2_clk>,
+ <&cpg_clocks R8A7790_CLK_PLL1>,
+ <&cpg_clocks R8A7790_CLK_PLL1>,
+ <&cpg_clocks R8A7790_CLK_PLL1>
+ /* not known for "z" ...,<> */;
#clock-cells = <1>;
clock-output-names = "main", "pll0", "pll1", "pll3",
"lb", "qspi", "sdh", "sd0", "sd1",
--
1.7.10.4
^ permalink raw reply related
* [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table
From: William Towle @ 2014-02-04 18:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391537858-28593-1-git-send-email-william.towle@codethink.co.uk>
The clk_div_table for cpg_sd01_div_table[] concurs with the manual
but not with values found in the device itself (which are also the
same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
Update the clk-rcar-gen2.c driver to have the same table as the one
used by the mach-shmobile driver which work once further issues are
fixed in the clk-rcar-gen2.c driver.
Part of the fix for the following error where the driver reports the
output as 1MHz but is really 97.5MHz:
sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
[ben.dooks at codethink.co.uk: updated patch description]
Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c b/drivers/clk/shmobile/clk-rcar-gen2.c
index a59ec21..df4a1e6 100644
--- a/drivers/clk/shmobile/clk-rcar-gen2.c
+++ b/drivers/clk/shmobile/clk-rcar-gen2.c
@@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] = {
};
static const struct clk_div_table cpg_sd01_div_table[] = {
+ { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 },
+ { 4, 8 },
{ 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 },
{ 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 },
};
--
1.7.10.4
^ permalink raw reply related
* Clock divisor/parent settings changes
From: William Towle @ 2014-02-04 18:17 UTC (permalink / raw)
To: linux-arm-kernel
Patches (3x) to ensure we see:
sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz
sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 clock rate 48 MHz
Attached:
[PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table
[PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for
[PATCH 3/3] clk: shmobile: handle multiple parent clocks for cpg
^ permalink raw reply
* [PATCH] arm64: Add architecture support for PCI
From: Jason Gunthorpe @ 2014-02-04 18:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <13031998.NR888KZWhk@wuerfel>
On Tue, Feb 04, 2014 at 10:44:52AM +0100, Arnd Bergmann wrote:
> Now I want to integrate the EHCI into my SoC and not waste one
> of my precious PCIe root ports, so I have to create another PCI
> domain with its own ECAM compliant config space to put it into.
> Fortunately SBSA lets me add an arbitrary number of PCI domains,
> as long as they are all strictly compliant. To software it will
Just to touch on this for others who might be reading..
IMHO any simple SOC that requires multiple domains is *broken*. A
single domain covers all reasonable needs until you get up to
mega-scale NUMA systems, encouraging people to design with multiple
domains only complicates the kernel :(
SOC internal peripherals should all show up in the bus 0 config space
of the only domain and SOC PCI-E physical ports should show up on bus
0 as PCI-PCI bridges. This is all covered in the PCI-E specs regarding
the root complex.
Generally I would expect the internal peripherals to still be
internally connected with AXI, but also connected through the ECAM
space for configuration, control, power management and address
assignment.
> 2. all address windows are set up by the boot loader, we only
> need to know the location (IMHO this should be the
> preferred way to do things regardless of SBSA).
Linux does a full address map re-assignment on boot, IIRC. You need
more magics to inhibit that if your BAR's and bridge windows don't
work.
Hot plug is a whole other thing..
> it's possible that the designware based ones get point 4 right.
The designware one's also appear to be re-purposed end point cores, so
their config handling is somewhat bonkers. Tegra got theirs sort of
close because they re-used knowledge/IP from their x86 south bridges -
but even then they didn't really implement ECAM properly for an ARM
environment.
Since config space is where everyone to date has fallen down, I think
the SBSA would have been wise to list dword by dword what a typical
ECAM config space should look like.
Jason
^ permalink raw reply
* [PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC
From: Mark Brown @ 2014-02-04 18:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8e4231b7a55802f58a14dd07ac5cd8b0babb1dce.1391274628.git.moinejf@free.fr>
On Sat, Feb 01, 2014 at 05:48:49PM +0100, Jean-Francois Moine wrote:
> + - compatible: must be "nxp,tda998x-codec".
It's not clear to me why there's a separate compatible here - as far as
I can see this can only appear as part of one of these devices and
there's no addressing or other information that'd account for chip
variation so I'd not expect to need to bind this independently of the
parent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/fbac5b49/attachment.sig>
^ permalink raw reply
* [PATCH v3 5/5] ASoC: tda998x: adjust the audio CTS_N pre-divider from audio format
From: Mark Brown @ 2014-02-04 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <df65a2de7d4afc2471846c02b73e8bbdc4f9a743.1391274628.git.moinejf@free.fr>
On Thu, Jan 30, 2014 at 12:08:06PM +0100, Jean-Francois Moine wrote:
> - if (format == p->audio_format) {
> + if (format == p->audio_format &&
> + params_format(params) == priv->audio_sample_format) {
> reg_write(priv, REG_ENA_AP, p->audio_cfg);
> return;
I find the above confusing and probably deserving of a comment
explaining the logic. I think it's trying to skip noop configuration
updates?
> }
> p->audio_format = format;
> + priv->audio_sample_format = params_format(params);
You should be using params_width() for the number of bits per sample.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/2983dfe7/attachment.sig>
^ permalink raw reply
* [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID
From: Mark Brown @ 2014-02-04 18:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1b15025671d9099863a3091346536e45891e4a26.1391274628.git.moinejf@free.fr>
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
> + /* change the snd_soc_pcm_stream values of the driver */
> + stream->rates = rate_mask;
> + stream->channels_max = max_channels;
> + stream->formats = formats;
> + /* copy the DAI driver to a writable area */
> + dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL);
> + if (!dai_drv)
> + return -ENOMEM;
> + memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
> +
The code should be doing this by setting constraints based on the
current setup rather than by editing the data structure - the expecation
is very much that the data won't change so this prevents surprises with
future work on the core.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/b5f17dd6/attachment.sig>
^ permalink raw reply
* [PATCH] regulator: core: Make regulator object reflect configured voltage
From: Bjorn Andersson @ 2014-02-04 18:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140204110531.GR22609@sirena.org.uk>
On Tue, Feb 4, 2014 at 3:05 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 03, 2014 at 09:54:28PM -0800, Bjorn Andersson wrote:
>
>> + /*
>> + * Make the regulator reflect the configured voltage selected in
>> + * machine_constraints_voltage()
>> + */
>> + if (rdev->constraints->apply_uV &&
>> + rdev->constraints->min_uV == rdev->constraints->max_uV) {
>> + regulator->min_uV = rdev->constraints->min_uV;
>> + regulator->max_uV = rdev->constraints->min_uV;
>> + }
>> +
>
> Why not do this at the time we apply the voltage? That would seem to be
> more robust, doing it in a separate place means that we might update one
> bit of code and not the other or might change the execution path so that
> one gets run and the other doesn't.
I do share your concerns about having this logic mirrored here is
risky, unfortunately the regulator object is created upon request from
a consumer; so it is not available when regulator_register() calls
set_machine_constraints().
An alternative is to drop the conditional setting of
REGULATOR_CHANGE_VOLTAGE from of_regulator.c and force the regulator
drivers to set this flag explicitly; to avoid the difference in
behavior depending on configuration.
Regards,
Bjorn
^ permalink raw reply
* Extending OPP bindings
From: Nishanth Menon @ 2014-02-04 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140131180929.GH2616@e102568-lin.cambridge.arm.com>
On 01/31/2014 12:09 PM, Lorenzo Pieralisi wrote:
> Hi Rob,
>
> thanks for having a look.
>
> On Fri, Jan 31, 2014 at 05:17:51PM +0000, Rob Herring wrote:
>> On Fri, Jan 31, 2014 at 6:46 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> Hi Nishanth,
>>>
>>> On Fri, Jan 31, 2014 at 12:43:54AM +0000, Nishanth Menon wrote:
>>>> Hi Sudeep,
>>>>
>>>> On 01/30/2014 07:43 AM, Sudeep Holla wrote:
>>>>
>>>>>
>>>>> I am looking into a couple shortcomings in the current OPP bindings and
>>>>> how to address them. Feel free to add to the list if you think of any more
>>>>> issues that needs to be addressed or if and how any problem mentioned below
>>>>> can be handled with the existing bindings.
>>>>>
>>>>> 1. indexing: currently there are no indices in the operating-points.
>>>> indexing is based on frequency which is why the accessors use
>>>> frequency to pull out the OPP data.
>>>>
>>>> indexing is a horrible idea - on platforms where OPP may be disabled
>>>> or enabled for various reasons(see arch/arm/mach-imx/mach-imx6q.c,
>>>> arch/arm/mach-omap2/board-omap3beagle.c etc) - the indexing you see in
>>>> dts is just a myth that may not exist once the nodes are loaded and
>>>> operated upon depending on SoC variations (example efuse describing
>>>> which OPPs can be used and which not).
>>>
>>> I do not understand why. As long as a mapping from DT to data structures
>>> in the kernel is done at boot, I can see how by indexing device nodes
>>> can refer to specific OPPs. If they are disabled, amen, as long as we
>>> can point at specific operating points that should be ok.
>>>
>>> Indexing does not mean that the index in the DT is the same as the one
>>> allocated by the OS. Indexing is there to point at specific OPPs from
>>> different DT nodes, a good example are clock bindings and that's exactly
>>> how they work.
>>
>> That is not a good comparison. With clocks, you are describing which
>> physical signal coming out of a hardware block much like interrupts.
>> Granted the h/w is not typically documented that way for clocks
>> although the numbering could correspond to register bit locations as
>> interrupt numbers do. With OPP indexes, they are a totally made up
>> software construct.
>
> Well ok, it might be, what I know is that current OPPs do not allow
> other nodes to reference their properties properly, I am not sure that adding
> an index make things worse, actually they are already broken as they are.
Let me look at this in two parts:
A: In kernel data representation:
If I were to use analogy of OPP library to database, A data base needs
a key to pick out data stored inside it -> the key in opp definition
as it stands right now is frequency. you can pick up any of the data
based on that key. proposal for using index into that table adds no
real value here.
Now, we can represent OPP data in many different forms. consider two
other properties per OPP (propx and propy)
data set #0:
+------------+------------+---------+-------------+
| freq | voltage | propx | propy |
+-------------------------------------------------+
| Fa | Va | PXa | PYa |
| Fb | Vb | PXb | PYb |
| Fc | Vc | PXc | PYc |
| Fd | Vd | PXd | PYd |
+------------+------------+---------+-------------+
Can then be represented as:
in opp library, considering it to be the least common denominator:
(data set #1)
+------------+------------+
| freq | voltage |
+--------------------------
| Fa | Va |
| Fb | Vb |
| Fc | Vc |
| Fd | Vd |
+------------+------------+
where ever propx makes sense. (data set #2)
+------------+---------+
| freq | propx |
+-----------------------
| Fa | PXa |
| Fb | PXb |
| Fc | PXc |
| Fd | PXd |
+------------+---------+
where ever propy makes sense (data set #3)
+------------+-------------+
| freq | propy |
+--------------------------+
| Fa | PYa |
| Fb | PYb |
| Fc | PYc |
| Fd | PYd |
+------------+-------------+
If my memory serves me right, in database terminology, this'd be first
normal form.
This also allows for data set #2 and #3 to modify or add a data set #4
with a propz at a later point in time without impacting set #1-3.
propx,y,z can be anything - efuse bits, cpuidle c state definition, etc..
As long as the key to the data sets are all the same (frequency),
information in data set #0 is maintained. It would be in our common
long term interest to maintain the split.
>> Perhaps OPPs should be nodes so new fields can be added easily and
>> then you have a phandle for each OPP.
>
> Yeah, but the end result is the same, it has more to do with how to
> express dependencies with DT than the question on whether to use a phandle
> or an index. If we have to add phandles so be it, it is still just a way
> to reference properties for me.
B) Device tree representation of an OPP:
Here we have a ton of flexibility- I love the idea of representing
each OPP as a phandle - However, a phandle has been traditionally
meant to represent a device, I might be wrong, but i am not sure if we
have a precedence where phandle represents a property.
Having each OPP as a phandle does provide a lots of representation,
extensibility and reuse opportunity. but the same phandle will have to
be parsed by different drivers to pull out relevant data.
However when Sudeep introduced phandle approach, we did debate it's
representation and appropriate topology. if we can consider OPP as
it's own phandle and operating-points a list of OPP phandles, the pain
that we face in various driver usage can be simplified.
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH] arm64: Align CMA sizes to PAGE_SIZE
From: Catalin Marinas @ 2014-02-04 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391214234-23526-1-git-send-email-lauraa@codeaurora.org>
On Sat, Feb 01, 2014 at 12:23:54AM +0000, Laura Abbott wrote:
> dma_alloc_from_contiguous takes number of pages for a size.
> Align up the dma size passed in to page size to avoid truncation
> and allocation failures on sizes less than PAGE_SIZE.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
> arch/arm64/mm/dma-mapping.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 864b256..be2696e 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -44,6 +44,8 @@ static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size,
> flags |= GFP_DMA32;
> if (IS_ENABLED(CONFIG_CMA)) {
> unsigned long pfn;
> +
> + size = PAGE_ALIGN(size);
> pfn = dma_alloc_from_contiguous(dev, size >> PAGE_SHIFT,
> get_order(size));
The patch looks ok, but it doesn't apply. In my kernel (3.14-rc1),
dma_alloc_from_contiguous returns a struct page.
Thanks.
--
Catalin
^ permalink raw reply
* [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
From: Mark Brown @ 2014-02-04 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140204181605.5b837a70@armhf>
On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > > + /* load the optional CODEC */
> > > + of_platform_populate(np, NULL, NULL, &client->dev);
> > Why is this using of_platform_populate()? That's a very odd way of
> > doing things.
> The i2c does not populate the subnodes in the DT. I did not find why,
> but, what is sure is that if of_platform_populate() is not called, the
> tda CODEC module is not loaded.
You shouldn't be representing this as a separate node in the DT unless
there really is a distinct and reusable IP, otherwise you're putting
Linux implementation details in there. Describe the hardware, not the
implemementation.
> You may find an other example in drivers/mfd/twl-core.c.
The TWL drivers aren't always a shining example of how to do things -
they were one of the earliest MFDs so there's warts in there.
> > > +config SND_SOC_TDA998X
> > > + tristate
> > > + depends on OF
> > > + default y if DRM_I2C_NXP_TDA998X=y
> > > + default m if DRM_I2C_NXP_TDA998X=m
> > Make this visible if it can be selected from DT so it can be used with
> > generic cards.
> I don't understand. The tda CODEC can only be used with the TDA998x I2C
> driver. It might have been included in the tda998x source as well.
You shouldn't have the default settings there at all, that's not the
normal idiom for MFDs. I'd also not expect to have to build the CODEC
driver just because I built the DRM component.
> Now, the CODEC is declared inside the tda998x as a node child. But, in
> a bad DT, the tda CODEC could be declared anywhere, even inside a other
> DRM I2C slave encoder, in which case, bad things would happen...
So, part of the problem here is that this is being explicitly declared
in the DT leading to more sources for error.
> > What does this actually do? No information is being passed in to the
> > core function here, not even any information on if it's starting or
> > stopping. Looking at the rest of the code I can't help thinking it
> > might be clearer to inline this possibly with a lookup helper, the code
> > is very small and the lack of parameters makes it hard to follow.
> I thought it was simple enough. The function tda_start_stop() is called
> from 2 places:
It's not at all obvious - _audio_update() isn't a terribly descriptive
name, just looking at that function by itself I had no idea what it was
supposed to be doing.
> - on audio start in tda_startup with the audio type (DAI id)
> priv->dai_id = dai->id;
> - on audio stop with a null audio type
> priv->dai_id = 0; /* streaming stop */
> On stream start, the DAI id is never null, as explained in the patch 1:
> The audio format values in the encoder configuration interface are
> changed to non null values so that the value 0 is used in the audio
> function to indicate that audio streaming is stopped.
> and on streaming stop the port is not meaningful.
> I will add a null item in the enum (AFMT_NO_AUDIO).
So we can't use both streams simultaneously then? That's a bit sad.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/037aca8f/attachment.sig>
^ permalink raw reply
* [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
From: Mark Brown @ 2014-02-04 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52F0ECF2.9070102@metafoo.de>
On Tue, Feb 04, 2014 at 02:36:50PM +0100, Lars-Peter Clausen wrote:
> On 02/04/2014 02:30 PM, Mark Brown wrote:
> >>+static const struct snd_soc_dapm_route tda_routes[] = {
> >>+ { "hdmi-out", NULL, "HDMI I2S Playback" },
> >>+ { "hdmi-out", NULL, "HDMI SPDIF Playback" },
> >>+};
> >S/PDIF.
> Won't this cause issues with the debugfs widget entries? It's
> fixable by escaping it (replace it by a dash or something) in the
> debugfs widget filename, but I don't think we do this right now.
Oh, bother, so it will.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/bf5057df/attachment.sig>
^ permalink raw reply
* [PATCH] ARM: at91: add Atmel's SAMA5D3 Xplained board
From: Nicolas Ferre @ 2014-02-04 17:42 UTC (permalink / raw)
To: linux-arm-kernel
Add DT file for new SAMA5D3 Xpained board.
This board is based on Atmel's SAMA5D36 Cortex-A5 SoC.
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
arch/arm/boot/dts/at91-sama5d3_xplained.dts | 233 ++++++++++++++++++++++++++++
1 file changed, 233 insertions(+)
create mode 100644 arch/arm/boot/dts/at91-sama5d3_xplained.dts
diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained.dts b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
new file mode 100644
index 000000000000..fb1349ca60a4
--- /dev/null
+++ b/arch/arm/boot/dts/at91-sama5d3_xplained.dts
@@ -0,0 +1,233 @@
+/*
+ * at91-sama5d3_xplained.dts - Device Tree file for the SAMA5D3 Xplained board
+ *
+ * Copyright (C) 2014 Atmel,
+ * 2014 Nicolas Ferre <nicolas.ferre@atmel.com>
+ *
+ * Licensed under GPLv2 or later.
+ */
+/dts-v1/;
+#include "sama5d36.dtsi"
+
+/ {
+ model = "SAMA5D3 Xplained";
+ compatible = "atmel,sama5d3-xplained", "atmel,sama5d3", "atmel,sama5";
+
+ chosen {
+ bootargs = "console=ttyS0,115200";
+ };
+
+ memory {
+ reg = <0x20000000 0x10000000>;
+ };
+
+ ahb {
+ apb {
+ mmc0: mmc at f0000000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_mmc0_clk_cmd_dat0 &pinctrl_mmc0_dat1_3 &pinctrl_mmc0_dat4_7 &pinctrl_mmc0_cd>;
+ status = "okay";
+ slot at 0 {
+ reg = <0>;
+ bus-width = <8>;
+ cd-gpios = <&pioE 0 GPIO_ACTIVE_LOW>;
+ };
+ };
+
+ spi0: spi at f0004000 {
+ cs-gpios = <&pioD 13 0>, <0>, <0>, <0>;
+ status = "okay";
+ };
+
+ can0: can at f000c000 {
+ status = "okay";
+ };
+
+ i2c0: i2c at f0014000 {
+ status = "okay";
+ };
+
+ i2c1: i2c at f0018000 {
+ status = "okay";
+ };
+
+ macb0: ethernet at f0028000 {
+ phy-mode = "rgmii";
+ status = "okay";
+ };
+
+ usart0: serial at f001c000 {
+ status = "okay";
+ };
+
+ usart1: serial at f0020000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
+ status = "okay";
+ };
+
+ uart0: serial at f0024000 {
+ status = "okay";
+ };
+
+ mmc1: mmc at f8000000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_mmc1_clk_cmd_dat0 &pinctrl_mmc1_dat1_3 &pinctrl_mmc1_cd>;
+ status = "okay";
+ slot at 0 {
+ reg = <0>;
+ bus-width = <4>;
+ cd-gpios = <&pioE 1 GPIO_ACTIVE_HIGH>;
+ };
+ };
+
+ spi1: spi at f8008000 {
+ cs-gpios = <&pioC 25 0>, <0>, <0>, <&pioD 16 0>;
+ status = "okay";
+ };
+
+ adc0: adc at f8018000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <
+ &pinctrl_adc0_adtrg
+ &pinctrl_adc0_ad0
+ &pinctrl_adc0_ad1
+ &pinctrl_adc0_ad2
+ &pinctrl_adc0_ad3
+ &pinctrl_adc0_ad4
+ &pinctrl_adc0_ad5
+ &pinctrl_adc0_ad6
+ &pinctrl_adc0_ad7
+ &pinctrl_adc0_ad8
+ &pinctrl_adc0_ad9
+ >;
+ status = "okay";
+ };
+
+ i2c2: i2c at f801c000 {
+ dmas = <0>, <0>; /* Do not use DMA for i2c2 */
+ status = "okay";
+ };
+
+ macb1: ethernet at f802c000 {
+ phy-mode = "rmii";
+ status = "okay";
+ };
+
+ dbgu: serial at ffffee00 {
+ status = "okay";
+ };
+
+ pinctrl at fffff200 {
+ board {
+ pinctrl_mmc0_cd: mmc0_cd {
+ atmel,pins =
+ <AT91_PIOE 0 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+ };
+
+ pinctrl_mmc1_cd: mmc1_cd {
+ atmel,pins =
+ <AT91_PIOE 1 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+ };
+
+ pinctrl_usba_vbus: usba_vbus {
+ atmel,pins =
+ <AT91_PIOE 9 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>; /* PE9, conflicts with A9 */
+ };
+ };
+ };
+
+ pmc: pmc at fffffc00 {
+ main: mainck {
+ clock-frequency = <12000000>;
+ };
+ };
+ };
+
+ nand0: nand at 60000000 {
+ nand-bus-width = <8>;
+ nand-ecc-mode = "hw";
+ atmel,has-pmecc;
+ atmel,pmecc-cap = <4>;
+ atmel,pmecc-sector-size = <512>;
+ nand-on-flash-bbt;
+ status = "okay";
+
+ at91bootstrap at 0 {
+ label = "at91bootstrap";
+ reg = <0x0 0x40000>;
+ };
+
+ bootloader at 40000 {
+ label = "bootloader";
+ reg = <0x40000 0x80000>;
+ };
+
+ bootloaderenv at c0000 {
+ label = "bootloader env";
+ reg = <0xc0000 0xc0000>;
+ };
+
+ dtb at 180000 {
+ label = "device tree";
+ reg = <0x180000 0x80000>;
+ };
+
+ kernel at 200000 {
+ label = "kernel";
+ reg = <0x200000 0x600000>;
+ };
+
+ rootfs at 800000 {
+ label = "rootfs";
+ reg = <0x800000 0x0f800000>;
+ };
+ };
+
+ usb0: gadget at 00500000 {
+ atmel,vbus-gpio = <&pioE 9 GPIO_ACTIVE_HIGH>; /* PE9, conflicts with A9 */
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usba_vbus>;
+ status = "okay";
+ };
+
+ usb1: ohci at 00600000 {
+ num-ports = <3>;
+ atmel,vbus-gpio = <0
+ &pioE 3 GPIO_ACTIVE_LOW
+ &pioE 4 GPIO_ACTIVE_LOW
+ >;
+ status = "okay";
+ };
+
+ usb2: ehci at 00700000 {
+ status = "okay";
+ };
+ };
+
+ gpio_keys {
+ compatible = "gpio-keys";
+
+ bp3 {
+ label = "PB_USER";
+ gpios = <&pioE 29 GPIO_ACTIVE_LOW>;
+ linux,code = <0x104>;
+ gpio-key,wakeup;
+ };
+ };
+
+ leds {
+ compatible = "gpio-leds";
+
+ d2 {
+ label = "d2";
+ gpios = <&pioE 23 GPIO_ACTIVE_LOW>; /* PE23, conflicts with A23, CTS2 */
+ linux,default-trigger = "heartbeat";
+ };
+
+ d3 {
+ label = "d3";
+ gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
+ };
+ };
+};
--
1.8.2.2
^ permalink raw reply related
* [PATCH 00/11] ARM: shmobile: RSPI RZ and QSPI SoC and board integration
From: Mark Brown @ 2014-02-04 17:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391527445-14881-1-git-send-email-geert@linux-m68k.org>
On Tue, Feb 04, 2014 at 04:23:54PM +0100, Geert Uytterhoeven wrote:
> Hi Simon, Magnus,
>
> This patch series contains SoC and board integration for
> 1. RSPI in the r7s72100 aka RZ/A1H SoC on the Genmai reference board,
> 2. QSPI in the r8a7791 aka R-Car M2 SoC on the Koelsch reference board.
> It was rebased on top of renesas-devel-v3.14-rc1-20140204.
Can you please stop CCing arch/arm only patch serieses like this to the
SPI list - they just mean I have to go through patchwork and mark them
as not applicable.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/cd832b70/attachment.sig>
^ permalink raw reply
* [PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS
From: Grant Likely @ 2014-02-04 17:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140117110830.GW3471@alberich>
On Fri, 17 Jan 2014 12:08:30 +0100, Andreas Herrmann <andreas.herrmann@calxeda.com> wrote:
>
> arm-smmu driver uses of_parse_phandle_with_args when parsing DT
> information to determine stream IDs for a master device.
> Thus the number of stream IDs per master device is bound by
> MAX_PHANDLE_ARGS.
>
> To support Calxeda ECX-2000 hardware arm-smmu driver requires a
> slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
> stream IDs for one master device.
>
> Increasing it to 16 seems a reasonable choice.
>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree at vger.kernel.org
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
I've merged this one, but I'm not excited about making it any larger
because this structure lives on the stack most of the time.
g.
^ permalink raw reply
* [PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS
From: Grant Likely @ 2014-02-04 17:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E93360.1000904@amd.com>
On Wed, 29 Jan 2014 10:59:12 -0600, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 1/29/2014 10:57 AM, Rob Herring wrote:
> >>> diff --git a/include/linux/of.h b/include/linux/of.h
> >>> >>index 276c546..24e1b28 100644
> >>> >>--- a/include/linux/of.h
> >>> >>+++ b/include/linux/of.h
> >>> >>@@ -67,7 +67,7 @@ struct device_node {
> >>> >> #endif
> >>> >> };
> >>> >>
> >>> >>-#define MAX_PHANDLE_ARGS 8
> >>> >>+#define MAX_PHANDLE_ARGS 16
> >> >
> >> >
> >> >Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
> >> >this be changed to be able to support 128 StreamIDs as well? Although I am
> >> >not sure if this would be too big to have on the stack per Rob's comment in
> >> >the previous patch set.
> > Do you actually need 128 now? If not, then we can deal with that when
> > we get there. There are lots of things in spec's that are not actually
> > implemented or supported.
>
> Actually, we are using 32 on the AMD system. So, do you think we can set
> this to 32 instead?
The helper really wasn't designed for large number of arguments to a
phandle. If the phandle args are really that large, then it may be
better to have a parser that allocates the space needed and/or puts the
data directly into the destination data structure.
g.
^ permalink raw reply
* [PATCH v3 3/3] arm64: audit: Add audit hook in ptrace/syscall_trace
From: Will Deacon @ 2014-02-04 17:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391410590-4884-4-git-send-email-takahiro.akashi@linaro.org>
On Mon, Feb 03, 2014 at 06:56:30AM +0000, AKASHI Takahiro wrote:
> This patch adds auditing functions on entry to or exit from
> every system call invocation.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/thread_info.h | 1 +
> arch/arm64/kernel/entry.S | 3 +++
> arch/arm64/kernel/ptrace.c | 10 ++++++++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 720e70b..7468388 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -101,6 +101,7 @@ static inline struct thread_info *current_thread_info(void)
> #define TIF_NEED_RESCHED 1
> #define TIF_NOTIFY_RESUME 2 /* callback before returning to user */
> #define TIF_SYSCALL_TRACE 8
> +#define TIF_SYSCALL_AUDIT 9
> #define TIF_POLLING_NRFLAG 16
> #define TIF_MEMDIE 18 /* is terminating due to OOM killer */
> #define TIF_FREEZE 19
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 827cbad..83c4b29 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -630,6 +630,9 @@ el0_svc_naked: // compat entry point
> get_thread_info tsk
> ldr x16, [tsk, #TI_FLAGS] // check for syscall tracing
> tbnz x16, #TIF_SYSCALL_TRACE, __sys_trace // are we tracing syscalls?
> +#ifdef CONFIG_AUDITSYSCALL
> + tbnz x16, #TIF_SYSCALL_AUDIT, __sys_trace // auditing syscalls?
> +#endif
Could we avoid the back-to-back tbnz instructions with a single mask? It's
not obvious that it will end up any better, but it would be good to know.
> adr lr, ret_fast_syscall // return address
> cmp scno, sc_nr // check upper syscall limit
> b.hs ni_sys
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 6777a21..75a3f23 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -19,6 +19,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <linux/audit.h>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <linux/mm.h>
> @@ -38,6 +39,7 @@
> #include <asm/compat.h>
> #include <asm/debug-monitors.h>
> #include <asm/pgtable.h>
> +#include <asm/syscall.h>
> #include <asm/traps.h>
> #include <asm/system_misc.h>
>
> @@ -1064,6 +1066,14 @@ asmlinkage int syscall_trace(int dir, struct pt_regs *regs)
> {
> unsigned long saved_reg;
>
> + if (dir)
> + audit_syscall_exit(regs);
> + else
> + audit_syscall_entry(syscall_get_arch(current, regs),
> + (int)regs->syscallno,
> + regs->orig_x0, regs->regs[1],
> + regs->regs[2], regs->regs[3]);
> +
Do we really want to perform the audit checks before the tracehook calls?
Remember that the latter can rewrite all of the registers.
Will
^ permalink raw reply
* [PATCH v4] of: add functions to count number of elements in a property
From: Grant Likely @ 2014-02-04 17:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL_JsqJord1OOjwcKk1jqF5o+XO3uw+Lc4ATtQacgwZWAoQq1g@mail.gmail.com>
On Sat, 18 Jan 2014 09:07:30 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On Sat, Jan 18, 2014 at 6:02 AM, Heiko St??bner <heiko@sntech.de> wrote:
> > The need to know the number of array elements in a property is
> > a common pattern. To prevent duplication of open-coded implementations
> > add a helper static function that also centralises strict sanity
> > checking and DTB format details, as well as a set of wrapper functions
> > for u8, u16, u32 and u64.
> >
> > Suggested-by: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
>
> Looks good. Do you plan to convert some users to use this?
I'll take that as an acked-by. Merged, thanks.
g.
^ permalink raw reply
* [PATCH v3 2/3] arm64: Add audit support
From: Will Deacon @ 2014-02-04 17:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391410590-4884-3-git-send-email-takahiro.akashi@linaro.org>
On Mon, Feb 03, 2014 at 06:56:29AM +0000, AKASHI Takahiro wrote:
> On AArch64, audit is supported through generic lib/audit.c and
> compat_audit.c, and so this patch adds arch specific definitions required.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/syscall.h | 15 +++++++++++++++
> include/uapi/linux/audit.h | 1 +
> 3 files changed, 17 insertions(+)
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* [PATCH v3 1/3] arm64: Add regs_return_value() in syscall.h
From: Will Deacon @ 2014-02-04 17:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391410590-4884-2-git-send-email-takahiro.akashi@linaro.org>
On Mon, Feb 03, 2014 at 06:56:28AM +0000, AKASHI Takahiro wrote:
> This macro, regs_return_value, is used mainly for audit to record system
> call's results, but may also be used in test_kprobes.c.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* [PATCH 4/6] regulator: add bcm59056 regulator driver
From: Mark Brown @ 2014-02-04 17:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391516352-32359-5-git-send-email-mporter@linaro.org>
On Tue, Feb 04, 2014 at 07:19:10AM -0500, Matt Porter wrote:
> +static unsigned int bcm59056_get_mode(struct regulator_dev *dev)
> +{
> + return REGULATOR_MODE_NORMAL;
> +}
> +
> +static int bcm59056_set_mode(struct regulator_dev *dev, unsigned int mode)
> +{
> + if (mode == REGULATOR_MODE_NORMAL)
> + return 0;
> + else
> + return -EINVAL;
> +}
These do nothing, don't implement them.
> + if (bcm59056->dev->of_node)
> + pmu_data = bcm59056_parse_dt_reg_data(pdev,
> + &bcm59056_reg_matches);
It'd seem a bit neater to put the OF check into the parse function but
meh.
> + if (!pmu_data) {
> + dev_err(&pdev->dev, "Platform data not found\n");
> + return -EINVAL;
> + }
Like I said when reviewing the binding this should not cause the driver
to fail to load.
> + /*
> + * Regulator API handles empty constraints but not NULL
> + * constraints
> + */
> + if (!reg_data)
> + continue;
It should do... if not then make it so since that'd mean most drivers
are buggy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/25b903bb/attachment.sig>
^ permalink raw reply
* [PATCH resend 1/2] arm64: defer reloading a task's FPSIMD state to userland resume
From: Will Deacon @ 2014-02-04 17:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu_cWueDaYsgBOUUQAT3ubyrKZtHc5DogvSS-=TpOrX+aQ@mail.gmail.com>
Hello,
On Tue, Feb 04, 2014 at 02:49:14PM +0000, Ard Biesheuvel wrote:
> On 3 February 2014 17:36, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jan 31, 2014 at 10:13:15AM +0000, Ard Biesheuvel wrote:
> >> If a task gets scheduled out and back in again and nothing has touched
> >> its FPSIMD state in the mean time, there is really no reason to reload
> >> it from memory. Similarly, repeated calls to kernel_neon_begin() and
> >> kernel_neon_end() will preserve and restore the FPSIMD state every time.
> >>
> >> This patch defers the FPSIMD state restore to the last possible moment,
> >> i.e., right before the task re-enters userland. If a task does not enter
> >> userland at all (for any reason), the existing FPSIMD state is preserved
> >> and may be reused by the owning task if it gets scheduled in again on the
> >> same CPU.
> >
> > The one situation I'm unsure of here is how you deal with the saved fpsimd
> > state potentially being updated by a signal handler or a debugger. In this
> > case, we probably need to set _TIF_FOREIGN_FPSTATE to force a reload, or are
> > you handling this some other way?
> >
>
> If I am reading the code correctly, the signal handler is entered
> using the normal userland resume path, so I don't think it requires
> special treatment.
It was the exiting of the signal handler that I was worried about, where it
may have modified the interrupted programs fpsimd state on the stack.
> For the ptrace() case, it should suffice to set the 'last_cpu' field
> to (u32)-1 to indicate that the FPSIMD context should be reloaded from
> memory regardless of which CPU the debuggee is restarted on.
Something like that sounds right, but it needs adding/testing.
Will
^ permalink raw reply
* [PATCH 2/6] regulator: add bcm59056 pmu DT binding
From: Mark Brown @ 2014-02-04 17:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391516352-32359-3-git-send-email-mporter@linaro.org>
On Tue, Feb 04, 2014 at 07:19:08AM -0500, Matt Porter wrote:
> Add a DT binding for the BCM59056 PMU. The binding inherits from
> the generic regulator bindings.
Is this really only a regulator - does the chip have no other functions?
> +- regulators: This is the list of child nodes that specify the regulator
> + initialization data for defined regulators. Generic regulator bindings
> + are described in regulator/regulator.txt.
The regulators property should always be optional, the driver should be
able to start up and read back the hardware state without any further
configuration.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140204/0b525720/attachment-0001.sig>
^ permalink raw reply
* [PATCH 1/2] arm64: atomics: fix use of acquire + release for full barrier semantics
From: Will Deacon @ 2014-02-04 17:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140204164308.GA5002@laptop.programming.kicks-ass.net>
Hi Peter,
On Tue, Feb 04, 2014 at 04:43:08PM +0000, Peter Zijlstra wrote:
> On Tue, Feb 04, 2014 at 12:29:12PM +0000, Will Deacon wrote:
> > @@ -112,17 +114,20 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new)
> > unsigned long tmp;
> > int oldval;
> >
> > + smp_mb();
> > +
> > asm volatile("// atomic_cmpxchg\n"
> > -"1: ldaxr %w1, %2\n"
> > +"1: ldxr %w1, %2\n"
> > " cmp %w1, %w3\n"
> > " b.ne 2f\n"
> > -" stlxr %w0, %w4, %2\n"
> > +" stxr %w0, %w4, %2\n"
> > " cbnz %w0, 1b\n"
> > "2:"
> > : "=&r" (tmp), "=&r" (oldval), "+Q" (ptr->counter)
> > : "Ir" (old), "r" (new)
> > : "cc", "memory");
> >
> > + smp_mb();
> > return oldval;
> > }
> >
>
> Any particular reason atomic_cmpxchg() doesn't use the proposed rel + mb
> scheme? It would be a waste to have atomic_cmpxchg() be more expensive
> than it needs to be.
Catalin mentioned this to me in the past, and I got hung up on providing full
barrier semantics in the case that the comparison fails and we don't perform
the release.
If we make your change,
ldxr
cmp
b.ne
stlxr
cbnz
dmb ish
which is basically just removing the first smp_mb(), then we allow the load
component of a failing cmpxchg to be speculated, which would affect code
doing:
/* Spin waiting for some flag to clear */
while (atomic_read(&flag));
/* Now do the cmpxchg, which will fail and return the old value */
return cmpxchg(...);
The cmpxchg could read old data and fail the comparison, because it
speculated the ldxr before the reading of flag.
Is that a problem?
Will
^ permalink raw reply
* [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
From: Jean-Francois Moine @ 2014-02-04 17:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140204133014.GA22609@sirena.org.uk>
On Tue, 4 Feb 2014 13:30:14 +0000
Mark Brown <broonie@kernel.org> wrote:
> On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
>
> > + /* load the optional CODEC */
> > + of_platform_populate(np, NULL, NULL, &client->dev);
> > +
>
> Why is this using of_platform_populate()? That's a very odd way of
> doing things.
The i2c does not populate the subnodes in the DT. I did not find why,
but, what is sure is that if of_platform_populate() is not called, the
tda CODEC module is not loaded.
You may find an other example in drivers/mfd/twl-core.c.
> > +config SND_SOC_TDA998X
> > + tristate
> > + depends on OF
> > + default y if DRM_I2C_NXP_TDA998X=y
> > + default m if DRM_I2C_NXP_TDA998X=m
> > +
>
> Make this visible if it can be selected from DT so it can be used with
> generic cards.
I don't understand. The tda CODEC can only be used with the TDA998x I2C
driver. It might have been included in the tda998x source as well.
> > +static int tda_get_encoder(struct tda_priv *priv)
> > +{
> > + struct snd_soc_codec *codec = priv->codec;
> > + struct device_node *np;
> > +
> > + /* get the parent tda998x device */
> > + np = of_get_parent(codec->dev->of_node);
> > + if (!np || !of_device_is_compatible(np, "nxp,tda998x")) {
> > + dev_err(codec->dev, "no or bad parent!\n");
> > + return -EINVAL;
> > + }
> > + priv->i2c_client = of_find_i2c_device_by_node(np);
> > + of_node_put(np);
> > + return 0;
> > +}
>
> Why does this need to be checked like this? We don't normally have this
> sort of code to check that the parent is correct.
In my previous submit, the tda CODEC was not declared inside the
tda998x I2c device, so, its location was searched from phandle.
Now, the CODEC is declared inside the tda998x as a node child. But, in
a bad DT, the tda CODEC could be declared anywhere, even inside a other
DRM I2C slave encoder, in which case, bad things would happen...
> > +static int tda_start_stop(struct tda_priv *priv)
> > +{
> > + int port;
> > +
> > + /* give the audio parameters to the HDMI encoder */
> > + if (priv->dai_id == AFMT_I2S)
> > + port = priv->ports[0];
> > + else
> > + port = priv->ports[1];
> > + tda998x_audio_update(priv->i2c_client, priv->dai_id, port);
> > + return 0;
> > +}
>
> What does this actually do? No information is being passed in to the
> core function here, not even any information on if it's starting or
> stopping. Looking at the rest of the code I can't help thinking it
> might be clearer to inline this possibly with a lookup helper, the code
> is very small and the lack of parameters makes it hard to follow.
I thought it was simple enough. The function tda_start_stop() is called
from 2 places:
- on audio start in tda_startup with the audio type (DAI id)
priv->dai_id = dai->id;
- on audio stop with a null audio type
priv->dai_id = 0; /* streaming stop */
On stream start, the DAI id is never null, as explained in the patch 1:
The audio format values in the encoder configuration interface are
changed to non null values so that the value 0 is used in the audio
function to indicate that audio streaming is stopped.
and on streaming stop the port is not meaningful.
I will add a null item in the enum (AFMT_NO_AUDIO).
> > +static const struct snd_soc_dapm_route tda_routes[] = {
> > + { "hdmi-out", NULL, "HDMI I2S Playback" },
> > + { "hdmi-out", NULL, "HDMI SPDIF Playback" },
> > +};
>
> S/PDIF.
Did you ever try that with debugfs?
BTW, this patch series may be delayed for some time: the tda998x driver
has to be reworked for DT support.
--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox