* [PATCH V4 5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver
From: Christopher Covington @ 2014-01-24 17:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124113920.GF814@e106331-lin.cambridge.arm.com>
On 01/24/2014 06:39 AM, Mark Rutland wrote:
> On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> Add documentation for generic SYSCON reboot driver.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>> .../bindings/power/reset/syscon-reboot.txt | 16 ++++++++++++++++
>> 1 files changed, 16 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> new file mode 100644
>> index 0000000..e9eb1fe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -0,0 +1,16 @@
>> +Generic SYSCON mapped register reset driver
>
> Bindings should describe hardware, not drivers.
How is this different than what's done for PSCI?
Thanks,
Christopher
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
^ permalink raw reply
* [PATCH RFC v2 2/2] Documentation: arm: define DT C-states bindings
From: Lorenzo Pieralisi @ 2014-01-24 17:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKfTPtDHTs67qvoA6++zo4SZdZJj4T_BV2gVECOu-76-2kLbuw@mail.gmail.com>
Hi Vincent,
On Fri, Jan 24, 2014 at 08:40:40AM +0000, Vincent Guittot wrote:
[...]
> Hi Lorenzo,
>
> Sorry for the late reply,
>
>
> > I had an idea. To simplify things, I think that one possibility is to
> > add a parameter to the power domain specifier (platform specific, see
> > Tomasz bindings):
>
> We can't use a simple boolean state (on/off) for defining the
> powerdomain state associated to a c-state so your proposal of being
> able to add a parameter that will define the power domain state is
> interesting.
>
> >
> > Documentation/devicetree/bindings/power/power_domain.txt
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224928.html
> >
> > to represent, when that state is entered the behavior of the power
> > controller (ie cache RAM retention or cache shutdown or in general any
> > substate within a power domain). Since it is platform specific, and since
> > we are able to link caches to the power domain, the power controller will
> > actually define what happens to the cache when that state is entered
> > (basically we use the power domain specifier additional parameter to define
> > a "substate" in that power domain e.g.:
> >
> > Example:
> >
> > foo_power_controller {
> > [...]
> > /*
> > * first cell is register index, second one is the state index
> > * that in turn implies the state behavior - eg cache lost or
> > * retained
> > */
> > #power-domain-cells = <2>;
> > };
> >
> > l1-cache {
> > [...]
> > /*
> > * syntax: power-domains = list of power domain specifiers
> > <[&power_domain_phandle register-index state],[&power_domain_phandle register-index state]>;
> > The syntax is defined by the power controller du jour
> > as described by Tomasz bindings
> > */
> > power-domains =<&foo_power_controller 0 0 &foo_power_controller 0 1>;
>
> Normally, power-domains describes a list of power domain specifiers
> that are necessary for the l1-cache to at least retain its state so
> i'm not sure understand your example above
>
> If we take the example of system that support running, retention and
> powerdown state described as state 0, 1 and 2 for the power domain, i
> would have set the l1-cache like:
> power-domains =<&foo_power_controller 0 1>;
>
> for saying that the state is retained up to state 1
>
> Please look below, i have modified the rest of your example accordingly
>
> >
> > }:
> >
> > and then
> >
> > state0 {
> > index = <2>;
> > compatible = "arm,cpu-power-state";
> > latency = <...>;
> > /*
> > * This means that when the state is entered, the power
> > * controller should use register index 0 and state 0,
> > * whose meaning is power controller specific. Since we
> > * know all components affected (for every component
> > * we declare its power domain(s) and states so we
> > * know what components are affected by the state entry.
> > * Given the cache node above and this phandle, the state
> > * implies that the cache is retained, register index == 0 state == 0
> > /*
> > power-domain =<&foo_power_controller 0 0>;
>
> for retention state we need to set the power domain in state 1
> power-domain =<&foo_power_controller 0 1>;
>
> > };
> >
> > state1 {
> > index = <3>;
> > compatible = "arm,cpu-power-state";
> > latency = <...>;
> > /*
> > * This means that when the state is entered, the power
> > * controller should use register index 0 and state 1,
> > * whose meaning is power controller specific. Since we
> > * know all components affected (for every component
> > * we declare its power domain(s) and states so we
> > * know what components are affected by the state entry.
> > * Given the cache node above and this phandle, the state
> > * implies that the cache is lost, register index == 0 state == 1
> > /*
> > power-domain =<&foo_power_controller 0 1>;
>
> for power down mode, we need to set thge power domain in state 2
> power-domain =<&foo_power_controller 0 2>;
Ok, what I meant was not what you got, but your approach looks sensible
too. What I do not like is that the power-domain specifier is power
controller specific (that was true even for my example). In theory
we can achieve something identical by forcing every component in a power
domain to specify the max C-state index that allows it to retain its
state (through a specific property). Same logic to your example applies.
Nice thing is that we do not change the power domain specifiers, bad thing
is that it adds two properties to each device (c-state index and
power-domain-specifier - but we can make it hierarchical so that device
nodes can inherit the maximum operating C-state by inheriting the value
from a parent node providing a common value).
In my example the third parameter was just a number that the power
controller would decode (eg 0 = cache retained, 1 = cache lost)
according to its implementation, it was not a "state index". The
power controller would know what to do with eg a cache component (that
declares to be in that power domain) when a C-state with that power
domain specifier was entered.
Not very different from what you are saying, let's get to the nub:
- Either we define it in a platform specific way through the power
domain specifier
- Or we force a max-c-state-supported property for every device,
possibly hierarchical
Thoughts ?
Thank you !
Lorenzo
^ permalink raw reply
* [PATCH V4 5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver
From: Feng Kan @ 2014-01-24 18:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124113920.GF814@e106331-lin.cambridge.arm.com>
On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> Add documentation for generic SYSCON reboot driver.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>> .../bindings/power/reset/syscon-reboot.txt | 16 ++++++++++++++++
>> 1 files changed, 16 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> new file mode 100644
>> index 0000000..e9eb1fe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -0,0 +1,16 @@
>> +Generic SYSCON mapped register reset driver
>
> Bindings should describe hardware, not drivers.
>
> What precisely does this binding describe?
>
>> +
>> +Required properties:
>> +- compatible: should contain "syscon-reboot"
>> +- regmap: this is phandle to the register map node
>> +- offset: offset in the register map for the reboot register
>> +- mask: the reset value written to the reboot register
>> +
>> +Examples:
>> +
>> +reboot {
>> + compatible = "syscon-reboot";
>> + regmap = <®mapnode>;
>> + offset = <0x0>;
>> + mask = <0x1>;
>> +};
>
> Access size? Endianness?
FKAN: are you asking for documentation? I don't see alot of example of
support for these.
>
> Why can we not have a binding for the register bank this exists in, and
> have that pass on the appropriate details to a syscon-reboot driver?
FKAN: Thats a good idea. But the hardware in this case (SCU) system
clock unit has a bunch of registers used for different functions. If syscon is
used alot in this case and we pile more attribute into it. It would get kinda
messy after a while.
FKAN: I still haven't figured out how to generically tie to
the reset handler? Maybe the next person can use #define to bridge in the
reset handler they want to use.
>
> That way we can change the way we poke things without requiring changes
> to bindings or dts.
>
> Thanks,
> Mark.
^ permalink raw reply
* [PATCH V4 5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver
From: Mark Rutland @ 2014-01-24 18:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E2A901.4050707@codeaurora.org>
On Fri, Jan 24, 2014 at 05:55:13PM +0000, Christopher Covington wrote:
> On 01/24/2014 06:39 AM, Mark Rutland wrote:
> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
> >> Add documentation for generic SYSCON reboot driver.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >> .../bindings/power/reset/syscon-reboot.txt | 16 ++++++++++++++++
> >> 1 files changed, 16 insertions(+), 0 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> new file mode 100644
> >> index 0000000..e9eb1fe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> @@ -0,0 +1,16 @@
> >> +Generic SYSCON mapped register reset driver
> >
> > Bindings should describe hardware, not drivers.
>
> How is this different than what's done for PSCI?
A PSCI node in the DT defines a standard interface to a firmware which
is external to Linux. The PSCI binding does not contain the word
"driver", but describes the interface that the binding describes, with
reference to approriate documentation.
All I'm arguing for here is a description of the class of hardware this
is applicable to, rather than "this is what this particular driver
uses".
Thanks,
Mark.
^ permalink raw reply
* [PATCH V4 5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver
From: Mark Rutland @ 2014-01-24 18:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL85gmDJeUY0zUjkmpMo7g_hLj34RsgU1VEF80wouLzOFGAdqQ@mail.gmail.com>
On Fri, Jan 24, 2014 at 06:03:10PM +0000, Feng Kan wrote:
> On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
> >> Add documentation for generic SYSCON reboot driver.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >> .../bindings/power/reset/syscon-reboot.txt | 16 ++++++++++++++++
> >> 1 files changed, 16 insertions(+), 0 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> new file mode 100644
> >> index 0000000..e9eb1fe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> @@ -0,0 +1,16 @@
> >> +Generic SYSCON mapped register reset driver
> >
> > Bindings should describe hardware, not drivers.
> >
> > What precisely does this binding describe?
> >
> >> +
> >> +Required properties:
> >> +- compatible: should contain "syscon-reboot"
> >> +- regmap: this is phandle to the register map node
> >> +- offset: offset in the register map for the reboot register
> >> +- mask: the reset value written to the reboot register
> >> +
> >> +Examples:
> >> +
> >> +reboot {
> >> + compatible = "syscon-reboot";
> >> + regmap = <®mapnode>;
> >> + offset = <0x0>;
> >> + mask = <0x1>;
> >> +};
> >
> > Access size? Endianness?
> FKAN: are you asking for documentation? I don't see alot of example of
> support for these.
If I used the enippet in the example, what endianness and access size
should I expect an OS to perform? That should be documented.
If this doesn't match the general case, we can add properties later to
adjust the access size and/or endianness. We just need to document what
the binding actually describes currently, or it's not possible to
implement anything based off of the binding documentation.
I should be able to read a binding document and write a dts. I shouldn't
have to read the code to figure out what the binding describes.
>
> >
> > Why can we not have a binding for the register bank this exists in, and
> > have that pass on the appropriate details to a syscon-reboot driver?
>
> FKAN: Thats a good idea. But the hardware in this case (SCU) system
> clock unit has a bunch of registers used for different functions. If syscon is
> used alot in this case and we pile more attribute into it. It would get kinda
> messy after a while.
Huh?
What's wrong with having a system clock unit binding, that the kernel
can decompose as appropriate?
I don't get your syscon argument.
Thanks,
Mark.
^ permalink raw reply
* [PATCH V4 5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver
From: Feng Kan @ 2014-01-24 18:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124182355.GF4758@e106331-lin.cambridge.arm.com>
On Fri, Jan 24, 2014 at 10:23 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 24, 2014 at 06:03:10PM +0000, Feng Kan wrote:
>> On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> >> Add documentation for generic SYSCON reboot driver.
>> >>
>> >> Signed-off-by: Feng Kan <fkan@apm.com>
>> >> ---
>> >> .../bindings/power/reset/syscon-reboot.txt | 16 ++++++++++++++++
>> >> 1 files changed, 16 insertions(+), 0 deletions(-)
>> >> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >> new file mode 100644
>> >> index 0000000..e9eb1fe
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >> @@ -0,0 +1,16 @@
>> >> +Generic SYSCON mapped register reset driver
>> >
>> > Bindings should describe hardware, not drivers.
>> >
>> > What precisely does this binding describe?
>> >
>> >> +
>> >> +Required properties:
>> >> +- compatible: should contain "syscon-reboot"
>> >> +- regmap: this is phandle to the register map node
>> >> +- offset: offset in the register map for the reboot register
>> >> +- mask: the reset value written to the reboot register
>> >> +
>> >> +Examples:
>> >> +
>> >> +reboot {
>> >> + compatible = "syscon-reboot";
>> >> + regmap = <®mapnode>;
>> >> + offset = <0x0>;
>> >> + mask = <0x1>;
>> >> +};
>> >
>> > Access size? Endianness?
>> FKAN: are you asking for documentation? I don't see alot of example of
>> support for these.
>
> If I used the enippet in the example, what endianness and access size
> should I expect an OS to perform? That should be documented.
>
> If this doesn't match the general case, we can add properties later to
> adjust the access size and/or endianness. We just need to document what
> the binding actually describes currently, or it's not possible to
> implement anything based off of the binding documentation.
>
> I should be able to read a binding document and write a dts. I shouldn't
> have to read the code to figure out what the binding describes.
>
>>
>> >
>> > Why can we not have a binding for the register bank this exists in, and
>> > have that pass on the appropriate details to a syscon-reboot driver?
>>
>> FKAN: Thats a good idea. But the hardware in this case (SCU) system
>> clock unit has a bunch of registers used for different functions. If syscon is
>> used alot in this case and we pile more attribute into it. It would get kinda
>> messy after a while.
>
> Huh?
>
> What's wrong with having a system clock unit binding, that the kernel
> can decompose as appropriate?
>
> I don't get your syscon argument.
FKAN: I do have a SCU binding, I thought you wanted to move the offset and
mask to the SCU binding. The only issue I see in that case is when we use
more such methods, the SCU binding would look rather crowded. If this is not
the case, I am a bit confused at what I should do next.
>
> Thanks,
> Mark.
^ permalink raw reply
* [RESEND PATCH] ARM: dts: bcm28155-ap: Fix Card Detection GPIO
From: Tim Kryger @ 2014-01-24 18:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAEPRUYpyr2_3pfa3rqYiYPAOwJfA+0AdqyGaf5s7YiWH=zfTeA@mail.gmail.com>
On Wed, Jan 8, 2014 at 4:54 PM, Christian Daudt <bcm@fixthebug.org> wrote:
> On Wed, Jan 8, 2014 at 4:28 PM, Tim Kryger <tim.kryger@linaro.org> wrote:
>> On Wed, Jan 8, 2014 at 3:38 PM, Christian Daudt <bcm@fixthebug.org> wrote:
>>> On Tue, Jan 7, 2014 at 10:53 AM, Tim Kryger <tim.kryger@linaro.org> wrote:
>>>> The board schematic states that the "SD_CARD_DET_N gets pulled to GND
>>>> when card is inserted" so the polarity has been updated to active low.
>>>>
>>>> Polarity is now specified with a GPIO define instead of a magic number.
>>>>
>>>> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
>>>> Reviewed-by: Matt Porter <matt.porter@linaro.org>
>>>> ---
>>>> arch/arm/boot/dts/bcm28155-ap.dts | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm28155-ap.dts b/arch/arm/boot/dts/bcm28155-ap.dts
>>>> index 08e47c2..27dd110 100644
>>>> --- a/arch/arm/boot/dts/bcm28155-ap.dts
>>>> +++ b/arch/arm/boot/dts/bcm28155-ap.dts
>>>> @@ -13,6 +13,8 @@
>>>>
>>>> /dts-v1/;
>>>>
>>>> +#include <dt-bindings/gpio/gpio.h>
>>>> +
>>>> #include "bcm11351.dtsi"
>>>>
>>>> / {
>>>> @@ -40,7 +42,7 @@
>>>>
>>>> sdio4: sdio at 3f1b0000 {
>>>> max-frequency = <48000000>;
>>>> - cd-gpios = <&gpio 14 0>;
>>>> + cd-gpios = <&gpio 14 GPIO_ACTIVE_LOW>;
>>>> status = "okay";
>>>> };
>>>> };
>>>> --
>>>> 1.8.0.1
>>>>
>>> Tim,
>>> Does bcm11351-brt not also suffer from the same bug? If it does can
>>> you pls update the patch to also fix it?
>>>
>>> Thanks,
>>> csd
>>
>> The BRT and AP boards are similar so it may have the same problem but
>> I don't have a BRT and wouldn't be able to test any changes to its DTS
>> file.
>>
>> -Tim
>
> [sorry for the resend for those that get it twice]
> Agreed - it's time that dts file go away. In this case:
> Acked-by: Christian Daudt <bcm@fixthebug.org>
>
> Olof - can you pls pull in this patch. This is the bugfix that was
> discussed in irc earlier today.
>
> thanks,
> csd
Christian,
I'm not sure Olof saw your reply. Olof and Kevin are now on CC.
Thanks,
Tim Kryger
^ permalink raw reply
* [PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS
From: Christopher Covington @ 2014-01-24 19:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E2A866.1050400@codeaurora.org>
On 01/24/2014 12:52 PM, Christopher Covington wrote:
> On 01/24/2014 12:17 PM, Will Deacon wrote:
>> On Fri, Jan 24, 2014 at 05:16:28PM +0000, Adrien Verg? wrote:
>>> 2014/1/24 Will Deacon <will.deacon@arm.com>:
>>>> Are you sure about this? The value we write is actually task_pid_nr, which I
>>>> believe to be globally unique.
>>>
>>> You are right: the task_pid_nr is unique in the system. However when
>>> using namespaces, the so called "PID" is the virtual number that
>>> processes in different namespaces can share.
>>>
>>> This PID is the one visible by user-space tasks, in particular
>>> user-space tracers and debuggers. These programs would expect to find
>>> the PID of the traced process in the Context ID reg, while it is not.
>>> I think it is better to remove confusion by making PID_IN_CONTEXTIDR
>>> and PID_NS incompatible.
>>>
>>> What do you think?
>>
>> I think I'd rather have the global ID than disable a potentially useful
>> feature, especially since this is likely to be consumed by external trace
>> tools as opposed to user-space tasks.
>
> We've discussed before that the ARM architecture doesn't say what should be
> written to the CONTEXTIDR, so it's up to us to decide. Will has a use case
> where the global PID is useful. Adrien's patches present a use case where I
> think the virtual PID would be useful. I've done work in the past where
> writing the process group ID was useful. Would it be reasonable to make what's
> written to the CONTEXTIDR run-time configurable? If so, what would be the best
> interface for configuring it?
D'oh, I mixed things up. For ETM to work it can only use global PID's in the
CONTEXTIDR.
Christopher
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
^ permalink raw reply
* [PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS
From: Adrien Vergé @ 2014-01-24 19:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E2A866.1050400@codeaurora.org>
2014/1/24 Will Deacon <will.deacon@arm.com>:
> I think I'd rather have the global ID than disable a potentially useful
> feature, especially since this is likely to be consumed by external trace
> tools as opposed to user-space tasks.
I understand.
2014/1/24 Christopher Covington <cov@codeaurora.org>:
> Would it be reasonable to make what's
> written to the CONTEXTIDR run-time configurable? If so, what would be the best
> interface for configuring it?
This is an interesting option.
An other option would be to keep the global PID in the Context ID
register, and rely on kernel support to translate virtual PID to
global PID when needed. Then, it would be possible to select a task to
trace via its PID, by asking the kernel to write its global ID to the
Context ID comparator.
^ permalink raw reply
* [PATCH] ARM: dts: imx28-m28cu3: Remove 'reset-active-high'
From: Fabio Estevam @ 2014-01-24 20:45 UTC (permalink / raw)
To: linux-arm-kernel
From: Fabio Estevam <fabio.estevam@freescale.com>
The 'reset-active-high' property is not defined anywhere, so just remove it.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
arch/arm/boot/dts/imx28-m28cu3.dts | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx28-m28cu3.dts b/arch/arm/boot/dts/imx28-m28cu3.dts
index d3958da..4174bb1 100644
--- a/arch/arm/boot/dts/imx28-m28cu3.dts
+++ b/arch/arm/boot/dts/imx28-m28cu3.dts
@@ -116,7 +116,6 @@
pinctrl-0 = <&lcdif_24bit_pins_a
&lcdif_pins_m28>;
display = <&display>;
- reset-active-high;
status = "okay";
display: display0 {
--
1.8.1.2
^ permalink raw reply related
* [PATCH V2 4/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to system cfg driver
From: Arnd Bergmann @ 2014-01-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124042959.GC2369@pratyush-vbox>
On Friday 24 January 2014, Pratyush Anand wrote:
> On Thu, Jan 23, 2014 at 08:22:54PM +0800, Arnd Bergmann wrote:
> > On Thursday 23 January 2014, Mohit Kumar wrote:
> >
> > I assume you'd want a phandle pointing to the syscon device in here
> > as well?
>
> Since there is only one syscon device in the whole DT, so do I really
> need to add phandle? Currently I am using
> syscon_regmap_lookup_by_compatible to find syscon device.
I'd much rather use syscon_regmap_lookup_by_phandle than
syscon_regmap_lookup_by_compatible, all the time, since this makes
the relationship between the devices explicit.
The phandle method also allows you to pass regmap indexes in the
same property, which can be handy if two variants of the chip have
the same registers at a different offset.
> > > +/* SPEAr1340 Registers */
> > > +/* Power Management Registers */
> > > +#define SPEAR1340_PCM_CFG 0x100
> > > + #define SPEAR1340_PCM_CFG_SATA_POWER_EN 0x800
> > > +#define SPEAR1340_PCM_WKUP_CFG 0x104
> > > +#define SPEAR1340_SWITCH_CTR 0x108
> > > +
> > > +#define SPEAR1340_PERIP1_SW_RST 0x318
> > > + #define SPEAR1340_PERIP1_SW_RST_SATA 0x1000
> > > +#define SPEAR1340_PERIP2_SW_RST 0x31C
> > > +#define SPEAR1340_PERIP3_SW_RST 0x320
> > > +
> > > +/* PCIE - SATA configuration registers */
> > > +#define SPEAR1340_PCIE_SATA_CFG 0x424
> > > + /* PCIE CFG MASks */
> > > + #define SPEAR1340_PCIE_CFG_DEVICE_PRESENT (1 << 11)
> > > + #define SPEAR1340_PCIE_CFG_POWERUP_RESET (1 << 10)
> > > + #define SPEAR1340_PCIE_CFG_CORE_CLK_EN (1 << 9)
> > > + #define SPEAR1340_PCIE_CFG_AUX_CLK_EN (1 << 8)
> > > + #define SPEAR1340_SATA_CFG_TX_CLK_EN (1 << 4)
> > > + #define SPEAR1340_SATA_CFG_RX_CLK_EN (1 << 3)
> > > + #define SPEAR1340_SATA_CFG_POWERUP_RESET (1 << 2)
> > > + #define SPEAR1340_SATA_CFG_PM_CLK_EN (1 << 1)
> > > + #define SPEAR1340_PCIE_SATA_SEL_PCIE (0)
> > > + #define SPEAR1340_PCIE_SATA_SEL_SATA (1)
> > > + #define SPEAR1340_PCIE_SATA_CFG_MASK 0xF1F
> > > + #define SPEAR1340_PCIE_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_PCIE | \
> > > + SPEAR1340_PCIE_CFG_AUX_CLK_EN | \
> > > + SPEAR1340_PCIE_CFG_CORE_CLK_EN | \
> > > + SPEAR1340_PCIE_CFG_POWERUP_RESET | \
> > > + SPEAR1340_PCIE_CFG_DEVICE_PRESENT)
> > > + #define SPEAR1340_SATA_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_SATA | \
> > > + SPEAR1340_SATA_CFG_PM_CLK_EN | \
> > > + SPEAR1340_SATA_CFG_POWERUP_RESET | \
> > > + SPEAR1340_SATA_CFG_RX_CLK_EN | \
> > > + SPEAR1340_SATA_CFG_TX_CLK_EN)
> > > +
> > > +#define SPEAR1340_PCIE_MIPHY_CFG 0x428
> > > + #define SPEAR1340_MIPHY_OSC_BYPASS_EXT (1 << 31)
> > > + #define SPEAR1340_MIPHY_CLK_REF_DIV2 (1 << 27)
> > > + #define SPEAR1340_MIPHY_CLK_REF_DIV4 (2 << 27)
> > > + #define SPEAR1340_MIPHY_CLK_REF_DIV8 (3 << 27)
> > > + #define SPEAR1340_MIPHY_PLL_RATIO_TOP(x) (x << 0)
> > > + #define SPEAR1340_PCIE_MIPHY_CFG_MASK 0xF80000FF
> > > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA \
> > > + (SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
> > > + SPEAR1340_MIPHY_CLK_REF_DIV2 | \
> > > + SPEAR1340_MIPHY_PLL_RATIO_TOP(60))
> > > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK \
> > > + (SPEAR1340_MIPHY_PLL_RATIO_TOP(120))
> > > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_PCIE \
> > > + (SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
> > > + SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
> > > +
> > > +struct spear13xx_cfg_priv {
> > > + struct regmap *misc;
> > > +};
> > > +
> > > +/* SATA device registration */
> > > +static void spear1340_sata_miphy_init(struct spear13xx_cfg_priv *cfgpriv)
> > > +{
> > > + regmap_update_bits(cfgpriv->misc, SPEAR1340_PCIE_SATA_CFG,
> > > + SPEAR1340_PCIE_SATA_CFG_MASK, SPEAR1340_SATA_CFG_VAL);
> > > + regmap_update_bits(cfgpriv->misc, SPEAR1340_PCIE_MIPHY_CFG,
> > > + SPEAR1340_PCIE_MIPHY_CFG_MASK,
> > > + SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK);
> > > + /* Switch on sata power domain */
> > > + regmap_update_bits(cfgpriv->misc, SPEAR1340_PCM_CFG,
> > > + SPEAR1340_PCM_CFG_SATA_POWER_EN,
> > > + SPEAR1340_PCM_CFG_SATA_POWER_EN);
> > > + msleep(20);
> > > + /* Disable PCIE SATA Controller reset */
> > > + regmap_update_bits(cfgpriv->misc, SPEAR1340_PERIP1_SW_RST,
> > > + SPEAR1340_PERIP1_SW_RST_SATA, 0);
> > > + msleep(20);
> > > +}
> >
> > Looking at the actual code now, this very much looks like it ought to
> > be a "phy" driver and get put in drivers/phy/.
>
> Actually these registers are part of common system configurations
> register space (called as misc space) for SPEAr SOC. So we opted for
> syscon framework.
The use of syscon for this is good, I have no objection to that, and
was suggesting that you create a logical "phy" device that uses the
misc syscon device as a backend.
> PHY registers space starts from 0xEB800000, which can be
> programmed for various phy specific functions like power management,
> tx/rx settings, comparator settings etc. In most of the cases phy
> works with default settings, however there are few exceptions for
> which we will be adding a phy driver for further improvement of SPEAr
> drivers.
I see. So while the code you have here could be expressed as a phy driver
by itself, there is another part of the SoC that controls the actual
phy. How about if you add the phy device node to DT, and write a driver
that doesn't actually program the phy registers for now, but does contain
the code that you have posted here. That would give you flexibility for
future extensions and at the same time let you remove all SPEAr specific
code from the actual AHCI driver by using the generic ahci-platform
driver.
Arnd
^ permalink raw reply
* [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
From: Arnd Bergmann @ 2014-01-24 20:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52E287EE.70407@linaro.org>
On Friday 24 January 2014, Hanjun Guo wrote:
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index e108d9c..c335c6d 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot);
> >> extern int (*acpi_suspend_lowlevel)(void);
> >> #define acpi_wakeup_address (0)
> >>
> >> +#define MAX_GIC_CPU_INTERFACE 256
> > I'll bite. Where on Earth is this value coming from?
>
> I just thought 256 is big enough for now :(
> Yes, should be a larger number for GICv3.
Could this just be set to NR_CPUS? That way it will be large enough for
any system you can actually run on.
Arnd
^ permalink raw reply
* iMX6SL Suspend mode (S3)
From: John Tobias @ 2014-01-24 21:14 UTC (permalink / raw)
To: linux-arm-kernel
Hello all,
I am testing the suspend mode (S3) for my custom board with iMX6SL and
the iMX6SL EVK as well. I am using 3.13 kernel, eMMC and MMC for the
storage and systemd to control the OS. If I put my device in S3 mode,
I got the following messages:
[ 27.949393] PM: Syncing filesystems ... done.
[ 27.972392] mmc0: card 0001 removed
[ 28.033613] end_request: I/O error, dev mmcblk0, sector 0
[ 28.063493] end_request: I/O error, dev mmcblk0, sector 0
[ 28.107933] end_request: I/O error, dev mmcblk0, sector 0
[ 28.131456] end_request: I/O error, dev mmcblk0, sector 0
[ 28.153651] end_request: I/O error, dev mmcblk0, sector 0
[ 28.215530] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 28.224284] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[ 28.233658] Suspending console(s) (use no_console_suspend to debug)
If I press any key, it will wake up. But, I got the following errors:
[ 17.815008] mmc0: new high speed DDR MMC card at address 0001
[ 17.828024] mmcblk0: mmc0:0001 SEM04G 3.68 GiB
[ 17.833522] mmcblk0boot0: mmc0:0001 SEM04G partition 1 2.00 MiB
[ 17.839699] mmcblk0boot1: mmc0:0001 SEM04G partition 2 2.00 MiB
[ 17.847150] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 2.00 MiB
[ 17.860116] mmcblk0: p1 p2 p3 p4 < p5 p6 >
[ 17.883550] mmcblk0boot1: unknown partition table
[ 17.899549] mmcblk0boot0: unknown partition table
[ 17.959186] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.126790] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.173370] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.207744] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.316736] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.448542] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.496749] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.530178] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.575861] EXT4-fs error (device mmcblk0p2):
__ext4_get_inode_loc:3909: inode #33300: block 131267: comm systemd:
unable to read itable block
[ 18.660139] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:908: error reading directory block (ino 33142,
block 0)
[ 18.717386] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 2001,
block 0)
[ 18.830148] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:908: error reading directory block (ino 33135,
block 0)
[ 19.045944] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 32928,
block 0)
[ 19.058510] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 32928,
block 0)
[ 19.071681] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 90, block
0)
[ 19.083810] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 90, block
0)
[ 19.097369] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 2001,
block 0)
[ 19.110210] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 2001,
block 0)
[ 19.153687] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 2001,
block 0)
[ 29.800522] EXT4-fs warning: 1 callbacks suppressed
[ 29.805425] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 2001,
block 0)
[ 29.842343] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 90, block
0)
[ 29.875698] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 32928,
block 0)
[ 29.906757] EXT4-fs warning (device mmcblk0p2):
__ext4_read_dirblock:681: error reading directory block (ino 90, block
0)
Regards,
john
^ permalink raw reply
* iMX6SL Suspend mode (S3)
From: Russell King - ARM Linux @ 2014-01-24 21:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACUGKYMxozVYUZ1neX9iRqRoaTCTOOY8VV92UAtcwUa_1f0XSw@mail.gmail.com>
On Fri, Jan 24, 2014 at 01:14:08PM -0800, John Tobias wrote:
> Hello all,
>
> I am testing the suspend mode (S3) for my custom board with iMX6SL and
> the iMX6SL EVK as well. I am using 3.13 kernel, eMMC and MMC for the
> storage and systemd to control the OS. If I put my device in S3 mode,
> I got the following messages:
>
> [ 27.949393] PM: Syncing filesystems ... done.
> [ 27.972392] mmc0: card 0001 removed
> [ 28.033613] end_request: I/O error, dev mmcblk0, sector 0
> [ 28.063493] end_request: I/O error, dev mmcblk0, sector 0
> [ 28.107933] end_request: I/O error, dev mmcblk0, sector 0
> [ 28.131456] end_request: I/O error, dev mmcblk0, sector 0
> [ 28.153651] end_request: I/O error, dev mmcblk0, sector 0
> [ 28.215530] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [ 28.224284] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) done.
> [ 28.233658] Suspending console(s) (use no_console_suspend to debug)
>
> If I press any key, it will wake up. But, I got the following errors:
>
> [ 17.815008] mmc0: new high speed DDR MMC card at address 0001
> [ 17.828024] mmcblk0: mmc0:0001 SEM04G 3.68 GiB
> [ 17.833522] mmcblk0boot0: mmc0:0001 SEM04G partition 1 2.00 MiB
> [ 17.839699] mmcblk0boot1: mmc0:0001 SEM04G partition 2 2.00 MiB
> [ 17.847150] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 2.00 MiB
> [ 17.860116] mmcblk0: p1 p2 p3 p4 < p5 p6 >
> [ 17.883550] mmcblk0boot1: unknown partition table
> [ 17.899549] mmcblk0boot0: unknown partition table
This is because MMC insists that when you suspend, the card will be
ejected, and when you resume, it will be re-inserted - it's effectively
the same as if you pulled the card out and then plugged it back in
without shutting the system down.
This is the default for the MMC subsystem, unless MMC_UNSAFE_RESUME
is set - and you can't have rootfs in MMC and suspend/resume unless
that's set.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply
* [PATCH v5 1/8] ARM: brcmstb: add infrastructure for ARM-based Broadcom STB SoCs
From: Marc C @ 2014-01-24 21:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124101407.GL15586@e106331-lin.cambridge.arm.com>
Hi Mark,
>> +static void __init brcmstb_init_early(void)
>> +{
>> + add_preferred_console("ttyS", 0, "115200");
>> +}
>
> Is this really required?
I think I can drop this. It was a holdover from our older kernels.
>> + /*
>> + * set the reset vector to point to the secondary_startup
>> + * routine
>> + */
>> + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup));
>> +
>> + flush_cache_all();
>
> Why? What does the new CPU need before its caches are coherent and up?
Absolutely nothing! I should be able to drop this as well.
Regarding the CPU power-down sequence, I'll review it and make sure it follows the
"Processor power domain" sequence in the A15 TRM. For any deviations, I'll double-check
with our H/W designers to ensure there aren't any magic requirements unaccounted for.
Thank you for taking a deep-dive into the code! I'll make the appropriate modifications
per your suggestions.
Regards,
Marc C
On 01/24/2014 02:14 AM, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 03:30:45AM +0000, Marc Carino wrote:
>> The BCM7xxx series of Broadcom SoCs are used primarily in set-top boxes.
>>
>> This patch adds machine support for the ARM-based Broadcom SoCs.
>>
>> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> arch/arm/configs/multi_v7_defconfig | 1 +
>> arch/arm/mach-bcm/Kconfig | 14 ++
>> arch/arm/mach-bcm/Makefile | 4 +
>> arch/arm/mach-bcm/brcmstb.c | 110 ++++++++++++
>> arch/arm/mach-bcm/brcmstb.h | 38 ++++
>> arch/arm/mach-bcm/headsmp-brcmstb.S | 34 ++++
>> arch/arm/mach-bcm/hotplug-brcmstb.c | 334 +++++++++++++++++++++++++++++++++++
>> 7 files changed, 535 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/mach-bcm/brcmstb.c
>> create mode 100644 arch/arm/mach-bcm/brcmstb.h
>> create mode 100644 arch/arm/mach-bcm/headsmp-brcmstb.S
>> create mode 100644 arch/arm/mach-bcm/hotplug-brcmstb.c
>>
>> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
>> index c1df4e9..7028d11 100644
>> --- a/arch/arm/configs/multi_v7_defconfig
>> +++ b/arch/arm/configs/multi_v7_defconfig
>> @@ -7,6 +7,7 @@ CONFIG_MACH_ARMADA_370=y
>> CONFIG_MACH_ARMADA_XP=y
>> CONFIG_ARCH_BCM=y
>> CONFIG_ARCH_BCM_MOBILE=y
>> +CONFIG_ARCH_BRCMSTB=y
>> CONFIG_GPIO_PCA953X=y
>> CONFIG_ARCH_HIGHBANK=y
>> CONFIG_ARCH_KEYSTONE=y
>> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
>> index 9fe6d88..2c1ae83 100644
>> --- a/arch/arm/mach-bcm/Kconfig
>> +++ b/arch/arm/mach-bcm/Kconfig
>> @@ -31,6 +31,20 @@ config ARCH_BCM_MOBILE
>> BCM11130, BCM11140, BCM11351, BCM28145 and
>> BCM28155 variants.
>>
>> +config ARCH_BRCMSTB
>> + bool "Broadcom BCM7XXX based boards" if ARCH_MULTI_V7
>> + depends on MMU
>> + select ARM_GIC
>> + select MIGHT_HAVE_PCI
>> + select HAVE_SMP
>> + select HAVE_ARM_ARCH_TIMER
>> + help
>> + Say Y if you intend to run the kernel on a Broadcom ARM-based STB
>> + chipset.
>> +
>> + This enables support for Broadcom ARM-based set-top box chipsets,
>> + including the 7445 family of chips.
>> +
>> endmenu
>>
>> endif
>> diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile
>> index c2ccd5a..b744a12 100644
>> --- a/arch/arm/mach-bcm/Makefile
>> +++ b/arch/arm/mach-bcm/Makefile
>> @@ -13,3 +13,7 @@
>> obj-$(CONFIG_ARCH_BCM_MOBILE) := board_bcm281xx.o bcm_kona_smc.o bcm_kona_smc_asm.o kona.o
>> plus_sec := $(call as-instr,.arch_extension sec,+sec)
>> AFLAGS_bcm_kona_smc_asm.o :=-Wa,-march=armv7-a$(plus_sec)
>> +
>> +obj-$(CONFIG_ARCH_BRCMSTB) := brcmstb.o
>> +obj-$(CONFIG_SMP) += headsmp-brcmstb.o
>> +obj-$(CONFIG_HOTPLUG_CPU) += hotplug-brcmstb.o
>> diff --git a/arch/arm/mach-bcm/brcmstb.c b/arch/arm/mach-bcm/brcmstb.c
>> new file mode 100644
>> index 0000000..7a6093d
>> --- /dev/null
>> +++ b/arch/arm/mach-bcm/brcmstb.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * Copyright (C) 2013 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/console.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/printk.h>
>> +#include <linux/smp.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/mach-types.h>
>> +#include <asm/mach/arch.h>
>> +#include <asm/mach/map.h>
>> +#include <asm/mach/time.h>
>> +
>> +#include "brcmstb.h"
>> +
>> +/***********************************************************************
>> + * STB CPU (main application processor)
>> + ***********************************************************************/
>> +
>> +static const char *brcmstb_match[] __initconst = {
>> + "brcm,bcm7445",
>> + "brcm,brcmstb",
>> + NULL
>> +};
>> +
>> +static void __init brcmstb_init_early(void)
>> +{
>> + add_preferred_console("ttyS", 0, "115200");
>> +}
>
> Is this really required?
>
>> +
>> +/***********************************************************************
>> + * SMP boot
>> + ***********************************************************************/
>> +
>> +#ifdef CONFIG_SMP
>> +static DEFINE_SPINLOCK(boot_lock);
>> +
>> +static void __cpuinit brcmstb_secondary_init(unsigned int cpu)
>> +{
>> + /*
>> + * Synchronise with the boot thread.
>> + */
>> + spin_lock(&boot_lock);
>> + spin_unlock(&boot_lock);
>> +}
>> +
>> +static int __cpuinit brcmstb_boot_secondary(unsigned int cpu,
>> + struct task_struct *idle)
>> +{
>> + /*
>> + * set synchronisation state between this boot processor
>> + * and the secondary one
>> + */
>> + spin_lock(&boot_lock);
>> +
>> + /* Bring up power to the core if necessary */
>> + if (brcmstb_cpu_get_power_state(cpu) == 0)
>> + brcmstb_cpu_power_on(cpu);
>> +
>> + brcmstb_cpu_boot(cpu);
>> +
>> + /*
>> + * now the secondary core is starting up let it run its
>> + * calibrations, then wait for it to finish
>> + */
>> + spin_unlock(&boot_lock);
>> +
>> + return 0;
>> +}
>> +
>> +struct smp_operations brcmstb_smp_ops __initdata = {
>> + .smp_prepare_cpus = brcmstb_cpu_ctrl_setup,
>> + .smp_secondary_init = brcmstb_secondary_init,
>> + .smp_boot_secondary = brcmstb_boot_secondary,
>> +#ifdef CONFIG_HOTPLUG_CPU
>> + .cpu_kill = brcmstb_cpu_kill,
>> + .cpu_die = brcmstb_cpu_die,
>> +#endif
>> +};
>> +#endif
>> +
>> +DT_MACHINE_START(BRCMSTB, "Broadcom STB (Flattened Device Tree)")
>> + .dt_compat = brcmstb_match,
>> +#ifdef CONFIG_SMP
>> + .smp = smp_ops(brcmstb_smp_ops),
>> +#endif
>> + .init_early = brcmstb_init_early,
>> +MACHINE_END
>> diff --git a/arch/arm/mach-bcm/brcmstb.h b/arch/arm/mach-bcm/brcmstb.h
>> new file mode 100644
>> index 0000000..e49bde6
>> --- /dev/null
>> +++ b/arch/arm/mach-bcm/brcmstb.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Copyright (C) 2013 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __BRCMSTB_H__
>> +#define __BRCMSTB_H__
>> +
>> +#if !defined(__ASSEMBLY__)
>> +#include <linux/smp.h>
>> +#endif
>> +
>> +#if !defined(__ASSEMBLY__)
>> +extern void brcmstb_secondary_startup(void);
>> +extern void brcmstb_cpu_boot(unsigned int cpu);
>> +extern void brcmstb_cpu_power_on(unsigned int cpu);
>> +extern int brcmstb_cpu_get_power_state(unsigned int cpu);
>> +extern struct smp_operations brcmstb_smp_ops;
>> +#if defined(CONFIG_HOTPLUG_CPU)
>> +extern void brcmstb_cpu_die(unsigned int cpu);
>> +extern int brcmstb_cpu_kill(unsigned int cpu);
>> +void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus);
>> +#else
>> +static inline void brcmstb_cpu_die(unsigned int cpu) {}
>> +static inline int brcmstb_cpu_kill(unsigned int cpu) {}
>> +static inline void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus) {}
>> +#endif
>> +#endif
>> +
>> +#endif /* __BRCMSTB_H__ */
>> diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S b/arch/arm/mach-bcm/headsmp-brcmstb.S
>> new file mode 100644
>> index 0000000..57ec438
>> --- /dev/null
>> +++ b/arch/arm/mach-bcm/headsmp-brcmstb.S
>> @@ -0,0 +1,34 @@
>> +/*
>> + * SMP boot code for secondary CPUs
>> + * Based on arch/arm/mach-tegra/headsmp.S
>> + *
>> + * Copyright (C) 2010 NVIDIA, Inc.
>> + * Copyright (C) 2013 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <asm/assembler.h>
>> +#include <linux/linkage.h>
>> +#include <linux/init.h>
>> +
>> + .section ".text.head", "ax"
>> + __CPUINIT
>
> __CPUINIT is either going or gone by now. This should disappear.
>
>> +
>> +ENTRY(brcmstb_secondary_startup)
>> + /*
>> + * Ensure CPU is in a sane state by disabling all IRQs and switching
>> + * into SVC mode.
>> + */
>> + setmode PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
>> +
>> + bl v7_invalidate_l1
>> + b secondary_startup
>> +ENDPROC(brcmstb_secondary_startup)
>> diff --git a/arch/arm/mach-bcm/hotplug-brcmstb.c b/arch/arm/mach-bcm/hotplug-brcmstb.c
>> new file mode 100644
>> index 0000000..ff4a732
>> --- /dev/null
>> +++ b/arch/arm/mach-bcm/hotplug-brcmstb.c
>> @@ -0,0 +1,334 @@
>> +/*
>> + * Broadcom STB CPU hotplug support for ARM
>> + *
>> + * Copyright (C) 2013 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/printk.h>
>> +#include <linux/regmap.h>
>> +#include <linux/smp.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/mach-types.h>
>> +
>> +#include "brcmstb.h"
>> +
>> +enum {
>> + ZONE_MAN_CLKEN_MASK = BIT(0),
>> + ZONE_MAN_RESET_CNTL_MASK = BIT(1),
>> + ZONE_MAN_MEM_PWR_MASK = BIT(4),
>> + ZONE_RESERVED_1_MASK = BIT(5),
>> + ZONE_MAN_ISO_CNTL_MASK = BIT(6),
>> + ZONE_MANUAL_CONTROL_MASK = BIT(7),
>> + ZONE_PWR_DN_REQ_MASK = BIT(9),
>> + ZONE_PWR_UP_REQ_MASK = BIT(10),
>> + ZONE_BLK_RST_ASSERT_MASK = BIT(10),
>> + ZONE_PWR_OFF_STATE_MASK = BIT(26),
>> + ZONE_PWR_ON_STATE_MASK = BIT(26),
>> + ZONE_DPG_PWR_STATE_MASK = BIT(28),
>> + ZONE_MEM_PWR_STATE_MASK = BIT(29),
>> + ZONE_RESET_STATE_MASK = BIT(31),
>> +};
>> +
>> +static void __iomem *cpubiuctrl_block;
>> +static void __iomem *hif_cont_block;
>> +static u32 cpu0_pwr_zone_ctrl_reg;
>> +static u32 cpu_rst_cfg_reg;
>> +static u32 hif_cont_reg;
>> +DEFINE_PER_CPU(int, per_cpu_sw_state);
>> +
>> +static void __iomem *pwr_ctrl_get_base(unsigned int cpu)
>> +{
>> + void __iomem *base = cpubiuctrl_block + cpu0_pwr_zone_ctrl_reg;
>> + base += (cpu * 4);
>
> CPU isn't guaranteed to be the physical CPU ID (MPIDR.Aff*). While it
> almost certainly will be, we can't guarantee it in the face of a kexec,
> for example.
>
> You can use cpu_logical_map(cpu) to get the physical ID.
>
>> + return base;
>> +}
>> +
>> +static u32 pwr_ctrl_rd(unsigned int cpu)
>> +{
>> + void __iomem *base = pwr_ctrl_get_base(cpu);
>> + return readl_relaxed(base);
>> +}
>> +
>> +static void pwr_ctrl_wr(unsigned int cpu, u32 val)
>> +{
>> + void __iomem *base = pwr_ctrl_get_base(cpu);
>> + writel(val, base);
>> +}
>> +
>> +static void cpu_rst_cfg_set(int cpu, int set)
>> +{
>> + u32 val;
>> + val = readl_relaxed(cpubiuctrl_block + cpu_rst_cfg_reg);
>> + if (set)
>> + val |= BIT(cpu);
>> + else
>> + val &= ~BIT(cpu);
>
> Likewise here.
>
>> + writel_relaxed(val, cpubiuctrl_block + cpu_rst_cfg_reg);
>> +}
>> +
>> +static void cpu_set_boot_addr(int cpu, unsigned long boot_addr)
>> +{
>> + const int reg_ofs = cpu * 8;
>
> And here.
>
>> + writel_relaxed(0, hif_cont_block + hif_cont_reg + reg_ofs);
>> + writel_relaxed(boot_addr, hif_cont_block + hif_cont_reg + 4 + reg_ofs);
>> +}
>> +
>> +void brcmstb_cpu_boot(unsigned int cpu)
>> +{
>> + pr_info("SMP: Booting CPU%d...\n", cpu);
>> +
>> + /*
>> + * set the reset vector to point to the secondary_startup
>> + * routine
>> + */
>> + cpu_set_boot_addr(cpu, virt_to_phys(brcmstb_secondary_startup));
>> +
>> + flush_cache_all();
>
> Why? What does the new CPU need before its caches are coherent and up?
>
>> +
>> + /* unhalt the cpu */
>> + cpu_rst_cfg_set(cpu, 0);
>> +}
>> +
>> +void brcmstb_cpu_power_on(unsigned int cpu)
>> +{
>> + /*
>> + * The secondary cores power was cut, so we must go through
>> + * power-on initialization.
>> + */
>> + u32 tmp;
>> +
>> + pr_info("SMP: Powering up CPU%d...\n", cpu);
>> +
>> + /* Request zone power up */
>> + pwr_ctrl_wr(cpu, ZONE_PWR_UP_REQ_MASK);
>> +
>> + /* Wait for the power up FSM to complete */
>> + do {
>> + tmp = pwr_ctrl_rd(cpu);
>> + } while (!(tmp & ZONE_PWR_ON_STATE_MASK));
>> +
>> + per_cpu(per_cpu_sw_state, cpu) = 1;
>> +}
>> +
>> +int brcmstb_cpu_get_power_state(unsigned int cpu)
>> +{
>> + int tmp = pwr_ctrl_rd(cpu);
>> + return (tmp & ZONE_RESET_STATE_MASK) ? 0 : 1;
>> +}
>> +
>> +void __ref brcmstb_cpu_die(unsigned int cpu)
>> +{
>> + /* Derived from misc_bpcm_arm.c */
>> +
>> + /* Clear SCTLR.C bit */
>> + __asm__(
>> + "mrc p15, 0, r0, c1, c0, 0\n"
>> + "bic r0, r0, #(1 << 2)\n"
>> + "mcr p15, 0, r0, c1, c0, 0\n"
>> + : /* no output */
>> + : /* no input */
>> + : "r0" /* clobber r0 */
>> + );
>
> This is odd. Why not allow GCC to allocate the register?
>
>> +
>> + /*
>> + * Instruction barrier to ensure cache is really disabled before
>> + * cleaning/invalidating the caches
>> + */
>> + isb();
>
> I think you could use:
>
> set_cr(get_cr() & ~CR_C))
>
> Which would do all of the above (including the isb), and will get GCC to
> allocate the register.
>
>> +
>> + flush_cache_all();
>> +
>> + /* Invalidate all instruction caches to PoU (ICIALLU) */
>> + /* Data sync. barrier to ensure caches have emptied out */
>> + __asm__("mcr p15, 0, r0, c7, c5, 0\n" : : : "r0");
>> + dsb();
>
> Why do you need to invalidate the I-cache?
>
>> +
>> + /*
>> + * Clear ACTLR.SMP bit to prevent broadcast TLB messages from reaching
>> + * this core
>> + */
>> + __asm__(
>> + "mrc p15, 0, r0, c1, c0, 1\n"
>> + "bic r0, r0, #(1 << 6)\n"
>> + "mcr p15, 0, r0, c1, c0, 1\n"
>> + : /* no output */
>> + : /* no input */
>> + : "r0" /* clobber r0 */
>> + );
>
> Surely you can use an output operand to get GCC to allocate the register
> for you?
>
>> +
>> + /* Disable all IRQs for this CPU */
>> + arch_local_irq_disable();
>> +
>> + per_cpu(per_cpu_sw_state, cpu) = 0;
>
> Your caches are off at this point, so this could be going straight to
> memory. Yet readers of this value aren't cleaning their caches before
> reading this, so they could hit a stale cached copy.
>
>> +
>> + /*
>> + * Final full barrier to ensure everything before this instruction has
>> + * quiesced.
>> + */
>> + isb();
>> + dsb();
>> +
>> + /* Sit and wait to die */
>> + wfi();
>> +
>> + /* We should never get here... */
>> + nop();
>
> Why the nop first?
>
>> + panic("Spurious interrupt on CPU %d received!\n", cpu);
>> +}
>> +
>> +int brcmstb_cpu_kill(unsigned int cpu)
>> +{
>> + u32 tmp;
>> +
>> + pr_info("SMP: Powering down CPU%d...\n", cpu);
>> +
>> + while (per_cpu(per_cpu_sw_state, cpu))
>> + ;
>
> As this was written to with caches disabled, the cached copy of the
> value (which this is reading) could be stale. Surely you need to
> clean+invalidate the line for this value each time you read it to give
> it a chance to update?
>
>> +
>> + /* Program zone reset */
>> + pwr_ctrl_wr(cpu, ZONE_RESET_STATE_MASK | ZONE_BLK_RST_ASSERT_MASK |
>> + ZONE_PWR_DN_REQ_MASK);
>> +
>> + /* Verify zone reset */
>> + tmp = pwr_ctrl_rd(cpu);
>> + if (!(tmp & ZONE_RESET_STATE_MASK))
>> + pr_err("%s: Zone reset bit for CPU %d not asserted!\n",
>> + __func__, cpu);
>> +
>> + /* Wait for power down */
>> + do {
>> + tmp = pwr_ctrl_rd(cpu);
>> + } while (!(tmp & ZONE_PWR_OFF_STATE_MASK));
>> +
>> + /* Settle-time from Broadcom-internal DVT reference code */
>> + udelay(7);
>> +
>> + /* Assert reset on the CPU */
>> + cpu_rst_cfg_set(cpu, 1);
>> +
>> + return 1;
>> +}
>> +
>> +static int __init setup_hifcpubiuctrl_regs(struct device_node *np)
>> +{
>> + int rc = 0;
>> + char *name;
>> + int index;
>> + struct device_node *syscon_np = NULL;
>> +
>> + name = "syscon-cpu";
>> +
>> + syscon_np = of_parse_phandle(np, name, 0);
>> + if (!syscon_np) {
>> + pr_err("can't find phandle %s\n", name);
>> + rc = -EINVAL;
>> + goto cleanup;
>> + }
>> +
>> + cpubiuctrl_block = of_iomap(syscon_np, 0);
>> + if (!cpubiuctrl_block) {
>> + pr_err("iomap failed for cpubiuctrl_block\n");
>> + rc = -EINVAL;
>> + goto cleanup;
>> + }
>> +
>> + index = 1;
>> + rc = of_property_read_u32_index(np, name, index,
>> + &cpu0_pwr_zone_ctrl_reg);
>
> The index variable seems rather pointless. Why not just use the value
> in-place?
>
>> + if (rc) {
>> + pr_err("failed to read %d from %s property (%d)\n", index, name,
>> + rc);
>
> It might be better to state _what_ you're looking for (what does the
> value represent?).
>
>> + rc = -EINVAL;
>> + goto cleanup;
>> + }
>> +
>> + index = 2;
>> + rc = of_property_read_u32_index(np, name, index, &cpu_rst_cfg_reg);
>
> Likewise for all of the above.
>
> Thanks,
> Mark.
>
^ permalink raw reply
* [RFC PATCH V2 1/4] pci: APM X-Gene PCIe controller driver
From: Tanmay Inamdar @ 2014-01-24 21:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACoXjc=ZT9fEm_KdY4WFSoS8n5FmO+hn9LjsR4m8YZG2iBbt5A@mail.gmail.com>
On Thu, Jan 16, 2014 at 5:10 PM, Tanmay Inamdar <tinamdar@apm.com> wrote:
> On Wed, Jan 15, 2014 at 4:39 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wednesday 15 January 2014, Tanmay Inamdar wrote:
>>> This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
>>> X-Gene PCIe controller supports maxmum upto 8 lanes and GEN3 speed.
>>> X-Gene has maximum 5 PCIe ports supported.
>>>
>>> Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
>>
>> This already looks much better than the first version, but I have a more
>> comments. Most importantly, it would help to know how the root ports
>> are structured. Is this a standard root complex and multiple ports,
>> multiple root complexes with one port each, or a nonstandard organization
>> that is a mix of those two models?
>
> This is multiple root complexes with one port each.
>
>>
>>> +
>>> +/* When the address bit [17:16] is 2'b01, the Configuration access will be
>>> + * treated as Type 1 and it will be forwarded to external PCIe device.
>>> + */
>>> +static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>>> +{
>>> + struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
>>> + u64 addr = (u64)port->cfg_base;
>>> +
>>> + if (bus->number >= (port->first_busno + 1))
>>> + addr |= AXI_EP_CFG_ACCESS;
>>> +
>>> + return (void *)addr;
>>> +}
>>
>> Wrong type, it should be 'void __iomem *'. Also you can't assume that
>> bit operations work on virtual __iomem addresses, so it should be better
>> to just add a constant integer to the pointer, which is a valid
>> operation.
>
> ok.
>
>>
>> I also wonder why you need to do this at all. If there isn't a global
>> config space for all ports, but rather a separate type0/type1 config
>> cycle based on the bus number, I see that as an indication that the
>> ports are in fact separate domains and should each start with bus 0.
>
> It is not a standard ECAM layout. We also have a separate RTDID
> register as well to program bus, device, function. While accessing EP
> config space, we have to set the bit 17:16 as 2b'01. The same config
> space address is utilized for enabling a customized nonstandard PCIe
> DMA feature. The bits are defined to differentiate the access purpose.
> The feature is not supported in this driver yet.
>
> Secondly I don't think it will matter if each port starts with bus 0.
> As long as we set the correct BDF in RTDID and set correct bits in
> config address, the config reads and writes would work. Right?
>
>>
>>> +static void xgene_pcie_setup_lanes(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_8);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_8);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_9);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_9);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_10);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_10);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_11);
>>> + val = eq_pre_cursor_lane0_set(val, 0x7);
>>> + val = eq_pre_cursor_lane1_set(val, 0x7);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_11);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_4);
>>> + val = (val & ~0x30) | (1 << 4);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_4);
>>> +}
>>
>> Please document what you are actually setting here. If the configuration
>> of the lanes is always the same, why do you have to set it here. If not,
>> why do you set constant values?
>
> Good point. Let me check if these values should be constant or tune-able.
>
>>
>>> +static void xgene_pcie_setup_link(struct xgene_pcie_port *port)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val |= DIRECT_TO_8GTS_MASK;
>>> + val |= SUPPORT_5GTS_MASK;
>>> + val |= SUPPORT_8GTS_MASK;
>>> + val |= DIRECT_TO_5GTS_MASK;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_14);
>>> + val &= ~ADVT_INFINITE_CREDITS;
>>> + writel(val, csr_base + BRIDGE_CFG_14);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= (val & ~0xf) | 7;
>>> + val |= (val & ~0xf00) | ((7 << 8) & 0xf00);
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +
>>> + val = readl(csr_base + BRIDGE_8G_CFG_0);
>>> + val |= DWNSTRM_EQ_SKP_PHS_2_3;
>>> + writel(val, csr_base + BRIDGE_8G_CFG_0);
>>> +}
>>
>> Same here.
>>
>>> +static void xgene_pcie_program_core(void *csr_base)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl(csr_base + BRIDGE_CFG_0);
>>> + val |= AER_OPTIONAL_ERROR_EN;
>>> + writel(val, csr_base + BRIDGE_CFG_0);
>>> + writel(0x0, csr_base + INTXSTATUSMASK);
>>> + val = readl(csr_base + BRIDGE_CTRL_1);
>>> + val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
>>> + writel(val, csr_base + BRIDGE_CTRL_1);
>>> +}
>>
>> 'program_core'?
>
> Some of the PCIe core related misc configurations.
>
>>
>>> +static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port, u32 *lanes)
>>> +{
>>> + void *csr_base = port->csr_base;
>>> + u32 val32;
>>> + u64 start_time, time;
>>> +
>>> + /*
>>> + * A component enters the LTSSM Detect state within
>>> + * 20ms of the end of fundamental core reset.
>>> + */
>>> + msleep(XGENE_LTSSM_DETECT_WAIT);
>>> + port->link_up = 0;
>>> + start_time = jiffies;
>>> + do {
>>> + val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
>>> + if (val32 & LINK_UP_MASK) {
>>> + port->link_up = 1;
>>> + port->link_speed = PIPE_PHY_RATE_RD(val32);
>>> + val32 = readl(csr_base + BRIDGE_STATUS_0);
>>> + *lanes = val32 >> 26;
>>> + }
>>> + time = jiffies_to_msecs(jiffies - start_time);
>>> + } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
>>> +}
>>
>> Maybe another msleep() in the loop? It seems weird to first do an
>> unconditional sleep but then busy-wait for the result.
>
> ok.
This loop can execute for maximum 4 msec. So putting msleep(1) won't
get us much.
>
>>
>>> +static void xgene_pcie_setup_primary_bus(struct xgene_pcie_port *port,
>>> + u32 first_busno, u32 last_busno)
>>> +{
>>> + u32 val;
>>> + void *cfg_addr = port->cfg_base;
>>> +
>>> + val = readl(cfg_addr + PCI_PRIMARY_BUS);
>>> + val &= ~PCI_PRIMARY_BUS_MASK;
>>> + val |= (last_busno << 16) | ((first_busno + 1) << 8) | (first_busno);
>>> + writel(val, cfg_addr + PCI_PRIMARY_BUS);
>>> +}
>>
>> Please explain what you are doing here. As mentioned above, I would expect
>> that each domain has visibility of all 255 buses. You shouldn't need any hacks
>> where you try to artificially squeeze the ports into a single domain when
>> they are separate in hardware.
>
> ok. I will check and get back.
You are right. I have removed this hack. It will be fixed in next version.
>
>>
>>> +/*
>>> + * read configuration values from DTS
>>> + */
>>> +static int xgene_pcie_read_dts_config(struct xgene_pcie_port *port)
>>
>> The comment and function name don't seem to match what the function
>> does. The main purpose of this function seems to be to ioremap
>> the resources, which have nothing to with configuration.
>
> ok.
>
>>
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct resource csr_res;
>>> + struct resource cfg_res;
>>> +
>>> + /* Get CSR space registers address */
>>> + if (of_address_to_resource(np, 0, &csr_res))
>>> + return -EINVAL;
>>> +
>>> + port->csr_base = devm_ioremap_nocache(port->dev, csr_res.start,
>>> + resource_size(&csr_res));
>>
>> You can also use platform_get_resource() to access the resource
>> that is already there, rather than creating another one.
>
> ok.
>
>>
>>> +static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
>>> + u32 addr, u32 restype)
>>> +{
>>> + struct resource *res = NULL;
>>> + void *base = port->csr_base + addr;
>>> + resource_size_t size;
>>> + u64 cpu_addr = 0;
>>> + u64 pci_addr = 0;
>>> + u64 mask = 0;
>>> + u32 min_size = 0;
>>
>> A general note: don't initialize local variables to a bogus valus (e.g. 0)
>> in their declaration. It prevents the compiler from warning about
>> incorrect uses.
>
> ok.
>
>>
>>> + u32 flag = EN_REG;
>>
>> This one on the other hand is ok, because you are actually going to
>> use that value.
>>
>>> + switch (restype) {
>>> + case IORESOURCE_MEM:
>>> + res = &port->mem.res;
>>> + pci_addr = port->mem.pci_addr;
>>> + min_size = SZ_128M;
>>> + break;
>>> + case IORESOURCE_IO:
>>> + res = &port->io.res;
>>> + pci_addr = port->io.pci_addr;
>>> + min_size = 128;
>>> + flag |= OB_LO_IO;
>>> + break;
>>> + }
>>
>> I assume this works ok, but seems wrong in one detail: If the resource
>> is marked IORESOURCE_IO, res->start is supposed to be in I/O space, not
>> in memory space, which would make it the wrong number to program
>> into the hardware registers.
>
> Yes for using ioport resource. However we have decided to defer using
> it since 'pci_ioremap_io' is not yet supported from arm64 side.
>
> From HW point of view, for memory mapped IO space, it is nothing but a
> piece taken out of the ranges in address map for outbound accesses. So
> while configuring registers from SOC side, it should take the CPU
> address which is address from SOC address map. Right?
>
> Later on we can have a separate io resource like 'realio' similar to
> what pci-mvebu.c does.
>
>>
>>> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct of_pci_range range;
>>> + struct of_pci_range_parser parser;
>>> + struct device *dev = port->dev;
>>> +
>>> + if (of_pci_range_parser_init(&parser, np)) {
>>> + dev_err(dev, "missing ranges property\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Get the I/O, memory, config ranges from DT */
>>
>> The comment needs updating now that you don't read config space here any more.
>
> ok.
>
>>
>>> +/* X-Gene PCIe support maximum 3 inbound memory regions
>>> + * This function helps to select a region based on size of region
>>> + */
>>> +static int xgene_pcie_select_ib_reg(u64 size)
>>> +{
>>> + static u8 ib_reg_mask;
>>> +
>>> + if ((size > 4) && (size < SZ_16M) && !(ib_reg_mask & (1 << 1))) {
>>> + ib_reg_mask |= (1 << 1);
>>> + return 1;
>>> + }
>>> +
>>> + if ((size > SZ_1K) && (size < SZ_1T) && !(ib_reg_mask & (1 << 0))) {
>>> + ib_reg_mask |= (1 << 0);
>>> + return 0;
>>> + }
>>> +
>>> + if ((size > SZ_1M) && (size < SZ_1T) && !(ib_reg_mask & (1 << 2))) {
>>> + ib_reg_mask |= (1 << 2);
>>> + return 2;
>>> + }
>>> + return -EINVAL;
>>> +}
>>
>> Shouldn't the ib_reg_mask variable be per host bridge? Static variables
>> are dangerous if you ever get multiple instances of the hardware in one
>> system.
>
> Yes. You are right. Thanks.
>
>>
>>> +static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
>>> +{
>>> + struct device_node *np = port->node;
>>> + struct of_pci_range range;
>>> + struct of_pci_range_parser parser;
>>> + struct device *dev = port->dev;
>>> + int region;
>>> +
>>> + if (pci_dma_range_parser_init(&parser, np)) {
>>> + dev_err(dev, "missing dma-ranges property\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Get the dma-ranges from DT */
>>> + for_each_of_pci_range(&parser, &range) {
>>> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
>>> + u64 end = range.cpu_addr + range.size - 1;
>>> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
>>> + range.flags, range.cpu_addr, end, range.pci_addr);
>>> + region = xgene_pcie_select_ib_reg(range.size);
>>> + if (region == -EINVAL) {
>>> + dev_warn(port->dev, "invalid pcie dma-range config\n");
>>> + continue;
>>> + }
>>> + xgene_pcie_setup_ib_reg(port, &range, restype, region);
>>> + }
>>> + return 0;
>>> +}
>>
>> I guess is could even be a local variable in this function, which you pass
>> by reference.
>>
>>> +
>>> +static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
>>> +{
>>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>>> +
>>> + if (pp == NULL)
>>> + return 0;
>>> +
>>> + sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
>>> + pci_add_resource_offset(&sys->resources, &pp->mem.res,
>>> + sys->mem_offset);
>>> + return 1;
>>> +}
>>
>> Please follow the regular error handling conventions, which are to
>> pass a negative errno value on error and zero on success.
>
> ok.
>
>>
>> Also, what would be a reason for the port to be zero here? If
>> it's something that can't happen in practice, don't try to handle
>> it gracefully. You can use BUG_ON() for fatal conditions that
>> are supposed to be impossible to reach.
>
> This function is a hook to upper layer. We register nr_controllers in
> hw_pci structure as MAX_PORTS we support. It can happen that number of
> ports actually enabled from device tree are less than the number in
> nr_controllers.
>
>>
>>> +static struct pci_bus __init *xgene_pcie_scan_bus(int nr,
>>> + struct pci_sys_data *sys)
>>> +{
>>> + struct xgene_pcie_port *pp = xgene_pcie_sys_to_port(sys);
>>> +
>>> + pp->first_busno = sys->busnr;
>>> + xgene_pcie_setup_primary_bus(pp, sys->busnr, 0xff);
>>> + return pci_scan_root_bus(NULL, sys->busnr, &xgene_pcie_ops,
>>> + sys, &sys->resources);
>>> +}
>>
>> You have a number of helper functions that don't seem to gain
>> much at all. Just move the call to pci_scan_root_bus() into
>> xgene_pcie_setup_primary_bus() here, and use that as the .scan
>> callback.
>>
>
> I can do that if I can get rid of setup_primary_bus api. Let me check.
>
>>
>>> + if (!port->link_up)
>>> + dev_info(port->dev, "(rc) link down\n");
>>> + else
>>> + dev_info(port->dev, "(rc) x%d gen-%d link up\n",
>>> + lanes, port->link_speed + 1);
>>> +#ifdef CONFIG_PCI_DOMAINS
>>> + xgene_pcie_hw.domain++;
>>> +#endif
>>> + xgene_pcie_hw.private_data[index++] = port;
>>> + platform_set_drvdata(pdev, port);
>>> + return 0;
>>> +}
>>
>> Do you have multiple domains or not? I don't see how it can work if you
>> make the domain setup conditional on a kernel configuration option.
>> If you in fact have multiple domains, make sure in Kconfig that
>> CONFIG_PCI_DOMAINS is enabled. Otherwise don't mess with the domain
>> number...
>
> It is enabled in Kconfig.
>
>>
>>> +static const struct of_device_id xgene_pcie_match_table[] __initconst = {
>>> + {.compatible = "apm,xgene-pcie",},
>>> + {},
>>> +};
>>
>> Another general note: Your "compatible" strings are rather unspecific.
>> Do you have a version number for this IP block? I suppose that it's related
>> to one that has been used in other chips before, or will be used in future
>> chips, if it's not actually licensed from some other company.
>
> I will have to check this.
>
We have decided to stick with current compatible string for now.
>>
>>> +static int __init xgene_pcie_init(void)
>>> +{
>>> + void *private;
>>> + int ret;
>>> +
>>> + pr_info("X-Gene: PCIe driver\n");
>>> +
>>> + /* allocate private data to keep xgene_pcie_port information */
>>> + private = kzalloc((XGENE_PCIE_MAX_PORTS * sizeof(void *)), GFP_KERNEL);
>>
>> This should not be done unconditionally: There is no point in printing
>> a message or allocating memory if you don't actually run on a system
>> with this device.
>
> I am doing this here because I have one instance of hw_pci structure
> with multiple pcie controllers. I can't do it from probe since it will
> be called once per instance in device tree.
>
>>
>>> + if (private == NULL)
>>> + return -ENOMEM;
>>
>> Style: if you are testing for an object, just write 'if (private)' or
>> 'if (!private)', but don't compare against NULL.
>
> ok.
>
>>
>>> + xgene_pcie_hw.private_data = private;
>>> + ret = platform_driver_probe(&xgene_pcie_driver,
>>> + xgene_pcie_probe_bridge);
>>> + if (ret)
>>> + return ret;
>>> + pci_common_init(&xgene_pcie_hw);
>>> + return 0;
>>
>> This seems wrong: You should not use platform_driver_probe() because
>> that has issues with deferred probing.
>
> I think 'platform_driver_probe' prevents the deferred probing.
> 'pci_common_init' needs to be called only once with current driver
> structure. The probes for all pcie ports should be finished (ports
> initialized) before 'pci_common_init' gets called.
>
>>
>> Arnd
^ permalink raw reply
* iMX6SL Suspend mode (S3)
From: John Tobias @ 2014-01-24 21:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124212410.GH15937@n2100.arm.linux.org.uk>
Thanks Russell. Got it working.
Regards,
john
On Fri, Jan 24, 2014 at 1:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 24, 2014 at 01:14:08PM -0800, John Tobias wrote:
>> Hello all,
>>
>> I am testing the suspend mode (S3) for my custom board with iMX6SL and
>> the iMX6SL EVK as well. I am using 3.13 kernel, eMMC and MMC for the
>> storage and systemd to control the OS. If I put my device in S3 mode,
>> I got the following messages:
>>
>> [ 27.949393] PM: Syncing filesystems ... done.
>> [ 27.972392] mmc0: card 0001 removed
>> [ 28.033613] end_request: I/O error, dev mmcblk0, sector 0
>> [ 28.063493] end_request: I/O error, dev mmcblk0, sector 0
>> [ 28.107933] end_request: I/O error, dev mmcblk0, sector 0
>> [ 28.131456] end_request: I/O error, dev mmcblk0, sector 0
>> [ 28.153651] end_request: I/O error, dev mmcblk0, sector 0
>> [ 28.215530] Freezing user space processes ... (elapsed 0.001 seconds) done.
>> [ 28.224284] Freezing remaining freezable tasks ... (elapsed 0.001
>> seconds) done.
>> [ 28.233658] Suspending console(s) (use no_console_suspend to debug)
>>
>> If I press any key, it will wake up. But, I got the following errors:
>>
>> [ 17.815008] mmc0: new high speed DDR MMC card at address 0001
>> [ 17.828024] mmcblk0: mmc0:0001 SEM04G 3.68 GiB
>> [ 17.833522] mmcblk0boot0: mmc0:0001 SEM04G partition 1 2.00 MiB
>> [ 17.839699] mmcblk0boot1: mmc0:0001 SEM04G partition 2 2.00 MiB
>> [ 17.847150] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 2.00 MiB
>> [ 17.860116] mmcblk0: p1 p2 p3 p4 < p5 p6 >
>> [ 17.883550] mmcblk0boot1: unknown partition table
>> [ 17.899549] mmcblk0boot0: unknown partition table
>
> This is because MMC insists that when you suspend, the card will be
> ejected, and when you resume, it will be re-inserted - it's effectively
> the same as if you pulled the card out and then plugged it back in
> without shutting the system down.
>
> This is the default for the MMC subsystem, unless MMC_UNSAFE_RESUME
> is set - and you can't have rootfs in MMC and suspend/resume unless
> that's set.
>
> --
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply
* [RFC PATCH V3 0/4] APM X-Gene PCIe controller
From: Tanmay Inamdar @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for AppliedMicro X-Gene PCIe host controller. The
driver is tested on X-Gene platform with different gen1/2/3 PCIe endpoint
cards.
X-Gene PCIe controller driver has depedency on the pcie arch support for
arm64. The arm64 pcie arch support is not yet part of mainline Linux kernel
and approach for arch support is under discussion with arm64 maintainers.
The reference patch can be found here --> https://lkml.org/lkml/2013/10/23/244
If someone wishes to test PCIe on X-Gene, arch support patch must be applied
before the patches in this patch set.
changes since V2:
1. redefined each PCI port in different PCI domain correctly.
2. removed setup_lane and setup_link functions from driver.
3. removed scan_bus wrapper and set_primary_bus hack.
4. added pci_ioremap_io for io resources.
changes since V1:
1. added PCI domain support
2. reading cpu and pci addresses from device tree to configure regions.
3. got rid of unnecessary wrappers for readl and writel.
4. got rid of endpoint configuration code.
5. added 'dma-ranges' property support to read inbound region configuration.
6. renamed host driver file to 'pci-xgene.c' from 'pcie-xgene.c'
7. dropped 'clock-names' property from bindings
8. added comments whereever requested.
Tanmay Inamdar (4):
pci: APM X-Gene PCIe controller driver
arm64: dts: APM X-Gene PCIe device tree nodes
dt-bindings: pci: xgene pcie device tree bindings
MAINTAINERS: entry for APM X-Gene PCIe host driver
.../devicetree/bindings/pci/xgene-pci.txt | 52 ++
MAINTAINERS | 7 +
arch/arm64/boot/dts/apm-mustang.dts | 8 +
arch/arm64/boot/dts/apm-storm.dtsi | 155 ++++
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-xgene.c | 784 ++++++++++++++++++++
7 files changed, 1017 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci.txt
create mode 100644 drivers/pci/host/pci-xgene.c
--
1.7.9.5
^ permalink raw reply
* [RFC PATCH V3 1/4] pci: APM X-Gene PCIe controller driver
From: Tanmay Inamdar @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390599168-13150-1-git-send-email-tinamdar@apm.com>
This patch adds the AppliedMicro X-Gene SOC PCIe controller driver.
X-Gene PCIe controller supports maximum upto 8 lanes and GEN3 speed.
X-Gene has maximum 5 PCIe ports supported.
Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
---
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-xgene.c | 784 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 795 insertions(+)
create mode 100644 drivers/pci/host/pci-xgene.c
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6..19ce97d 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,14 @@ config PCI_RCAR_GEN2
There are 3 internal PCI controllers available with a single
built-in EHCI/OHCI host controller present on each one.
+config PCI_XGENE
+ bool "X-Gene PCIe controller"
+ depends on ARCH_XGENE
+ depends on OF
+ select PCIEPORTBUS
+ help
+ Say Y here if you want internal PCI support on APM X-Gene SoC.
+ There are 5 internal PCIe ports available. Each port is GEN3 capable
+ and have varied lanes from x1 to x8.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb333..34c7c36 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
+obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
new file mode 100644
index 0000000..650a860
--- /dev/null
+++ b/drivers/pci/host/pci-xgene.c
@@ -0,0 +1,784 @@
+/**
+ * APM X-Gene PCIe Driver
+ *
+ * Copyright (c) 2013 Applied Micro Circuits Corporation.
+ *
+ * Author: Tanmay Inamdar <tinamdar@apm.com>.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk-private.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/memblock.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <asm/pcibios.h>
+
+#define PCIECORE_LTSSM 0x4c
+#define PCIECORE_CTLANDSTATUS 0x50
+#define PIPE_PHY_RATE_RD(src) ((0xc000 & (u32)(src)) >> 0xe)
+#define INTXSTATUSMASK 0x6c
+#define PIM1_1L 0x80
+#define IBAR2 0x98
+#define IR2MSK 0x9c
+#define PIM2_1L 0xa0
+#define IBAR3L 0xb4
+#define IR3MSKL 0xbc
+#define PIM3_1L 0xc4
+#define OMR1BARL 0x100
+#define OMR2BARL 0x118
+#define CFGBARL 0x154
+#define CFGBARH 0x158
+#define CFGCTL 0x15c
+#define RTDID 0x160
+#define BRIDGE_CFG_0 0x2000
+#define BRIDGE_CFG_1 0x2004
+#define BRIDGE_CFG_4 0x2010
+#define BRIDGE_CFG_32 0x2030
+#define BRIDGE_CFG_14 0x2038
+#define BRIDGE_CTRL_1 0x2204
+#define BRIDGE_CTRL_2 0x2208
+#define BRIDGE_CTRL_5 0x2214
+#define BRIDGE_STATUS_0 0x2600
+#define MEM_RAM_SHUTDOWN 0xd070
+#define BLOCK_MEM_RDY 0xd074
+
+#define DEVICE_PORT_TYPE_MASK 0x03c00000
+#define PM_FORCE_RP_MODE_MASK 0x00000400
+#define SWITCH_PORT_MODE_MASK 0x00000800
+#define CLASS_CODE_MASK 0xffffff00
+#define LINK_UP_MASK 0x00000100
+#define AER_OPTIONAL_ERROR_EN 0xffc00000
+#define XGENE_PCIE_DEV_CTRL 0x2f0f
+#define AXI_EP_CFG_ACCESS 0x10000
+#define ENABLE_ASPM 0x08000000
+#define XGENE_PORT_TYPE_RC 0x05000000
+#define BLOCK_MEM_RDY_VAL 0xFFFFFFFF
+#define EN_COHERENCY 0xF0000000
+#define EN_REG 0x00000001
+#define OB_LO_IO 0x00000002
+#define XGENE_PCIE_VENDORID 0xE008
+#define XGENE_PCIE_DEVICEID 0xE004
+#define XGENE_PCIE_TIMEOUT (500*1000) /* us */
+#define XGENE_LTSSM_DETECT_WAIT 20
+#define XGENE_LTSSM_L0_WAIT 4
+#define SZ_1T (SZ_1G*1024ULL)
+
+struct xgene_res_cfg {
+ struct resource res;
+ u64 pci_addr;
+};
+
+struct xgene_pcie_port {
+ struct device_node *node;
+ struct device *dev;
+ struct clk *clk;
+ struct xgene_res_cfg mem;
+ struct xgene_res_cfg io;
+ struct resource realio;
+ void __iomem *csr_base;
+ void __iomem *cfg_base;
+ u8 link_up;
+};
+
+static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
+{
+ return (addr & PCI_BASE_ADDRESS_MEM_MASK) | flags;
+}
+
+static inline struct xgene_pcie_port *
+xgene_pcie_bus_to_port(struct pci_bus *bus)
+{
+ struct pci_sys_data *sys = bus->sysdata;
+ return sys->private_data;
+}
+
+/* PCIE Configuration Out/In */
+static inline void xgene_pcie_cfg_out32(void __iomem *addr, u32 val)
+{
+ writel(val, addr);
+}
+
+static inline void xgene_pcie_cfg_out16(void __iomem *addr, u16 val)
+{
+ u64 temp_addr = (u64)addr & ~0x3;
+ u32 val32 = readl((void __iomem *)temp_addr);
+
+ switch ((u64) addr & 0x3) {
+ case 2:
+ val32 &= ~0xFFFF0000;
+ val32 |= (u32) val << 16;
+ break;
+ case 0:
+ default:
+ val32 &= ~0xFFFF;
+ val32 |= val;
+ break;
+ }
+ writel(val32, (void __iomem *)temp_addr);
+}
+
+static inline void xgene_pcie_cfg_out8(void __iomem *addr, u8 val)
+{
+ phys_addr_t temp_addr = (u64) addr & ~0x3;
+ u32 val32 = readl((void __iomem *)temp_addr);
+
+ switch ((u64) addr & 0x3) {
+ case 0:
+ val32 &= ~0xFF;
+ val32 |= val;
+ break;
+ case 1:
+ val32 &= ~0xFF00;
+ val32 |= (u32) val << 8;
+ break;
+ case 2:
+ val32 &= ~0xFF0000;
+ val32 |= (u32) val << 16;
+ break;
+ case 3:
+ default:
+ val32 &= ~0xFF000000;
+ val32 |= (u32) val << 24;
+ break;
+ }
+ writel(val32, (void __iomem *)temp_addr);
+}
+
+static inline void xgene_pcie_cfg_in32(void __iomem *addr, u32 *val)
+{
+ *val = readl(addr);
+}
+
+static inline void xgene_pcie_cfg_in16(void __iomem *addr, u16 *val)
+{
+ u64 temp_addr = (u64)addr & ~0x3;
+ u32 val32;
+
+ val32 = readl((void __iomem *)temp_addr);
+
+ switch ((u64)addr & 0x3) {
+ case 2:
+ *val = val32 >> 16;
+ break;
+ case 0:
+ default:
+ *val = val32;
+ break;
+ }
+}
+
+static inline void xgene_pcie_cfg_in8(void __iomem *addr, u8 *val)
+{
+ u64 temp_addr = (u64)addr & ~0x3;
+ u32 val32;
+
+ val32 = readl((void __iomem *)temp_addr);
+
+ switch ((u64)addr & 0x3) {
+ case 3:
+ *val = val32 >> 24;
+ break;
+ case 2:
+ *val = val32 >> 16;
+ break;
+ case 1:
+ *val = val32 >> 8;
+ break;
+ case 0:
+ default:
+ *val = val32;
+ break;
+ }
+}
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */
+static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+
+ if (bus->number >= (bus->primary + 1))
+ return port->cfg_base + AXI_EP_CFG_ACCESS;
+
+ return port->cfg_base;
+}
+
+/* For Configuration request, RTDID register is used as Bus Number,
+ * Device Number and Function number of the header fields.
+ */
+static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ unsigned int b, d, f;
+ u32 rtdid_val = 0;
+
+ b = bus->number;
+ d = PCI_SLOT(devfn);
+ f = PCI_FUNC(devfn);
+
+ if (!pci_is_root_bus(bus))
+ rtdid_val = (b << 8) | (d << 3) | f;
+
+ writel(rtdid_val, port->csr_base + RTDID);
+ /* read the register back to ensure flush */
+ readl(port->csr_base + RTDID);
+}
+
+static int xgene_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
+ int offset, int len, u32 *val)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ void __iomem *addr;
+ u8 val8;
+ u16 val16;
+
+ if ((pci_is_root_bus(bus) && devfn != 0) || !port->link_up)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ xgene_pcie_set_rtdid_reg(bus, devfn);
+ addr = xgene_pcie_get_cfg_base(bus);
+ switch (len) {
+ case 1:
+ xgene_pcie_cfg_in8(addr + offset, &val8);
+ *val = val8;
+ break;
+ case 2:
+ xgene_pcie_cfg_in16(addr + offset, &val16);
+ *val = val16;
+ break;
+ default:
+ xgene_pcie_cfg_in32(addr + offset, val);
+ break;
+ }
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int xgene_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
+ int offset, int len, u32 val)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+ void __iomem *addr;
+
+ if ((pci_is_root_bus(bus) && devfn != 0) || !port->link_up)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ xgene_pcie_set_rtdid_reg(bus, devfn);
+ addr = xgene_pcie_get_cfg_base(bus);
+ switch (len) {
+ case 1:
+ xgene_pcie_cfg_out8(addr + offset, (u8) val);
+ break;
+ case 2:
+ xgene_pcie_cfg_out16(addr + offset, (u16) val);
+ break;
+ default:
+ xgene_pcie_cfg_out32(addr + offset, val);
+ break;
+ }
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops xgene_pcie_ops = {
+ .read = xgene_pcie_read_config,
+ .write = xgene_pcie_write_config
+};
+
+static void xgene_pcie_program_core(void __iomem *csr_base)
+{
+ u32 val;
+
+ val = readl(csr_base + BRIDGE_CFG_0);
+ val |= AER_OPTIONAL_ERROR_EN;
+ writel(val, csr_base + BRIDGE_CFG_0);
+ writel(0x0, csr_base + INTXSTATUSMASK);
+ val = readl(csr_base + BRIDGE_CTRL_1);
+ val = (val & ~0xffff) | XGENE_PCIE_DEV_CTRL;
+ writel(val, csr_base + BRIDGE_CTRL_1);
+}
+
+static u64 xgene_pcie_set_ib_mask(void __iomem *csr_base, u32 addr,
+ u32 flags, u64 size)
+{
+ u64 mask = (~(size - 1) & PCI_BASE_ADDRESS_MEM_MASK) | flags;
+ u32 val32 = 0;
+ u32 val;
+
+ val32 = readl(csr_base + addr);
+ val = (val32 & 0x0000ffff) | (lower_32_bits(mask) << 16);
+ writel(val, csr_base + addr);
+
+ val32 = readl(csr_base + addr + 0x04);
+ val = (val32 & 0xffff0000) | (lower_32_bits(mask) >> 16);
+ writel(val, csr_base + addr + 0x04);
+
+ val32 = readl(csr_base + addr + 0x04);
+ val = (val32 & 0x0000ffff) | (upper_32_bits(mask) << 16);
+ writel(val, csr_base + addr + 0x04);
+
+ val32 = readl(csr_base + addr + 0x08);
+ val = (val32 & 0xffff0000) | (upper_32_bits(mask) >> 16);
+ writel(val, csr_base + addr + 0x08);
+
+ return mask;
+}
+
+static void xgene_pcie_poll_linkup(struct xgene_pcie_port *port,
+ u32 *lanes, u32 *speed)
+{
+ void __iomem *csr_base = port->csr_base;
+ u32 val32;
+ u64 start_time, time;
+
+ /*
+ * A component enters the LTSSM Detect state within
+ * 20ms of the end of fundamental core reset.
+ */
+ msleep(XGENE_LTSSM_DETECT_WAIT);
+ port->link_up = 0;
+ start_time = jiffies;
+ do {
+ val32 = readl(csr_base + PCIECORE_CTLANDSTATUS);
+ if (val32 & LINK_UP_MASK) {
+ port->link_up = 1;
+ *speed = PIPE_PHY_RATE_RD(val32);
+ val32 = readl(csr_base + BRIDGE_STATUS_0);
+ *lanes = val32 >> 26;
+ }
+ time = jiffies_to_msecs(jiffies - start_time);
+ } while ((!port->link_up) || (time <= XGENE_LTSSM_L0_WAIT));
+}
+
+static void xgene_pcie_setup_root_complex(struct xgene_pcie_port *port)
+{
+ void __iomem *csr_base = port->csr_base;
+ u32 val;
+
+ val = (XGENE_PCIE_DEVICEID << 16) | XGENE_PCIE_VENDORID;
+ writel(val, csr_base + BRIDGE_CFG_0);
+
+ val = readl(csr_base + BRIDGE_CFG_1);
+ val &= ~CLASS_CODE_MASK;
+ val |= PCI_CLASS_BRIDGE_PCI << 16;
+ writel(val, csr_base + BRIDGE_CFG_1);
+
+ val = readl(csr_base + BRIDGE_CFG_14);
+ val |= SWITCH_PORT_MODE_MASK;
+ val &= ~PM_FORCE_RP_MODE_MASK;
+ writel(val, csr_base + BRIDGE_CFG_14);
+
+ val = readl(csr_base + BRIDGE_CTRL_5);
+ val &= ~DEVICE_PORT_TYPE_MASK;
+ val |= XGENE_PORT_TYPE_RC;
+ writel(val, csr_base + BRIDGE_CTRL_5);
+
+ val = readl(csr_base + BRIDGE_CTRL_2);
+ val |= ENABLE_ASPM;
+ writel(val, csr_base + BRIDGE_CTRL_2);
+
+ val = readl(csr_base + BRIDGE_CFG_32);
+ writel(val | (1 << 19), csr_base + BRIDGE_CFG_32);
+}
+
+/* Return 0 on success */
+static int xgene_pcie_init_ecc(struct xgene_pcie_port *port)
+{
+ void __iomem *csr_base = port->csr_base;
+ int timeout = XGENE_PCIE_TIMEOUT;
+ u32 val;
+
+ val = readl(csr_base + MEM_RAM_SHUTDOWN);
+ if (val == 0)
+ return 0;
+ writel(0x0, csr_base + MEM_RAM_SHUTDOWN);
+ do {
+ val = readl(csr_base + BLOCK_MEM_RDY);
+ udelay(1);
+ } while ((val != BLOCK_MEM_RDY_VAL) && timeout--);
+
+ return !(timeout > 0);
+}
+
+static int xgene_pcie_init_port(struct xgene_pcie_port *port)
+{
+ int rc;
+
+ port->clk = clk_get(port->dev, NULL);
+ if (IS_ERR_OR_NULL(port->clk)) {
+ dev_err(port->dev, "clock not available\n");
+ return -ENODEV;
+ }
+
+ rc = clk_prepare_enable(port->clk);
+ if (rc) {
+ dev_err(port->dev, "clock enable failed\n");
+ return rc;
+ }
+
+ rc = xgene_pcie_init_ecc(port);
+ if (rc) {
+ dev_err(port->dev, "memory init failed\n");
+ return rc;
+ }
+
+ return 0;
+}
+
+struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
+{
+ struct xgene_pcie_port *port = xgene_pcie_bus_to_port(bus);
+
+ return of_node_get(port->node);
+}
+
+static void xgene_pcie_fixup_bridge(struct pci_dev *dev)
+{
+ int i;
+
+ /* Hide the PCI host BARs from the kernel as their content doesn't
+ * fit well in the resource management
+ */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ dev->resource[i].start = dev->resource[i].end = 0;
+ dev->resource[i].flags = 0;
+ }
+ dev_info(&dev->dev, "Hiding X-Gene pci host bridge resources %s\n",
+ pci_name(dev));
+}
+DECLARE_PCI_FIXUP_HEADER(XGENE_PCIE_VENDORID, XGENE_PCIE_DEVICEID,
+ xgene_pcie_fixup_bridge);
+
+static int xgene_pcie_map_reg(struct xgene_pcie_port *port,
+ struct platform_device *pdev, u64 *cfg_addr)
+{
+ struct resource *res;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
+ port->csr_base = devm_ioremap_resource(port->dev, res);
+ if (IS_ERR(port->csr_base))
+ return PTR_ERR(port->csr_base);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+ port->cfg_base = devm_ioremap_resource(port->dev, res);
+ if (IS_ERR(port->cfg_base))
+ return PTR_ERR(port->cfg_base);
+ *cfg_addr = res->start;
+
+ return 0;
+}
+
+static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
+ u32 addr, u32 restype)
+{
+ struct resource *res;
+ void __iomem *base = port->csr_base + addr;
+ resource_size_t size;
+ u64 cpu_addr, pci_addr;
+ u64 mask = 0;
+ u32 min_size;
+ u32 flag = EN_REG;
+
+ if (restype == IORESOURCE_MEM) {
+ res = &port->mem.res;
+ pci_addr = port->mem.pci_addr;
+ min_size = SZ_128M;
+ } else {
+ res = &port->io.res;
+ pci_addr = port->io.pci_addr;
+ min_size = 128;
+ flag |= OB_LO_IO;
+ }
+ size = resource_size(res);
+ if (size >= min_size)
+ mask = ~(size - 1) | flag;
+ else
+ dev_warn(port->dev, "res size 0x%llx less than minimum 0x%x\n",
+ (u64)size, min_size);
+ cpu_addr = res->start;
+ writel(lower_32_bits(cpu_addr), base);
+ writel(upper_32_bits(cpu_addr), base + 0x04);
+ writel(lower_32_bits(mask), base + 0x08);
+ writel(upper_32_bits(mask), base + 0x0c);
+ writel(lower_32_bits(pci_addr), base + 0x10);
+ writel(upper_32_bits(pci_addr), base + 0x14);
+}
+
+static void xgene_pcie_setup_cfg_reg(void __iomem *csr_base, u64 addr)
+{
+ writel(lower_32_bits(addr), csr_base + CFGBARL);
+ writel(upper_32_bits(addr), csr_base + CFGBARH);
+ writel(EN_REG, csr_base + CFGCTL);
+}
+
+static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port,
+ u64 cfg_addr)
+{
+ struct device_node *np = port->node;
+ struct of_pci_range range;
+ struct of_pci_range_parser parser;
+ struct device *dev = port->dev;
+
+ if (of_pci_range_parser_init(&parser, np)) {
+ dev_err(dev, "missing ranges property\n");
+ return -EINVAL;
+ }
+
+ /* Get the I/O, memory ranges from DT */
+ for_each_of_pci_range(&parser, &range) {
+ struct resource *res = NULL;
+ u64 restype = range.flags & IORESOURCE_TYPE_BITS;
+ u64 end = range.cpu_addr + range.size - 1;
+ dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+ range.flags, range.cpu_addr, end, range.pci_addr);
+
+ switch (restype) {
+ case IORESOURCE_IO:
+ res = &port->io.res;
+ port->io.pci_addr = range.pci_addr;
+ of_pci_range_to_resource(&range, np, res);
+ xgene_pcie_setup_ob_reg(port, OMR1BARL, restype);
+ break;
+ case IORESOURCE_MEM:
+ res = &port->mem.res;
+ port->mem.pci_addr = range.pci_addr;
+ of_pci_range_to_resource(&range, np, res);
+ xgene_pcie_setup_ob_reg(port, OMR2BARL, restype);
+ break;
+ default:
+ dev_err(dev, "invalid io resource!");
+ return -EINVAL;
+ }
+ }
+ xgene_pcie_setup_cfg_reg(port->csr_base, cfg_addr);
+ return 0;
+}
+
+static void xgene_pcie_setup_pims(void *addr, u64 pim, u64 size)
+{
+ writel(lower_32_bits(pim), addr);
+ writel(upper_32_bits(pim) | EN_COHERENCY, addr + 0x04);
+ writel(lower_32_bits(size), addr + 0x10);
+ writel(upper_32_bits(size), addr + 0x14);
+}
+
+/*
+ * X-Gene PCIe support maximum 3 inbound memory regions
+ * This function helps to select a region based on size of region
+ */
+static int xgene_pcie_select_ib_reg(u8 *ib_reg_mask, u64 size)
+{
+ if ((size > 4) && (size < SZ_16M) && !(*ib_reg_mask & (1 << 1))) {
+ *ib_reg_mask |= (1 << 1);
+ return 1;
+ }
+
+ if ((size > SZ_1K) && (size < SZ_1T) && !(*ib_reg_mask & (1 << 0))) {
+ *ib_reg_mask |= (1 << 0);
+ return 0;
+ }
+
+ if ((size > SZ_1M) && (size < SZ_1T) && !(*ib_reg_mask & (1 << 2))) {
+ *ib_reg_mask |= (1 << 2);
+ return 2;
+ }
+ return -EINVAL;
+}
+
+static void xgene_pcie_setup_ib_reg(struct xgene_pcie_port *port,
+ struct of_pci_range *range, u8 *ib_reg_mask)
+{
+ void __iomem *csr_base = port->csr_base;
+ void __iomem *cfg_base = port->cfg_base;
+ void *bar_addr;
+ void *pim_addr;
+ u64 restype = range->flags & IORESOURCE_TYPE_BITS;
+ u64 cpu_addr = range->cpu_addr;
+ u64 pci_addr = range->pci_addr;
+ u64 size = range->size;
+ u64 mask = ~(size - 1) | EN_REG;
+ u32 flags = PCI_BASE_ADDRESS_MEM_TYPE_64;
+ u32 bar_low;
+ int region;
+
+ region = xgene_pcie_select_ib_reg(ib_reg_mask, range->size);
+ if (region < 0) {
+ dev_warn(port->dev, "invalid pcie dma-range config\n");
+ return;
+ }
+
+ if (restype == PCI_BASE_ADDRESS_MEM_PREFETCH)
+ flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
+ bar_low = pcie_bar_low_val((u32)cpu_addr, flags);
+ switch (region) {
+ case 0:
+ xgene_pcie_set_ib_mask(csr_base, BRIDGE_CFG_4, flags, size);
+ bar_addr = cfg_base + PCI_BASE_ADDRESS_0;
+ writel(bar_low, bar_addr);
+ writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
+ pim_addr = csr_base + PIM1_1L;
+ break;
+ case 1:
+ bar_addr = csr_base + IBAR2;
+ writel(bar_low, bar_addr);
+ writel(lower_32_bits(mask), csr_base + IR2MSK);
+ pim_addr = csr_base + PIM2_1L;
+ break;
+ case 2:
+ bar_addr = csr_base + IBAR3L;
+ writel(bar_low, bar_addr);
+ writel(upper_32_bits(cpu_addr), bar_addr + 0x4);
+ writel(lower_32_bits(mask), csr_base + IR3MSKL);
+ writel(upper_32_bits(mask), csr_base + IR3MSKL + 0x4);
+ pim_addr = csr_base + PIM3_1L;
+ break;
+ }
+
+ xgene_pcie_setup_pims(pim_addr, pci_addr, size);
+}
+
+static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
+ struct device_node *node)
+{
+ const int na = 3, ns = 2;
+ int rlen;
+
+ parser->node = node;
+ parser->pna = of_n_addr_cells(node);
+ parser->np = parser->pna + na + ns;
+
+ parser->range = of_get_property(node, "dma-ranges", &rlen);
+ if (!parser->range)
+ return -ENOENT;
+
+ parser->end = parser->range + rlen / sizeof(__be32);
+ return 0;
+}
+
+static int xgene_pcie_parse_map_dma_ranges(struct xgene_pcie_port *port)
+{
+ struct device_node *np = port->node;
+ struct of_pci_range range;
+ struct of_pci_range_parser parser;
+ struct device *dev = port->dev;
+ u8 ib_reg_mask = 0;
+
+ if (pci_dma_range_parser_init(&parser, np)) {
+ dev_err(dev, "missing dma-ranges property\n");
+ return -EINVAL;
+ }
+
+ /* Get the dma-ranges from DT */
+ for_each_of_pci_range(&parser, &range) {
+ u64 end = range.cpu_addr + range.size - 1;
+ dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
+ range.flags, range.cpu_addr, end, range.pci_addr);
+ xgene_pcie_setup_ib_reg(port, &range, &ib_reg_mask);
+ }
+ return 0;
+}
+
+static int xgene_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct xgene_pcie_port *pp = sys->private_data;
+ struct resource *io = &pp->realio;
+
+ io->start = sys->domain * SZ_64K;
+ io->end = io->start + SZ_64K;
+ io->flags = pp->io.res.flags;
+ io->name = "PCI IO";
+ pci_ioremap_io(io->start, pp->io.res.start);
+
+ pci_add_resource_offset(&sys->resources, io, sys->io_offset);
+ sys->mem_offset = pp->mem.res.start - pp->mem.pci_addr;
+ pci_add_resource_offset(&sys->resources, &pp->mem.res,
+ sys->mem_offset);
+ return 1;
+}
+
+static int __init xgene_pcie_probe_bridge(struct platform_device *pdev)
+{
+ struct device_node *np = of_node_get(pdev->dev.of_node);
+ struct xgene_pcie_port *port;
+ struct hw_pci xgene_pcie_hw;
+ u32 lanes = 0, speed = 0;
+ u64 cfg_addr = 0;
+ static u32 domain;
+ int ret;
+
+ port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+ port->node = np;
+ port->dev = &pdev->dev;
+
+ ret = xgene_pcie_map_reg(port, pdev, &cfg_addr);
+ if (ret)
+ return ret;
+
+ ret = xgene_pcie_init_port(port);
+ if (ret)
+ goto skip;
+ xgene_pcie_program_core(port->csr_base);
+ xgene_pcie_setup_root_complex(port);
+ ret = xgene_pcie_parse_map_ranges(port, cfg_addr);
+ if (ret)
+ goto skip;
+ ret = xgene_pcie_parse_map_dma_ranges(port);
+ if (ret)
+ goto skip;
+ xgene_pcie_poll_linkup(port, &lanes, &speed);
+skip:
+ if (!port->link_up)
+ dev_info(port->dev, "(rc) link down\n");
+ else
+ dev_info(port->dev, "(rc) x%d gen-%d link up\n",
+ lanes, speed + 1);
+ platform_set_drvdata(pdev, port);
+ memset(&xgene_pcie_hw, 0, sizeof(xgene_pcie_hw));
+ xgene_pcie_hw.domain = domain++;
+ xgene_pcie_hw.private_data = (void **)&port;
+ xgene_pcie_hw.nr_controllers = 1;
+ xgene_pcie_hw.setup = xgene_pcie_setup;
+ xgene_pcie_hw.map_irq = of_irq_parse_and_map_pci;
+ xgene_pcie_hw.ops = &xgene_pcie_ops;
+ pci_common_init(&xgene_pcie_hw);
+ return 0;
+}
+
+static const struct of_device_id xgene_pcie_match_table[] __initconst = {
+ {.compatible = "apm,xgene-pcie",},
+ {},
+};
+
+static struct platform_driver xgene_pcie_driver = {
+ .driver = {
+ .name = "xgene-pcie",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(xgene_pcie_match_table),
+ },
+ .probe = xgene_pcie_probe_bridge,
+};
+module_platform_driver(xgene_pcie_driver);
+
+MODULE_AUTHOR("Tanmay Inamdar <tinamdar@apm.com>");
+MODULE_DESCRIPTION("APM X-Gene PCIe driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH V3 2/4] arm64: dts: APM X-Gene PCIe device tree nodes
From: Tanmay Inamdar @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390599168-13150-1-git-send-email-tinamdar@apm.com>
This patch adds the device tree nodes for APM X-Gene PCIe controller and
PCIe clock interface. Since X-Gene SOC supports maximum 5 ports, 5 dts
nodes are added.
Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
---
arch/arm64/boot/dts/apm-mustang.dts | 8 ++
arch/arm64/boot/dts/apm-storm.dtsi | 155 +++++++++++++++++++++++++++++++++++
2 files changed, 163 insertions(+)
diff --git a/arch/arm64/boot/dts/apm-mustang.dts b/arch/arm64/boot/dts/apm-mustang.dts
index 1247ca1..507b6c9 100644
--- a/arch/arm64/boot/dts/apm-mustang.dts
+++ b/arch/arm64/boot/dts/apm-mustang.dts
@@ -24,3 +24,11 @@
reg = < 0x1 0x00000000 0x0 0x80000000 >; /* Updated by bootloader */
};
};
+
+&pcie0clk {
+ status = "ok";
+};
+
+&pcie0 {
+ status = "ok";
+};
diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..e579a6f 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -176,6 +176,161 @@
reg-names = "csr-reg";
clock-output-names = "eth8clk";
};
+
+ pcie0clk: pcie0clk at 1f2bc000 {
+ status = "disabled";
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2bc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie0clk";
+ };
+
+ pcie1clk: pcie1clk at 1f2cc000 {
+ status = "disabled";
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2cc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie1clk";
+ };
+
+ pcie2clk: pcie2clk at 1f2dc000 {
+ status = "disabled";
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f2dc000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie2clk";
+ };
+
+ pcie3clk: pcie3clk at 1f50c000 {
+ status = "disabled";
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f50c000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie3clk";
+ };
+
+ pcie4clk: pcie4clk at 1f51c000 {
+ status = "disabled";
+ compatible = "apm,xgene-device-clock";
+ #clock-cells = <1>;
+ clocks = <&socplldiv2 0>;
+ reg = <0x0 0x1f51c000 0x0 0x1000>;
+ reg-names = "csr-reg";
+ clock-output-names = "pcie4clk";
+ };
+ };
+
+ pcie0: pcie at 1f2b0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */
+ 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc3 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xc4 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xc5 0x1>;
+ clocks = <&pcie0clk 0>;
+ };
+
+ pcie1: pcie at 1f2c0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2c0000 0x0 0x00010000 /* Controller registers */
+ 0xd0 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x0 0x00000000 0xd0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0xd0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc8 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc9 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xca 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xcb 0x1>;
+ clocks = <&pcie1clk 0>;
+ };
+
+ pcie2: pcie at 1f2d0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2d0000 0x0 0x00010000 /* Controller registers */
+ 0x90 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x0 0x00000000 0x90 0x00000000 0x0 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0x90 0x10000000 0x0 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xce 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xcf 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xd0 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xd1 0x1>;
+ clocks = <&pcie2clk 0>;
+ };
+
+ pcie3: pcie at 1f500000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f500000 0x0 0x00010000 /* Controller registers */
+ 0xa0 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x0 0x00000000 0xa0 0x00000000 0x0 0x00010000 /* mem */
+ 0x02000000 0x0 0x10000000 0xa0 0x10000000 0x0 0x80000000>; /* io */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xd4 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xd5 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xd6 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xd7 0x1>;
+ clocks = <&pcie3clk 0>;
+ };
+
+ pcie4: pcie at 1f510000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f510000 0x0 0x00010000 /* Controller registers */
+ 0xc0 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x0 0x00000000 0xc0 0x00000000 0x0 0x00010000 /* io */
+ 0x02000000 0x0 0x10000000 0xc0 0x10000000 0x0 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xda 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xdb 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xdc 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xdd 0x1>;
+ clocks = <&pcie4clk 0>;
};
serial0: serial at 1c020000 {
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH V3 3/4] dt-bindings: pci: xgene pcie device tree bindings
From: Tanmay Inamdar @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390599168-13150-1-git-send-email-tinamdar@apm.com>
This patch adds the bindings for X-Gene PCIe driver. The driver resides
under 'drivers/pci/host/pci-xgene.c' file.
Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
---
.../devicetree/bindings/pci/xgene-pci.txt | 52 ++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci.txt
diff --git a/Documentation/devicetree/bindings/pci/xgene-pci.txt b/Documentation/devicetree/bindings/pci/xgene-pci.txt
new file mode 100644
index 0000000..60e4a54
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xgene-pci.txt
@@ -0,0 +1,52 @@
+* AppliedMicro X-Gene PCIe interface
+
+Required properties:
+- device_type: set to "pci"
+- compatible: should contain "xgene,pcie" to identify the core.
+- reg: A list of physical base address and length for each set of controller
+ registers. Must contain an entry for each entry in the reg-names
+ property.
+- reg-names: Must include the following entries:
+ "csr": controller configuration registers.
+ "cfg": pcie configuration space registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- ranges: ranges for the outbound memory, I/O regions.
+- dma-ranges: ranges for the inbound memory regions.
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+ to define the mapping of the PCIe interface to interrupt
+ numbers.
+- clocks: from common clock binding: handle to pci clock.
+
+Optional properties:
+- status: Either "ok" or "disabled".
+
+Example:
+
+SoC specific DT Entry:
+ pcie0: pcie at 1f2b0000 {
+ status = "disabled";
+ device_type = "pci";
+ compatible = "apm,xgene-pcie";
+ #interrupt-cells = <1>;
+ #size-cells = <2>;
+ #address-cells = <3>;
+ reg = < 0x00 0x1f2b0000 0x0 0x00010000 /* Controller registers */
+ 0xe0 0xd0000000 0x0 0x00200000>; /* PCI config space */
+ reg-names = "csr", "cfg";
+ ranges = <0x01000000 0x00 0x00000000 0xe0 0x00000000 0x00 0x00010000 /* io */
+ 0x02000000 0x00 0x10000000 0xe0 0x10000000 0x00 0x80000000>; /* mem */
+ dma-ranges = <0x42000000 0x40 0x00000000 0x40 0x00000000 0x40 0x00000000>;
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1
+ 0x0 0x0 0x0 0x2 &gic 0x0 0xc3 0x1
+ 0x0 0x0 0x0 0x3 &gic 0x0 0xc4 0x1
+ 0x0 0x0 0x0 0x4 &gic 0x0 0xc5 0x1>;
+ clocks = <&pcie0clk 0>;
+ };
+
+Board specific DT Entry:
+ &pcie0 {
+ status = "ok";
+ };
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH V3 4/4] MAINTAINERS: entry for APM X-Gene PCIe host driver
From: Tanmay Inamdar @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390599168-13150-1-git-send-email-tinamdar@apm.com>
Add entry for AppliedMicro X-Gene PCIe host driver.
Signed-off-by: Tanmay Inamdar <tinamdar@apm.com>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5c21402..721fec7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6539,6 +6539,13 @@ L: linux-pci at vger.kernel.org
S: Maintained
F: drivers/pci/host/*designware*
+PCI DRIVER FOR APPLIEDMICRO XGENE
+M: Tanmay Inamdar <tinamdar@apm.com>
+L: linux-pci at vger.kernel.org
+L: linux-arm-kernel at lists.infradead.org
+S: Maintained
+F: drivers/pci/host/pci-xgene.c
+
PCMCIA SUBSYSTEM
P: Linux PCMCIA Team
L: linux-pcmcia at lists.infradead.org
--
1.7.9.5
^ permalink raw reply related
* [PATCH v5 6/8] ARM: brcmstb: add misc. DT bindings for brcmstb
From: Marc C @ 2014-01-24 21:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124110300.GB814@e106331-lin.cambridge.arm.com>
Hi Mark,
>> +reboot
>> +-------
>> +Required properties
>> +
>> + - compatible
>> + The string property "brcm,brcmstb-reboot".
>> +
>> + - syscon
>> + A phandle / integer array that points to the syscon node which describes
>> + the general system reset registers.
>> + o a phandle to "sun_top_ctrl"
>> + o offset to the "reset source enable" register
>> + o offset to the "software master reset" register
>
> How variable are these values?
Very much so. Future chips will have different register maps. Because of this, Arnd
suggested that we use 'syscon' and 'regmap' to alleviate this maintenance burden.
>> +example:
>> + smpboot {
>> + compatible = "brcm,brcmstb-smpboot";
>> + syscon-cpu = <&hif_cpubiuctrl 0x88 0x178>;
>> + syscon-cont = <&hif_continuation>;
>> + };
>
> This looks odd. This doesn't seem like a device, but rather a grouping
> of disparate devices used for a particular software purpose.
>
>> +
>> +example:
>> + reboot {
>> + compatible = "brcm,brcmstb-reboot";
>> + syscon = <&sun_top_ctrl 0x304 0x308>;
>> + };
>
> As with smpboot, this seems odd.
Sure. Our H/W designers unfortunately didn't put the boot and restart registers into a
logical grouping, or standard register interface. Instead, they're all over the place.
How do you suggest naming the nodes to indicate this?
Thanks,
Marc C
On 01/24/2014 03:03 AM, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 03:30:50AM +0000, Marc Carino wrote:
>> Document the bindings that the Broadcom STB platform needs
>> for proper bootup.
>>
>> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> .../devicetree/bindings/arm/brcm-brcmstb.txt | 95 ++++++++++++++++++++
>> 1 files changed, 95 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt b/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>> new file mode 100644
>> index 0000000..3c436cc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>> @@ -0,0 +1,95 @@
>> +ARM Broadcom STB platforms Device Tree Bindings
>> +-----------------------------------------------
>> +Boards with Broadcom Brahma15 ARM-based BCMxxxx (generally BCM7xxx variants)
>> +SoC shall have the following DT organization:
>> +
>> +Required root node properties:
>> + - compatible: "brcm,bcm<chip_id>", "brcm,brcmstb"
>> +
>> +example:
>> +/ {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + model = "Broadcom STB (bcm7445)";
>> + compatible = "brcm,bcm7445", "brcm,brcmstb";
>> +
>> +Further, syscon nodes that map platform-specific registers used for general
>> +system control is required:
>> +
>> + - compatible: "brcm,bcm<chip_id>-sun-top-ctrl", "syscon"
>> + - compatible: "brcm,bcm<chip_id>-hif-cpubiuctrl", "syscon"
>> + - compatible: "brcm,bcm<chip_id>-hif-continuation", "syscon"
>> +
>> +example:
>> + rdb {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "simple-bus";
>> + ranges = <0 0x00 0xf0000000 0x1000000>;
>> +
>> + sun_top_ctrl: syscon at 404000 {
>> + compatible = "brcm,bcm7445-sun-top-ctrl", "syscon";
>> + reg = <0x404000 0x51c>;
>> + };
>> +
>> + hif_cpubiuctrl: syscon at 3e2400 {
>> + compatible = "brcm,bcm7445-hif-cpubiuctrl", "syscon";
>> + reg = <0x3e2400 0x5b4>;
>> + };
>> +
>> + hif_continuation: syscon at 452000 {
>> + compatible = "brcm,bcm7445-hif-continuation", "syscon";
>> + reg = <0x452000 0x100>;
>> + };
>> + };
>> +
>> +Lastly, nodes that allow for support of SMP initialization and reboot are
>> +required:
>> +
>> +smpboot
>> +-------
>> +Required properties:
>> +
>> + - compatible
>> + The string "brcm,brcmstb-smpboot".
>> +
>> + - syscon-cpu
>> + A phandle / integer array property which lets the BSP know the location
>> + of certain CPU power-on registers.
>> +
>> + The layout of the property is as follows:
>> + o a phandle to the "hif_cpubiuctrl" syscon node
>> + o offset to the base CPU power zone register
>> + o offset to the base CPU reset register
>
> How variable are these values?
>
>> +
>> + - syscon-cont
>> + A phandle pointing to the syscon node which describes the CPU boot
>> + continuation registers.
>> + o a phandle to the "hif_continuation" syscon node
>> +
>> +example:
>> + smpboot {
>> + compatible = "brcm,brcmstb-smpboot";
>> + syscon-cpu = <&hif_cpubiuctrl 0x88 0x178>;
>> + syscon-cont = <&hif_continuation>;
>> + };
>
> This looks odd. This doesn't seem like a device, but rather a grouping
> of disparate devices used for a particular software purpose.
>
>> +
>> +reboot
>> +-------
>> +Required properties
>> +
>> + - compatible
>> + The string property "brcm,brcmstb-reboot".
>> +
>> + - syscon
>> + A phandle / integer array that points to the syscon node which describes
>> + the general system reset registers.
>> + o a phandle to "sun_top_ctrl"
>> + o offset to the "reset source enable" register
>> + o offset to the "software master reset" register
>
> How variable are these values?
>
>> +
>> +example:
>> + reboot {
>> + compatible = "brcm,brcmstb-reboot";
>> + syscon = <&sun_top_ctrl 0x304 0x308>;
>> + };
>
> As with smpboot, this seems odd.
>
> Thanks,
> Mark.
>
^ permalink raw reply
* [PATCH v5 8/8] ARM: brcmstb: dts: add a reference DTS for Broadcom 7445
From: Marc C @ 2014-01-24 21:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140124110910.GC814@e106331-lin.cambridge.arm.com>
Hi Mark,
> As I commented on v3 [1], these are contiguous and can be described with
> a single entry:
>
> memory {
> device_type = "memory";
> reg = <0x0 0x00000000 0x0 0xc0000000>;
> };
>
> Is there any reason to have three entries?
Oopsies, sorry for missing that.
On BCM7445 and derivatives, there are 3 memory controllers. For each memory controller,
the first 1GB of physical DRAM is mapped to:
* 0x00_0000_0000
* 0x00_4000_0000
* 0x00_8000_0000
The memory controllers aren't interleaved. So, it's possible for the SoC to have a
discontiguous memory-mapping, where a designer chooses not to populate physical DRAM in
the middle.
The 'reg' property was broken-up to have each chunk of memory given a dedicated memblock.
All that said, if you like, I can rework the patch as you've suggested.
Thanks,
Marc C
On 01/24/2014 03:09 AM, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 03:30:52AM +0000, Marc Carino wrote:
>> Add a sample DTS which will allow bootup of a board populated
>> with the BCM7445 chip.
>>
>> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> arch/arm/boot/dts/bcm7445.dts | 111 +++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 111 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/boot/dts/bcm7445.dts
>>
>> diff --git a/arch/arm/boot/dts/bcm7445.dts b/arch/arm/boot/dts/bcm7445.dts
>> new file mode 100644
>> index 0000000..ffa3305
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/bcm7445.dts
>> @@ -0,0 +1,111 @@
>> +/dts-v1/;
>> +/include/ "skeleton.dtsi"
>> +
>> +/ {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + model = "Broadcom STB (bcm7445)";
>> + compatible = "brcm,bcm7445", "brcm,brcmstb";
>> + interrupt-parent = <&gic>;
>> +
>> + chosen {};
>> +
>> + memory {
>> + device_type = "memory";
>> + reg = <0x00 0x00000000 0x00 0x40000000>,
>> + <0x00 0x40000000 0x00 0x40000000>,
>> + <0x00 0x80000000 0x00 0x40000000>;
>> + };
>
> As I commented on v3 [1], these are contiguous and can be described with
> a single entry:
>
> memory {
> device_type = "memory";
> reg = <0x0 0x00000000 0x0 0xc0000000>;
> };
>
> Is there any reason to have three entries?
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/225899.html
>
^ permalink raw reply
* [PATCH 1/3] irqchip: orion: clear bridge cause register on init
From: Ezequiel Garcia @ 2014-01-24 21:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1390516686-2224-2-git-send-email-sebastian.hesselbarth@gmail.com>
On Thu, Jan 23, 2014 at 11:38:04PM +0100, Sebastian Hesselbarth wrote:
> It is good practice to mask and clear pending irqs on init. We already
> mask all irqs, so also clear the bridge irq cause register.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
> drivers/irqchip/irq-orion.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
> index e51d40031884..4137c3d15284 100644
> --- a/drivers/irqchip/irq-orion.c
> +++ b/drivers/irqchip/irq-orion.c
> @@ -180,8 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
> gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
> gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
>
> - /* mask all interrupts */
> + /* mask and clear all interrupts */
> writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
> + writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
>
This looks a bit bogus to me, now that we are clearing the cause upon
irq_startup(). Don't have a strong opinion, it's just that I fail to see
why we'd want or need this change...
--
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ 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