lkml.org 
[lkml]   [2018]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
From
Date


On 26-Sep-18 7:23 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj
> <rajneesh.bhardwaj@linux.intel.com> wrote:
>> The LTR values follow PCIE LTR encoding format and can be decoded as per
>> https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf
>>
>> This adds support to translate the raw LTR values as read from the PMC
>> to meaningful values in nanosecond units of time.
>> +static void get_ltr_scale(u32 *val)
> What's wrong to return converted value? Actually the name should
> reflect what it does, ie *convert* value.

I can change it as per your suggestion.

>
>> +{
>> + /*
>> + * As per PCIE specification supprting document
> supporting

oops. Will fix.

>
>> + * ECN_LatencyTolnReporting_14Aug08.pdf the Latency
>> + * Tolerance Reporting data payload is encoded in a
>> + * 3 bit scale and 10 bit value fields. Values are
>> + * multiplied by the indicated scale to yield an absolute time
>> + * value, expressible in a range from 1 nanosecond to
>> + * 2^25*(2^10-1) = 34,326,183,936 nanoseconds.
>> + *
>> + * scale encoding is as follows:
>> + *
>> + * ----------------------------------------------
>> + * |scale factor | Multiplier (ns) |
>> + * ----------------------------------------------
>> + * | 0 | 1 |
>> + * | 1 | 32 |
>> + * | 2 | 1024 |
>> + * | 3 | 32768 |
>> + * | 4 | 1048576 |
>> + * | 5 | 33554432 |
>> + * | 6 | Invalid |
>> + * | 7 | Invalid |
>> + * ----------------------------------------------
>> + */
>> + if (*val > 5) {
>> + *val = 0;
>> + pr_warn("Invalid LTR scale factor.\n");
>> + } else {
>> + *val = 1U << (5 * (*val));
>> + }
>> +}
>> +
>> static int pmc_core_ltr_show(struct seq_file *s, void *unused)
>> {
>> struct pmc_dev *pmcdev = s->private;
>> const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts;
>> + u64 decoded_snoop_ltr = 0, decoded_non_snoop_ltr = 0;
>> + union ltr_payload ltr_data;
>> + u32 scale = 0;
> Redundant assignment.

Ok

>
>> int index;
>>
>> for (index = 0; map[index].name ; index++) {
>> - seq_printf(s, "%-32s\tRAW LTR: 0x%x\n",
>> + ltr_data.raw_data = pmc_core_reg_read(pmcdev,
>> + map[index].bit_mask);
>> +
>> + if (ltr_data.bits.non_snoop_req) {
>> + scale = ltr_data.bits.non_snoop_scale;
>> + get_ltr_scale(&scale);
>> + decoded_non_snoop_ltr =
>> + ltr_data.bits.non_snoop_val * scale;
>> + }
>> +
>> + if (ltr_data.bits.snoop_req) {
>> + scale = ltr_data.bits.snoop_scale;
>> + get_ltr_scale(&scale);
>> + decoded_snoop_ltr =
>> + ltr_data.bits.snoop_val * scale;
>> + }
>> +
>> + seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n",
>> map[index].name,
>> - pmc_core_reg_read(pmcdev, map[index].bit_mask));
>> + ltr_data.raw_data,
>> + decoded_non_snoop_ltr,
>> + decoded_snoop_ltr);
>> +
>> + decoded_snoop_ltr = decoded_non_snoop_ltr = 0;
> You may do this at the beginning of the loop and get rid of assignment
> in the definition block.

Fine.

>
>> }
>> return 0;
>> }
>> +union ltr_payload {
>> + u32 raw_data;
>> + struct {
>> + u32 snoop_val : 10;
>> + u32 snoop_scale : 3;
>> + u32 snoop_res : 2;
>> + u32 snoop_req : 1;
>> + u32 non_snoop_val : 10;
>> + u32 non_snoop_scale : 3;
>> + u32 non_snoop_res : 2;
>> + u32 non_snoop_req : 1;
>> + } bits;
>> +};
> Just use normal masks and shifts.

I chose union over masks and shifts to reduce code size and ensured
correct endian-ness. Just for my understanding, can you please let me
know why you feel masks/shift are better suited here?

>

\
 
 \ /
  Last update: 2018-09-26 16:20    [W:0.295 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site