| Message ID | 20240327055732.28198-3-yu-chang.lee@mediatek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pmdomain: mediatek: solve power domain glitch issue | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On 27/03/2024 06:57, yu-chang.lee wrote: > Add Smart Multimedia Interface Local Arbiter to mediatek > power domain. > > Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> > --- > .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml > index 8985e2df8a56..228c0dec5253 100644 > --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml > +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml > @@ -125,6 +125,10 @@ $defs: > $ref: /schemas/types.yaml#/definitions/phandle > description: phandle to the device containing the SMI register range. > > + mediatek,larb: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the device containing the LARB register range. Why do you need it? Plus I also see mediatek,larbs and mediatek,larb-id... so now we have third one similar. Best regards, Krzysztof
On Wed, 27 Mar 2024 13:57:31 +0800, yu-chang.lee wrote: > Add Smart Multimedia Interface Local Arbiter to mediatek > power domain. > > Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> > --- > .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax) ./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation) dtschema/dtc warnings/errors: make[2]: *** Deleting file 'Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts' Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts] Error 1 make[2]: *** Waiting for unfinished jobs.... /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml: while parsing a block mapping in "<unicode string>", line 64, column 5 did not find expected key in "<unicode string>", line 128, column 6 ./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2 make: *** [Makefile:240: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240327055732.28198-3-yu-chang.lee@mediatek.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 27/03/2024 09:39, Krzysztof Kozlowski wrote: > On 27/03/2024 06:57, yu-chang.lee wrote: >> Add Smart Multimedia Interface Local Arbiter to mediatek >> power domain. >> >> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> >> --- >> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml >> index 8985e2df8a56..228c0dec5253 100644 >> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml >> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml >> @@ -125,6 +125,10 @@ $defs: >> $ref: /schemas/types.yaml#/definitions/phandle >> description: phandle to the device containing the SMI register range. >> >> + mediatek,larb: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: phandle to the device containing the LARB register range. > > Why do you need it? > > Plus I also see mediatek,larbs and mediatek,larb-id... so now we have > third one similar. ... and not even tested! Best regards, Krzysztof
On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 27/03/2024 09:39, Krzysztof Kozlowski wrote: > > On 27/03/2024 06:57, yu-chang.lee wrote: > >> Add Smart Multimedia Interface Local Arbiter to mediatek > >> power domain. > >> > >> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> > >> --- > >> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 > ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git > a/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > b/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > >> index 8985e2df8a56..228c0dec5253 100644 > >> --- a/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > >> +++ b/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > >> @@ -125,6 +125,10 @@ $defs: > >> $ref: /schemas/types.yaml#/definitions/phandle > >> description: phandle to the device containing the SMI > register range. > >> > >> + mediatek,larb: > >> + $ref: /schemas/types.yaml#/definitions/phandle > >> + description: phandle to the device containing the LARB > register range. > > > > Why do you need it? > > > > Plus I also see mediatek,larbs and mediatek,larb-id... so now we > have > > third one similar. > > ... and not even tested! > > Best regards, > Krzysztof > Hi, I will double check the format of yaml for the next version, sorry for inconvenience. But I did test it on mt8188 chromebook, the reason why power domain need larb node is that when mtcmos power on, signal glitch may produce. Power domain driver must reset larb when this happen to prevent dummy transaction on bus. That why I need larb node in dts. Best Regards, Yu-chang
On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote: > On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 27/03/2024 09:39, Krzysztof Kozlowski wrote: >>> On 27/03/2024 06:57, yu-chang.lee wrote: >>>> Add Smart Multimedia Interface Local Arbiter to mediatek >>>> power domain. >>>> >>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> >>>> --- >>>> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 >> ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git >> a/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >> b/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>>> index 8985e2df8a56..228c0dec5253 100644 >>>> --- a/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>>> @@ -125,6 +125,10 @@ $defs: >>>> $ref: /schemas/types.yaml#/definitions/phandle >>>> description: phandle to the device containing the SMI >> register range. >>>> >>>> + mediatek,larb: >>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>> + description: phandle to the device containing the LARB >> register range. >>> >>> Why do you need it? >>> >>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we >> have >>> third one similar. >> >> ... and not even tested! >> >> Best regards, >> Krzysztof >> > Hi, > > I will double check the format of yaml for the next version, sorry for > inconvenience. But I did test it on mt8188 chromebook, the reason why How do you test a binding on chromebook? > power domain need larb node is that when mtcmos power on, signal glitch > may produce. Power domain driver must reset larb when this happen to > prevent dummy transaction on bus. That why I need larb node in dts. No one talks here about larb node... Best regards, Krzysztof
On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 27/03/2024 06:57, yu-chang.lee wrote: > > Add Smart Multimedia Interface Local Arbiter to mediatek > > power domain. > > > > Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> > > --- > > .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 > ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git > a/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > b/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > > index 8985e2df8a56..228c0dec5253 100644 > > --- a/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > > +++ b/Documentation/devicetree/bindings/power/mediatek,power- > controller.yaml > > @@ -125,6 +125,10 @@ $defs: > > $ref: /schemas/types.yaml#/definitions/phandle > > description: phandle to the device containing the SMI > register range. > > > > + mediatek,larb: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle to the device containing the LARB > register range. > > Why do you need it? > > Plus I also see mediatek,larbs and mediatek,larb-id... so now we have > third one similar. > MM driver used "mediatek,larbs" for it larb node. Power domain driver used "mediatek,larb". "mediatek,larb-id" is for larb in dts. The naming is no related to each other. Best Regards, Yu-chang. > Best regards, > Krzysztof >
On 27/03/2024 11:01, Yu-chang Lee (李禹璋) wrote: > On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 27/03/2024 06:57, yu-chang.lee wrote: >>> Add Smart Multimedia Interface Local Arbiter to mediatek >>> power domain. >>> >>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> >>> --- >>> .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 >> ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git >> a/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >> b/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>> index 8985e2df8a56..228c0dec5253 100644 >>> --- a/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>> +++ b/Documentation/devicetree/bindings/power/mediatek,power- >> controller.yaml >>> @@ -125,6 +125,10 @@ $defs: >>> $ref: /schemas/types.yaml#/definitions/phandle >>> description: phandle to the device containing the SMI >> register range. >>> >>> + mediatek,larb: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: phandle to the device containing the LARB >> register range. >> >> Why do you need it? >> >> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have >> third one similar. >> > MM driver used "mediatek,larbs" for it larb node. > Power domain driver used "mediatek,larb". > "mediatek,larb-id" is for larb in dts. > > The naming is no related to each other. Then it is just confusing. Best regards, Krzysztof
On Wed, 2024-03-27 at 10:59 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote: > > On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 27/03/2024 09:39, Krzysztof Kozlowski wrote: > >>> On 27/03/2024 06:57, yu-chang.lee wrote: > >>>> Add Smart Multimedia Interface Local Arbiter to mediatek > >>>> power domain. > >>>> > >>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> > >>>> --- > >>>> .../devicetree/bindings/power/mediatek,power-controller.yaml | > 4 > >> ++++ > >>>> 1 file changed, 4 insertions(+) > >>>> > >>>> diff --git > >> a/Documentation/devicetree/bindings/power/mediatek,power- > >> controller.yaml > >> b/Documentation/devicetree/bindings/power/mediatek,power- > >> controller.yaml > >>>> index 8985e2df8a56..228c0dec5253 100644 > >>>> --- a/Documentation/devicetree/bindings/power/mediatek,power- > >> controller.yaml > >>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power- > >> controller.yaml > >>>> @@ -125,6 +125,10 @@ $defs: > >>>> $ref: /schemas/types.yaml#/definitions/phandle > >>>> description: phandle to the device containing the SMI > >> register range. > >>>> > >>>> + mediatek,larb: > >>>> + $ref: /schemas/types.yaml#/definitions/phandle > >>>> + description: phandle to the device containing the LARB > >> register range. > >>> > >>> Why do you need it? > >>> > >>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we > >> have > >>> third one similar. > >> > >> ... and not even tested! > >> > >> Best regards, > >> Krzysztof > >> > > Hi, > > > > I will double check the format of yaml for the next version, sorry > for > > inconvenience. But I did test it on mt8188 chromebook, the reason > why > > How do you test a binding on chromebook? > > > power domain need larb node is that when mtcmos power on, signal > glitch > > may produce. Power domain driver must reset larb when this happen > to > > prevent dummy transaction on bus. That why I need larb node in dts. > > No one talks here about larb node... Sorry, May you elaborate on what information I need to provide to you or it is just a syntax problem I need to fix? Thanks Best Regards, Yu-chang > > Best regards, > Krzysztof >
On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: >>>> >>> Hi, >>> >>> I will double check the format of yaml for the next version, sorry >> for >>> inconvenience. But I did test it on mt8188 chromebook, the reason >> why >> >> How do you test a binding on chromebook? >> >>> power domain need larb node is that when mtcmos power on, signal >> glitch >>> may produce. Power domain driver must reset larb when this happen >> to >>> prevent dummy transaction on bus. That why I need larb node in dts. >> >> No one talks here about larb node... > > Sorry, May you elaborate on what information I need to provide to you > or it is just a syntax problem I need to fix? Please explain the purpose of this property (how is it going to be used by drivers) and what does it represent. Best regards, Krzysztof
On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: > >>>> > >>> Hi, > >>> > >>> I will double check the format of yaml for the next version, > sorry > >> for > >>> inconvenience. But I did test it on mt8188 chromebook, the reason > >> why > >> > >> How do you test a binding on chromebook? > >> > >>> power domain need larb node is that when mtcmos power on, signal > >> glitch > >>> may produce. Power domain driver must reset larb when this happen > >> to > >>> prevent dummy transaction on bus. That why I need larb node in > dts. > >> > >> No one talks here about larb node... > > > > Sorry, May you elaborate on what information I need to provide to > you > > or it is just a syntax problem I need to fix? > > Please explain the purpose of this property (how is it going to be > used by drivers)and what does it represent. > It represent SMI LARB(Local ARBitration). In power domain driver when power on power domain, It need to reset LARB to prevent potential power glitch which may cause dummy transaction on bus. Without taking care of this issue it often leads to camera hang in stress test. Best Regards, Yu-chang
On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote: > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: >>>>>> >>>>> Hi, >>>>> >>>>> I will double check the format of yaml for the next version, >> sorry >>>> for >>>>> inconvenience. But I did test it on mt8188 chromebook, the reason >>>> why >>>> >>>> How do you test a binding on chromebook? >>>> >>>>> power domain need larb node is that when mtcmos power on, signal >>>> glitch >>>>> may produce. Power domain driver must reset larb when this happen >>>> to >>>>> prevent dummy transaction on bus. That why I need larb node in >> dts. >>>> >>>> No one talks here about larb node... >>> >>> Sorry, May you elaborate on what information I need to provide to >> you >>> or it is just a syntax problem I need to fix? >> >> Please explain the purpose of this property (how is it going to be >> used by drivers)and what does it represent. >> > > It represent SMI LARB(Local ARBitration). In power domain driver when > power on power domain, It need to reset LARB to prevent potential power > glitch which may cause dummy transaction on bus. Without taking care of > this issue it often leads to camera hang in stress test. That sounds rather like missing resets... or something else connecting these devices. Maybe the explanation is just imprecise... Best regards, Krzysztof
Hello Yu-chang Lee, SMI LARB must have a power domain, according to "mediatek,smi-larb.yaml" Now you try to create a link from power domain to larb. Here is my understanding: when you enable/disable power domain, the larb linked to this power domain may have an issue. Then you want to retrieve de LARB linked to the power domain though the dts to manage the LARB. IMHO, using the dts to have this information into the power driver isn't necessary and may introduce some bugs if the LARB node and power node in the DTS aren't aligned. It seems not implemented today but during the LARB probe, it should "subscribe" to the linked power domain. Then, when the power domain status is changing, it is able to "notify" (callback) the LARB, then implement the good stuff to handle this power domain status change into LARB driver. Regards, Alexandre On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote: > > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments until > >> you have verified the sender or the content. > >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: > >>>>>> > >>>>> Hi, > >>>>> > >>>>> I will double check the format of yaml for the next version, > >> sorry > >>>> for > >>>>> inconvenience. But I did test it on mt8188 chromebook, the reason > >>>> why > >>>> > >>>> How do you test a binding on chromebook? > >>>> > >>>>> power domain need larb node is that when mtcmos power on, signal > >>>> glitch > >>>>> may produce. Power domain driver must reset larb when this happen > >>>> to > >>>>> prevent dummy transaction on bus. That why I need larb node in > >> dts. > >>>> > >>>> No one talks here about larb node... > >>> > >>> Sorry, May you elaborate on what information I need to provide to > >> you > >>> or it is just a syntax problem I need to fix? > >> > >> Please explain the purpose of this property (how is it going to be > >> used by drivers)and what does it represent. > >> > > > > It represent SMI LARB(Local ARBitration). In power domain driver when > > power on power domain, It need to reset LARB to prevent potential power > > glitch which may cause dummy transaction on bus. Without taking care of > > this issue it often leads to camera hang in stress test. > > That sounds rather like missing resets... or something else connecting > these devices. Maybe the explanation is just imprecise... > > Best regards, > Krzysztof > >
On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Hello Yu-chang Lee, > > SMI LARB must have a power domain, according to "mediatek,smi- > larb.yaml" > Now you try to create a link from power domain to larb. > > Here is my understanding: when you enable/disable power domain, the > larb linked to this power domain may have an issue. Then you want to > retrieve de LARB linked to the power domain though the dts to manage > the LARB. Yes, this is what I am trying to do. > IMHO, using the dts to have this information into the power > driver isn't necessary and may introduce some bugs if the LARB node > and power node in the DTS aren't aligned. > > It seems not implemented today but during the LARB probe, it should > "subscribe" to the linked power domain. Then, when the power domain > status is changing, it is able to "notify" (callback) the LARB, then > implement the good stuff to handle this power domain status change > into LARB driver. > The problem with this method and why "smi clamp" is in power domain driver is that our HW designer gave us a programming guide strictly states the sequence of what we need to do to power on/off power domain. Using callback, this sequence is no longer guaranteed and the side effect is unknown... So I would like to stick with adding larbs just like smi into power domain node. > Regards, > Alexandre Best Regards, Yu-chang > > On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote: > > > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote: > > >> > > >> External email : Please do not click links or open attachments > until > > >> you have verified the sender or the content. > > >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: > > >>>>>> > > >>>>> Hi, > > >>>>> > > >>>>> I will double check the format of yaml for the next version, > > >> sorry > > >>>> for > > >>>>> inconvenience. But I did test it on mt8188 chromebook, the > reason > > >>>> why > > >>>> > > >>>> How do you test a binding on chromebook? > > >>>> > > >>>>> power domain need larb node is that when mtcmos power on, > signal > > >>>> glitch > > >>>>> may produce. Power domain driver must reset larb when this > happen > > >>>> to > > >>>>> prevent dummy transaction on bus. That why I need larb node > in > > >> dts. > > >>>> > > >>>> No one talks here about larb node... > > >>> > > >>> Sorry, May you elaborate on what information I need to provide > to > > >> you > > >>> or it is just a syntax problem I need to fix? > > >> > > >> Please explain the purpose of this property (how is it going to > be > > >> used by drivers)and what does it represent. > > >> > > > > > > It represent SMI LARB(Local ARBitration). In power domain driver > when > > > power on power domain, It need to reset LARB to prevent potential > power > > > glitch which may cause dummy transaction on bus. Without taking > care of > > > this issue it often leads to camera hang in stress test. > > > > That sounds rather like missing resets... or something else > connecting > > these devices. Maybe the explanation is just imprecise... > > > > Best regards, > > Krzysztof > > > >
On Wed, 2024-03-27 at 12:04 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote: > > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote: > >>>>>> > >>>>> Hi, > >>>>> > >>>>> I will double check the format of yaml for the next version, > >> sorry > >>>> for > >>>>> inconvenience. But I did test it on mt8188 chromebook, the > reason > >>>> why > >>>> > >>>> How do you test a binding on chromebook? > >>>> > >>>>> power domain need larb node is that when mtcmos power on, > signal > >>>> glitch > >>>>> may produce. Power domain driver must reset larb when this > happen > >>>> to > >>>>> prevent dummy transaction on bus. That why I need larb node in > >> dts. > >>>> > >>>> No one talks here about larb node... > >>> > >>> Sorry, May you elaborate on what information I need to provide to > >> you > >>> or it is just a syntax problem I need to fix? > >> > >> Please explain the purpose of this property (how is it going to be > >> used by drivers)and what does it represent. > >> > > > > It represent SMI LARB(Local ARBitration). In power domain driver > when > > power on power domain, It need to reset LARB to prevent potential > power > > glitch which may cause dummy transaction on bus. Without taking > care of > > this issue it often leads to camera hang in stress test. > > That sounds rather like missing resets... or something else > connecting > these devices. Maybe the explanation is just imprecise... > Maybe the term "reset" is misleading here. What power domain driver trying to do is "set and then clr" the smi larb to clear potential glitch signal that is already in there. And this step is strongly related to power domain onf that why I want to add it in to power domain node without depending larb driver to do the work to prevent this setting missing. > Best regards, > Krzysztof Best Regards, yu-chang >
On 28/03/2024 07:06, Yu-chang Lee (李禹璋) wrote: > On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> Hello Yu-chang Lee, >> >> SMI LARB must have a power domain, according to "mediatek,smi- >> larb.yaml" >> Now you try to create a link from power domain to larb. >> >> Here is my understanding: when you enable/disable power domain, the >> larb linked to this power domain may have an issue. Then you want to >> retrieve de LARB linked to the power domain though the dts to manage >> the LARB. > > Yes, this is what I am trying to do. > >> IMHO, using the dts to have this information into the power >> driver isn't necessary and may introduce some bugs if the LARB node >> and power node in the DTS aren't aligned. >> >> It seems not implemented today but during the LARB probe, it should >> "subscribe" to the linked power domain. Then, when the power domain >> status is changing, it is able to "notify" (callback) the LARB, then >> implement the good stuff to handle this power domain status change >> into LARB driver. >> > > The problem with this method and why "smi clamp" is in power domain > driver is that our HW designer gave us a programming guide strictly > states the sequence of what we need to do to power on/off power domain. > Using callback, this sequence is no longer guaranteed and the side > effect is unknown... > > So I would like to stick with adding larbs just like smi into powe You want your power domain driver to poke, asynchronously, without locking into registers of another device. And how does this not cause issues? No, work with your hardware engineers or Linux engineers on sane behavior. Best regards, Krzysztof
Hi yu-chang.lee, kernel test robot noticed the following build warnings: [auto build test WARNING on robh/for-next] [also build test WARNING on krzk-dt/for-next linus/master v6.9-rc1 next-20240328] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/yu-chang-lee/pmdomain-mediatek-add-smi_larb_reset-function-when-power-on/20240327-140007 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20240327055732.28198-3-yu-chang.lee%40mediatek.com patch subject: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20240331/202403312222.fjYPC06h-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202403312222.fjYPC06h-lkp@intel.com/ dtcheck warnings: (new ones prefixed by >>) >> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax) >> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation) -- >> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key >> Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml: while parsing a block mapping in "<unicode string>", line 64, column 5 did not find expected key in "<unicode string>", line 128, column 6 -- >> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file vim +128 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml 8 9 maintainers: 10 - MandyJH Liu <mandyjh.liu@mediatek.com> 11 - Matthias Brugger <mbrugger@suse.com> 12 13 description: | 14 Mediatek processors include support for multiple power domains which can be 15 powered up/down by software based on different application scenes to save power. 16 17 IP cores belonging to a power domain should contain a 'power-domains' 18 property that is a phandle for SCPSYS node representing the domain. 19 20 properties: 21 $nodename: 22 pattern: '^power-controller(@[0-9a-f]+)?$' 23 24 compatible: 25 enum: 26 - mediatek,mt6795-power-controller 27 - mediatek,mt8167-power-controller 28 - mediatek,mt8173-power-controller 29 - mediatek,mt8183-power-controller 30 - mediatek,mt8186-power-controller 31 - mediatek,mt8188-power-controller 32 - mediatek,mt8192-power-controller 33 - mediatek,mt8195-power-controller 34 - mediatek,mt8365-power-controller 35 36 '#power-domain-cells': 37 const: 1 38 39 '#address-cells': 40 const: 1 41 42 '#size-cells': 43 const: 0 44 45 patternProperties: 46 "^power-domain@[0-9a-f]+$": 47 $ref: "#/$defs/power-domain-node" 48 patternProperties: 49 "^power-domain@[0-9a-f]+$": 50 $ref: "#/$defs/power-domain-node" 51 patternProperties: 52 "^power-domain@[0-9a-f]+$": 53 $ref: "#/$defs/power-domain-node" 54 patternProperties: 55 "^power-domain@[0-9a-f]+$": 56 $ref: "#/$defs/power-domain-node" 57 unevaluatedProperties: false 58 unevaluatedProperties: false 59 unevaluatedProperties: false 60 unevaluatedProperties: false 61 62 $defs: 63 power-domain-node: 64 type: object 65 description: | 66 Represents the power domains within the power controller node as documented 67 in Documentation/devicetree/bindings/power/power-domain.yaml. 68 69 properties: 70 71 '#power-domain-cells': 72 description: 73 Must be 0 for nodes representing a single PM domain and 1 for nodes 74 providing multiple PM domains. 75 76 '#address-cells': 77 const: 1 78 79 '#size-cells': 80 const: 0 81 82 reg: 83 description: | 84 Power domain index. Valid values are defined in: 85 "include/dt-bindings/power/mt6795-power.h" - for MT8167 type power domain. 86 "include/dt-bindings/power/mt8167-power.h" - for MT8167 type power domain. 87 "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain. 88 "include/dt-bindings/power/mt8183-power.h" - for MT8183 type power domain. 89 "include/dt-bindings/power/mediatek,mt8188-power.h" - for MT8188 type power domain. 90 "include/dt-bindings/power/mt8192-power.h" - for MT8192 type power domain. 91 "include/dt-bindings/power/mt8195-power.h" - for MT8195 type power domain. 92 "include/dt-bindings/power/mediatek,mt8365-power.h" - for MT8365 type power domain. 93 maxItems: 1 94 95 clocks: 96 description: | 97 A number of phandles to clocks that need to be enabled during domain 98 power-up sequencing. 99 100 clock-names: 101 description: | 102 List of names of clocks, in order to match the power-up sequencing 103 for each power domain we need to group the clocks by name. BASIC 104 clocks need to be enabled before enabling the corresponding power 105 domain, and should not have a '-' in their name (i.e mm, mfg, venc). 106 SUSBYS clocks need to be enabled before releasing the bus protection, 107 and should contain a '-' in their name (i.e mm-0, isp-0, cam-0). 108 109 In order to follow properly the power-up sequencing, the clocks must 110 be specified by order, adding first the BASIC clocks followed by the 111 SUSBSYS clocks. 112 113 domain-supply: 114 description: domain regulator supply. 115 116 mediatek,infracfg: 117 $ref: /schemas/types.yaml#/definitions/phandle 118 description: phandle to the device containing the INFRACFG register range. 119 120 mediatek,infracfg-nao: 121 $ref: /schemas/types.yaml#/definitions/phandle 122 description: phandle to the device containing the INFRACFG-NAO register range. 123 124 mediatek,smi: 125 $ref: /schemas/types.yaml#/definitions/phandle 126 description: phandle to the device containing the SMI register range. 127 > 128 mediatek,larb: > 129 $ref: /schemas/types.yaml#/definitions/phandle 130 description: phandle to the device containing the LARB register range. 131 132 required: 133 - reg 134 135 required: 136 - compatible 137
On Thu, 28 Mar 2024 at 07:06, Yu-chang Lee (李禹璋) <Yu-chang.Lee@mediatek.com> wrote: > > On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Hello Yu-chang Lee, > > > > SMI LARB must have a power domain, according to "mediatek,smi- > > larb.yaml" > > Now you try to create a link from power domain to larb. > > > > Here is my understanding: when you enable/disable power domain, the > > larb linked to this power domain may have an issue. Then you want to > > retrieve de LARB linked to the power domain though the dts to manage > > the LARB. > > Yes, this is what I am trying to do. > > > IMHO, using the dts to have this information into the power > > driver isn't necessary and may introduce some bugs if the LARB node > > and power node in the DTS aren't aligned. > > > > It seems not implemented today but during the LARB probe, it should > > "subscribe" to the linked power domain. Then, when the power domain > > status is changing, it is able to "notify" (callback) the LARB, then > > implement the good stuff to handle this power domain status change > > into LARB driver. > > > > The problem with this method and why "smi clamp" is in power domain > driver is that our HW designer gave us a programming guide strictly > states the sequence of what we need to do to power on/off power domain. > Using callback, this sequence is no longer guaranteed and the side > effect is unknown... In most cases, using the runtime PM callbacks in the consumer driver (LARB driver) is sufficient to deal with resets. For some special cases drivers are making use of the genpd on/off notifiers (GENPD_NOTIFY_*), as they really need to know when their devices have been power collapsed. Have you tried both these options? [...] Kind regards Uffe
diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml index 8985e2df8a56..228c0dec5253 100644 --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml @@ -125,6 +125,10 @@ $defs: $ref: /schemas/types.yaml#/definitions/phandle description: phandle to the device containing the SMI register range. + mediatek,larb: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to the device containing the LARB register range. + required: - reg
Add Smart Multimedia Interface Local Arbiter to mediatek power domain. Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com> --- .../devicetree/bindings/power/mediatek,power-controller.yaml | 4 ++++ 1 file changed, 4 insertions(+)