On Fri, Nov 25, 2022 at 04:55:11PM +0100, Krzysztof Kozlowski wrote: > 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? I don't think we disagree here. I'm not in favour of the defines either here. Perhaps I confused you by accidentally not adding Prabhakar to the to field. The dt needs to convey his particular cache implementation's bufferable and/or coherent regions so I was suggesting alternatives for conveying this information, without resorting to defines. > > - 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
