[v3,04/10] nvmem: qfprom: Add support for secure reading on QDU1000/QRU1000

Add qfprom driver support for QDU1000/QRU1000 SOCs.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 drivers/nvmem/qfprom.c | 5 +++++
 1 file changed, 5 insertions(+)

--
2.17.1

On 12/05/2023 14:21, Komal Bajaj wrote:
> Add qfprom driver support for QDU1000/QRU1000 SOCs.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/nvmem/qfprom.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 20662e2d3732..12a7981a8a71 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c
> @@ -109,6 +109,10 @@ struct qfprom_soc_compatible_data {
>  	bool secure;
>  };
> 
> +static const struct qfprom_soc_compatible_data qdu1000_qfprom = {
> +	.secure = true
> +};
> +
>  static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
>  	{.start = 0x128, .end = 0x148},
>  	{.start = 0x220, .end = 0x228}
> @@ -490,6 +494,7 @@ static int qfprom_probe(struct platform_device *pdev)
> 
>  static const struct of_device_id qfprom_of_match[] = {
>  	{ .compatible = "qcom,qfprom",},
> +	{ .compatible = "qcom,qdu1000-qfprom", .data = &qdu1000_qfprom},
>  	{ .compatible = "qcom,sc7180-qfprom", .data = &sc7180_qfprom},

I have doubts that this is still compatible with qcom,qfprom. It uses
entirely different read method. That's why generic fallbacks are bad,
one more case to my growing list of awesome examples. :)

Best regards,
Krzysztof

On 5/12/2023 11:01 PM, Dmitry Baryshkov wrote:
> On Fri, 12 May 2023 at 20:01, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 12/05/2023 14:21, Komal Bajaj wrote:
>>> Add qfprom driver support for QDU1000/QRU1000 SOCs.
>>>
>>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>>> ---
>>>   drivers/nvmem/qfprom.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
>>> index 20662e2d3732..12a7981a8a71 100644
>>> --- a/drivers/nvmem/qfprom.c
>>> +++ b/drivers/nvmem/qfprom.c
>>> @@ -109,6 +109,10 @@ struct qfprom_soc_compatible_data {
>>>        bool secure;
>>>   };
>>>
>>> +static const struct qfprom_soc_compatible_data qdu1000_qfprom = {
>>> +     .secure = true
>>> +};
>>> +
>>>   static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
>>>        {.start = 0x128, .end = 0x148},
>>>        {.start = 0x220, .end = 0x228}
>>> @@ -490,6 +494,7 @@ static int qfprom_probe(struct platform_device *pdev)
>>>
>>>   static const struct of_device_id qfprom_of_match[] = {
>>>        { .compatible = "qcom,qfprom",},
>>> +     { .compatible = "qcom,qdu1000-qfprom", .data = &qdu1000_qfprom},
>>>        { .compatible = "qcom,sc7180-qfprom", .data = &sc7180_qfprom},
>> I have doubts that this is still compatible with qcom,qfprom. It uses
>> entirely different read method. That's why generic fallbacks are bad,
>> one more case to my growing list of awesome examples. :)
Okay, will do that.
> Yes, it looks like it should be 'qcom,qdu1000-qfprom",
> "qcom,scm-qfprom". And possibly a separate driver for scm-qfprom.
The only difference here is in read method, which can be controlled by a 
single property,
do we really need to write a separate driver for just reading secure 
feature register.

Thanks,
Komal
>
>

On Mon, May 15, 2023 at 02:02:11PM +0530, Komal Bajaj wrote:
> 
> 
> On 5/12/2023 11:01 PM, Dmitry Baryshkov wrote:
> > On Fri, 12 May 2023 at 20:01, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > > On 12/05/2023 14:21, Komal Bajaj wrote:
> > > > Add qfprom driver support for QDU1000/QRU1000 SOCs.
> > > > 
> > > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> > > > ---
> > > >   drivers/nvmem/qfprom.c | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> > > > index 20662e2d3732..12a7981a8a71 100644
> > > > --- a/drivers/nvmem/qfprom.c
> > > > +++ b/drivers/nvmem/qfprom.c
> > > > @@ -109,6 +109,10 @@ struct qfprom_soc_compatible_data {
> > > >        bool secure;
> > > >   };
> > > > 
> > > > +static const struct qfprom_soc_compatible_data qdu1000_qfprom = {
> > > > +     .secure = true
> > > > +};
> > > > +
> > > >   static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
> > > >        {.start = 0x128, .end = 0x148},
> > > >        {.start = 0x220, .end = 0x228}
> > > > @@ -490,6 +494,7 @@ static int qfprom_probe(struct platform_device *pdev)
> > > > 
> > > >   static const struct of_device_id qfprom_of_match[] = {
> > > >        { .compatible = "qcom,qfprom",},
> > > > +     { .compatible = "qcom,qdu1000-qfprom", .data = &qdu1000_qfprom},
> > > >        { .compatible = "qcom,sc7180-qfprom", .data = &sc7180_qfprom},
> > > I have doubts that this is still compatible with qcom,qfprom. It uses
> > > entirely different read method. That's why generic fallbacks are bad,
> > > one more case to my growing list of awesome examples. :)
> Okay, will do that.
> > Yes, it looks like it should be 'qcom,qdu1000-qfprom",
> > "qcom,scm-qfprom". And possibly a separate driver for scm-qfprom.
> The only difference here is in read method, which can be controlled by a
> single property,
> do we really need to write a separate driver for just reading secure feature
> register.

I presume that if reads are hidden behind scm, then the most of the
driver - which deals with writing to qfprom - isn't going to be at all
applicable.

So, I actually think it would make sense to put that in a separate
qfprom-scm driver, which handles the generic fallback of
"qcom,qfprom-scm".

Regards,
Bjorn

> 
> Thanks,
> Komal
> > 
> > 
>