| Message ID | 20240610084032.3096614-4-adeep@lexina.in (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: amlogic: add new meson-gx-socinfo-sm driver | expand |
On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: > Add secure-monitor property to schema for meson-gx-socinfo-sm driver. "bindings are for hardware, not drivers". Why purpose does the "secure monitor" serve that the secure firmware needs a reference to it? Thanks, Conor. > > Signed-off-by: Viacheslav Bocharov <adeep@lexina.in> > --- > .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > index 7dff32f373cb..1128a794ec89 100644 > --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > @@ -32,6 +32,10 @@ properties: > reg: > maxItems: 1 > > + secure-monitor: Missing a vendor prefix. > + description: phandle to the secure-monitor node > + $ref: /schemas/types.yaml#/definitions/phandle > + > amlogic,has-chip-id: > description: | > A firmware register encodes the SoC type, package and revision > -- > 2.45.2 >
Hi! 10/06/2024 19.08, Conor Dooley wrote: > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: >> Add secure-monitor property to schema for meson-gx-socinfo-sm driver. > > "bindings are for hardware, not drivers". Why purpose does the "secure > monitor" serve that the secure firmware needs a reference to it? This driver is an extension to the meson-gx-socinfo driver: it supplements information obtained from the register with information from the SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module loading order, we cannot do away with meson-gx-socinfo, as it is used for platform identification in some drivers. Therefore, the extended information is formatted as a separate driver, which is loaded after the secure-monitor driver. The ability to obtain additional information depends on the support for the call in the secure-monitor, which can be described by an additional link from the amlogic,meson-gx-ao-secure node to the secure-monitor node, similar to how it is done for amlogic,meson-gxbb-efuse. > > Thanks, > Conor. > >> >> Signed-off-by: Viacheslav Bocharov <adeep@lexina.in> >> --- >> .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml >> index 7dff32f373cb..1128a794ec89 100644 >> --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml >> +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml >> @@ -32,6 +32,10 @@ properties: >> reg: >> maxItems: 1 >> >> + secure-monitor: > > Missing a vendor prefix. > >> + description: phandle to the secure-monitor node >> + $ref: /schemas/types.yaml#/definitions/phandle >> + >> amlogic,has-chip-id: >> description: | >> A firmware register encodes the SoC type, package and revision >> -- >> 2.45.2 >> >> >> _______________________________________________ >> linux-amlogic mailing list >> linux-amlogic@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic -- with regards, Viacheslav
On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: > Hi! > > 10/06/2024 19.08, Conor Dooley wrote: > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver. > > > > "bindings are for hardware, not drivers". Why purpose does the "secure > > monitor" serve that the secure firmware needs a reference to it? > > This driver is an extension to the meson-gx-socinfo driver: it supplements > information obtained from the register with information from the > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module > loading order, we cannot do away with meson-gx-socinfo, as it is used for > platform identification in some drivers. Therefore, the extended information > is formatted as a separate driver, which is loaded after the secure-monitor > driver. Please stop talking about drivers, this is a binding which is about hardware. Please provide, in your next version, a commit message that justifies adding this property without talking about driver probing order etc, and instead focuses on what service the "secure monitor" provides etc. > The ability to obtain additional information depends on the support for the > call in the secure-monitor, which can be described by an additional link > from the amlogic,meson-gx-ao-secure node to the secure-monitor node, similar > to how it is done for amlogic,meson-gxbb-efuse. > > > > > Thanks, > > Conor. > > > > > > > > Signed-off-by: Viacheslav Bocharov <adeep@lexina.in> > > > --- > > > .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > > > index 7dff32f373cb..1128a794ec89 100644 > > > --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > > > +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml > > > @@ -32,6 +32,10 @@ properties: > > > reg: > > > maxItems: 1 > > > + secure-monitor: > > > > Missing a vendor prefix. > > > > > + description: phandle to the secure-monitor node > > > + $ref: /schemas/types.yaml#/definitions/phandle > > > + > > > amlogic,has-chip-id: > > > description: | > > > A firmware register encodes the SoC type, package and revision > > > -- > > > 2.45.2 > > > > > > > > > _______________________________________________ > > > linux-amlogic mailing list > > > linux-amlogic@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-amlogic > > -- > with regards, > Viacheslav
On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: > > Hi! > > > > 10/06/2024 19.08, Conor Dooley wrote: > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver. > > > > > > "bindings are for hardware, not drivers". Why purpose does the "secure > > > monitor" serve that the secure firmware needs a reference to it? > > > > This driver is an extension to the meson-gx-socinfo driver: it supplements > > information obtained from the register with information from the > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module > > loading order, we cannot do away with meson-gx-socinfo, as it is used for > > platform identification in some drivers. Therefore, the extended information > > is formatted as a separate driver, which is loaded after the secure-monitor > > driver. > > Please stop talking about drivers, this is a binding which is about > hardware. Please provide, in your next version, a commit message that > justifies adding this property without talking about driver probing > order etc, and instead focuses on what service the "secure monitor" > provides etc. To put it another way, how many secure monitors does 1 system have? What do you do if the property is not present? You didn't make it required which is good because that would be an ABI break. You only need a link in DT if there are different possible providers or some per consumer information to describe (e.g. an interrupt number or clock ID). You don't have the latter and likely there is only 1 possible provider. Rob
Thanks for review. 13/06/2024 19.42, Rob Herring wrote: > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: >> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: >>> Hi! >>> >>> 10/06/2024 19.08, Conor Dooley wrote: >>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: >>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver. >>>> >>>> "bindings are for hardware, not drivers". Why purpose does the "secure >>>> monitor" serve that the secure firmware needs a reference to it? >>> >>> This driver is an extension to the meson-gx-socinfo driver: it supplements >>> information obtained from the register with information from the >>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module >>> loading order, we cannot do away with meson-gx-socinfo, as it is used for >>> platform identification in some drivers. Therefore, the extended information >>> is formatted as a separate driver, which is loaded after the secure-monitor >>> driver. >> >> Please stop talking about drivers, this is a binding which is about >> hardware. Please provide, in your next version, a commit message that >> justifies adding this property without talking about driver probing >> order etc, and instead focuses on what service the "secure monitor" >> provides etc. > > To put it another way, how many secure monitors does 1 system have? One per system in current device tree. > > What do you do if the property is not present? You didn't make it > required which is good because that would be an ABI break. We need an indication of the ability to use the secure-monitor to obtain additional information within the soc driver. It seemed to me that using an explicit reference to the secure-monitor is the best choice. > > You only need a link in DT if there are different possible providers or > some per consumer information to describe (e.g. an interrupt number or > clock ID). You don't have the latter and likely there is only 1 possible > provider. Would replacing the reference to sm with an option, for example, use-secure-monitor = <1>; look more appropriate in this case? > > Rob > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote: > Thanks for review. > > 13/06/2024 19.42, Rob Herring wrote: > > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: > > > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: > > > > Hi! > > > > > > > > 10/06/2024 19.08, Conor Dooley wrote: > > > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: > > > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver. > > > > > > > > > > "bindings are for hardware, not drivers". Why purpose does the "secure > > > > > monitor" serve that the secure firmware needs a reference to it? > > > > > > > > This driver is an extension to the meson-gx-socinfo driver: it supplements > > > > information obtained from the register with information from the > > > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module > > > > loading order, we cannot do away with meson-gx-socinfo, as it is used for > > > > platform identification in some drivers. Therefore, the extended information > > > > is formatted as a separate driver, which is loaded after the secure-monitor > > > > driver. > > > > > > Please stop talking about drivers, this is a binding which is about > > > hardware. Please provide, in your next version, a commit message that > > > justifies adding this property without talking about driver probing > > > order etc, and instead focuses on what service the "secure monitor" > > > provides etc. > > > > To put it another way, how many secure monitors does 1 system have? > > One per system in current device tree. One per system, or one is currently described per system, but more might be added later? > > What do you do if the property is not present? You didn't make it > > required which is good because that would be an ABI break. > > We need an indication of the ability to use the secure-monitor to obtain > additional information within the soc driver. It seemed to me that using an > explicit reference to the secure-monitor is the best choice. > > > > > You only need a link in DT if there are different possible providers or > > some per consumer information to describe (e.g. an interrupt number or > > clock ID). You don't have the latter and likely there is only 1 possible > > provider. > > Would replacing the reference to sm with an option, for example, > use-secure-monitor = <1>; look more appropriate in this case? Perhaps a silly question, but (provided there's only one per system, why can't the secure-monitor driver expose a function that you can call to get a reference to the system-monitor? I did something similar before with a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted, mpfs-rng is probed from software so it's slightly different to your case, but the principle is the same and it's not unheard of for code in drivers/soc to expose interfaces to other drivers like this. You can just call a function like that, and know whether there's a secure monitor, without having to retrofit a DT property. Thanks, Conor.
17/06/2024 19.57, Conor Dooley пишет: > On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote: >> Thanks for review. >> >> 13/06/2024 19.42, Rob Herring wrote: >>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: >>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: >>>>> Hi! >>>>> >>>>> 10/06/2024 19.08, Conor Dooley wrote: >>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: >>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver. >>>>>> >>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure >>>>>> monitor" serve that the secure firmware needs a reference to it? >>>>> >>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements >>>>> information obtained from the register with information from the >>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module >>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for >>>>> platform identification in some drivers. Therefore, the extended information >>>>> is formatted as a separate driver, which is loaded after the secure-monitor >>>>> driver. >>>> >>>> Please stop talking about drivers, this is a binding which is about >>>> hardware. Please provide, in your next version, a commit message that >>>> justifies adding this property without talking about driver probing >>>> order etc, and instead focuses on what service the "secure monitor" >>>> provides etc. >>> >>> To put it another way, how many secure monitors does 1 system have? >> >> One per system in current device tree. > > One per system, or one is currently described per system, but more might > be added later? it turns out to be one per system. It's either there or it's not. > >>> What do you do if the property is not present? You didn't make it >>> required which is good because that would be an ABI break. >> >> We need an indication of the ability to use the secure-monitor to obtain >> additional information within the soc driver. It seemed to me that using an >> explicit reference to the secure-monitor is the best choice. >> >>> >>> You only need a link in DT if there are different possible providers or >>> some per consumer information to describe (e.g. an interrupt number or >>> clock ID). You don't have the latter and likely there is only 1 possible >>> provider. >> >> Would replacing the reference to sm with an option, for example, >> use-secure-monitor = <1>; look more appropriate in this case? > > Perhaps a silly question, but (provided there's only one per system, why > can't the secure-monitor driver expose a function that you can call to get > a reference to the system-monitor? I did something similar before with > a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted, > mpfs-rng is probed from software so it's slightly different to your > case, but the principle is the same and it's not unheard of for code in > drivers/soc to expose interfaces to other drivers like this. You can > just call a function like that, and know whether there's a secure > monitor, without having to retrofit a DT property. That could be an option. But again, nothing prevents me from searching for the secure-monitor node throughout the entire DT array. The question is more about something else, let me try to explain from the beginning: We currently have a soc driver that uses only the register to get basic information and it must be loaded early because other modules' behavior depends on its information. There is an option to supplement the register information with information from the secure-monitor. For this, we had to write a new driver that uses the same register information as a fallback but can wait for the secure-monitor driver to load and add its information to soc. It seemed logical to me to keep the DT structure the same and just add a reference to the secure-monitor (or as a second option, create a variable indicating support) for those SoCs that have been tested and can provide this information. Not all Amlogic SoCs support this call, in some (mostly newer generations of SoCs), this call returns incorrect information and we and colleagues are still figuring out what has changed. But most established platforms support this. We could add this information retrieval to the secure-monitor itself, but that would be a completely different story and would not constitute a soc driver. In the end, we need information about the support of the secure-monitor call for obtaining information for the soc driver. In my opinion, this can only be done by specifying it in the DT in specific files for Amlogic platforms: either by referencing the SM or by an option that allows checking the SM. > > Thanks, > Conor. > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
On 20/06/2024 09:14, Viacheslav wrote: > > > 17/06/2024 19.57, Conor Dooley пишет: >> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote: >>> Thanks for review. >>> >>> 13/06/2024 19.42, Rob Herring wrote: >>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: >>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: >>>>>> Hi! >>>>>> >>>>>> 10/06/2024 19.08, Conor Dooley wrote: >>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: >>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver. >>>>>>> >>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure >>>>>>> monitor" serve that the secure firmware needs a reference to it? >>>>>> >>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements >>>>>> information obtained from the register with information from the >>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module >>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for >>>>>> platform identification in some drivers. Therefore, the extended information >>>>>> is formatted as a separate driver, which is loaded after the secure-monitor >>>>>> driver. >>>>> >>>>> Please stop talking about drivers, this is a binding which is about >>>>> hardware. Please provide, in your next version, a commit message that >>>>> justifies adding this property without talking about driver probing >>>>> order etc, and instead focuses on what service the "secure monitor" >>>>> provides etc. >>>> >>>> To put it another way, how many secure monitors does 1 system have? >>> >>> One per system in current device tree. >> >> One per system, or one is currently described per system, but more might >> be added later? > > it turns out to be one per system. It's either there or it's not. > >> >>>> What do you do if the property is not present? You didn't make it >>>> required which is good because that would be an ABI break. >>> >>> We need an indication of the ability to use the secure-monitor to obtain >>> additional information within the soc driver. It seemed to me that using an >>> explicit reference to the secure-monitor is the best choice. >>> >>>> >>>> You only need a link in DT if there are different possible providers or >>>> some per consumer information to describe (e.g. an interrupt number or >>>> clock ID). You don't have the latter and likely there is only 1 possible >>>> provider. >>> >>> Would replacing the reference to sm with an option, for example, >>> use-secure-monitor = <1>; look more appropriate in this case? >> >> Perhaps a silly question, but (provided there's only one per system, why >> can't the secure-monitor driver expose a function that you can call to get >> a reference to the system-monitor? I did something similar before with >> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted, >> mpfs-rng is probed from software so it's slightly different to your >> case, but the principle is the same and it's not unheard of for code in >> drivers/soc to expose interfaces to other drivers like this. You can >> just call a function like that, and know whether there's a secure >> monitor, without having to retrofit a DT property. > > That could be an option. But again, nothing prevents me from searching > for the secure-monitor node throughout the entire DT array. > > The question is more about something else, let me try to explain from > the beginning: > > We currently have a soc driver that uses only the register to get basic > information and it must be loaded early because other modules' behavior > depends on its information. Please provide name/link to the upstream source code (downstream does not matter). > There is an option to supplement the register information with > information from the secure-monitor. > For this, we had to write a new driver that uses the same register > information as a fallback but can wait for the secure-monitor driver to > load and add its information to soc. > It seemed logical to me to keep the DT structure the same and just add a > reference to the secure-monitor (or as a second option, create a > variable indicating support) for those SoCs that have been tested and > can provide this information. > Not all Amlogic SoCs support this call, in some (mostly newer > generations of SoCs), this call returns incorrect information and we and > colleagues are still figuring out what has changed. But most established > platforms support this. > We could add this information retrieval to the secure-monitor itself, > but that would be a completely different story and would not constitute > a soc driver. > > In the end, we need information about the support of the secure-monitor > call for obtaining information for the soc driver. In my opinion, this > can only be done by specifying it in the DT in specific files for > Amlogic platforms: either by referencing the SM or by an option that > allows checking the SM. That's not the only option. This is SoC specific so can be deduced from the compatible as well. And this is kind of obvious from this patchset (actually patch 4): you add it per SoC. Best regards, Krzysztof
On 20/06/2024 09:19, Krzysztof Kozlowski wrote: > On 20/06/2024 09:14, Viacheslav wrote: >> >> >> 17/06/2024 19.57, Conor Dooley пишет: >>> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote: >>>> Thanks for review. >>>> >>>> 13/06/2024 19.42, Rob Herring wrote: >>>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: >>>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: >>>>>>> Hi! >>>>>>> >>>>>>> 10/06/2024 19.08, Conor Dooley wrote: >>>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: >>>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver. >>>>>>>> >>>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure >>>>>>>> monitor" serve that the secure firmware needs a reference to it? >>>>>>> >>>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements >>>>>>> information obtained from the register with information from the >>>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module >>>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for >>>>>>> platform identification in some drivers. Therefore, the extended information >>>>>>> is formatted as a separate driver, which is loaded after the secure-monitor >>>>>>> driver. >>>>>> >>>>>> Please stop talking about drivers, this is a binding which is about >>>>>> hardware. Please provide, in your next version, a commit message that >>>>>> justifies adding this property without talking about driver probing >>>>>> order etc, and instead focuses on what service the "secure monitor" >>>>>> provides etc. >>>>> >>>>> To put it another way, how many secure monitors does 1 system have? >>>> >>>> One per system in current device tree. >>> >>> One per system, or one is currently described per system, but more might >>> be added later? >> >> it turns out to be one per system. It's either there or it's not. >> >>> >>>>> What do you do if the property is not present? You didn't make it >>>>> required which is good because that would be an ABI break. >>>> >>>> We need an indication of the ability to use the secure-monitor to obtain >>>> additional information within the soc driver. It seemed to me that using an >>>> explicit reference to the secure-monitor is the best choice. >>>> >>>>> >>>>> You only need a link in DT if there are different possible providers or >>>>> some per consumer information to describe (e.g. an interrupt number or >>>>> clock ID). You don't have the latter and likely there is only 1 possible >>>>> provider. >>>> >>>> Would replacing the reference to sm with an option, for example, >>>> use-secure-monitor = <1>; look more appropriate in this case? >>> >>> Perhaps a silly question, but (provided there's only one per system, why >>> can't the secure-monitor driver expose a function that you can call to get >>> a reference to the system-monitor? I did something similar before with >>> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted, >>> mpfs-rng is probed from software so it's slightly different to your >>> case, but the principle is the same and it's not unheard of for code in >>> drivers/soc to expose interfaces to other drivers like this. You can >>> just call a function like that, and know whether there's a secure >>> monitor, without having to retrofit a DT property. >> >> That could be an option. But again, nothing prevents me from searching >> for the secure-monitor node throughout the entire DT array. >> >> The question is more about something else, let me try to explain from >> the beginning: >> >> We currently have a soc driver that uses only the register to get basic >> information and it must be loaded early because other modules' behavior >> depends on its information. > > Please provide name/link to the upstream source code (downstream does > not matter). > >> There is an option to supplement the register information with >> information from the secure-monitor. >> For this, we had to write a new driver that uses the same register >> information as a fallback but can wait for the secure-monitor driver to >> load and add its information to soc. >> It seemed logical to me to keep the DT structure the same and just add a >> reference to the secure-monitor (or as a second option, create a >> variable indicating support) for those SoCs that have been tested and >> can provide this information. >> Not all Amlogic SoCs support this call, in some (mostly newer >> generations of SoCs), this call returns incorrect information and we and >> colleagues are still figuring out what has changed. But most established >> platforms support this. >> We could add this information retrieval to the secure-monitor itself, >> but that would be a completely different story and would not constitute >> a soc driver. >> >> In the end, we need information about the support of the secure-monitor >> call for obtaining information for the soc driver. In my opinion, this >> can only be done by specifying it in the DT in specific files for >> Amlogic platforms: either by referencing the SM or by an option that >> allows checking the SM. > > That's not the only option. This is SoC specific so can be deduced from > the compatible as well. And this is kind of obvious from this patchset > (actually patch 4): you add it per SoC. BTW, that's one more DT maintainer (so the third) telling you property is not needed yet. I think we used enough of our time here. Best regards, Krzysztof
On Mon, Jun 17, 2024 at 05:57:53PM +0100, Conor Dooley wrote: > On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote: > > Thanks for review. > > > > 13/06/2024 19.42, Rob Herring wrote: > > > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote: > > > > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote: > > > > > Hi! > > > > > > > > > > 10/06/2024 19.08, Conor Dooley wrote: > > > > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote: > > > > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver. > > > > > > > > > > > > "bindings are for hardware, not drivers". Why purpose does the "secure > > > > > > monitor" serve that the secure firmware needs a reference to it? > > > > > > > > > > This driver is an extension to the meson-gx-socinfo driver: it supplements > > > > > information obtained from the register with information from the > > > > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module > > > > > loading order, we cannot do away with meson-gx-socinfo, as it is used for > > > > > platform identification in some drivers. Therefore, the extended information > > > > > is formatted as a separate driver, which is loaded after the secure-monitor > > > > > driver. > > > > > > > > Please stop talking about drivers, this is a binding which is about > > > > hardware. Please provide, in your next version, a commit message that > > > > justifies adding this property without talking about driver probing > > > > order etc, and instead focuses on what service the "secure monitor" > > > > provides etc. > > > > > > To put it another way, how many secure monitors does 1 system have? > > > > One per system in current device tree. > > One per system, or one is currently described per system, but more might > be added later? > > > > What do you do if the property is not present? You didn't make it > > > required which is good because that would be an ABI break. > > > > We need an indication of the ability to use the secure-monitor to obtain > > additional information within the soc driver. It seemed to me that using an > > explicit reference to the secure-monitor is the best choice. > > > > > > > > You only need a link in DT if there are different possible providers or > > > some per consumer information to describe (e.g. an interrupt number or > > > clock ID). You don't have the latter and likely there is only 1 possible > > > provider. > > > > Would replacing the reference to sm with an option, for example, > > use-secure-monitor = <1>; look more appropriate in this case? > > Perhaps a silly question, but (provided there's only one per system, why > can't the secure-monitor driver expose a function that you can call to get > a reference to the system-monitor? I did something similar before with > a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted, > mpfs-rng is probed from software so it's slightly different to your > case, but the principle is the same and it's not unheard of for code in > drivers/soc to expose interfaces to other drivers like this. You can > just call a function like that, and know whether there's a secure > monitor, without having to retrofit a DT property. Another thing, without having a driver expose an API, is calling of_find_compatible_node() to find the node. That also doesn't require retrofitting properties.
diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml index 7dff32f373cb..1128a794ec89 100644 --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml @@ -32,6 +32,10 @@ properties: reg: maxItems: 1 + secure-monitor: + description: phandle to the secure-monitor node + $ref: /schemas/types.yaml#/definitions/phandle + amlogic,has-chip-id: description: | A firmware register encodes the SoC type, package and revision
Add secure-monitor property to schema for meson-gx-socinfo-sm driver. Signed-off-by: Viacheslav Bocharov <adeep@lexina.in> --- .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml | 4 ++++ 1 file changed, 4 insertions(+)