* [PATCH v3 REPOST 0/4] i2c : i2c-ibm-iic : use interrupts to perform the data transfer
From: jean-jacques hiblot @ 2013-12-20 15:12 UTC (permalink / raw)
To: wsa; +Cc: gregory.clement, jean-jacques hiblot, linuxppc-dev, linux-i2c
Hello Wolfram,
I'm reposting this patch set with the linux-ppc list in cc as the ibm I2c
controller is primarily (only?) available on ppc platforms.
This patch set aims to improve the performance of the driver for the IBM iic
controller by implementing the data transfer in the interrupt handler.
Using interrupts to trigger the data transfer reduces and make more
deterministic the latencies between indivdual bytes, and consequently reduces
the total transfer time,
In our test environement with multiple masters, this significantly reduces the
rate of i2c errors.
Changes since v2:
- cosmectics change (comments style, removed a hard-coded value)
- moved some parts from one patch to another
Changes since v1:
- split the patch in 4 individual patches. The code has been refactored a bit
to make the diff easier to read.
- changed some dev_dbg in dev_err or dev_warn when more appropriate
jean-jacques hiblot (4):
i2c: i2c-ibm-iic: cleanup.
i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
i2c: i2c-ibm-iic: Implements transfer abortion
i2c: i2c-ibm-iic: Implements a polling mode
drivers/i2c/busses/i2c-ibm_iic.c | 490 +++++++++++++++++++++++----------------
drivers/i2c/busses/i2c-ibm_iic.h | 20 +-
2 files changed, 309 insertions(+), 201 deletions(-)
--
1.8.4.2
^ permalink raw reply
* Re: [PATCH V4 10/10] powerpc, perf: Cleanup SW branch filter list look up
From: Anshuman Khandual @ 2013-12-20 11:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <20131209062147.B874D2C00C6@ozlabs.org>
On 12/09/2013 11:51 AM, Michael Ellerman wrote:
> On Wed, 2013-04-12 at 10:32:42 UTC, Anshuman Khandual wrote:
>> This patch adds enumeration for all available SW branch filters
>> in powerpc book3s code and also streamlines the look for the
>> SW branch filter entries while trying to figure out which all
>> branch filters can be supported in SW.
>
> This appears to patch code that was only added in 8/10 ?
>
> Was there any reason not to do it the right way from the beginning?
No reason, merged this into the 8/10th patch. Working on the V5 of
this patchset. Will send out a draft V5 version for early review.
Regards
Anshuman
^ permalink raw reply
* Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branch stack sampling framework
From: Anshuman Khandual @ 2013-12-20 11:01 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <20131209062146.EABBE2C00C1@ozlabs.org>
On 12/09/2013 11:51 AM, Michael Ellerman wrote:
> On Wed, 2013-04-12 at 10:32:40 UTC, Anshuman Khandual wrote:
>> This patch enables SW based post processing of BHRB captured branches
>> to be able to meet more user defined branch filtration criteria in perf
>> branch stack sampling framework. These changes increase the number of
>> branch filters and their valid combinations on any powerpc64 server
>> platform with BHRB support. Find the summary of code changes here.
>>
>> (1) struct cpu_hw_events
>>
>> Introduced two new variables track various filter values and mask
>>
>> (a) bhrb_sw_filter Tracks SW implemented branch filter flags
>> (b) filter_mask Tracks both (SW and HW) branch filter flags
>
> The name 'filter_mask' doesn't mean much to me. I'd rather it was 'bhrb_filter'.
Done.
>
>
>> (2) Event creation
>>
>> Kernel will figure out supported BHRB branch filters through a PMU call
>> back 'bhrb_filter_map'. This function will find out how many of the
>> requested branch filters can be supported in the PMU HW. It will not
>> try to invalidate any branch filter combinations. Event creation will not
>> error out because of lack of HW based branch filters. Meanwhile it will
>> track the overall supported branch filters in the "filter_mask" variable.
>>
>> Once the PMU call back returns kernel will process the user branch filter
>> request against available SW filters while looking at the "filter_mask".
>> During this phase all the branch filters which are still pending from the
>> user requested list will have to be supported in SW failing which the
>> event creation will error out.
>>
>> (3) SW branch filter
>>
>> During the BHRB data capture inside the PMU interrupt context, each
>> of the captured 'perf_branch_entry.from' will be checked for compliance
>> with applicable SW branch filters. If the entry does not conform to the
>> filter requirements, it will be discarded from the final perf branch
>> stack buffer.
>>
>> (4) Supported SW based branch filters
>>
>> (a) PERF_SAMPLE_BRANCH_ANY_RETURN
>> (b) PERF_SAMPLE_BRANCH_IND_CALL
>> (c) PERF_SAMPLE_BRANCH_ANY_CALL
>> (d) PERF_SAMPLE_BRANCH_COND
>>
>> Please refer patch to understand the classification of instructions into
>> these branch filter categories.
>>
>> (5) Multiple branch filter semantics
>>
>> Book3 sever implementation follows the same OR semantics (as implemented in
>> x86) while dealing with multiple branch filters at any point of time. SW
>> branch filter analysis is carried on the data set captured in the PMU HW.
>> So the resulting set of data (after applying the SW filters) will inherently
>> be an AND with the HW captured set. Hence any combination of HW and SW branch
>> filters will be invalid. HW based branch filters are more efficient and faster
>> compared to SW implemented branch filters. So at first the PMU should decide
>> whether it can support all the requested branch filters itself or not. In case
>> it can support all the branch filters in an OR manner, we dont apply any SW
>> branch filter on top of the HW captured set (which is the final set). This
>> preserves the OR semantic of multiple branch filters as required. But in case
>> where the PMU cannot support all the requested branch filters in an OR manner,
>> it should not apply any it's filters and leave it upto the SW to handle them
>> all. Its the PMU code's responsibility to uphold this protocol to be able to
>> conform to the overall OR semantic of perf branch stack sampling framework.
>
>
> I'd prefer this level of commentary was in a block comment in the code. It's
> much more likely to be seen by a future hacker than here in the commit log.
>
I felt it was pretty big to be inside the code blocks. Though I have improved in-code
documentation substantially in the next version.
>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 2de7d48..54d39a5 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -48,6 +48,8 @@ struct cpu_hw_events {
>>
>> /* BHRB bits */
>> u64 bhrb_hw_filter; /* BHRB HW branch filter */
>> + u64 bhrb_sw_filter; /* BHRB SW branch filter */
>> + u64 filter_mask; /* Branch filter mask */
>> int bhrb_users;
>> void *bhrb_context;
>> struct perf_branch_stack bhrb_stack;
>> @@ -400,6 +402,228 @@ static __u64 power_pmu_bhrb_to(u64 addr)
>> return target - (unsigned long)&instr + addr;
>> }
>>
>> +/*
>> + * Instruction opcode analysis
>> + *
>> + * Analyse instruction opcodes and classify them
>> + * into various branch filter options available.
>> + * This follows the standard semantics of OR which
>> + * means that instructions which conforms to `any`
>> + * of the requested branch filters get picked up.
>> + */
>> +static bool validate_instruction(unsigned int *addr, u64 bhrb_sw_filter)
>> +{
>
> "validate" is not a good name here. That implies that this routine identifies
> "valid" and "invalid" instructions - but that's not really correct.
>
Done.
validate_instruction --> check_instruction
> Also it's preferable to not use the same variable name for the local as for the
> cpuhw->bhrb_sw_filter global. Although technically it doesn't shadow the global
> it can still be confusing to a human, ie. me. A good name for the local would
> just be "sw_filter" because we know in this code that we're dealing with the
> BHRB.
>
Done.
local variable bhrb_sw_filter ---> sw_filter
>
>> + bool result = false;
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN) {
>> +
>> + /* XL-form instruction */
>> + if (instr_is_branch_xlform(*addr)) {
>> +
>> + /* LR should not be set */
>> + /*
>> + * Conditional and unconditional
>> + * branch to LR register.
>> + */
>> + if (is_xlform_lr(*addr))
>> + result = true;
>> + }
>> + }
>> + }
>
> is_xform_lr() implies instr_is_branch_xlform(), and once you get a hit you can
> short-circuit and exit the function, so this should boil down to just:
>
> if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)
> if (is_xlform_lr(*addr) && !is_branch_link_set(*addr))
> return true;
>
Done
>
> Having said that I think it should move into a routine in code-patching as I
> said in the comments to the previous patch.
>
Done
>
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) {
>> + /* XL-form instruction */
>> + if (instr_is_branch_xlform(*addr)) {
>> +
>> + /* LR should be set */
>> + if (is_branch_link_set(*addr)) {
>> + /*
>> + * Conditional and unconditional
>> + * branch to CTR.
>> + */
>> + if (is_xlform_ctr(*addr))
>> + result = true;
>> +
>> + /*
>> + * Conditional and unconditional
>> + * branch to LR.
>> + */
>> + if (is_xlform_lr(*addr))
>> + result = true;
>> +
>> + /*
>> + * Conditional and unconditional
>> + * branch to TAR.
>> + */
>> + if (is_xlform_tar(*addr))
>> + result = true;
>
> What other kind of XL-Form branch is there?
I am not sure. Do you know of any ?
>
>> + }
>> + }
>> + }
>
> The comments above all have a bogus leading space.
>
Rectified.
>> +
>> + /* Any-form branch */
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_ANY_CALL) {
>> + /* LR should be set */
>> + if (is_branch_link_set(*addr))
>> + result = true;
>
> Short circuit.
>
Rectified.
>> + }
>> +
>> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_COND) {
>> +
>> + /* I-form instruction - excluded */
>> + if (instr_is_branch_iform(*addr))
>> + goto out;
>> +
>> + /* B-form or XL-form instruction */
>> + if (instr_is_branch_bform(*addr) || instr_is_branch_xlform(*addr)) {
>> +
>> + /* Not branch always */
>> + if (!is_bo_always(*addr)) {
>> +
>> + /* Conditional branch to CTR register */
>> + if (is_bo_ctr(*addr))
>> + goto out;
>
> We might have discussed this but why not?
Did not get that, discuss what ?
>
>> +
>> + /* CR[BI] conditional branch with static hint */
>
> A conditional branch with a static hint is still a conditional branch?
>
No its not.
>> + if (is_bo_crbi_off(*addr) || is_bo_crbi_on(*addr)) {
>> + if (is_bo_crbi_hint(*addr))
>> + goto out;
>> + }
>> +
>> + result = true;
>> + }
>> + }
>> + }
>> +out:
>> + return result;
>> +}
>> +
>> +static bool check_instruction(u64 addr, u64 bhrb_sw_filter)
>> +{
>
>
> "check" is not a very descriptive name here, especially when "check" calls
> "validate".
>
> "filter" is also not good because a filter keeps some things and rejects others,
> and the directionality is not clear.
>
> I'd suggest "filter_selects_branch()" or just "keep_branch()".
>
keep_branch() now calls check_instruction()
>
>> + unsigned int instr;
>> + bool ret;
>> +
>> + if (bhrb_sw_filter == 0)
>> + return true;
>> +
>> + if (is_kernel_addr(addr)) {
>> + ret = validate_instruction((unsigned int *) addr, bhrb_sw_filter);
>
> No reason not to return directly here.
>
> That would then remove the need for an else block.
Done.
>
>> + } else {
>> + /*
>> + * Userspace address needs to be
>> + * copied first before analysis.
>> + */
>> + pagefault_disable();
>> + ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
>
> I suspect you borrowed this incantation from the callchain code. Unlike that
> code you don't fallback to reading the page tables directly.
>
> I'd rather see the accessor in the callchain code made generic and have you
> call it here.
You have mentioned to take care of this issue yourself.
>
>> +
>> + /*
>> + * If the instruction could not be accessible
>> + * from user space, we still 'okay' the entry.
>> + */
>> + if (ret) {
>> + pagefault_enable();
>> + return true;
>> + }
>> + pagefault_enable();
>> + ret = validate_instruction(&instr, bhrb_sw_filter);
>
> No reason not to return directly here.
>
Done.
>> + }
>> + return ret;
>> +}
>> +
>> +/*
>> + * Validate whether all requested branch filters
>> + * are getting processed either in the PMU or in SW.
>> + */
>> +static int match_filters(u64 branch_sample_type, u64 filter_mask)
>
> I don't really understand why we have this routine?
>
> We should implement the filter in HW if we can, or in SW. Which filters can't we
> implement in SW?
>
As of now in POWER8, we implement all the filters either in HW or SW. But this framework
allows us to have a combined HW and SW branch filter implementation where PMU HW support
ORing of branch filters (which is not true for POWER8). This functions just runs a sanity
check to make sure that we got all branch filters covered either in HW or SW. BTW changed
name of the function from "match_filters" to all_filters_covered.
>> +{
>> + u64 x;
>> +
>> + if (filter_mask == PERF_SAMPLE_BRANCH_ANY)
>> + return true;
>> +
>> + for_each_branch_sample_type(x) {
>> + if (!(branch_sample_type & x))
>> + continue;
>> + /*
>> + * Privilege filter requests have been already
>> + * taken care during the base PMU configuration.
>> + */
>> + if (x == PERF_SAMPLE_BRANCH_USER)
>> + continue;
>> + if (x == PERF_SAMPLE_BRANCH_KERNEL)
>> + continue;
>> + if (x == PERF_SAMPLE_BRANCH_HV)
>> + continue;
>> +
>> + /*
>> + * Requested filter not available either
>> + * in PMU or in SW.
>> + */
>> + if (!(filter_mask & x))
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> +/*
>> + * Required SW based branch filters
>> + *
>> + * This is called after figuring out what all branch filters the
>> + * PMU HW supports for the requested branch filter set. Here we
>> + * will go through all the SW implemented branch filters one by
>> + * one and pick them up if its not already supported in the PMU.
>> + */
>> +static u64 branch_filter_map(u64 branch_sample_type, u64 pmu_bhrb_filter,
>> + u64 *filter_mask)
>
> Whitespace is foobar here ^
>
Will fix it.
> This function deals exclusively with the software filter IIUI, but the name
> doesn't indicate that in any way.
Correct, changed the name from branch_filter_map to bhrb_sw_filter_map which
will complement bhrb_filter_map used for figuring out PMU supported filters.
>
> As far as the logic goes, you return the software filter value, as well as
> mutating the *filter_mask. And in all cases you make the same modification to
> both. That seems very dubious.
>
yeah, thats right. Because we will use cpuhw->bhrb_sw_filter to apply SW filters on
branch records once they are captured from BHRB and cpuhw->bhrb_filter
(cpuhw->filter_mask before) to track the overall coverage of branch filters either
in HW or SW. While we modify bhrb_filter (filter mask before) inside this function,
it previous contains branch filters which is promised to be implemented by the PMU
for this session.
> Shouldn't this routine just setup the software filter, and leave the upper
> level code to deal with the HW & SW filter values?
>
bhrb_filter (filter_mask before) runs through two functions one after the other. First
one being PMU specific bhrb_filter_map to figure out available HW filters for the session
and then bhrb_sw_filter_map to figure out available SW filters for the session. There is
no high level code dealing with bhrb_filter mask.
>> +{
>> + u64 branch_sw_filter = 0;
>> +
>> + /* No branch filter requested */
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY) {
>> + WARN_ON(pmu_bhrb_filter != 0);
>> + WARN_ON(*filter_mask != PERF_SAMPLE_BRANCH_ANY);
>> + return branch_sw_filter;
>> + }
>> +
>> + /*
>> + * PMU supported branch filters must also be implemented in SW
>> + * in the event when the PMU is unable to process them for some
>> + * reason. This all those branch filters can be satisfied with
>> + * SW implemented filters. But right now, there is now way to
>> + * initimate the user about this decision.
>
> Please proof read these comments, I don't entirely follow this one.
>
> You say "must also be implemented in SW" - but I think it's actually "must be
> implemented in SW", ie. the HW is not "also" implementing the filter.
>
> You say "in the event when" but I think you just mean "when" - the word "event"
> has a particular meaning in this code so you should only use it for that if at
> all possible.
>
> I don't follow "This all those".
>
> You should just drop the last sentence, there is never going to be any way to
> notify the user that their filter is implemented in HW vs SW, that's an
> implementation detail.
>
Took care of these observations.
>> + */
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_CALL)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_CALL;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_CALL;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_COND)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_COND;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_COND;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_ANY_RETURN)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_ANY_RETURN;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_ANY_RETURN;
>> + }
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
>> + if (!(pmu_bhrb_filter & PERF_SAMPLE_BRANCH_IND_CALL)) {
>> + branch_sw_filter |= PERF_SAMPLE_BRANCH_IND_CALL;
>> + *filter_mask |= PERF_SAMPLE_BRANCH_IND_CALL;
>> + }
>> + }
>> +
>> + return branch_sw_filter;
>> +}
>> +
>> /* Processing BHRB entries */
>> void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>> {
>> @@ -459,17 +683,29 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>> addr = 0;
>> }
>> cpuhw->bhrb_entries[u_index].from = addr;
>> +
>> + if (!check_instruction(cpuhw->
>> + bhrb_entries[u_index].from,
>> + cpuhw->bhrb_sw_filter))
>> + u_index--;
>> } else {
>> /* Branches to immediate field
>> (ie I or B form) */
>> cpuhw->bhrb_entries[u_index].from = addr;
>> - cpuhw->bhrb_entries[u_index].to =
>> - power_pmu_bhrb_to(addr);
>> - cpuhw->bhrb_entries[u_index].mispred = pred;
>> - cpuhw->bhrb_entries[u_index].predicted = ~pred;
>> + if (check_instruction(cpuhw->
>> + bhrb_entries[u_index].from,
>> + cpuhw->bhrb_sw_filter)) {
>> + cpuhw->bhrb_entries[u_index].
>> + to = power_pmu_bhrb_to(addr);
>> + cpuhw->bhrb_entries[u_index].
>> + mispred = pred;
>> + cpuhw->bhrb_entries[u_index].
>> + predicted = ~pred;
>> + } else {
>> + u_index--;
>> + }
>> }
>> u_index++;
>
>
> This code was already in need of some unindentation, and now it's just
> ridiculous.
>
> To start with at the beginning of this routine we have:
>
> while (..) {
> if (!val)
> break;
> else {
> // Bulk of the logic
> ...
> }
> }
>
> That should almost always become:
>
> while (..) {
> if (!val)
> break;
>
> // Bulk of the logic
> ...
> }
>
>
> But in this case that's not enough. Please send a precursor patch which moves
> this logic out into a helper function.
>
Done
>
>> -
>> }
>> }
>> cpuhw->bhrb_stack.nr = u_index;
>> @@ -1255,7 +1491,11 @@ nocheck:
>> if (has_branch_stack(event)) {
>> power_pmu_bhrb_enable(event);
>> cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
>> - event->attr.branch_sample_type);
>> + event->attr.branch_sample_type,
>> + &cpuhw->filter_mask);
>> + cpuhw->bhrb_sw_filter = branch_filter_map
>> + (event->attr.branch_sample_type,
>> + cpuhw->bhrb_hw_filter, &cpuhw->filter_mask);
>> }
>>
>> perf_pmu_enable(event->pmu);
>> @@ -1637,10 +1877,16 @@ static int power_pmu_event_init(struct perf_event *event)
>> err = power_check_constraints(cpuhw, events, cflags, n + 1);
>>
>> if (has_branch_stack(event)) {
>> - cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map(
>> - event->attr.branch_sample_type);
>> -
>> - if(cpuhw->bhrb_hw_filter == -1)
>> + cpuhw->bhrb_hw_filter = ppmu->bhrb_filter_map
>> + (event->attr.branch_sample_type,
>> + &cpuhw->filter_mask);
>> + cpuhw->bhrb_sw_filter = branch_filter_map
>> + (event->attr.branch_sample_type,
>> + cpuhw->bhrb_hw_filter,
>> + &cpuhw->filter_mask);
>> +
>> + if(!match_filters(event->attr.branch_sample_type,
>> + cpuhw->filter_mask))
>> return -EOPNOTSUPP;
>
> The above two hunks look too similar for my liking.
Moved the SW filter check below the else block to make it common for both the type branches.
Wanted to save some cycles by not accessing the user space (power_pmu_bhrb_to) in case
we know that the "from" is not going to pass the SW branch filter check.
^ permalink raw reply
* Re: [PATCH V4 07/10] powerpc, lib: Add new branch instruction analysis support functions
From: Anshuman Khandual @ 2013-12-20 10:06 UTC (permalink / raw)
To: Michael Ellerman
Cc: mikey, ak, linux-kernel, eranian, linuxppc-dev, acme, sukadev,
mingo
In-Reply-To: <52A6B028.3090001@linux.vnet.ibm.com>
On 12/10/2013 11:39 AM, Anshuman Khandual wrote:
> On 12/09/2013 11:51 AM, Michael Ellerman wrote:
>> On Wed, 2013-04-12 at 10:32:39 UTC, Anshuman Khandual wrote:
>>> Generic powerpc branch instruction analysis support added in the code
>>> patching library which will help the subsequent patch on SW based
>>> filtering of branch records in perf. This patch also converts and
>>> exports some of the existing local static functions through the header
>>> file to be used else where.
>>>
>>> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
>>> index a6f8c7a..8bab417 100644
>>> --- a/arch/powerpc/include/asm/code-patching.h
>>> +++ b/arch/powerpc/include/asm/code-patching.h
>>> @@ -22,6 +22,36 @@
>>> #define BRANCH_SET_LINK 0x1
>>> #define BRANCH_ABSOLUTE 0x2
>>>
>>> +#define XL_FORM_LR 0x4C000020
>>> +#define XL_FORM_CTR 0x4C000420
>>> +#define XL_FORM_TAR 0x4C000460
>>> +
>>> +#define BO_ALWAYS 0x02800000
>>> +#define BO_CTR 0x02000000
>>> +#define BO_CRBI_OFF 0x00800000
>>> +#define BO_CRBI_ON 0x01800000
>>> +#define BO_CRBI_HINT 0x00400000
>>> +
>>> +/* Forms of branch instruction */
>>> +int instr_is_branch_iform(unsigned int instr);
>>> +int instr_is_branch_bform(unsigned int instr);
>>> +int instr_is_branch_xlform(unsigned int instr);
>>> +
>>> +/* Classification of XL-form instruction */
>>> +int is_xlform_lr(unsigned int instr);
>>> +int is_xlform_ctr(unsigned int instr);
>>> +int is_xlform_tar(unsigned int instr);
>>> +
>>> +/* Branch instruction is a call */
>>> +int is_branch_link_set(unsigned int instr);
>>> +
>>> +/* BO field analysis (B-form or XL-form) */
>>> +int is_bo_always(unsigned int instr);
>>> +int is_bo_ctr(unsigned int instr);
>>> +int is_bo_crbi_off(unsigned int instr);
>>> +int is_bo_crbi_on(unsigned int instr);
>>> +int is_bo_crbi_hint(unsigned int instr);
>>
>>
>> I think this is the wrong API.
>>
>> We end up with all these micro checks, which don't actually encapsulate much,
>> and don't implement the logic perf needs. If we had another user for this level
>> of detail then it might make sense, but for a single user I think we're better
>> off just implementing the semantics it wants.
>>
>
> Having a comprehensive list of branch instruction analysis APIs which some other
> user can also use in the future does not make it wrong. Being more elaborate and
> detailed makes this one a better choice than the API you have suggested below.
>
>> So that would be something more like:
>>
>> bool instr_is_return_branch(unsigned int instr);
>> bool instr_is_conditional_branch(unsigned int instr);
>> bool instr_is_func_call(unsigned int instr);
>> bool instr_is_indirect_func_call(unsigned int instr);
>>
>>
>> These would then encapsulate something like the logic in your 8/10 patch. You
>> can hopefully also optimise the checking logic in each routine because you know
>> the exact semantics you're implementing.
Any ways, here is the patch which is will supersede the present patch for adding
required library functions. Hope this works.
commit 9d9f11a6b778b51732aaa0e7c9dea4be3385df56
Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Date: Fri Dec 20 13:46:15 2013 +0530
powerpc, lib: Add new branch analysis support functions
Generic powerpc branch analysis support added in the code patching
library which will help the subsequent patch on SW based filtering
of branch records in perf.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index a6f8c7a..15700b5 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -22,6 +22,16 @@
#define BRANCH_SET_LINK 0x1
#define BRANCH_ABSOLUTE 0x2
+#define XL_FORM_LR 0x4C000020
+#define XL_FORM_CTR 0x4C000420
+#define XL_FORM_TAR 0x4C000460
+
+#define BO_ALWAYS 0x02800000
+#define BO_CTR 0x02000000
+#define BO_CRBI_OFF 0x00800000
+#define BO_CRBI_ON 0x01800000
+#define BO_CRBI_HINT 0x00400000
+
unsigned int create_branch(const unsigned int *addr,
unsigned long target, int flags);
unsigned int create_cond_branch(const unsigned int *addr,
@@ -49,4 +59,10 @@ static inline unsigned long ppc_function_entry(void *func)
#endif
}
+/* Perf branch filters */
+bool instr_is_return_branch(unsigned int instr);
+bool instr_is_conditional_branch(unsigned int instr);
+bool instr_is_func_call(unsigned int instr);
+bool instr_is_indirect_func_call(unsigned int instr);
+
#endif /* _ASM_POWERPC_CODE_PATCHING_H */
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 17e5b23..ad39c58 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -77,6 +77,7 @@ static unsigned int branch_opcode(unsigned int instr)
return (instr >> 26) & 0x3F;
}
+/* Forms of branch instruction */
static int instr_is_branch_iform(unsigned int instr)
{
return branch_opcode(instr) == 18;
@@ -87,6 +88,140 @@ static int instr_is_branch_bform(unsigned int instr)
return branch_opcode(instr) == 16;
}
+static int instr_is_branch_xlform(unsigned int instr)
+{
+ return branch_opcode(instr) == 19;
+}
+
+/* Classification of XL-form instruction */
+static int is_xlform_lr(unsigned int instr)
+{
+ return (instr & XL_FORM_LR) == XL_FORM_LR;
+}
+
+static int is_xlform_ctr(unsigned int instr)
+{
+ return (instr & XL_FORM_CTR) == XL_FORM_CTR;
+}
+
+static int is_xlform_tar(unsigned int instr)
+{
+ return (instr & XL_FORM_TAR) == XL_FORM_TAR;
+}
+
+/* BO field analysis (B-form or XL-form) */
+static int is_bo_always(unsigned int instr)
+{
+ return (instr & BO_ALWAYS) == BO_ALWAYS;
+}
+
+static int is_bo_ctr(unsigned int instr)
+{
+ return (instr & BO_CTR) == BO_CTR;
+}
+
+static int is_bo_crbi_off(unsigned int instr)
+{
+ return (instr & BO_CRBI_OFF) == BO_CRBI_OFF;
+}
+
+static int is_bo_crbi_on(unsigned int instr)
+{
+ return (instr & BO_CRBI_ON) == BO_CRBI_ON;
+}
+
+static int is_bo_crbi_hint(unsigned int instr)
+{
+ return (instr & BO_CRBI_HINT) == BO_CRBI_HINT;
+}
+
+/* Link bit is set */
+static int is_branch_link_set(unsigned int instr)
+{
+ return (instr & BRANCH_SET_LINK) == BRANCH_SET_LINK;
+}
+
+/* Perf branch filters */
+bool instr_is_return_branch(unsigned int instr)
+{
+ /*
+ * Conditional and unconditional branch to LR register
+ * without seting the link register.
+ */
+ if (is_xlform_lr(instr) && !is_branch_link_set(instr))
+ return true;
+
+ return false;
+}
+
+bool instr_is_conditional_branch(unsigned int instr)
+{
+ /* I-form instruction - excluded */
+ if (instr_is_branch_iform(instr))
+ return false;
+
+ /* B-form or XL-form instruction */
+ if (instr_is_branch_bform(instr) || instr_is_branch_xlform(instr)) {
+
+ /* Not branch always */
+ if (!is_bo_always(instr)) {
+
+ /* Conditional branch to CTR register */
+ if (is_bo_ctr(instr))
+ return false;
+
+ /* CR[BI] conditional branch with static hint */
+ if (is_bo_crbi_off(instr) || is_bo_crbi_on(instr)) {
+ if (is_bo_crbi_hint(instr))
+ return false;;
+ }
+ return true;
+ }
+ }
+ return false;
+}
+
+bool instr_is_func_call(unsigned int instr)
+{
+ /* LR should be set */
+ if (is_branch_link_set(instr))
+ return true;
+
+ return false;
+}
+
+bool instr_is_indirect_func_call(unsigned int instr)
+{
+ /* XL-form instruction */
+ if (instr_is_branch_xlform(instr)) {
+
+ /* LR should be set */
+ if (is_branch_link_set(instr)) {
+ /*
+ * Conditional and unconditional
+ * branch to CTR register.
+ */
+ if (is_xlform_ctr(instr))
+ return true;
+
+ /*
+ * Conditional and unconditional
+ * branch to LR register.
+ */
+ if (is_xlform_lr(instr))
+ return true;
+
+ /*
+ * Conditional and unconditional
+ * branch to TAR register.
+ */
+ if (is_xlform_tar(instr))
+ return true;
+ }
+ }
+ return false;
+}
+
int instr_is_relative_branch(unsigned int instr)
{
if (instr & BRANCH_ABSOLUTE)
^ permalink raw reply related
* [RFC] linux/pci: move pci_platform_pm_ops to linux/pci.h
From: Dongsheng Wang @ 2013-12-20 10:03 UTC (permalink / raw)
To: rjw, bhelgaas
Cc: linux-pm, roy.zang, Wang Dongsheng, linux-pci, scottwood,
linuxppc-dev
From: Wang Dongsheng <dongsheng.wang@freescale.com>
make Freescale platform use pci_platform_pm_ops struct.
Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
If device's not set power state, we will use this interface to put the
device's into the correct state.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9c91ecc..48f8b1a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -33,36 +33,6 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
#endif
int pci_probe_reset_function(struct pci_dev *dev);
-/**
- * struct pci_platform_pm_ops - Firmware PM callbacks
- *
- * @is_manageable: returns 'true' if given device is power manageable by the
- * platform firmware
- *
- * @set_state: invokes the platform firmware to set the device's power state
- *
- * @choose_state: returns PCI power state of given device preferred by the
- * platform; to be used during system-wide transitions from a
- * sleeping state to the working state and vice versa
- *
- * @sleep_wake: enables/disables the system wake up capability of given device
- *
- * @run_wake: enables/disables the platform to generate run-time wake-up events
- * for given device (the device's wake-up capability has to be
- * enabled by @sleep_wake for this feature to work)
- *
- * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
- */
-struct pci_platform_pm_ops {
- bool (*is_manageable)(struct pci_dev *dev);
- int (*set_state)(struct pci_dev *dev, pci_power_t state);
- pci_power_t (*choose_state)(struct pci_dev *dev);
- int (*sleep_wake)(struct pci_dev *dev, bool enable);
- int (*run_wake)(struct pci_dev *dev, bool enable);
-};
-
-int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
void pci_power_up(struct pci_dev *dev);
void pci_disable_enabled_device(struct pci_dev *dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1084a15..20e07b8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -365,6 +365,37 @@ struct pci_dev {
size_t romlen; /* Length of ROM if it's not from the BAR */
};
+/**
+ * struct pci_platform_pm_ops - Firmware PM callbacks
+ *
+ * @is_manageable: returns 'true' if given device is power manageable by the
+ * platform firmware
+ *
+ * @set_state: invokes the platform firmware to set the device's power state
+ *
+ * @choose_state: returns PCI power state of given device preferred by the
+ * platform; to be used during system-wide transitions from a
+ * sleeping state to the working state and vice versa
+ *
+ * @sleep_wake: enables/disables the system wake up capability of given device
+ *
+ * @run_wake: enables/disables the platform to generate run-time wake-up events
+ * for given device (the device's wake-up capability has to be
+ * enabled by @sleep_wake for this feature to work)
+ *
+ * If given platform is generally capable of power managing PCI devices, all of
+ * these callbacks are mandatory.
+ */
+struct pci_platform_pm_ops {
+ bool (*is_manageable)(struct pci_dev *dev);
+ int (*set_state)(struct pci_dev *dev, pci_power_t state);
+ pci_power_t (*choose_state)(struct pci_dev *dev);
+ int (*sleep_wake)(struct pci_dev *dev, bool enable);
+ int (*run_wake)(struct pci_dev *dev, bool enable);
+};
+
+int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
+
static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
{
#ifdef CONFIG_PCI_IOV
--
1.8.5
^ permalink raw reply related
* [PATCH v2] powerpc/512x: dts: disable MPC5125 usb module
From: Matteo Facchinetti @ 2013-12-20 9:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: gsi, agust
At the moment the USB controller's pin muxing is not setup
correctly and causes a kernel panic upon system startup, so
disable the USB1 device tree node in the MPC5125 tower board
dts file.
The USB controller is connected to an USB3320 ULPI transceiver
and the device tree should receive an update to reflect correct
dependencies and required initialization data before the USB1
node can get re-enabled.
Signed-off-by: Matteo Facchinetti <matteo.facchinetti@sirius-es.it>
---
v2:
* improve the text of the commit as suggested by Gerhard Sittig <gsi@denx.de>
* put the 'status = "disabled"' to the last line in the list of properties
* rewiew the comment related to USB1 device tree node
---
arch/powerpc/boot/dts/mpc5125twr.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/powerpc/boot/dts/mpc5125twr.dts b/arch/powerpc/boot/dts/mpc5125twr.dts
index 806479f..2fa1d17 100644
--- a/arch/powerpc/boot/dts/mpc5125twr.dts
+++ b/arch/powerpc/boot/dts/mpc5125twr.dts
@@ -229,6 +229,10 @@
reg = <0xA000 0x1000>;
};
+ // disable USB1 port
+ // TODO:
+ // correct pinmux config and fix USB3320 ulpi dependency
+ // before re-enable it
usb@3000 {
compatible = "fsl,mpc5121-usb2-dr";
reg = <0x3000 0x400>;
@@ -239,6 +243,7 @@
phy_type = "ulpi";
clocks = <&clks MPC512x_CLK_USB1>;
clock-names = "ipg";
+ status = "disabled";
};
// 5125 PSCs are not 52xx or 5121 PSC compatible
--
1.8.3.2
^ permalink raw reply related
* Re: [v3, 3/7] powerpc: enable the relocatable support for the fsl booke 32bit kernel
From: Kevin Hao @ 2013-12-20 7:43 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <20131218234825.GA6959@home.buserror.net>
[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]
On Wed, Dec 18, 2013 at 05:48:25PM -0600, Scott Wood wrote:
> On Wed, Aug 07, 2013 at 09:18:31AM +0800, Kevin Hao wrote:
> > This is based on the codes in the head_44x.S. The difference is that
> > the init tlb size we used is 64M. With this patch we can only load the
> > kernel at address between memstart_addr ~ memstart_addr + 64M. We will
> > fix this restriction in the following patches.
>
> Which following patch fixes the restriction? With all seven patches
> applied, I was still only successful booting within 64M of memstart_addr.
There is bug in this patch series when booting above the 64M. It seems
that I missed to test this previously. Sorry for that. With the following
change I can boot the kernel at 0x5000000.
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index 048d716ae706..ce0c7d7db6c3 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -171,11 +171,10 @@ unsigned long calc_cam_sz(unsigned long ram, unsigned long virt,
return 1UL << camsize;
}
-unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
+static unsigned long map_mem_in_cams_addr(phys_addr_t phys, unsigned long virt,
+ unsigned long ram, int max_cam_idx)
{
int i;
- unsigned long virt = PAGE_OFFSET;
- phys_addr_t phys = memstart_addr;
unsigned long amount_mapped = 0;
/* Calculate CAM values */
@@ -195,6 +194,14 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
return amount_mapped;
}
+unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx)
+{
+ unsigned long virt = PAGE_OFFSET;
+ phys_addr_t phys = memstart_addr;
+
+ return map_mem_in_cams_addr(phys, virt, ram, max_cam_idx);
+}
+
#ifdef CONFIG_PPC32
#if defined(CONFIG_LOWMEM_CAM_NUM_BOOL) && (CONFIG_LOWMEM_CAM_NUM >= NUM_TLBCAMS)
@@ -289,7 +296,11 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
is_second_reloc = 1;
n = switch_to_as1();
/* map a 64M area for the second relocation */
- map_mem_in_cams(0x4000000UL, CONFIG_LOWMEM_CAM_NUM);
+ if (memstart_addr > start)
+ map_mem_in_cams(0x4000000, CONFIG_LOWMEM_CAM_NUM);
+ else
+ map_mem_in_cams_addr(start, PAGE_OFFSET - offset,
+ 0x4000000, CONFIG_LOWMEM_CAM_NUM);
restore_to_as0(n, offset, __va(dt_ptr));
/* We should never reach here */
panic("Relocation error");
I will do more test and then create a new spin to merge this change and rebase
on the latest kernel. Thanks for the review.
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* RE: [PATCH 4/5] powerpc/fsl-booke: Add initial T208x QDS board support
From: Shengzhou.Liu @ 2013-12-20 6:42 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1387310222.10013.439.camel@snotra.buserror.net>
DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIx
DQo+IFNlbnQ6IFdlZG5lc2RheSwgRGVjZW1iZXIgMTgsIDIwMTMgMzo1NyBBTQ0KPiBUbzogTGl1
IFNoZW5nemhvdS1CMzY2ODUNCj4gQ2M6IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnDQo+
IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC81XSBwb3dlcnBjL2ZzbC1ib29rZTogQWRkIGluaXRpYWwg
VDIwOHggUURTIGJvYXJkDQo+IHN1cHBvcnQNCj4gDQo+IE9uIFdlZCwgMjAxMy0xMi0xMSBhdCAx
OToxOSArMDgwMCwgU2hlbmd6aG91IExpdSB3cm90ZToNCj4gPiArCQlib2FyZGN0cmw6IGJvYXJk
LWNvbnRyb2xAMywwIHsNCj4gPiArCQkJI2FkZHJlc3MtY2VsbHMgPSA8MT47DQo+ID4gKwkJCSNz
aXplLWNlbGxzID0gPDE+Ow0KPiA+ICsJCQljb21wYXRpYmxlID0gImZzbCxmcGdhLXFpeGlzIjsN
Cj4gPiArCQkJcmVnID0gPDMgMCAweDMwMD47DQo+ID4gKwkJCXJhbmdlcyA9IDwwIDMgMCAweDMw
MD47DQo+ID4gKwkJfTsNCj4gDQo+IFdoeSBkbyB5b3UgaGF2ZSByYW5nZXMgYW5kICNhZGRyZXNz
LWNlbGxzLyNzaXplLWNlbGxzIGhlcmU/ICBXaGVuIHdvdWxkDQo+IHRoZXJlIGV2ZXIgYmUgYSBj
aGlsZCBub2RlPw0KDQpbU2hlbmd6aG91XSBUaGVyZSBhcmUgbXVsdGlwbGUgY2hpbGQgbm9kZXMg
Zm9yIHRoaXMoaW4gYSBzZXBhcmF0ZSBEUEFBLXJlbGF0ZWQgcGF0Y2gsIGJ1dCB3aG9sZSBEUEFB
IG1vZHVsZSBoYXMgbm90IGJlZW4gdXBzdHJlYW1lZCB5ZXQgc28gZmFyKQ0KDQo+IA0KPiA+ICsJ
fTsNCj4gPiArDQo+ID4gKwltZW1vcnkgew0KPiA+ICsJCWRldmljZV90eXBlID0gIm1lbW9yeSI7
DQo+ID4gKwl9Ow0KPiA+ICsNCj4gPiArCWRjc3I6IGRjc3JAZjAwMDAwMDAwIHsNCj4gPiArCQly
YW5nZXMgPSA8MHgwMDAwMDAwMCAweGYgMHgwMDAwMDAwMCAweDAxMDcyMDAwPjsNCj4gPiArCX07
DQo+ID4gKw0KPiA+ICsJc29jOiBzb2NAZmZlMDAwMDAwIHsNCj4gPiArCQlyYW5nZXMgPSA8MHgw
MDAwMDAwMCAweGYgMHhmZTAwMDAwMCAweDEwMDAwMDA+Ow0KPiA+ICsJCXJlZyA9IDwweGYgMHhm
ZTAwMDAwMCAwIDB4MDAwMDEwMDA+Ow0KPiA+ICsJCXNwaUAxMTAwMDAgew0KPiA+ICsJCQlmbGFz
aEAwIHsNCj4gPiArCQkJCSNhZGRyZXNzLWNlbGxzID0gPDE+Ow0KPiA+ICsJCQkJI3NpemUtY2Vs
bHMgPSA8MT47DQo+ID4gKwkJCQljb21wYXRpYmxlID0gInNwYW5zaW9uLHMyNXNsMTI4MDEiOw0K
PiA+ICsJCQkJcmVnID0gPDA+Ow0KPiA+ICsJCQkJc3BpLW1heC1mcmVxdWVuY3kgPSA8NDAwMDAw
MDA+OyAvKiBpbnB1dCBjbG9jayAqLw0KPiA+ICsJCQkJcGFydGl0aW9uQHUtYm9vdCB7DQo+ID4g
KwkJCQkJbGFiZWwgPSAiU1BJIFUtQm9vdCI7DQo+ID4gKwkJCQkJcmVnID0gPDB4MDAwMDAwMDAg
MHgwMDEwMDAwMD47DQo+ID4gKwkJCQkJcmVhZC1vbmx5Ow0KPiA+ICsJCQkJfTsNCj4gPiArCQkJ
CXBhcnRpdGlvbkBrZXJuZWwgew0KPiA+ICsJCQkJCWxhYmVsID0gIlNQSSBLZXJuZWwiOw0KPiA+
ICsJCQkJCXJlZyA9IDwweDAwMTAwMDAwIDB4MDA1MDAwMDA+Ow0KPiA+ICsJCQkJCXJlYWQtb25s
eTsNCj4gPiArCQkJCX07DQo+ID4gKwkJCQlwYXJ0aXRpb25AZHRiIHsNCj4gPiArCQkJCQlsYWJl
bCA9ICJTUEkgRFRCIjsNCj4gPiArCQkJCQlyZWcgPSA8MHgwMDYwMDAwMCAweDAwMTAwMDAwPjsN
Cj4gPiArCQkJCQlyZWFkLW9ubHk7DQo+ID4gKwkJCQl9Ow0KPiA+ICsJCQkJcGFydGl0aW9uQGZz
IHsNCj4gPiArCQkJCQlsYWJlbCA9ICJTUEkgRmlsZSBTeXN0ZW0iOw0KPiA+ICsJCQkJCXJlZyA9
IDwweDAwNzAwMDAwIDB4MDA5MDAwMDA+Ow0KPiA+ICsJCQkJfTsNCj4gDQo+IFRoZXNlIGFyZSBu
b3QgdmFsaWQgdW5pdCBhZGRyZXNzZXMgLS0gYW5kIHRoZSBrZXJuZWwvZHRiIHNob3VsZCBub3Qg
YmUgcmVhZC1vbmx5LiAgDQo+IERvIHlvdSByZWFsbHkgd2FudCB0byBkZWRpY2F0ZSBhIHdob2xl
IG1lYmlieXRlIHRvIHRoZSBkdGIsIGdpdmVuIHRoYXQNCj4gdGhlIGZsYXNoIGlzIG9ubHkgMTYg
TWlCIHRvdGFsPw0KPiANCj4gQWN0dWFsbHksIGxldCdzIHBsZWFzZSBqdXN0IHN0b3AgcHV0dGlu
ZyBwYXJ0aXRpb25zIGluIHRoZSBkdHMuICBFaXRoZXINCj4gdXNlIG10ZHBhcnRzIG9uIHRoZSBj
b21tYW5kIGxpbmUsIG9yIGhhdmUgVS1Cb290IGZpbGwgaW4gdGhlIHBhcnRpdGlvbg0KPiBpbmZv
ICh0aGVyZSBpcyBjb2RlIGluIFUtQm9vdCB0byBkbyB0aGlzIGJhc2VkIG9uIHRoZSBtdGRwYXJ0
cyBlbnYNCj4gdmFyaWFibGUpLg0KPiANCiANCltTaGVuZ3pob3VdIG9rYXksIHdpbGwgdXNlIG10
ZHBhcnRzIGluc3RlYWQgb2YgdGhpcyB3YXkuDQoNCg==
^ permalink raw reply
* Re: [PATCH] powerpc: book3s: kvm: Use the saved dsisr and dar values
From: Alexander Graf @ 2013-12-20 6:38 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org mailing list
In-Reply-To: <8738lo9lzg.fsf@linux.vnet.ibm.com>
> Am 20.12.2013 um 05:37 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet=
.ibm.com>:
>=20
> Alexander Graf <agraf@suse.de> writes:
>=20
>>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vn=
et.ibm.com>:
>>>=20
>>> Alexander Graf <agraf@suse.de> writes:
>>>=20
>>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm=
.com> wrote:
>>>>>=20
>>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>>=20
>>>>> Don't try to compute these values.
>>>>>=20
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>>> ---
>>>>>=20
>>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So=
may be
>>>>> we need a variant of this patch. But with this and the additional patc=
h
>>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a=
Little Endian
>>>>> PR guest to boot.
>>>>=20
>>>> It's quite easy to find out - git blame tells you all the history and p=
oints you to commit ca7f4203b.
>>>>=20
>>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>>> Author: Alexander Graf <agraf@suse.de>
>>>> Date: Wed Mar 24 21:48:28 2010 +0100
>>>>=20
>>>> KVM: PPC: Implement alignment interrupt
>>>>=20
>>>> Mac OS X has some applications - namely the Finder - that require ali=
gnment
>>>> interrupts to work properly. So we need to implement them.
>>>>=20
>>>> But the spec for 970 and 750 also looks different. While 750 requires=
the
>>>> DSISR and DAR fields to reflect some instruction bits (DSISR) and the=
fault
>>>> address (DAR), the 970 declares this as an optional feature. So we ne=
ed
>>>> to reconstruct DSISR and DAR manually.
>>>>=20
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>>=20
>>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR o=
f the faulting instruction" as otherwise I wouldn't have implemented it.
>>>>=20
>>>> So this is clearly a nack on this patch :).
>>>=20
>>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we ne=
ed
>>> to do that ? According to Paul we should always find DAR.
>>=20
>> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a p=
roper DAR value - we can then remove that part.
>>=20
>> But for DSISR I'm not convinced CPUs above 970 handle this
>> correctly. So we would at least need a guest cpu check to find out
>> whether the vcpu expects a working dsisr and emulate it then.
>=20
>>=20
>> I don't really fully understand the problem though. Why does the
>> calculation break at all for you?
>=20
>=20
> IIRC this was to get little endian PR setup to work. This is to avoid
> handling new instructions, because in little endian mode we get
> alignment interrupt for a larger instructon set
Ok, please limit dar/dsisr calculation to book3s_32 vcpus then :).
Alex
>=20
> -aneesh
>=20
^ permalink raw reply
* [PATCH V4] powerpc/kernel/sysfs: cleanup set up macros for PMC/non-PMC SPRs
From: Madhavan Srinivasan @ 2013-12-20 5:01 UTC (permalink / raw)
To: benh; +Cc: michael, olof, Madhavan Srinivasan, linuxppc-dev
Currently PMC (Performance Monitor Counter) setup macros are used
for other SPRs. Since not all SPRs are PMC related, this patch
modifies the exisiting macro and uses it to setup both PMC and
non PMC SPRs accordingly.
V4 changes:
1) No logic changes, just rebase with upstream.
V3 changes:
1) No logic change, just renamed generic macro and removed #define for
empty string
2) Changes in the comment to explain better.
V2 changes:
1) Modified SYSFS_PMCSETUP to a generic macro with additional parameter
2) Added PMC and SPR macro to call the generic macro
3) Changes in the comment to explain better.
Acked-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Olof Johansson <olof@lixom.net>
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/kernel/sysfs.c | 72 +++++++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index b4e6676..cad777e 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -108,14 +108,14 @@ void ppc_enable_pmcs(void)
}
EXPORT_SYMBOL(ppc_enable_pmcs);
-#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+#define __SYSFS_SPRSETUP(NAME, ADDRESS, EXTRA) \
static void read_##NAME(void *val) \
{ \
*(unsigned long *)val = mfspr(ADDRESS); \
} \
static void write_##NAME(void *val) \
{ \
- ppc_enable_pmcs(); \
+ EXTRA; \
mtspr(ADDRESS, *(unsigned long *)val); \
} \
static ssize_t show_##NAME(struct device *dev, \
@@ -140,6 +140,10 @@ static ssize_t __used \
return count; \
}
+#define SYSFS_PMCSETUP(NAME, ADDRESS) \
+ __SYSFS_SPRSETUP(NAME, ADDRESS, ppc_enable_pmcs())
+#define SYSFS_SPRSETUP(NAME, ADDRESS) \
+ __SYSFS_SPRSETUP(NAME, ADDRESS, )
/* Let's define all possible registers, we'll only hook up the ones
* that are implemented on the current processor
@@ -175,10 +179,10 @@ SYSFS_PMCSETUP(pmc7, SPRN_PMC7);
SYSFS_PMCSETUP(pmc8, SPRN_PMC8);
SYSFS_PMCSETUP(mmcra, SPRN_MMCRA);
-SYSFS_PMCSETUP(purr, SPRN_PURR);
-SYSFS_PMCSETUP(spurr, SPRN_SPURR);
-SYSFS_PMCSETUP(dscr, SPRN_DSCR);
-SYSFS_PMCSETUP(pir, SPRN_PIR);
+SYSFS_SPRSETUP(purr, SPRN_PURR);
+SYSFS_SPRSETUP(spurr, SPRN_SPURR);
+SYSFS_SPRSETUP(dscr, SPRN_DSCR);
+SYSFS_SPRSETUP(pir, SPRN_PIR);
/*
Lets only enable read for phyp resources and
@@ -249,34 +253,34 @@ SYSFS_PMCSETUP(pa6t_pmc3, SPRN_PA6T_PMC3);
SYSFS_PMCSETUP(pa6t_pmc4, SPRN_PA6T_PMC4);
SYSFS_PMCSETUP(pa6t_pmc5, SPRN_PA6T_PMC5);
#ifdef CONFIG_DEBUG_KERNEL
-SYSFS_PMCSETUP(hid0, SPRN_HID0);
-SYSFS_PMCSETUP(hid1, SPRN_HID1);
-SYSFS_PMCSETUP(hid4, SPRN_HID4);
-SYSFS_PMCSETUP(hid5, SPRN_HID5);
-SYSFS_PMCSETUP(ima0, SPRN_PA6T_IMA0);
-SYSFS_PMCSETUP(ima1, SPRN_PA6T_IMA1);
-SYSFS_PMCSETUP(ima2, SPRN_PA6T_IMA2);
-SYSFS_PMCSETUP(ima3, SPRN_PA6T_IMA3);
-SYSFS_PMCSETUP(ima4, SPRN_PA6T_IMA4);
-SYSFS_PMCSETUP(ima5, SPRN_PA6T_IMA5);
-SYSFS_PMCSETUP(ima6, SPRN_PA6T_IMA6);
-SYSFS_PMCSETUP(ima7, SPRN_PA6T_IMA7);
-SYSFS_PMCSETUP(ima8, SPRN_PA6T_IMA8);
-SYSFS_PMCSETUP(ima9, SPRN_PA6T_IMA9);
-SYSFS_PMCSETUP(imaat, SPRN_PA6T_IMAAT);
-SYSFS_PMCSETUP(btcr, SPRN_PA6T_BTCR);
-SYSFS_PMCSETUP(pccr, SPRN_PA6T_PCCR);
-SYSFS_PMCSETUP(rpccr, SPRN_PA6T_RPCCR);
-SYSFS_PMCSETUP(der, SPRN_PA6T_DER);
-SYSFS_PMCSETUP(mer, SPRN_PA6T_MER);
-SYSFS_PMCSETUP(ber, SPRN_PA6T_BER);
-SYSFS_PMCSETUP(ier, SPRN_PA6T_IER);
-SYSFS_PMCSETUP(sier, SPRN_PA6T_SIER);
-SYSFS_PMCSETUP(siar, SPRN_PA6T_SIAR);
-SYSFS_PMCSETUP(tsr0, SPRN_PA6T_TSR0);
-SYSFS_PMCSETUP(tsr1, SPRN_PA6T_TSR1);
-SYSFS_PMCSETUP(tsr2, SPRN_PA6T_TSR2);
-SYSFS_PMCSETUP(tsr3, SPRN_PA6T_TSR3);
+SYSFS_SPRSETUP(hid0, SPRN_HID0);
+SYSFS_SPRSETUP(hid1, SPRN_HID1);
+SYSFS_SPRSETUP(hid4, SPRN_HID4);
+SYSFS_SPRSETUP(hid5, SPRN_HID5);
+SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
+SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
+SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
+SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
+SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
+SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
+SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
+SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
+SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
+SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
+SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
+SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
+SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
+SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
+SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
+SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
+SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
+SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
+SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
+SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
+SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
+SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
+SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
+SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
#endif /* CONFIG_DEBUG_KERNEL */
#endif /* HAS_PPC_PMC_PA6T */
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] powerpc: book3s: kvm: Use the saved dsisr and dar values
From: Aneesh Kumar K.V @ 2013-12-20 4:37 UTC (permalink / raw)
To: Alexander Graf
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org mailing list
In-Reply-To: <CA6C7C49-C1EC-4277-9777-650026C9127D@suse.de>
Alexander Graf <agraf@suse.de> writes:
>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>>
>> Alexander Graf <agraf@suse.de> writes:
>>
>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>>>
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>
>>>> Don't try to compute these values.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>>>> we need a variant of this patch. But with this and the additional patch
>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>>>> PR guest to boot.
>>>
>>> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>>>
>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>> Author: Alexander Graf <agraf@suse.de>
>>> Date: Wed Mar 24 21:48:28 2010 +0100
>>>
>>> KVM: PPC: Implement alignment interrupt
>>>
>>> Mac OS X has some applications - namely the Finder - that require alignment
>>> interrupts to work properly. So we need to implement them.
>>>
>>> But the spec for 970 and 750 also looks different. While 750 requires the
>>> DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>>> address (DAR), the 970 declares this as an optional feature. So we need
>>> to reconstruct DSISR and DAR manually.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>>
>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>>>
>>> So this is clearly a nack on this patch :).
>>
>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
>> to do that ? According to Paul we should always find DAR.
>
> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a proper DAR value - we can then remove that part.
>
> But for DSISR I'm not convinced CPUs above 970 handle this
> correctly. So we would at least need a guest cpu check to find out
> whether the vcpu expects a working dsisr and emulate it then.
>
> I don't really fully understand the problem though. Why does the
> calculation break at all for you?
IIRC this was to get little endian PR setup to work. This is to avoid
handling new instructions, because in little endian mode we get
alignment interrupt for a larger instructon set
-aneesh
^ permalink raw reply
* Re: [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Gavin Shan @ 2013-12-20 1:59 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <20131220013539.GA10795@shangw.(null)>
On Fri, Dec 20, 2013 at 09:35:39AM +0800, Gavin Shan wrote:
>On Thu, Dec 19, 2013 at 05:14:07PM -0600, Brian W Hart wrote:
>>PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
>>PAGE_SIZE > 4KB.
>>
>
>I think you're talking about that PAGE_SIZE could be configured
>to have variable size (e.g. 4KB). So it's not safe to pass PAGE_SIZE
>to OPAL API opal_pci_get_phb_diag_data2(). Instead, we should pass
>PNV_PCI_DIAG_BUF_SIZE and it makes sense to me :-)
>
>Also, it needs to be backported to stable kernel as well.
>
>>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
>
>Acked-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>
Sorry, Brian. It has been fixed as part of the following commit, which
has been put into Ben's powerpc-next branch :-)
commit 93aef2a789778e7ec787179fc9b34ca4885a5ef3
161 static void ioda_eeh_phb_diag(struct pci_controller *hose)
162 {
163 struct pnv_phb *phb = hose->private_data;
164 - struct OpalIoPhbErrorCommon *common;
165 long rc;
166
167 - common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
168 - rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
169 + rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
170 + PNV_PCI_DIAG_BUF_SIZE);
>>---
>> arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>index 02245ce..8184ef5 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
>> struct OpalIoPhbErrorCommon *common;
>> long rc;
>>
>>- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>>- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
>>+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>>+ PNV_PCI_DIAG_BUF_SIZE);
>> if (rc != OPAL_SUCCESS) {
>> pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
>> __func__, hose->global_number, rc);
>> return;
>> }
>>
>>+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>> switch (common->ioType) {
>> case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
>> ioda_eeh_p7ioc_phb_diag(hose, common);
>
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] powernv: eeh: add buffer for P7IOC hub error data
From: Gavin Shan @ 2013-12-20 1:45 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131219231853.GB22418@oc3347516403.ibm.com>
On Thu, Dec 19, 2013 at 05:18:53PM -0600, Brian W Hart wrote:
>Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
>a buffer for P7IOC hub diagnostic data. Take care to inform OPAL of the
>correct size for the buffer.
>
>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
>---
>
>I hope I've understood this correctly. It looks to me like
>ioda_eeh_hub_data is effectively asking OPAL to clobber its own
>text (via 'data') when it makes the call to retrieve the hub data.
>
Yeah, we should have used following variable as HUB diag-data instead.
static char *hub_diag = NULL;
However, it's not safe to allocate page-sized buffer for "hub_diag".
>Added a hub diagnostic structure per-phb. Perhaps the diagnostic
>structure better belongs in the phb->diag union, but I wasn't sure whether
>we'd need to carry the hub and PHB diag data at the same time.
>
Please put hub diag-data to struct pnv_phb::diag since we don't need
carry hub and PHB diag-data at same time. With it, please remove
variable "hub_diag" as well.
> arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
> arch/powerpc/platforms/powernv/pci.h | 2 ++
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 8184ef5..dfd9134 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
> struct OpalIoP7IOCErrorData *data;
> long rc;
>
>- data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
>- rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
>+ data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
>+ rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
> if (rc != OPAL_SUCCESS) {
> pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
> __func__, phb->hub_id, rc);
>diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>index 911c24e..d1b6d8c 100644
>--- a/arch/powerpc/platforms/powernv/pci.h
>+++ b/arch/powerpc/platforms/powernv/pci.h
>@@ -177,6 +177,8 @@ struct pnv_phb {
> unsigned char blob[PNV_PCI_DIAG_BUF_SIZE];
> struct OpalIoP7IOCPhbErrorData p7ioc;
> } diag;
>+
>+ struct OpalIoP7IOCErrorData p7ioc_err;
> };
>
> extern struct pci_ops pnv_pci_ops;
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Gavin Shan @ 2013-12-20 1:35 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20131219231407.GA22418@oc3347516403.ibm.com>
On Thu, Dec 19, 2013 at 05:14:07PM -0600, Brian W Hart wrote:
>PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
>PAGE_SIZE > 4KB.
>
I think you're talking about that PAGE_SIZE could be configured
to have variable size (e.g. 4KB). So it's not safe to pass PAGE_SIZE
to OPAL API opal_pci_get_phb_diag_data2(). Instead, we should pass
PNV_PCI_DIAG_BUF_SIZE and it makes sense to me :-)
Also, it needs to be backported to stable kernel as well.
>Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
Acked-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>---
> arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 02245ce..8184ef5 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
> struct OpalIoPhbErrorCommon *common;
> long rc;
>
>- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
>- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
>+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>+ PNV_PCI_DIAG_BUF_SIZE);
> if (rc != OPAL_SUCCESS) {
> pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
> __func__, hose->global_number, rc);
> return;
> }
>
>+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
> switch (common->ioType) {
> case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
> ioda_eeh_p7ioc_phb_diag(hose, common);
Thanks,
Gavin
^ permalink raw reply
* [PATCH] powernv: eeh: add buffer for P7IOC hub error data
From: Brian W Hart @ 2013-12-19 23:18 UTC (permalink / raw)
To: linuxppc-dev
Prevent ioda_eeh_hub_diag() from clobbering itself when called by supplying
a buffer for P7IOC hub diagnostic data. Take care to inform OPAL of the
correct size for the buffer.
Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
---
I hope I've understood this correctly. It looks to me like
ioda_eeh_hub_data is effectively asking OPAL to clobber its own
text (via 'data') when it makes the call to retrieve the hub data.
Added a hub diagnostic structure per-phb. Perhaps the diagnostic
structure better belongs in the phb->diag union, but I wasn't sure whether
we'd need to carry the hub and PHB diag data at the same time.
arch/powerpc/platforms/powernv/eeh-ioda.c | 4 ++--
arch/powerpc/platforms/powernv/pci.h | 2 ++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 8184ef5..dfd9134 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -636,8 +636,8 @@ static void ioda_eeh_hub_diag(struct pci_controller *hose)
struct OpalIoP7IOCErrorData *data;
long rc;
- data = (struct OpalIoP7IOCErrorData *)ioda_eeh_hub_diag;
- rc = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
+ data = (struct OpalIoP7IOCErrorData *)&phb->p7ioc_err;
+ rc = opal_pci_get_hub_diag_data(phb->hub_id, data, sizeof *data);
if (rc != OPAL_SUCCESS) {
pr_warning("%s: Failed to get HUB#%llx diag-data (%ld)\n",
__func__, phb->hub_id, rc);
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 911c24e..d1b6d8c 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -177,6 +177,8 @@ struct pnv_phb {
unsigned char blob[PNV_PCI_DIAG_BUF_SIZE];
struct OpalIoP7IOCPhbErrorData p7ioc;
} diag;
+
+ struct OpalIoP7IOCErrorData p7ioc_err;
};
extern struct pci_ops pnv_pci_ops;
--
1.8.3.1
^ permalink raw reply related
* [PATCH] powernv: eeh: fix possible buffer overrun in ioda_eeh_phb_diag()
From: Brian W Hart @ 2013-12-19 23:14 UTC (permalink / raw)
To: linuxppc-dev
PHB diagnostic buffer may be smaller than PAGE_SIZE, especially when
PAGE_SIZE > 4KB.
Signed-off-by: Brian W Hart <hartb@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/eeh-ioda.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 02245ce..8184ef5 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -820,14 +820,15 @@ static void ioda_eeh_phb_diag(struct pci_controller *hose)
struct OpalIoPhbErrorCommon *common;
long rc;
- common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
- rc = opal_pci_get_phb_diag_data2(phb->opal_id, common, PAGE_SIZE);
+ rc = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
+ PNV_PCI_DIAG_BUF_SIZE);
if (rc != OPAL_SUCCESS) {
pr_warning("%s: Failed to get diag-data for PHB#%x (%ld)\n",
__func__, hose->global_number, rc);
return;
}
+ common = (struct OpalIoPhbErrorCommon *)phb->diag.blob;
switch (common->ioType) {
case OPAL_PHB_ERROR_DATA_TYPE_P7IOC:
ioda_eeh_p7ioc_phb_diag(hose, common);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] powerpc/sysdev: Fix a mpic section mismatch for MPC85xx
From: Christian Engelmayer @ 2013-12-19 23:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Scott Wood; +Cc: linuxppc-dev, Kevin Hao
In-Reply-To: <1387152653.15730.175.camel@pasglop>
On Mon, 16 Dec 2013 11:10:53 +1100 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sun, 2013-12-15 at 19:38 +0100, Christian Engelmayer wrote:
> > Moved arch/powerpc/sysdev/mpic.c : smp_mpic_probe() out of the __init section.
> > It is referenced by arch/powerpc/platforms/85xx/smp.c : smp_85xx_setup_cpu().
>
> I don't like this. The reference is not actually going to call into the
> code at all and as such is not an error, it's just a pointer comparison.
That's correct. I proposed it that way because on first sight I was concerned
that there is an address of an __init function assigned to a function pointer
within a non __initdata struct at all that can be compared against. However,
further usage of smp_ops->probe is currently safe of course and *_ops symbols
within .data are whitelisted to refer to init sections.
> If there is no way to silence the warning, then I'd suggest to use a
> global flag, something like mpc85xx_pic_type and test that instead
> of comparing the pointers.
I've seen that there is currently a patch proposed against
commit dc2c9c52b604f51b1416ed87ff54a1c77a1a8b5b
powerpc/85xx: Set up doorbells even with no mpic
that introduced the section causing the warning:
http://patchwork.ozlabs.org/patch/289214/
powerpc/85xx: don't init the mpic ipi for the SoC which has doorbell support
This patch also removes the affected pointer comparison and if accepted would
thus also silence this warning.
> > Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> > ---
> > arch/powerpc/sysdev/mpic.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 0e166ed..72c1e65 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1924,7 +1924,7 @@ void smp_mpic_message_pass(int cpu, int msg)
> > msg * MPIC_INFO(CPU_IPI_DISPATCH_STRIDE), physmask);
> > }
> >
> > -int __init smp_mpic_probe(void)
> > +int smp_mpic_probe(void)
> > {
> > int nr_cpus;
> >
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/512x: dts: disable MPC5125 usb module
From: Matteo Facchinetti @ 2013-12-19 17:49 UTC (permalink / raw)
To: linuxppc-dev, agust, Gerhard Sittig
In-Reply-To: <20131219123017.GM8064@book.gsilab.sittig.org>
On 19/12/2013 13:30, Gerhard Sittig wrote:
> On Thu, Dec 19, 2013 at 11:23 +0100, Matteo Facchinetti wrote:
>> USB controller pin-muxing is not initialized correctly and when system boot,
>> causes a kernel panic.
>> USB controller is also connected with a USB3320 ulpi tranciever and
>> DTS should be includes the correct dependency for initialize and activate
>> this component.
>>
>> Signed-off-by: Matteo Facchinetti <matteo.facchinetti@sirius-es.it>
>> ---
>> arch/powerpc/boot/dts/mpc5125twr.dts | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/boot/dts/mpc5125twr.dts b/arch/powerpc/boot/dts/mpc5125twr.dts
>> index 806479f..85452a7 100644
>> --- a/arch/powerpc/boot/dts/mpc5125twr.dts
>> +++ b/arch/powerpc/boot/dts/mpc5125twr.dts
>> @@ -230,6 +230,9 @@
>> };
>>
>> usb@3000 {
>> + /* TODO correct pinmux config and fix USB3320 ulpi dependency */
>> + status = "disabled";
>> +
>> compatible = "fsl,mpc5121-usb2-dr";
>> reg = <0x3000 0x400>;
>> #address-cells = <1>;
>> --
>> 1.8.3.2
> I agree on the change to the board dts file, but suggest to
> reword the commit description for improved reception.
>
> I feel it's worth trying to phrase the subject line, the commit
> message, and the patch such that they can get considered
> independently from each other, as not all of them are necessarily
> available at the same time. Often they get looked up from
> different perspectives, like terse listing first for orientation,
> log with description then to determine whether to have a closer
> look, the patch only at the end after the other checks told you
> to look into more details. Assuming that they always show up in
> combination may turn out to be inaccurate.
>
> So I suggest some text along those lines:
>
> at the moment the USB controller's pin muxing is not setup
> correctly and causes a kernel panic upon system startup, so
> disable the USB1 device tree node in the MPC5125 tower board
> dts file
>
> the USB controller is connected to an USB3320 ULPI transceiver
> and the device tree should receive an update to reflect correct
> dependencies and required initialization data before the USB1
> node can get re-enabled
>
> Does that sound correct to you? Does it reflect your intention,
> or did I put something in wrong terms?
Yes, it's exactly what I tried to explain. All is right.
Now, I learned. Thank you.
> A minor nit would be that other reviewers in the past suggested
> to put the 'status = "disabled"' line last in the list of
> properties (right before optional children). I don't have strong
> feelings about this. Putting it first might better reflect your
> motivation of only re-enabling the node after fixing the lack or
> inappropriateness of existing information first.
I put it as first property because is very temporary and because is the
most important information for the moment:
"the USB1 port doesn't works for these reasons...".
But, I think that if this property is usually at the and of the node
list, it's better follow this suggestion.
Then I'll send a v2 patch with the newer description and with this change.
>
> A different matter is that I'd suggest to re-work the MPC5125
> device tree. It recently escaped my attention because it did not
> share any information with the MPC5121 trees. Comparing the
> MPC5125 board DTS with the MPC5121 DTS include file resulted in a
> lot of unnecessary "differences" that turned out to be whitespace
> or comment style only, or differences in the order of nodes.
> There were only few real differences in the information, and the
> MPC5125 device tree appears to only describe a subset of what the
> SoC actually contains.
>
> It may be worth looking into
> - identifying common parts that are shared among the MPC5121 and
> MPC5125 (my recent CCF update lists differences, but does not
> explicitly list similarities, and is from the clocks
> perspective and may not cover all of the SoC components)
> - putting those common parts into .dtsi files if possible
> - making the MPC5125 tower board reference the DTS includes,
> sharing as much as possible with the other SoC variants
>
> This may involve another split of the mpc5121.dtsi into what's
> common to all MPC512x variants, and what's exclusive to MPC5121
> only.
>
> But that is a bigger task than the above quick adjustment, and is
> not a required fix but just an improvement in maintainability or
> completeness of information. So I suggest to pick your USB1
> disabling for -next and 3.14 now, and to address the DTS cleanup
> and sharing later.
It was from the first commit of mpc5125twr.dts file that I would have
liked to rework all mpc5xxx DTS tree.
At the moment I have already started this task but it's better to open
another thread for this discussion and I agree with you that it's not a
priority.
Now, I'm working to the NFC controller and I'm porting to linux 3.13 the
driver that we are using in linux 3.9.4.
When I done a preliminary version I'll post for starting a discussion
about its correct integration.
I think it's time to add this driver in mainline for use NFC as well.
Best regards,
Matteo Facchinetti
^ permalink raw reply
* [PATCH 04/38] powerpc: kernel: add missing put_device call
From: Levente Kurusa @ 2013-12-19 15:03 UTC (permalink / raw)
To: LKML
Cc: Prarit Bhargava, Levente Kurusa, Paul Mackerras, Anton Blanchard,
Greg Kroah-Hartman, linuxppc-dev
In-Reply-To: <1387465429-3568-2-git-send-email-levex@linux.com>
This is required so that we give up the last reference to the device.
Signed-off-by: Levente Kurusa <levex@linux.com>
---
arch/powerpc/kernel/vio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
index e7d0c88f..e8ed87e 100644
--- a/arch/powerpc/kernel/vio.c
+++ b/arch/powerpc/kernel/vio.c
@@ -1494,6 +1494,7 @@ static int __init vio_bus_init(void)
if (err) {
printk(KERN_WARNING "%s: device_register returned %i\n",
__func__, err);
+ put_device(&vio_bus_device.dev);
return err;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH 02/38] powerpc: qe_lib: add missing put_device call
From: Levente Kurusa @ 2013-12-19 15:03 UTC (permalink / raw)
To: LKML; +Cc: Levente Kurusa, Paul Mackerras, linuxppc-dev
In-Reply-To: <1387465429-3568-2-git-send-email-levex@linux.com>
This is required so that we give up the last reference to the device.
Signed-off-by: Levente Kurusa <levex@linux.com>
---
arch/powerpc/sysdev/qe_lib/qe_ic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c b/arch/powerpc/sysdev/qe_lib/qe_ic.c
index b2b87c3..2e15262 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_ic.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c
@@ -493,6 +493,7 @@ static int __init init_qe_ic_sysfs(void)
rc = device_register(&device_qe_ic);
if (rc) {
printk(KERN_ERR "Failed registering qe_ic sys device\n");
+ put_device(&device_qe_ic);
return -ENODEV;
}
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 03/38] powerpc: cell: add missing put_device call
From: Levente Kurusa @ 2013-12-19 15:03 UTC (permalink / raw)
To: LKML
Cc: cbe-oss-dev, Levente Kurusa, Arnd Bergmann, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1387465429-3568-2-git-send-email-levex@linux.com>
This is required so that we give up the last reference to the device.
Signed-off-by: Levente Kurusa <levex@linux.com>
---
arch/powerpc/platforms/cell/spu_base.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/cell/spu_base.c b/arch/powerpc/platforms/cell/spu_base.c
index f85db3a..f7fb0d9 100644
--- a/arch/powerpc/platforms/cell/spu_base.c
+++ b/arch/powerpc/platforms/cell/spu_base.c
@@ -597,6 +597,7 @@ static int spu_create_dev(struct spu *spu)
if (ret) {
printk(KERN_ERR "Can't register SPU %d with sysfs\n",
spu->number);
+ put_device(&spu->dev);
return ret;
}
--
1.8.3.1
^ permalink raw reply related
* Re: commit e38c0a1f breaks powerpc boards with uli1575 chip
From: Thierry Reding @ 2013-12-19 14:24 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree@vger.kernel.org, Arnd Bergmann, Dmitry Krivoschekov,
Nikita Yushchenko, Thierry Reding, linux-kernel, Grant Likely,
linuxppc-dev, Alexey Lugovskoy
In-Reply-To: <52B1EC15.5070606@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4008 bytes --]
On Wed, Dec 18, 2013 at 12:40:21PM -0600, Rob Herring wrote:
> [fixed DT maillist address]
>
> On 12/17/2013 01:35 AM, Nikita Yushchenko wrote:
> > Hi
> >
> > While trying to make freescale p2020ds and mpc8572ds boards working with mainline kernel, I faced that commit
> > e38c0a1f (Handle #address-cells > 2 specially) breaks things with these boards.
>
> Good to see this broke in v3.7 and is just now found...
>
> >
> > Both these boards have uli1575 chip.
> > Corresponding part in device tree is something like
> >
> > uli1575@0 {
> > reg = <0x0 0x0 0x0 0x0 0x0>;
> > #size-cells = <2>;
> > #address-cells = <3>;
> > ranges = <0x2000000 0x0 0x80000000
> > 0x2000000 0x0 0x80000000
> > 0x0 0x20000000
> >
> > 0x1000000 0x0 0x0
> > 0x1000000 0x0 0x0
> > 0x0 0x10000>;
> > isa@1e {
> > ...
> >
> > I.e. it has #address-cells = <3>
> >
> >
> > With commit e38c0a1f reverted, devices under uli1575 are registered correctly, e.g. for rtc
> >
> > OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> > OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> > OF: translating address: 00000001 00000070
> > OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> > OF: walking ranges...
> > OF: ISA map, cp=0, s=1000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 00000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> > OF: walking ranges...
> > OF: default map, cp=a0000000, s=20000000, da=70
> > OF: default map, cp=0, s=10000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 01000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000
> > OF: walking ranges...
> > OF: PCI map, cp=0, s=10000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 01000000 00000000 00000070
> > OF: parent bus is default (na=2, ns=2) on /
> > OF: walking ranges...
> > OF: PCI map, cp=0, s=10000, da=70
> > OF: parent translation for: 00000000 ffc10000
> > OF: with offset: 70
> > OF: one level translation: 00000000 ffc10070
> > OF: reached root node
> >
> > With commit e38c0a1f in place, address translation fails:
> >
> > OF: ** translation for device /pcie@ffe09000/pcie@0/uli1575@0/isa@1e/rtc@70 **
> > OF: bus is isa (na=2, ns=1) on /pcie@ffe09000/pcie@0/uli1575@0/isa@1e
> > OF: translating address: 00000001 00000070
> > OF: parent bus is default (na=3, ns=2) on /pcie@ffe09000/pcie@0/uli1575@0
> > OF: walking ranges...
> > OF: ISA map, cp=0, s=1000, da=70
> > OF: parent translation for: 01000000 00000000 00000000
> > OF: with offset: 70
> > OF: one level translation: 00000000 00000000 00000070
> > OF: parent bus is pci (na=3, ns=2) on /pcie@ffe09000/pcie@0
> > OF: walking ranges...
> > OF: default map, cp=a0000000, s=20000000, da=70
> > OF: default map, cp=0, s=10000, da=70
> > OF: not found !
> >
> > Either e38c0a1f should be reverted, or uli1575 (and perhaps other similar devices) have to be described in device
> > trees differently.
>
> Reverting would break Tegra PCIe, but you should not have to change the
> DT either. So we need a solution.
We ended up merging a different address representation for Tegra PCIe
and I've confirmed that reverting this commit doesn't cause any obvious
regressions. I think all other drivers in drivers/pci/host ended up
copying what we did on Tegra, so I wouldn't expect any other breakage
either.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: Make 64-bit non-VMX __copy_tofrom_user bi-endian
From: Michael Ellerman @ 2013-12-19 13:17 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulmck, paulus, linuxppc-dev
In-Reply-To: <20131218211502.034c3eb0@kryten>
On Wed, 2013-12-18 at 21:15 +1100, Anton Blanchard wrote:
> Hi,
>
> > [ This is a rare but nasty LE issue. Most of the time we use the
> > POWER7 optimised __copy_tofrom_user_power7 loop, but when it hits an
> > exception we fall back to the base __copy_tofrom_user loop. - Anton ]
>
> To try and catch any screw ups in our ppc64 memcpy and copy_tofrom_user
> loops, I wrote a quick test:
>
> http://ozlabs.org/~anton/junkcode/validate_kernel_copyloops.tar.gz
Nice! How's this look?
cheers
selftests: Import Anton's memcpy / copy_tofrom_user tests
Turn Anton's memcpy / copy_tofrom_user test into something that can
live in tools/testing/selftests.
It requires one turd in arch/powerpc/lib/memcpy_64.S, but it's pretty
harmless IMHO.
We are sailing very close to the wind with the feature macros. We define
them to nothing, which currently means we get a few extra nops and
include the unaligned calls.
---
arch/powerpc/lib/memcpy_64.S | 2 +
tools/testing/selftests/powerpc/Makefile | 2 +-
tools/testing/selftests/powerpc/copyloops/Makefile | 29 +++++++
.../selftests/powerpc/copyloops/asm/ppc_asm.h | 86 +++++++++++++++++++
.../selftests/powerpc/copyloops/asm/processor.h | 0
.../selftests/powerpc/copyloops/copyuser_64.S | 1 +
.../selftests/powerpc/copyloops/copyuser_power7.S | 1 +
.../selftests/powerpc/copyloops/memcpy_64.S | 1 +
.../selftests/powerpc/copyloops/memcpy_power7.S | 1 +
.../testing/selftests/powerpc/copyloops/validate.c | 99 ++++++++++++++++++++++
tools/testing/selftests/powerpc/utils.h | 3 +
11 files changed, 224 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/copyloops/Makefile
create mode 100644 tools/testing/selftests/powerpc/copyloops/asm/ppc_asm.h
create mode 100644 tools/testing/selftests/powerpc/copyloops/asm/processor.h
create mode 120000 tools/testing/selftests/powerpc/copyloops/copyuser_64.S
create mode 120000 tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
create mode 120000 tools/testing/selftests/powerpc/copyloops/memcpy_64.S
create mode 120000 tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
create mode 100644 tools/testing/selftests/powerpc/copyloops/validate.c
diff --git a/arch/powerpc/lib/memcpy_64.S b/arch/powerpc/lib/memcpy_64.S
index d2bbbc8..72ad055 100644
--- a/arch/powerpc/lib/memcpy_64.S
+++ b/arch/powerpc/lib/memcpy_64.S
@@ -14,7 +14,9 @@ _GLOBAL(memcpy)
BEGIN_FTR_SECTION
std r3,48(r1) /* save destination pointer for return value */
FTR_SECTION_ELSE
+#ifndef SELFTEST
b memcpy_power7
+#endif
ALT_FTR_SECTION_END_IFCLR(CPU_FTR_VMX_COPY)
PPC_MTOCRF(0x01,r5)
cmpldi cr1,r5,16
diff --git a/tools/testing/selftests/powerpc/Makefile b/tools/testing/selftests/powerpc/Makefile
index bd24ae5..316194f 100644
--- a/tools/testing/selftests/powerpc/Makefile
+++ b/tools/testing/selftests/powerpc/Makefile
@@ -13,7 +13,7 @@ CFLAGS := -Wall -O2 -flto -Wall -Werror -DGIT_VERSION='"$(GIT_VERSION)"' -I$(CUR
export CC CFLAGS
-TARGETS = pmu
+TARGETS = pmu copyloops
endif
diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile
new file mode 100644
index 0000000..6f2d3be
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/Makefile
@@ -0,0 +1,29 @@
+# The loops are all 64-bit code
+CFLAGS += -m64
+CFLAGS += -I$(CURDIR)
+CFLAGS += -D SELFTEST
+
+# Use our CFLAGS for the implicit .S rule
+ASFLAGS = $(CFLAGS)
+
+PROGS := copyuser_64 copyuser_power7 memcpy_64 memcpy_power7
+EXTRA_SOURCES := validate.c ../harness.c
+
+all: $(PROGS)
+
+copyuser_64: CPPFLAGS += -D COPY_LOOP=test___copy_tofrom_user_base
+copyuser_power7: CPPFLAGS += -D COPY_LOOP=test___copy_tofrom_user_power7
+memcpy_64: CPPFLAGS += -D COPY_LOOP=test_memcpy
+memcpy_power7: CPPFLAGS += -D COPY_LOOP=test_memcpy_power7
+
+$(PROGS): $(EXTRA_SOURCES)
+
+run_tests: all
+ @-for PROG in $(PROGS); do \
+ ./$$PROG; \
+ done;
+
+clean:
+ rm -f $(PROGS) *.o
+
+.PHONY: all run_tests clean
diff --git a/tools/testing/selftests/powerpc/copyloops/asm/ppc_asm.h b/tools/testing/selftests/powerpc/copyloops/asm/ppc_asm.h
new file mode 100644
index 0000000..ccd9c84
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/asm/ppc_asm.h
@@ -0,0 +1,86 @@
+#include <ppc-asm.h>
+
+#define CONFIG_ALTIVEC
+
+#define r1 1
+
+#define vr0 0
+#define vr1 1
+#define vr2 2
+#define vr3 3
+#define vr4 4
+#define vr5 5
+#define vr6 6
+#define vr7 7
+#define vr8 8
+#define vr9 9
+#define vr10 10
+#define vr11 11
+#define vr12 12
+#define vr13 13
+#define vr14 14
+#define vr15 15
+#define vr16 16
+#define vr17 17
+#define vr18 18
+#define vr19 19
+#define vr20 20
+#define vr21 21
+#define vr22 22
+#define vr23 23
+#define vr24 24
+#define vr25 25
+#define vr26 26
+#define vr27 27
+#define vr28 28
+#define vr29 29
+#define vr30 30
+#define vr31 31
+
+#define R14 r14
+#define R15 r15
+#define R16 r16
+#define R17 r17
+#define R18 r18
+#define R19 r19
+#define R20 r20
+#define R21 r21
+#define R22 r22
+
+#define STACKFRAMESIZE 256
+#define STK_PARAM(i) (48 + ((i)-3)*8)
+#define STK_REG(i) (112 + ((i)-14)*8)
+
+#define _GLOBAL(A) FUNC_START(test_ ## A)
+
+#define PPC_MTOCRF(A, B) mtocrf A, B
+
+FUNC_START(enter_vmx_usercopy)
+ li r3,1
+ blr
+
+FUNC_START(exit_vmx_usercopy)
+ li r3,0
+ blr
+
+FUNC_START(enter_vmx_copy)
+ li r3,1
+ blr
+
+FUNC_START(exit_vmx_copy)
+ blr
+
+FUNC_START(memcpy_power7)
+ blr
+
+FUNC_START(__copy_tofrom_user_power7)
+ blr
+
+FUNC_START(__copy_tofrom_user_base)
+ blr
+
+#define BEGIN_FTR_SECTION
+#define FTR_SECTION_ELSE
+#define ALT_FTR_SECTION_END_IFCLR(x)
+#define ALT_FTR_SECTION_END(x, y)
+#define END_FTR_SECTION_IFCLR(x)
diff --git a/tools/testing/selftests/powerpc/copyloops/asm/processor.h b/tools/testing/selftests/powerpc/copyloops/asm/processor.h
new file mode 100644
index 0000000..e69de29
diff --git a/tools/testing/selftests/powerpc/copyloops/copyuser_64.S b/tools/testing/selftests/powerpc/copyloops/copyuser_64.S
new file mode 120000
index 0000000..f1c418a
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/copyuser_64.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/copyuser_64.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S b/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
new file mode 120000
index 0000000..4786895
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/copyuser_power7.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_64.S b/tools/testing/selftests/powerpc/copyloops/memcpy_64.S
new file mode 120000
index 0000000..cce33fb
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/memcpy_64.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/memcpy_64.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S b/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
new file mode 120000
index 0000000..0d6fbfa
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
@@ -0,0 +1 @@
+../../../../../arch/powerpc/lib/memcpy_power7.S
\ No newline at end of file
diff --git a/tools/testing/selftests/powerpc/copyloops/validate.c b/tools/testing/selftests/powerpc/copyloops/validate.c
new file mode 100644
index 0000000..1750ff5
--- /dev/null
+++ b/tools/testing/selftests/powerpc/copyloops/validate.c
@@ -0,0 +1,99 @@
+#include <malloc.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdbool.h>
+
+#include "../utils.h"
+
+#define MAX_LEN 8192
+#define MAX_OFFSET 16
+#define MIN_REDZONE 128
+#define BUFLEN (MAX_LEN+MAX_OFFSET+2*MIN_REDZONE)
+#define POISON 0xa5
+
+unsigned long COPY_LOOP(void *to, const void *from, unsigned long size);
+
+static void do_one(char *src, char *dst, unsigned long src_off,
+ unsigned long dst_off, unsigned long len, void *redzone,
+ void *fill)
+{
+ char *srcp, *dstp;
+ unsigned long ret;
+ unsigned long i;
+
+ srcp = src + MIN_REDZONE + src_off;
+ dstp = dst + MIN_REDZONE + dst_off;
+
+ memset(src, POISON, BUFLEN);
+ memset(dst, POISON, BUFLEN);
+ memcpy(srcp, fill, len);
+
+ ret = COPY_LOOP(dstp, srcp, len);
+ if (ret && ret != (unsigned long)dstp) {
+ printf("(%p,%p,%ld) returned %ld\n", dstp, srcp, len, ret);
+ abort();
+ }
+
+ if (memcmp(dstp, srcp, len)) {
+ printf("(%p,%p,%ld) miscompare\n", dstp, srcp, len);
+ printf("src: ");
+ for (i = 0; i < len; i++)
+ printf("%02x ", srcp[i]);
+ printf("\ndst: ");
+ for (i = 0; i < len; i++)
+ printf("%02x ", dstp[i]);
+ printf("\n");
+ abort();
+ }
+
+ if (memcmp(dst, redzone, dstp - dst)) {
+ printf("(%p,%p,%ld) redzone before corrupted\n",
+ dstp, srcp, len);
+ abort();
+ }
+
+ if (memcmp(dstp+len, redzone, dst+BUFLEN-(dstp+len))) {
+ printf("(%p,%p,%ld) redzone after corrupted\n",
+ dstp, srcp, len);
+ abort();
+ }
+}
+
+int test_copy_loop(void)
+{
+ char *src, *dst, *redzone, *fill;
+ unsigned long len, src_off, dst_off;
+ unsigned long i;
+
+ src = memalign(BUFLEN, BUFLEN);
+ dst = memalign(BUFLEN, BUFLEN);
+ redzone = malloc(BUFLEN);
+ fill = malloc(BUFLEN);
+
+ if (!src || !dst || !redzone || !fill) {
+ fprintf(stderr, "malloc failed\n");
+ exit(1);
+ }
+
+ memset(redzone, POISON, BUFLEN);
+
+ /* Fill with sequential bytes */
+ for (i = 0; i < BUFLEN; i++)
+ fill[i] = i & 0xff;
+
+ for (len = 1; len < MAX_LEN; len++) {
+ for (src_off = 0; src_off < MAX_OFFSET; src_off++) {
+ for (dst_off = 0; dst_off < MAX_OFFSET; dst_off++) {
+ do_one(src, dst, src_off, dst_off, len,
+ redzone, fill);
+ }
+ }
+ }
+
+ return 0;
+}
+
+int main(void)
+{
+ return test_harness(test_copy_loop, str(COPY_LOOP));
+}
diff --git a/tools/testing/selftests/powerpc/utils.h b/tools/testing/selftests/powerpc/utils.h
index 5851c4b..0de0644 100644
--- a/tools/testing/selftests/powerpc/utils.h
+++ b/tools/testing/selftests/powerpc/utils.h
@@ -31,4 +31,7 @@ do { \
} \
} while (0)
+#define _str(s) #s
+#define str(s) _str(s)
+
#endif /* _SELFTESTS_POWERPC_UTILS_H */
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH] powerpc: book3s: kvm: Use the saved dsisr and dar values
From: Alexander Graf @ 2013-12-19 12:35 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org mailing list
In-Reply-To: <87bo0d9vc5.fsf@linux.vnet.ibm.com>
> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet=
.ibm.com>:
>=20
> Alexander Graf <agraf@suse.de> writes:
>=20
>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.c=
om> wrote:
>>>=20
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>=20
>>> Don't try to compute these values.
>>>=20
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>>=20
>>> NOTE: I am not sure why we were originally computing dsisr and dar. So m=
ay be
>>> we need a variant of this patch. But with this and the additional patch
>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a L=
ittle Endian
>>> PR guest to boot.
>>=20
>> It's quite easy to find out - git blame tells you all the history and poi=
nts you to commit ca7f4203b.
>>=20
>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>> Author: Alexander Graf <agraf@suse.de>
>> Date: Wed Mar 24 21:48:28 2010 +0100
>>=20
>> KVM: PPC: Implement alignment interrupt
>>=20
>> Mac OS X has some applications - namely the Finder - that require alig=
nment
>> interrupts to work properly. So we need to implement them.
>>=20
>> But the spec for 970 and 750 also looks different. While 750 requires t=
he
>> DSISR and DAR fields to reflect some instruction bits (DSISR) and the f=
ault
>> address (DAR), the 970 declares this as an optional feature. So we nee=
d
>> to reconstruct DSISR and DAR manually.
>>=20
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>=20
>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of=
the faulting instruction" as otherwise I wouldn't have implemented it.
>>=20
>> So this is clearly a nack on this patch :).
>=20
> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need=
> to do that ? According to Paul we should always find DAR.
Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a pro=
per DAR value - we can then remove that part.
But for DSISR I'm not convinced CPUs above 970 handle this correctly. So we w=
ould at least need a guest cpu check to find out whether the vcpu expects a w=
orking dsisr and emulate it then.
I don't really fully understand the problem though. Why does the calculation=
break at all for you?
Alex
^ permalink raw reply
* Re: [PATCH] powerpc/512x: dts: disable MPC5125 usb module
From: Gerhard Sittig @ 2013-12-19 12:30 UTC (permalink / raw)
To: Matteo Facchinetti; +Cc: agust, linuxppc-dev
In-Reply-To: <1387448639-11050-1-git-send-email-matteo.facchinetti@sirius-es.it>
On Thu, Dec 19, 2013 at 11:23 +0100, Matteo Facchinetti wrote:
>
> USB controller pin-muxing is not initialized correctly and when system boot,
> causes a kernel panic.
> USB controller is also connected with a USB3320 ulpi tranciever and
> DTS should be includes the correct dependency for initialize and activate
> this component.
>
> Signed-off-by: Matteo Facchinetti <matteo.facchinetti@sirius-es.it>
> ---
> arch/powerpc/boot/dts/mpc5125twr.dts | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/boot/dts/mpc5125twr.dts b/arch/powerpc/boot/dts/mpc5125twr.dts
> index 806479f..85452a7 100644
> --- a/arch/powerpc/boot/dts/mpc5125twr.dts
> +++ b/arch/powerpc/boot/dts/mpc5125twr.dts
> @@ -230,6 +230,9 @@
> };
>
> usb@3000 {
> + /* TODO correct pinmux config and fix USB3320 ulpi dependency */
> + status = "disabled";
> +
> compatible = "fsl,mpc5121-usb2-dr";
> reg = <0x3000 0x400>;
> #address-cells = <1>;
> --
> 1.8.3.2
I agree on the change to the board dts file, but suggest to
reword the commit description for improved reception.
I feel it's worth trying to phrase the subject line, the commit
message, and the patch such that they can get considered
independently from each other, as not all of them are necessarily
available at the same time. Often they get looked up from
different perspectives, like terse listing first for orientation,
log with description then to determine whether to have a closer
look, the patch only at the end after the other checks told you
to look into more details. Assuming that they always show up in
combination may turn out to be inaccurate.
So I suggest some text along those lines:
at the moment the USB controller's pin muxing is not setup
correctly and causes a kernel panic upon system startup, so
disable the USB1 device tree node in the MPC5125 tower board
dts file
the USB controller is connected to an USB3320 ULPI transceiver
and the device tree should receive an update to reflect correct
dependencies and required initialization data before the USB1
node can get re-enabled
Does that sound correct to you? Does it reflect your intention,
or did I put something in wrong terms?
A minor nit would be that other reviewers in the past suggested
to put the 'status = "disabled"' line last in the list of
properties (right before optional children). I don't have strong
feelings about this. Putting it first might better reflect your
motivation of only re-enabling the node after fixing the lack or
inappropriateness of existing information first.
A different matter is that I'd suggest to re-work the MPC5125
device tree. It recently escaped my attention because it did not
share any information with the MPC5121 trees. Comparing the
MPC5125 board DTS with the MPC5121 DTS include file resulted in a
lot of unnecessary "differences" that turned out to be whitespace
or comment style only, or differences in the order of nodes.
There were only few real differences in the information, and the
MPC5125 device tree appears to only describe a subset of what the
SoC actually contains.
It may be worth looking into
- identifying common parts that are shared among the MPC5121 and
MPC5125 (my recent CCF update lists differences, but does not
explicitly list similarities, and is from the clocks
perspective and may not cover all of the SoC components)
- putting those common parts into .dtsi files if possible
- making the MPC5125 tower board reference the DTS includes,
sharing as much as possible with the other SoC variants
This may involve another split of the mpc5121.dtsi into what's
common to all MPC512x variants, and what's exclusive to MPC5121
only.
But that is a bigger task than the above quick adjustment, and is
not a required fix but just an improvement in maintainability or
completeness of information. So I suggest to pick your USB1
disabling for -next and 3.14 now, and to address the DTS cleanup
and sharing later.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ 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