Messages in this thread | | | Subject | Re: [PATCH 10/11] coresight: sink: Add TRBE driver | From | Suzuki K Poulose <> | Date | Thu, 7 Jan 2021 14:01:12 +0000 |
| |
On 1/6/21 11:50 AM, Anshuman Khandual wrote: > > > On 1/5/21 5:07 PM, Suzuki K Poulose wrote: >> On 1/5/21 9:29 AM, Anshuman Khandual wrote:
...
>>>>> +{ >>>>> + struct trbe_buf *buf = etm_perf_sink_config(handle); >>>>> + unsigned long offset; >>>>> + >>>>> + if (buf->snapshot) >>>>> + offset = trbe_snapshot_offset(handle); >>>>> + else >>>>> + offset = trbe_normal_offset(handle); >>>>> + return buf->trbe_base + offset; >>>>> +} >>>>> + >>>>> +static void clear_trbe_state(void) >>>> >>>> nit: The name doesn't give much clue about what it is doing, especially, given >>>> the following "set_trbe_state()" which does completely different from this "clear" >>>> operation. >>> >>> I agree that these names could have been better. >>> >>> s/clear_trbe_state/trbe_reset_perf_state - Clears TRBE from current perf config >>> s/set_trbe_state/trbe_prepare_perf_state - Prepares TRBE for the next perf config >> >> Please don't tie them to "perf". This is pure hardware configuration, not perf. > > Okay. > >> >> Also, I wonder if we need a separate "set_trbe_state". Could we not initialize the LIMITR >> at one go ? > > There are some limitations which could prevent that. > >> >> i.e, do something like : >> >> set_trbe_limit_pointer(limit, mode) ? >> >> where it sets all the fields of limit pointer. Also, you may want to document the mode we >> choose for TRBE. i.e, FILL STOP mode for us to collect the trace. > > Sure, will document the TRBE mode being choosen here. > >> >>> >>> >>>> >>>> I would rather open code this with a write of 0 to trbsr in the caller. >>>> >>>>> +{ >>>>> + u64 trbsr = read_sysreg_s(SYS_TRBSR_EL1); >>>>> + >>>>> + WARN_ON(is_trbe_enabled()); >>>>> + trbsr &= ~TRBSR_IRQ; >>>>> + trbsr &= ~TRBSR_TRG; >>>>> + trbsr &= ~TRBSR_WRAP; >>>>> + trbsr &= ~(TRBSR_EC_MASK << TRBSR_EC_SHIFT); >>>>> + trbsr &= ~(TRBSR_BSC_MASK << TRBSR_BSC_SHIFT); >>>>> + trbsr &= ~(TRBSR_FSC_MASK << TRBSR_FSC_SHIFT); >>>> >>>> BSC and FSC are the same fields under MSS, with their meanings determined by the EC field. >>> >>> Could just drop the FSC part if required. >>> >>>> >>>> Could we simply write 0 to the register ? >>> >>> I would really like to avoid that. This function clearly enumerates all >>> individual bit fields being cleared for resetting as well as preparing >>> the TRBE for the next perf session. Converting this into a 0 write for >>> SYS_TRBSR_EL1 sounds excessive and the only thing it would save is the >>> register read. >> >>> >>>> >>>>> + write_sysreg_s(trbsr, SYS_TRBSR_EL1); >>>>> +} >>>>> + >>>>> +static void set_trbe_state(void) >>>>> +{ >>>>> + u64 trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1); >>>>> + >>>>> + trblimitr &= ~TRBLIMITR_NVM; >>>>> + trblimitr &= ~(TRBLIMITR_FILL_MODE_MASK << TRBLIMITR_FILL_MODE_SHIFT); >>>>> + trblimitr &= ~(TRBLIMITR_TRIG_MODE_MASK << TRBLIMITR_TRIG_MODE_SHIFT); >>>>> + trblimitr |= (TRBE_FILL_STOP & TRBLIMITR_FILL_MODE_MASK) << TRBLIMITR_FILL_MODE_SHIFT; >>>>> + trblimitr |= (TRBE_TRIGGER_IGNORE & TRBLIMITR_TRIG_MODE_MASK) << TRBLIMITR_TRIG_MODE_SHIFT; >>>>> + write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1); >>>> >>>> Do we need to read-copy-update here ? Could we simply write 0 ? >>>> Same as above comment, could we not simply opencode it at the caller ? >>>> Clearly the names don't help. >>> >>> Will change the names as proposed or something better. But lets leave >>> these functions as is. Besides TRBE_TRIGGER_IGNORE also has a positive >>> value (i.e 3), writing all 0s into SYS_TRBLIMITR_EL1 will not be ideal. >>> >> >> The point is, we don't need to preserve the values for LIMITR. Also see my comment >> above, for folding this to set_trbe_limit_pointer(). In any case, I don't think >> we should rely on the values of fields we change. So it is safer and cleaner to >> set set all the bits for LIMITR, including the LIMIT address in one go, without >> ready-copy-update. > > TRBE needs to be disabled (which is also in the LIMIT register) before we can update > any other fields in the LIMIT register. So there is already an order dependency here. > Looking at the function trbe_enable_hw(), it follows something like > > 1. Clear and set the TRBE mode - followed by an isb() > 2. Update the TRBE pointers - followed by an isb() > 3. Set it rolling - followed by TSB_CSYNC > > static void trbe_enable_hw(struct trbe_buf *buf) > { > > [Software checks] > WARN_ON(buf->trbe_write < buf->trbe_base); > WARN_ON(buf->trbe_write >= buf->trbe_limit); > > [Disable TRBE in the limit register] > set_trbe_disabled(); >
We need an isb() here.
> [Clears TRBE status register] > trbe_reset_perf_state();
Please be explicit here. Make the function name reflect the fact that we are simply clearing the status register and nothing related to perf.
> > [Configures TRBE mode in the limit register] > trbe_prepare_perf_state();
This is unnecessarily introducing a dependency not enforced by the HW. You could program the LIMIT register with all the setting, mode, limit and *enable TBRE* once we have programmed base and write pointer at one shot.
> > isb();
Drop the ISB
> > [Update all required pointers] > set_trbe_base_pointer(buf->trbe_base); > set_trbe_limit_pointer(buf->trbe_limit);
As mentioned above, this could be done in set_trbe_enabled()
> set_trbe_write_pointer(buf->trbe_write); > isb(); > > [Set it rolling] > > [Update TRBE status register stop bit] > set_trbe_running();
This doesn't have any significance with Hardware. It is a status bit from the HW, which is writable only for "state" save/restore, when switching between contexts. Otherwise, this write doesn't do anything. So, please combine this with the clear_status operation above.
> > [Update TRBE limit register enable bit] > set_trbe_enabled();
Here we could set all the fileds of the LIMIT register, followed by an isb()
Kind regards Suzuki
| |