| Message ID | 1516924513-20183-2-git-send-email-ckadabi@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: > Documentation for last level cache controller device tree bindings, > client bindings usage examples. > > Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> > --- > .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 ++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > new file mode 100644 > index 0000000..d433b0c > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > @@ -0,0 +1,93 @@ > +* LLCC (Last Level Cache Controller) > + > +Properties: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be "qcom,llcc-core" > + > +- reg: > + Usage: required > + Value Type: <prop-encoded-array> > + Definition: must be addresses and sizes of the LLCC registers > + > +- llcc-bank-off: > + Usage: required > + Value Type: <u32 array> > + Definition: Offsets of llcc banks from llcc base address starting from > + LLCC bank0. > + > +- llcc-broadcast-off: > + Usage: required > + Value Type: <u32> > + Definition: Offset of broadcast register from LLCC bank0 address. Please could we use "offset" rather than "off" for both of these? That way it's obvious these aren't properties for disabling some feature. How variable are these offsets in practice? Is the memory map not fixed? > + > +- #cache-cells: > + Usage: required > + Value Type: <u32> > + Definition: Number of cache cells, must be 1 What's this for, and how is it used? > + > +- max-slices: > + usage: required > + Value Type: <u32> > + Definition: Number of cache slices supported by hardware > + > +- status: > + Usage: optional > + Value type: <string> > + Definition: Property to enable or disable the driver This is a standard property, so I don't think it needs to be described here. > + > +== llcc amon device == > + > +Properties: > +-qcom,fg-cnt : The value of fine grained counter of activity monitor > + block. Could you elaborate on this? > + > +compatible devices: > + qcom,sdm845-llcc Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's not clear what this means. > + > +Example: > + > + qcom,system-cache@1300000 { > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; This looks very wrong. Why do you need syscon and simple-mfd? > + reg = <0x1300000 0x50000>; > + reg-names = "llcc_base"; > + > + llcc: qcom,sdm845-llcc { > + compatible = "qcom,sdm845-llcc"; Why is this a sub-node? Why isn't the top-level node just "qcom,sdm845-llcc" ? > + #cache-cells = <1>; > + max-slices = <32>; > + }; > + > + qcom,llcc-ecc { > + compatible = "qcom,llcc-ecc"; > + }; > + > + qcom,llcc-amon { > + compatible = "qcom,llcc-amon"; > + qcom,fg-cnt = <0x7>; > + }; > + > + }; > + > +== Client == > + > +Properties: > +- cache-slice-names: > + Usage: required > + Value type: <stringlist> > + Definition: A set of names that identify the usecase names of a client that uses > + cache slice. These strings are used to look up the cache slice > + entries by name. > + > +- cache-slices: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: The tuple has phandle to llcc device as the first argument and the > + second argument is the usecase id of the client. What is a "usecase id" ? Is this meant to align with #cache-cells? It would be best to keep a common prefix (i.e. call that #cache-slice-cells). Thanks, Mark.
On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: > Documentation for last level cache controller device tree bindings, > client bindings usage examples. > > Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> > --- > .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 ++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > +Example: > + > + qcom,system-cache@1300000 { > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; > + reg = <0x1300000 0x50000>; > + reg-names = "llcc_base"; > + > + llcc: qcom,sdm845-llcc { > + compatible = "qcom,sdm845-llcc"; > + #cache-cells = <1>; > + max-slices = <32>; > + }; > + > + qcom,llcc-ecc { > + compatible = "qcom,llcc-ecc"; > + }; > + > + qcom,llcc-amon { > + compatible = "qcom,llcc-amon"; > + qcom,fg-cnt = <0x7>; > + }; > + > + }; The "qcom,llcc-ecc" and "qcom,llcc-amon" bindings doesn't seem to be used by the driver in patch 2, and it's not clear how they are intended to be used, so I think they should go from the binding for now. I don't think you need syscon and simple-mfd, and I think you can simplify the binding to a single node like: qcom,system-cache@1300000 { compatible = "qcom,sdm845-llcc"; reg = <0x1300000 0x50000>; #cache-slice-cells = <1>; max-slices = <32>; } If ECC and AMON are option features, we can always add boolean properties for those later, e.g. "has-ecc". Thanks, Mark.
On 2018-02-01 02:44, Mark Rutland wrote: > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: >> Documentation for last level cache controller device tree bindings, >> client bindings usage examples. >> >> Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> >> --- >> .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 >> ++++++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> new file mode 100644 >> index 0000000..d433b0c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> @@ -0,0 +1,93 @@ >> +* LLCC (Last Level Cache Controller) >> + >> +Properties: >> +- compatible: >> + Usage: required >> + Value type: <string> >> + Definition: must be "qcom,llcc-core" >> + >> +- reg: >> + Usage: required >> + Value Type: <prop-encoded-array> >> + Definition: must be addresses and sizes of the LLCC registers >> + >> +- llcc-bank-off: >> + Usage: required >> + Value Type: <u32 array> >> + Definition: Offsets of llcc banks from llcc base address starting >> from >> + LLCC bank0. >> + >> +- llcc-broadcast-off: >> + Usage: required >> + Value Type: <u32> >> + Definition: Offset of broadcast register from LLCC bank0 address. > > Please could we use "offset" rather than "off" for both of these? That > way it's obvious these aren't properties for disabling some feature. > > How variable are these offsets in practice? Is the memory map not > fixed? The offsets depends on the number of LLCC HW blocks. These number of HW blocks vary from chipset to chipset and new registers could be added that changes the offset. > >> + >> +- #cache-cells: >> + Usage: required >> + Value Type: <u32> >> + Definition: Number of cache cells, must be 1 > > What's this for, and how is it used? This is to obtain the phandle arguments from client devices. Related to cache-slices property. > >> + >> +- max-slices: >> + usage: required >> + Value Type: <u32> >> + Definition: Number of cache slices supported by hardware >> + >> +- status: >> + Usage: optional >> + Value type: <string> >> + Definition: Property to enable or disable the driver > > This is a standard property, so I don't think it needs to be described > here. Sure, will remove it. > >> + >> +== llcc amon device == >> + >> +Properties: >> +-qcom,fg-cnt : The value of fine grained counter of activity monitor >> + block. > > Could you elaborate on this? This is counter value programmed in the HW to detect live locks. This parameter is tunable to avoid false positives. > >> + >> +compatible devices: >> + qcom,sdm845-llcc > > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's > not clear what this means. > >> + >> +Example: >> + >> + qcom,system-cache@1300000 { >> + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; > > This looks very wrong. Why do you need syscon and simple-mfd? LLCC HW block has 3 functionalities: System cache core, ECC & AMON drivers for debugging. All three drivers use the same register space for configuration, status etc. In order to avoid remapping the same address region across multiple drivers, I have implemented this driver as a syncon and simple-mfd. > >> + reg = <0x1300000 0x50000>; >> + reg-names = "llcc_base"; >> + >> + llcc: qcom,sdm845-llcc { >> + compatible = "qcom,sdm845-llcc"; > > Why is this a sub-node? qcom,sdm845-llcc: This core driver as mentioned in the list above. > > Why isn't the top-level node just "qcom,sdm845-llcc" ? > >> + #cache-cells = <1>; >> + max-slices = <32>; >> + }; >> + >> + qcom,llcc-ecc { >> + compatible = "qcom,llcc-ecc"; >> + }; qcom,llcc-ecc: Driver #2 for ECC >> + >> + qcom,llcc-amon { >> + compatible = "qcom,llcc-amon"; >> + qcom,fg-cnt = <0x7>; >> + }; >> + qcom,llcc-amon: Driver #3 for AMON >> + }; >> + >> +== Client == >> + >> +Properties: >> +- cache-slice-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: A set of names that identify the usecase names of a >> client that uses >> + cache slice. These strings are used to look up the cache slice >> + entries by name. >> + >> +- cache-slices: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: The tuple has phandle to llcc device as the first >> argument and the >> + second argument is the usecase id of the client. > > What is a "usecase id" ? Usecase id for use case that wants to use system cache for eg: video-encode and video-decode > > Is this meant to align with #cache-cells? It would be best to keep a > common prefix (i.e. call that #cache-slice-cells). Yes. Will update the name. > > Thanks, > Mark.
On 2018-02-01 02:48, Mark Rutland wrote: > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: >> Documentation for last level cache controller device tree bindings, >> client bindings usage examples. >> >> Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> >> --- >> .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 >> ++++++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> >> +Example: >> + >> + qcom,system-cache@1300000 { >> + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; >> + reg = <0x1300000 0x50000>; >> + reg-names = "llcc_base"; >> + >> + llcc: qcom,sdm845-llcc { >> + compatible = "qcom,sdm845-llcc"; >> + #cache-cells = <1>; >> + max-slices = <32>; >> + }; >> + >> + qcom,llcc-ecc { >> + compatible = "qcom,llcc-ecc"; >> + }; >> + >> + qcom,llcc-amon { >> + compatible = "qcom,llcc-amon"; >> + qcom,fg-cnt = <0x7>; >> + }; >> + >> + }; > > The "qcom,llcc-ecc" and "qcom,llcc-amon" bindings doesn't seem to be > used by the driver in patch 2, and it's not clear how they are intended > to be used, so I think they should go from the binding for now. Sure I can remove them for now and add them when the I push other drivers for review. > > I don't think you need syscon and simple-mfd, and I think you can I used syscon and simple-mfd because three drivers touch the same address space. Driver 1: system cache core: The purpose of the driver is to partition the system cache and program the settings such as priority, lines to probe while doing a look up in the system cache, low power related settings etc. The driver also provides API for clients to query the cache slice details, activate and deactivate them. Driver 2: ECC to detect single and double bit errors in LLCC memory. Implemented using EDAC framework. Driver 3: AMON: Activity Monitor to detect live lock ups in the HW block. Implemented as SOC driver similar to driver #1. Since the hardware block has multiple functional units the driver is implemented to use syscon/regmap interface. > simplify the binding to a single node like: > > qcom,system-cache@1300000 { > compatible = "qcom,sdm845-llcc"; > reg = <0x1300000 0x50000>; > #cache-slice-cells = <1>; > max-slices = <32>; > } > > If ECC and AMON are option features, we can always add boolean > properties for those later, e.g. "has-ecc". > > Thanks, > Mark.
On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote: > On 2018-02-01 02:44, Mark Rutland wrote: > > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: > > > Documentation for last level cache controller device tree bindings, > > > client bindings usage examples. > > > > > > Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> > > > --- > > > .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 > > > ++++++++++++++++++++++ > > > 1 file changed, 93 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > > > > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > > b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > > new file mode 100644 > > > index 0000000..d433b0c > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > > @@ -0,0 +1,93 @@ > > > +* LLCC (Last Level Cache Controller) > > > + > > > +Properties: > > > +- compatible: > > > + Usage: required > > > + Value type: <string> > > > + Definition: must be "qcom,llcc-core" > > > + > > > +- reg: > > > + Usage: required > > > + Value Type: <prop-encoded-array> > > > + Definition: must be addresses and sizes of the LLCC registers > > > + > > > +- llcc-bank-off: > > > + Usage: required > > > + Value Type: <u32 array> > > > + Definition: Offsets of llcc banks from llcc base address starting > > > from > > > + LLCC bank0. > > > + > > > +- llcc-broadcast-off: > > > + Usage: required > > > + Value Type: <u32> > > > + Definition: Offset of broadcast register from LLCC bank0 address. > > > > Please could we use "offset" rather than "off" for both of these? That > > way it's obvious these aren't properties for disabling some feature. > > > > How variable are these offsets in practice? Is the memory map not fixed? > > The offsets depends on the number of LLCC HW blocks. These number of HW > blocks vary from > chipset to chipset and new registers could be added that changes the offset. Surely if new registers are added, we need a new compatible string? Can't we encode the number of LLCC HW blocks, instead? Presumably that would give enough information to cover both llcc-bank-off and llcc-broadcast-off. [...] > > > + > > > +compatible devices: > > > + qcom,sdm845-llcc > > > > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's > > not clear what this means. > > > > > + > > > +Example: > > > + > > > + qcom,system-cache@1300000 { > > > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; > > > > This looks very wrong. Why do you need syscon and simple-mfd? > > LLCC HW block has 3 functionalities: > System cache core, ECC & AMON drivers for debugging. > All three drivers use the same register space for configuration, status etc. > In order to avoid remapping the same address region across multiple drivers, > I have implemented this driver as a syncon and simple-mfd. Please don't do that; that's completely dependent on Linux implementation details. Have one top level driver for the whole LLCC block, which maps the registers, and provides an API for accessing them. When that probes, it can cause the other drivers to be probed (e.g. with a platform device), and those can access the LLCC registers via that API. > > > + reg = <0x1300000 0x50000>; > > > + reg-names = "llcc_base"; > > > + > > > + llcc: qcom,sdm845-llcc { > > > + compatible = "qcom,sdm845-llcc"; > > > > Why is this a sub-node? > qcom,sdm845-llcc: This core driver as mentioned in the list above. > > > > Why isn't the top-level node just "qcom,sdm845-llcc" ? > > > > > + #cache-cells = <1>; > > > + max-slices = <32>; > > > + }; > > > + > > > + qcom,llcc-ecc { > > > + compatible = "qcom,llcc-ecc"; > > > + }; > > qcom,llcc-ecc: Driver #2 for ECC > > > > + > > > + qcom,llcc-amon { > > > + compatible = "qcom,llcc-amon"; > > > + qcom,fg-cnt = <0x7>; > > > + }; > > > + > > qcom,llcc-amon: Driver #3 for AMON Please describe the HW, not the drivers. As above, I don't believe you need multiple nodes here. Linux can instantiate the drivers as necessary. [...] > > > +- cache-slices: > > > + Usage: required > > > + Value type: <prop-encoded-array> > > > + Definition: The tuple has phandle to llcc device as the first > > > argument and the > > > + second argument is the usecase id of the client. > > > > What is a "usecase id" ? > > Usecase id for use case that wants to use system cache for eg: video-encode > and video-decode Sure, but how is the value used? Is it the index of a slice? Or something more abstract? Thanks, Mark.
On Thu, Feb 01, 2018 at 12:47:01PM -0800, Channa wrote: > On 2018-02-01 02:48, Mark Rutland wrote: > > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: > > > Documentation for last level cache controller device tree bindings, > > > client bindings usage examples. > > > > > > Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> > > > --- > > > .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 > > > ++++++++++++++++++++++ > > > 1 file changed, 93 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt > > > > > > +Example: > > > + > > > + qcom,system-cache@1300000 { > > > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; > > > + reg = <0x1300000 0x50000>; > > > + reg-names = "llcc_base"; > > > + > > > + llcc: qcom,sdm845-llcc { > > > + compatible = "qcom,sdm845-llcc"; > > > + #cache-cells = <1>; > > > + max-slices = <32>; > > > + }; > > > + > > > + qcom,llcc-ecc { > > > + compatible = "qcom,llcc-ecc"; > > > + }; > > > + > > > + qcom,llcc-amon { > > > + compatible = "qcom,llcc-amon"; > > > + qcom,fg-cnt = <0x7>; > > > + }; > > > + > > > + }; > > > > The "qcom,llcc-ecc" and "qcom,llcc-amon" bindings doesn't seem to be > > used by the driver in patch 2, and it's not clear how they are intended > > to be used, so I think they should go from the binding for now. > > Sure I can remove them for now and add them when the I push other drivers > for review. > > > I don't think you need syscon and simple-mfd, and I think you can > > I used syscon and simple-mfd because three drivers touch the same address > space. As in my other reply, I don't think that's the right way to solve the problem. Please have one top-level driver and associated binding. That driver can carve up the functional units and allow other drivers to access the registers as necessary. Thanks, Mark.
On 2018-02-02 03:05, Mark Rutland wrote: > On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote: >> On 2018-02-01 02:44, Mark Rutland wrote: >> > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: >> > > Documentation for last level cache controller device tree bindings, >> > > client bindings usage examples. >> > > >> > > Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> >> > > --- >> > > .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 >> > > ++++++++++++++++++++++ >> > > 1 file changed, 93 insertions(+) >> > > create mode 100644 >> > > Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> > > >> > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> > > b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> > > new file mode 100644 >> > > index 0000000..d433b0c >> > > --- /dev/null >> > > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt >> > > @@ -0,0 +1,93 @@ >> > > +* LLCC (Last Level Cache Controller) >> > > + >> > > +Properties: >> > > +- compatible: >> > > + Usage: required >> > > + Value type: <string> >> > > + Definition: must be "qcom,llcc-core" >> > > + >> > > +- reg: >> > > + Usage: required >> > > + Value Type: <prop-encoded-array> >> > > + Definition: must be addresses and sizes of the LLCC registers >> > > + >> > > +- llcc-bank-off: >> > > + Usage: required >> > > + Value Type: <u32 array> >> > > + Definition: Offsets of llcc banks from llcc base address starting >> > > from >> > > + LLCC bank0. >> > > + >> > > +- llcc-broadcast-off: >> > > + Usage: required >> > > + Value Type: <u32> >> > > + Definition: Offset of broadcast register from LLCC bank0 address. >> > >> > Please could we use "offset" rather than "off" for both of these? That >> > way it's obvious these aren't properties for disabling some feature. >> > >> > How variable are these offsets in practice? Is the memory map not fixed? >> >> The offsets depends on the number of LLCC HW blocks. These number of >> HW >> blocks vary from >> chipset to chipset and new registers could be added that changes the >> offset. > > Surely if new registers are added, we need a new compatible string? > > Can't we encode the number of LLCC HW blocks, instead? Presumably that > would give enough information to cover both llcc-bank-off and > llcc-broadcast-off. > > [...] Are you suggesting to move these offset handing out of DTS files and manage in the driver? > >> > > + >> > > +compatible devices: >> > > + qcom,sdm845-llcc >> > >> > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's >> > not clear what this means. >> > >> > > + >> > > +Example: >> > > + >> > > + qcom,system-cache@1300000 { >> > > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; >> > >> > This looks very wrong. Why do you need syscon and simple-mfd? >> >> LLCC HW block has 3 functionalities: >> System cache core, ECC & AMON drivers for debugging. >> All three drivers use the same register space for configuration, >> status etc. >> In order to avoid remapping the same address region across multiple >> drivers, >> I have implemented this driver as a syncon and simple-mfd. > > Please don't do that; that's completely dependent on Linux > implementation details. Why do you think simple-mfd is not good here? The LLCC HW clock is outside of CPUSS and has multiple functional blocks. > > Have one top level driver for the whole LLCC block, which maps the > registers, and provides an API for accessing them. When that probes, it > can cause the other drivers to be probed (e.g. with a platform device), > and those can access the LLCC registers via that API. > >> > > + reg = <0x1300000 0x50000>; >> > > + reg-names = "llcc_base"; >> > > + >> > > + llcc: qcom,sdm845-llcc { >> > > + compatible = "qcom,sdm845-llcc"; >> > >> > Why is this a sub-node? >> qcom,sdm845-llcc: This core driver as mentioned in the list above. >> > >> > Why isn't the top-level node just "qcom,sdm845-llcc" ? >> > >> > > + #cache-cells = <1>; >> > > + max-slices = <32>; >> > > + }; >> > > + >> > > + qcom,llcc-ecc { >> > > + compatible = "qcom,llcc-ecc"; >> > > + }; >> >> qcom,llcc-ecc: Driver #2 for ECC >> >> > > + >> > > + qcom,llcc-amon { >> > > + compatible = "qcom,llcc-amon"; >> > > + qcom,fg-cnt = <0x7>; >> > > + }; >> > > + >> >> qcom,llcc-amon: Driver #3 for AMON > > Please describe the HW, not the drivers. > > As above, I don't believe you need multiple nodes here. Linux can > instantiate the drivers as necessary. > > [...] > >> > > +- cache-slices: >> > > + Usage: required >> > > + Value type: <prop-encoded-array> >> > > + Definition: The tuple has phandle to llcc device as the first >> > > argument and the >> > > + second argument is the usecase id of the client. >> > >> > What is a "usecase id" ? >> >> Usecase id for use case that wants to use system cache for eg: >> video-encode >> and video-decode > > Sure, but how is the value used? Is it the index of a slice? Or > something more abstract? This is used as an index to the SCT (System cache Table) configuration data that controls the behavior of each cache slice. > > Thanks, > Mark.
Hiya, On 25 January 2018 at 17:55, Channagoud Kadabi <ckadabi@codeaurora.org> wrote: > Documentation for last level cache controller device tree bindings, > client bindings usage examples. [snippety snip] > +- llcc-bank-off: > + Usage: required > + Value Type: <u32 array> > + Definition: Offsets of llcc banks from llcc base address starting from > + LLCC bank0. > + > +- llcc-broadcast-off: > + Usage: required > + Value Type: <u32> > + Definition: Offset of broadcast register from LLCC bank0 address. What's with the redundant namespacing? Have we not, as a community, realised that we do not need to namespace properties which are only present under a single binding or node, or even those that aren't? This mess started with the regulator bindings and it's never stopped. What are we at now, 25 years of device trees, 10 years of FDT on Arm? Notwithstanding the complete waste of rodata in the kernel image for matching (& increased time to compare), why wouldn't you consider why "bank-offset" for your node be any different than a common property for any other node? And if you need to describe register offsets... why aren't you able to use the reg property? Ta, Matt
On 2018-02-08 08:52, Matt Sealey wrote: > Hiya, > > On 25 January 2018 at 17:55, Channagoud Kadabi <ckadabi@codeaurora.org> > wrote: >> Documentation for last level cache controller device tree bindings, >> client bindings usage examples. > > [snippety snip] > >> +- llcc-bank-off: >> + Usage: required >> + Value Type: <u32 array> >> + Definition: Offsets of llcc banks from llcc base address >> starting from >> + LLCC bank0. >> + >> +- llcc-broadcast-off: >> + Usage: required >> + Value Type: <u32> >> + Definition: Offset of broadcast register from LLCC bank0 >> address. > > What's with the redundant namespacing? > > Have we not, as a community, realised that we do not need to namespace > properties which are only present under > a single binding or node, or even those that aren't? This mess started > with the regulator bindings and it's never > stopped. What are we at now, 25 years of device trees, 10 years of FDT > on Arm? > > Notwithstanding the complete waste of rodata in the kernel image for > matching (& increased time to compare), why > wouldn't you consider why "bank-offset" for your node be any different > than a common property for any other node? I will clean up the name space and use bank-offset for the property name. > > And if you need to describe register offsets... why aren't you able to > use the reg property? Reg property did not suit well for my need, so I choose to maintain offsets instead. The registers in the HW block are organized as (offset1) (offset2) (offset3) (offset4) Base(Block0) -- Block1 -- Block 2 -- Block 3 -- Broadcast_Block Each block has identical register mapping. You can think of it as 4 instances of identical HW. Broadcast block is to simplify writes, you don't need to write to individual blocks instead write to broadcast block. I use simple-mfd/syscon as the hardware has multiple functions. Doing regmap on the the Base (Block0) and maintaining offset makes the driver cleaner rather than using the reg property for each instance. > > Ta, > Matt
On Thu, Feb 08, 2018 at 04:24:16PM -0800, Channa wrote: > On 2018-02-08 08:52, Matt Sealey wrote: > > On 25 January 2018 at 17:55, Channagoud Kadabi <ckadabi@codeaurora.org> > > wrote: > > > Documentation for last level cache controller device tree bindings, > > > client bindings usage examples. > > > > [snippety snip] > > > > > +- llcc-bank-off: > > > + Usage: required > > > + Value Type: <u32 array> > > > + Definition: Offsets of llcc banks from llcc base address > > > starting from > > > + LLCC bank0. > > > + > > > +- llcc-broadcast-off: > > > + Usage: required > > > + Value Type: <u32> > > > + Definition: Offset of broadcast register from LLCC bank0 > > > address. > > And if you need to describe register offsets... why aren't you able to > > use the reg property? > > Reg property did not suit well for my need, so I choose to maintain offsets > instead. > > The registers in the HW block are organized as > (offset1) (offset2) (offset3) (offset4) > Base(Block0) -- Block1 -- Block 2 -- Block 3 -- Broadcast_Block > > Each block has identical register mapping. You can think of it as 4 > instances of identical HW. We have similar in other devices (e.g. GICv3 has multiple instances of the same register block). If there's nothing between those blocks, using a reg entry sounds prefectly reasonable. You can list the broadcast block first, then the others, or use reg-names. Thanks, Mark.
On Tue, Feb 06, 2018 at 11:56:50AM -0800, Channa wrote: > On 2018-02-02 03:05, Mark Rutland wrote: > > On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote: > > > On 2018-02-01 02:44, Mark Rutland wrote: > > > > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: > > > > > +- llcc-bank-off: > > > > > + Usage: required > > > > > + Value Type: <u32 array> > > > > > + Definition: Offsets of llcc banks from llcc base address starting > > > > > from > > > > > + LLCC bank0. > > > > > + > > > > > +- llcc-broadcast-off: > > > > > + Usage: required > > > > > + Value Type: <u32> > > > > > + Definition: Offset of broadcast register from LLCC bank0 address. > > > > > > > > Please could we use "offset" rather than "off" for both of these? That > > > > way it's obvious these aren't properties for disabling some feature. > > > > > > > > How variable are these offsets in practice? Is the memory map not fixed? > > > > > > The offsets depends on the number of LLCC HW blocks. These number of > > > HW > > > blocks vary from > > > chipset to chipset and new registers could be added that changes the > > > offset. > > > > Surely if new registers are added, we need a new compatible string? > > > > Can't we encode the number of LLCC HW blocks, instead? Presumably that > > would give enough information to cover both llcc-bank-off and > > llcc-broadcast-off. > > > > [...] > > Are you suggesting to move these offset handing out of DTS files and manage > in the driver? Something like that, though it depends on how exactly the offsets can be derived. Using reg entries, as Matt suggested, sounds better though. > > > > > +compatible devices: > > > > > + qcom,sdm845-llcc > > > > > > > > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's > > > > not clear what this means. > > > > > > > > > + > > > > > +Example: > > > > > + > > > > > + qcom,system-cache@1300000 { > > > > > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; > > > > > > > > This looks very wrong. Why do you need syscon and simple-mfd? > > > > > > LLCC HW block has 3 functionalities: > > > System cache core, ECC & AMON drivers for debugging. > > > All three drivers use the same register space for configuration, > > > status etc. > > > In order to avoid remapping the same address region across multiple > > > drivers, > > > I have implemented this driver as a syncon and simple-mfd. > > > > Please don't do that; that's completely dependent on Linux > > implementation details. > > Why do you think simple-mfd is not good here? The LLCC HW clock is outside > of CPUSS and has > multiple functional blocks. For one thing, there's no need for this to be a syscon *and* a simple-mfd. W.R.T. simple-mfd, I think it would bet better to decompose the device in a top-level driver, as I described in my prior reply, rather than describing a set of drivers (which are not themselves HW). > > > > > +- cache-slices: > > > > > + Usage: required > > > > > + Value type: <prop-encoded-array> > > > > > + Definition: The tuple has phandle to llcc device as the first > > > > > argument and the > > > > > + second argument is the usecase id of the client. > > > > > > > > What is a "usecase id" ? > > > > > > Usecase id for use case that wants to use system cache for eg: > > > video-encode > > > and video-decode > > > > Sure, but how is the value used? Is it the index of a slice? Or > > something more abstract? > > This is used as an index to the SCT (System cache Table) configuration > data that controls the behavior of each cache slice. Ok. Where does that SCT live? Is that in HW? Is it programmed by SW, or statically configured for a given platform? Thanks, Mark.
On 2018-02-13 06:37, Mark Rutland wrote: > On Tue, Feb 06, 2018 at 11:56:50AM -0800, Channa wrote: >> On 2018-02-02 03:05, Mark Rutland wrote: >> > On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote: >> > > On 2018-02-01 02:44, Mark Rutland wrote: >> > > > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote: >> > > > > +- llcc-bank-off: >> > > > > + Usage: required >> > > > > + Value Type: <u32 array> >> > > > > + Definition: Offsets of llcc banks from llcc base address starting >> > > > > from >> > > > > + LLCC bank0. >> > > > > + >> > > > > +- llcc-broadcast-off: >> > > > > + Usage: required >> > > > > + Value Type: <u32> >> > > > > + Definition: Offset of broadcast register from LLCC bank0 address. >> > > > >> > > > Please could we use "offset" rather than "off" for both of these? That >> > > > way it's obvious these aren't properties for disabling some feature. >> > > > >> > > > How variable are these offsets in practice? Is the memory map not fixed? >> > > >> > > The offsets depends on the number of LLCC HW blocks. These number of >> > > HW >> > > blocks vary from >> > > chipset to chipset and new registers could be added that changes the >> > > offset. >> > >> > Surely if new registers are added, we need a new compatible string? >> > >> > Can't we encode the number of LLCC HW blocks, instead? Presumably that >> > would give enough information to cover both llcc-bank-off and >> > llcc-broadcast-off. >> > >> > [...] >> >> Are you suggesting to move these offset handing out of DTS files and >> manage >> in the driver? > > Something like that, though it depends on how exactly the offsets can > be > derived. > > Using reg entries, as Matt suggested, sounds better though. > >> > > > > +compatible devices: >> > > > > + qcom,sdm845-llcc >> > > > >> > > > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's >> > > > not clear what this means. >> > > > >> > > > > + >> > > > > +Example: >> > > > > + >> > > > > + qcom,system-cache@1300000 { >> > > > > + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; >> > > > >> > > > This looks very wrong. Why do you need syscon and simple-mfd? >> > > >> > > LLCC HW block has 3 functionalities: >> > > System cache core, ECC & AMON drivers for debugging. >> > > All three drivers use the same register space for configuration, >> > > status etc. >> > > In order to avoid remapping the same address region across multiple >> > > drivers, >> > > I have implemented this driver as a syncon and simple-mfd. >> > >> > Please don't do that; that's completely dependent on Linux >> > implementation details. >> >> Why do you think simple-mfd is not good here? The LLCC HW clock is >> outside >> of CPUSS and has >> multiple functional blocks. > > For one thing, there's no need for this to be a syscon *and* a > simple-mfd. > > W.R.T. simple-mfd, I think it would bet better to decompose the device > in a top-level driver, as I described in my prior reply, rather than > describing a set of drivers (which are not themselves HW). > >> > > > > +- cache-slices: >> > > > > + Usage: required >> > > > > + Value type: <prop-encoded-array> >> > > > > + Definition: The tuple has phandle to llcc device as the first >> > > > > argument and the >> > > > > + second argument is the usecase id of the client. >> > > > >> > > > What is a "usecase id" ? >> > > >> > > Usecase id for use case that wants to use system cache for eg: >> > > video-encode >> > > and video-decode >> > >> > Sure, but how is the value used? Is it the index of a slice? Or >> > something more abstract? >> >> This is used as an index to the SCT (System cache Table) configuration >> data that controls the behavior of each cache slice. > > Ok. Where does that SCT live? Is that in HW? Is it programmed by SW, or > statically configured for a given platform? SCT is in the HW. Kernel driver programs these settings for a given platform. The table is also used to lookup size, cache slice id details requested by client drivers. > > Thanks, > Mark.
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt new file mode 100644 index 0000000..d433b0c --- /dev/null +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt @@ -0,0 +1,93 @@ +* LLCC (Last Level Cache Controller) + +Properties: +- compatible: + Usage: required + Value type: <string> + Definition: must be "qcom,llcc-core" + +- reg: + Usage: required + Value Type: <prop-encoded-array> + Definition: must be addresses and sizes of the LLCC registers + +- llcc-bank-off: + Usage: required + Value Type: <u32 array> + Definition: Offsets of llcc banks from llcc base address starting from + LLCC bank0. + +- llcc-broadcast-off: + Usage: required + Value Type: <u32> + Definition: Offset of broadcast register from LLCC bank0 address. + +- #cache-cells: + Usage: required + Value Type: <u32> + Definition: Number of cache cells, must be 1 + +- max-slices: + usage: required + Value Type: <u32> + Definition: Number of cache slices supported by hardware + +- status: + Usage: optional + Value type: <string> + Definition: Property to enable or disable the driver + +== llcc amon device == + +Properties: +-qcom,fg-cnt : The value of fine grained counter of activity monitor + block. + +compatible devices: + qcom,sdm845-llcc + +Example: + + qcom,system-cache@1300000 { + compatible = "qcom,llcc-core", "syscon", "simple-mfd"; + reg = <0x1300000 0x50000>; + reg-names = "llcc_base"; + + llcc: qcom,sdm845-llcc { + compatible = "qcom,sdm845-llcc"; + #cache-cells = <1>; + max-slices = <32>; + }; + + qcom,llcc-ecc { + compatible = "qcom,llcc-ecc"; + }; + + qcom,llcc-amon { + compatible = "qcom,llcc-amon"; + qcom,fg-cnt = <0x7>; + }; + + }; + +== Client == + +Properties: +- cache-slice-names: + Usage: required + Value type: <stringlist> + Definition: A set of names that identify the usecase names of a client that uses + cache slice. These strings are used to look up the cache slice + entries by name. + +- cache-slices: + Usage: required + Value type: <prop-encoded-array> + Definition: The tuple has phandle to llcc device as the first argument and the + second argument is the usecase id of the client. +For example: + + video-decoder-encoder { + cache-slice-names = "vidsc0", "vidsc1"; + cache-slices = <&llcc 2>, <&llcc 3>; + };
Documentation for last level cache controller device tree bindings, client bindings usage examples. Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org> --- .../devicetree/bindings/arm/msm/qcom,llcc.txt | 93 ++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt