| Message ID | 20241125174511.45-1-quic_rlaggysh@quicinc.com |
|---|---|
| Headers | show |
| Series | Add EPSS L3 provider support on SA8775P SoC | expand |
On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of > REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible > "qcom,epss-l3-perf" for these targets. Is this a h/w difference from prior blocks or you just want to use B instead of A while the h/w has both A and B? The latter sounds like driver policy. It is also an ABI break for s/w that didn't understand qcom,epss-l3-perf. > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > --- > .../devicetree/bindings/interconnect/qcom,osm-l3.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > index 21dae0b92819..e24399ca110f 100644 > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > @@ -28,12 +28,15 @@ properties: > - const: qcom,osm-l3 > - items: > - enum: > - - qcom,sc7280-epss-l3 > - qcom,sc8280xp-epss-l3 > - qcom,sm6375-cpucp-l3 > - - qcom,sm8250-epss-l3 > - qcom,sm8350-epss-l3 > - const: qcom,epss-l3 > + - items: > + - enum: > + - qcom,sc7280-epss-l3 > + - qcom,sm8250-epss-l3 > + - const: qcom,epss-l3-perf > > reg: > maxItems: 1 > -- > 2.39.2 >
On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: > On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: > > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of > > REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible > > "qcom,epss-l3-perf" for these targets. > > Is this a h/w difference from prior blocks or you just want to use B > instead of A while the h/w has both A and B? The latter sounds like > driver policy. > > It is also an ABI break for s/w that didn't understand > qcom,epss-l3-perf. As the bindings keep old compatible strings in addition to the new qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, newer can use either of those. > > > > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > > --- > > .../devicetree/bindings/interconnect/qcom,osm-l3.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > index 21dae0b92819..e24399ca110f 100644 > > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml > > @@ -28,12 +28,15 @@ properties: > > - const: qcom,osm-l3 > > - items: > > - enum: > > - - qcom,sc7280-epss-l3 > > - qcom,sc8280xp-epss-l3 > > - qcom,sm6375-cpucp-l3 > > - - qcom,sm8250-epss-l3 > > - qcom,sm8350-epss-l3 > > - const: qcom,epss-l3 > > + - items: > > + - enum: > > + - qcom,sc7280-epss-l3 > > + - qcom,sm8250-epss-l3 > > + - const: qcom,epss-l3-perf > > > > reg: > > maxItems: 1 > > -- > > 2.39.2 > >
On 27/11/2024 17:53, Dmitry Baryshkov wrote: > On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>> "qcom,epss-l3-perf" for these targets. >> >> Is this a h/w difference from prior blocks or you just want to use B >> instead of A while the h/w has both A and B? The latter sounds like >> driver policy. >> >> It is also an ABI break for s/w that didn't understand >> qcom,epss-l3-perf. > > As the bindings keep old compatible strings in addition to the new > qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, > newer can use either of those. No, this change drops qcom,epss-l3 and adds new fallback. How old software can work in such case? It's broken. Best regards, Krzysztof
On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >On 27/11/2024 17:53, Dmitry Baryshkov wrote: >> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>> "qcom,epss-l3-perf" for these targets. >>> >>> Is this a h/w difference from prior blocks or you just want to use B >>> instead of A while the h/w has both A and B? The latter sounds like >>> driver policy. >>> >>> It is also an ABI break for s/w that didn't understand >>> qcom,epss-l3-perf. >> >> As the bindings keep old compatible strings in addition to the new >> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >> newer can use either of those. >No, this change drops qcom,epss-l3 and adds new fallback. How old >software can work in such case? It's broken. Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". > >Best regards, >Krzysztof
On 27/11/2024 19:49, Dmitry Baryshkov wrote: > On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 27/11/2024 17:53, Dmitry Baryshkov wrote: >>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>>> "qcom,epss-l3-perf" for these targets. >>>> >>>> Is this a h/w difference from prior blocks or you just want to use B >>>> instead of A while the h/w has both A and B? The latter sounds like >>>> driver policy. >>>> >>>> It is also an ABI break for s/w that didn't understand >>>> qcom,epss-l3-perf. >>> >>> As the bindings keep old compatible strings in addition to the new >>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >>> newer can use either of those. >> No, this change drops qcom,epss-l3 and adds new fallback. How old >> software can work in such case? It's broken. > > Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". It depends for example whether epss-l3 is valid at all. ABI is not broken if nothing was working in the first place, assuming it is explained in commit msg (not the case here). Best regards, Krzysztof
On 27 November 2024 21:22:02 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >On 27/11/2024 19:49, Dmitry Baryshkov wrote: >> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> On 27/11/2024 17:53, Dmitry Baryshkov wrote: >>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>>>> "qcom,epss-l3-perf" for these targets. >>>>> >>>>> Is this a h/w difference from prior blocks or you just want to use B >>>>> instead of A while the h/w has both A and B? The latter sounds like >>>>> driver policy. >>>>> >>>>> It is also an ABI break for s/w that didn't understand >>>>> qcom,epss-l3-perf. >>>> >>>> As the bindings keep old compatible strings in addition to the new >>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >>>> newer can use either of those. >>> No, this change drops qcom,epss-l3 and adds new fallback. How old >>> software can work in such case? It's broken. >> >> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". > >It depends for example whether epss-l3 is valid at all. ABI is not >broken if nothing was working in the first place, assuming it is >explained in commit msg (not the case here). Judging by the current schema, epss-l3 is defined as new HW block of aka not OSM L3, no matter which register is used for programming. > >Best regards, >Krzysztof
On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote: > EPSS on SA8775P has two instances which requires creation of two device > nodes with different compatible and device data because of unique > icc node id and name limitation in interconnect framework. > Add multidevice support to osm-l3 code to get unique node id from IDA > and node name is made unique by appending node address. > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > --- [...] > + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL); > + if (ret) > + return ret; > + > qp->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(qp->base)) > return PTR_ERR(qp->base); > @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > > icc_provider_init(provider); > > + /* Allocate unique id for qnodes */ > + for (i = 0; i < num_nodes; i++) > + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL); As I've said in my previous emails, this is a framework-level problem. Up until now we've simply silently ignored the possibility of an interconnect provider having more than one instance, as conveniently most previous SoCs had a bunch of distinct bus masters. Currently, debugfs-client.c relies on the node names being unique. Keeping them as such is also useful for having a sane sysfs/debugfs interface. But it's not always feasible, and a hierarchical approach (like in pmdomain) may be a better fit. Then, node->id is used for creating links, and we unfortunately cannot assume that both src and dst are within the same provider. I'm not a fan of these IDs being hardcoded, but there are some drivers that rely on that, which itself is also a bit unfortunate.. If Mike (who introduced debugfs-client and is probably the main user) doesn't object to a small ABI break (which is "fine" with a debugfs driver that requires editing the source code to be compiled), we could add a property within icc_provider like `bool dynamic_ids` and have an ICC-global IDA that would take care of any conflicts. Provider drivers whose consumers don't already rely on programmatical use of hardcoded IDs *and* don't have cross-provider links could then enable that flag and have the node IDs and names set like you did in this patch. This also sounds very useful for icc-clk. Konrad
On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote: > On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote: > > EPSS on SA8775P has two instances which requires creation of two device > > nodes with different compatible and device data because of unique > > icc node id and name limitation in interconnect framework. > > Add multidevice support to osm-l3 code to get unique node id from IDA > > and node name is made unique by appending node address. > > > > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > > --- > > [...] > > > + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL); > > + if (ret) > > + return ret; > > + > > qp->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(qp->base)) > > return PTR_ERR(qp->base); > > @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > > > > icc_provider_init(provider); > > > > + /* Allocate unique id for qnodes */ > > + for (i = 0; i < num_nodes; i++) > > + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL); > > As I've said in my previous emails, this is a framework-level problem. > > Up until now we've simply silently ignored the possibility of an > interconnect provider having more than one instance, as conveniently > most previous SoCs had a bunch of distinct bus masters. > > Currently, debugfs-client.c relies on the node names being unique. > Keeping them as such is also useful for having a sane sysfs/debugfs > interface. But it's not always feasible, and a hierarchical approach > (like in pmdomain) may be a better fit. > > Then, node->id is used for creating links, and we unfortunately cannot > assume that both src and dst are within the same provider. > I'm not a fan of these IDs being hardcoded, but there are some drivers > that rely on that, which itself is also a bit unfortunate.. > > > If Mike (who introduced debugfs-client and is probably the main user) > doesn't object to a small ABI break (which is "fine" with a debugfs > driver that requires editing the source code to be compiled), we could > add a property within icc_provider like `bool dynamic_ids` and have an > ICC-global IDA that would take care of any conflicts. Frankly speaking, I think this just delays the inevitable. We have been there with GPIOs and with some other suppliers. In my opinion the ICC subsystem needs to be refactored in order to support linking based on the supplier (fwnode?) + offset_id, but that's a huuuge rework. > Provider drivers whose consumers don't already rely on programmatical > use of hardcoded IDs *and* don't have cross-provider links could then > enable that flag and have the node IDs and names set like you did in > this patch. This also sounds very useful for icc-clk.
On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote: > On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote: >> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote: >>> EPSS on SA8775P has two instances which requires creation of two device >>> nodes with different compatible and device data because of unique >>> icc node id and name limitation in interconnect framework. >>> Add multidevice support to osm-l3 code to get unique node id from IDA >>> and node name is made unique by appending node address. >>> >>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> >>> --- >> >> [...] >> >>> + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL); >>> + if (ret) >>> + return ret; >>> + >>> qp->base = devm_platform_ioremap_resource(pdev, 0); >>> if (IS_ERR(qp->base)) >>> return PTR_ERR(qp->base); >>> @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) >>> >>> icc_provider_init(provider); >>> >>> + /* Allocate unique id for qnodes */ >>> + for (i = 0; i < num_nodes; i++) >>> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL); >> >> As I've said in my previous emails, this is a framework-level problem. >> >> Up until now we've simply silently ignored the possibility of an >> interconnect provider having more than one instance, as conveniently >> most previous SoCs had a bunch of distinct bus masters. >> >> Currently, debugfs-client.c relies on the node names being unique. >> Keeping them as such is also useful for having a sane sysfs/debugfs >> interface. But it's not always feasible, and a hierarchical approach >> (like in pmdomain) may be a better fit. >> >> Then, node->id is used for creating links, and we unfortunately cannot >> assume that both src and dst are within the same provider. >> I'm not a fan of these IDs being hardcoded, but there are some drivers >> that rely on that, which itself is also a bit unfortunate.. >> >> >> If Mike (who introduced debugfs-client and is probably the main user) >> doesn't object to a small ABI break (which is "fine" with a debugfs >> driver that requires editing the source code to be compiled), we could >> add a property within icc_provider like `bool dynamic_ids` and have an >> ICC-global IDA that would take care of any conflicts. > > Frankly speaking, I think this just delays the inevitable. We have been > there with GPIOs and with some other suppliers. In my opinion the ICC > subsystem needs to be refactored in order to support linking based on > the supplier (fwnode?) + offset_id, but that's a huuuge rework. I thought about this too, but ended up not including it in the email.. I think this will be more difficult with ICC, as tons of circular dependencies are inevitable by design and we'd essentially have to either provide placeholder nodes (like it's the case today) or probe only parts of a device, recursively, to make sure all links can be created Konrad >> Provider drivers whose consumers don't already rely on programmatical >> use of hardcoded IDs *and* don't have cross-provider links could then >> enable that flag and have the node IDs and names set like you did in >> this patch. This also sounds very useful for icc-clk. >
On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote: > On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote: > > On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote: > >> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote: > >>> EPSS on SA8775P has two instances which requires creation of two device > >>> nodes with different compatible and device data because of unique > >>> icc node id and name limitation in interconnect framework. > >>> Add multidevice support to osm-l3 code to get unique node id from IDA > >>> and node name is made unique by appending node address. > >>> > >>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com> > >>> --- > >> > >> [...] > >> > >>> + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL); > >>> + if (ret) > >>> + return ret; > >>> + > >>> qp->base = devm_platform_ioremap_resource(pdev, 0); > >>> if (IS_ERR(qp->base)) > >>> return PTR_ERR(qp->base); > >>> @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev) > >>> > >>> icc_provider_init(provider); > >>> > >>> + /* Allocate unique id for qnodes */ > >>> + for (i = 0; i < num_nodes; i++) > >>> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL); > >> > >> As I've said in my previous emails, this is a framework-level problem. > >> > >> Up until now we've simply silently ignored the possibility of an > >> interconnect provider having more than one instance, as conveniently > >> most previous SoCs had a bunch of distinct bus masters. > >> > >> Currently, debugfs-client.c relies on the node names being unique. > >> Keeping them as such is also useful for having a sane sysfs/debugfs > >> interface. But it's not always feasible, and a hierarchical approach > >> (like in pmdomain) may be a better fit. > >> > >> Then, node->id is used for creating links, and we unfortunately cannot > >> assume that both src and dst are within the same provider. > >> I'm not a fan of these IDs being hardcoded, but there are some drivers > >> that rely on that, which itself is also a bit unfortunate.. > >> > >> > >> If Mike (who introduced debugfs-client and is probably the main user) > >> doesn't object to a small ABI break (which is "fine" with a debugfs > >> driver that requires editing the source code to be compiled), we could > >> add a property within icc_provider like `bool dynamic_ids` and have an > >> ICC-global IDA that would take care of any conflicts. > > > > Frankly speaking, I think this just delays the inevitable. We have been > > there with GPIOs and with some other suppliers. In my opinion the ICC > > subsystem needs to be refactored in order to support linking based on > > the supplier (fwnode?) + offset_id, but that's a huuuge rework. > > I thought about this too, but ended up not including it in the email.. > > I think this will be more difficult with ICC, as tons of circular > dependencies are inevitable by design and we'd essentially have to > either provide placeholder nodes (like it's the case today) or probe > only parts of a device, recursively, to make sure all links can be > created Or just allow probing, but then fail path creation. It will be a redesign, but I think it is inevitable in the end. > > Konrad > > >> Provider drivers whose consumers don't already rely on programmatical > >> use of hardcoded IDs *and* don't have cross-provider links could then > >> enable that flag and have the node IDs and names set like you did in > >> this patch. This also sounds very useful for icc-clk. > >
On 11/28/2024 1:15 AM, Dmitry Baryshkov wrote: > On 27 November 2024 21:22:02 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 27/11/2024 19:49, Dmitry Baryshkov wrote: >>> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>> On 27/11/2024 17:53, Dmitry Baryshkov wrote: >>>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote: >>>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote: >>>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of >>>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible >>>>>>> "qcom,epss-l3-perf" for these targets. >>>>>> >>>>>> Is this a h/w difference from prior blocks or you just want to use B >>>>>> instead of A while the h/w has both A and B? The latter sounds like >>>>>> driver policy. >>>>>> >>>>>> It is also an ABI break for s/w that didn't understand >>>>>> qcom,epss-l3-perf. >>>>> >>>>> As the bindings keep old compatible strings in addition to the new >>>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries, >>>>> newer can use either of those. >>>> No, this change drops qcom,epss-l3 and adds new fallback. How old >>>> software can work in such case? It's broken. >>> >>> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". >> >> It depends for example whether epss-l3 is valid at all. ABI is not >> broken if nothing was working in the first place, assuming it is >> explained in commit msg (not the case here). > > Judging by the current schema, epss-l3 is defined as new HW block of aka not OSM L3, no matter which register is used for programming. > I am going to remove the newly added "qcom,epss-l3-perf" compatible and add target specific compatible "qcom,sa8775p-epss-l3" along with existing generic "qcom,epss-l3" compatible. > >> >> Best regards, >> Krzysztof >