Re: [PATCH v1 3/4] soc: starfive: Add StarFive JH71XX pmu driver

[Conor Dooley <conor@kernel.org>]

Hey Walker,

Apologies for my formatting here. 

On 1 December 2022 03:56:27 GMT, Walker Chen <walker.chen@starfivetech.com> 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 :)