On 25/11/2022 13:25, Conor Dooley wrote: > On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote: >> On 25/11/2022 11:34, Lad, Prabhakar wrote: >>>>> +/* Device, Non-bufferable */ >>>>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF (0 << 2) >>>>> +/* Device, bufferable */ >>>>> +#define AX45MP_PMACFG_MTYP_DEV_BUF (1 << 2) >>>>> +/* Memory, Non-cacheable, Non-bufferable */ >>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF (2 << 2) >>>>> +/* Memory, Non-cacheable, Bufferable */ >>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF (3 << 2) >>>> >>>> What are all these? They don't look like flags, because 3 = 1 | 2... >>>> they don't look like constants, because we do not use shifts in >>>> constants. Are these some register values? I also do not see the header >>>> being used in the code, so why having a bindings header if it is not >>>> used (DTS is not usage...)? >>>> >>> These are register bit values for the MTYP[5:2] field. The DTS example >>> in the binding doc (above) uses these macros. I haven't included the >>> DTS/I patches with this patchset yet do think I should? >> >> Then why storing it as bindings? Bindings headers describe the interface >> implemented by drivers and used by DTS, but this is not implemented by >> drivers. > > IIUC, some of these properties are non-discoverable attributes of the > cache controller. I see two things that could be done here that are > "better" than #defining bits: I did not comment about properties. I comment about constants. Why register values/offsets/addresses are in this particular case suitable for binding headers? > - add an RZ/Five specific compatible and use match data to set the > attributes which is only possible if the pma-regions are set on a > per SoC basis > - make pma-regions into a child node, in which andestech,non-cacheable > andestech,non-bufferable etc are properties of the child node > > Prabhakar, does that make sense or am I off with my understanding of the > attributes? Best regards, Krzysztof
