Hey Walker,
Apologies for my formatting here.
On 1 December 2022 03:56:27 GMT, Walker Chen <walker.chen@xxxxxxxxxxxxxxxx> wrote:
>On 2022/11/25 19:17, Conor Dooley wrote:
>> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote:
>>> On 2022/11/19 8:24, Conor Dooley wrote:
>>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote:
>>
>>> >> +void starfive_pmu_hw_event_turn_off(u32 mask)
>>> >> +{
>>> >> + pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
>>> >> +}
>>> >> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
>>> >
>>> > Where are the users for these exports? Also, should they be exported as
>>> > GPL?
>>> >
>>> > Either way, what is the point of the extra layer of abstraction here
>>> > around the writel()?
>>>
>>> The two export functions are only prepared for GPU module. But accordint to
>>> the latest information, it seems that there is no open source plan for GPU.
>>> So the two functions will be drop in next version of patch.
>>
>> That's a shame!
>
>Need to comply with certain commercial terms.
>
>>
>>> >> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
>>> >> +{
>>> >> + struct starfive_pmu *pmu = pmd->power;
>>> >> +
>>> >> + if (!pmd->mask) {
>>> >> + *is_on = false;
>>> >> + return -EINVAL;
>>> >> + }
>>> >> +
>>> >> + *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
>>> >
>>> > Is there a specific reason that you are using the __raw variants here
>>> > (and elsewhere) in the driver?
>>>
>>> Will use unified function '__raw_readl' and '__raw_writel'
>>
>> No no, I want to know *why* you are using the __raw accessors here. My
>> understanding was that __raw variants are unbarriered & unordered with
>> respect to other io accesses.
>>
>> I do notice that the bcm driver you mentioned uses the __raw variants,
>> but only __raw variants - whereas you use readl() which is ordered and
>> barriered & __raw_readl().
>>
>> Is there a reason why you would not use readl() or readl_relaxed()?
>
>Your question led me to deeply understand the usage of these io accessors.
>__raw_readl / __raw_writel denotes native byte order, no memory barrier.
>readl / writel do guarantee the byte order with barrier, ensure that previous writes are done.
>Seem that non-raw accessors are more safe.
Yeah, if there's no good reason to use these "raw" versions then please use readl/readl_relaxed.
>> No worries, looking forward to getting my board :)
>>
>Have you purchased a VisionFive 2 board online?
I have :)
