RE: [PATCH 01/10] soc: qcom: Separate kryo l2 accessors from PMU driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




This is address in the V2: https://patchwork.kernel.org/patch/10144473/

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@xxxxxxx]
> Sent: Tuesday, December 12, 2017 4:03 PM
> To: Ilia Lin <ilialin@xxxxxxxxxxxxxx>
> Cc: linux-clk@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
linux-
> arm-msm@xxxxxxxxxxxxxxx; sboyd@xxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; will.deacon@xxxxxxx;
> rnayak@xxxxxxxxxxxxxx; qualcomm-lt@xxxxxxxxxxxxxxxx;
> celster@xxxxxxxxxxxxxx; tfinkel@xxxxxxxxxxxxxx
> Subject: Re: [PATCH 01/10] soc: qcom: Separate kryo l2 accessors from PMU
> driver
> 
> Hi,
> 
> On Tue, Dec 12, 2017 at 02:31:28PM +0200, Ilia Lin wrote:
> > The driver provides kernel level API for other drivers to access the
> > MSM8996 L2 cache registers.
> > Separating the L2 access code from the PMU driver and making it public
> > to allow other drivers use it.
> > The accesses must be separated with a single spinlock, maintained in
> > this driver.
> 
> > -static void set_l2_indirect_reg(u64 reg, u64 val) -{
> > -	unsigned long flags;
> > -
> > -	raw_spin_lock_irqsave(&l2_access_lock, flags);
> > -	write_sysreg_s(reg, L2CPUSRSELR_EL1);
> > -	isb();
> > -	write_sysreg_s(val, L2CPUSRDR_EL1);
> > -	isb();
> > -	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
> > -}
> 
> > +/**
> > + * set_l2_indirect_reg: write value to an L2 register
> > + * @reg: Address of L2 register.
> > + * @value: Value to be written to register.
> > + *
> > + * Use architecturally required barriers for ordering between system
> > +register
> > + * accesses, and system registers with respect to device memory  */
> > +void set_l2_indirect_reg(u64 reg, u64 val) {
> > +	unsigned long flags;
> > +	mb();
> 
> We didn't need this for the PMU driver, so it's unfortuante that it now
has to
> pay the cost.
> 
> Can we please factor this mb() into the callers that need it?
> 
> > +	raw_spin_lock_irqsave(&l2_access_lock, flags);
> > +	write_sysreg_s(reg, L2CPUSRSELR_EL1);
> > +	isb();
> > +	write_sysreg_s(val, L2CPUSRDR_EL1);
> > +	isb();
> > +	raw_spin_unlock_irqrestore(&l2_access_lock, flags); }
> > +EXPORT_SYMBOL(set_l2_indirect_reg);
> 
> [...]
> 
> > +#ifdef CONFIG_ARCH_QCOM
> > +void set_l2_indirect_reg(u64 reg_addr, u64 val);
> > +u64 get_l2_indirect_reg(u64 reg_addr); #else static inline void
> > +set_l2_indirect_reg(u32 reg_addr, u32 val) {} static inline u32
> > +get_l2_indirect_reg(u32 reg_addr) {
> > +	return 0;
> > +}
> > +#endif
> > +#endif
> 
> Are there any drivers that will bne built for !CONFIG_ARCH_QCOM that
> reference this?
> 
> It might be better to not have the stub versions, so that we get a
build-error
> if they are erroneously used.
> 
> Thannks,
> Mark.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux