diff mbox series

[v8,3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

Message ID 20230717103236.1246771-4-bhupesh.sharma@linaro.org
State New
Headers
Series Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support |

Commit Message

Bhupesh Sharma July 17, 2023, 10:32 a.m. UTC 
  Add the Embedded USB Debugger(EUD) device tree node for
SM6115 / SM4250 SoC.

The node contains EUD base register region, EUD mode manager
register region and TCSR Base register region along with the
interrupt entry.

Also add the typec connector node for EUD which is attached to
EUD node via port. EUD is also attached to DWC3 node via port.

To enable the role switch, we need to set dr_mode = "otg" property
for 'usb_dwc3' sub-node in the board dts file.

Also the EUD device can be enabled on a board once linux is boot'ed
by setting:
 $ echo 1 > /sys/bus/platform/drivers/qcom_eud/../enable

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Bhupesh Sharma July 17, 2023, 6:03 p.m. UTC | #1 
On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > Add the Embedded USB Debugger(EUD) device tree node for
> > SM6115 / SM4250 SoC.
> >
> > The node contains EUD base register region, EUD mode manager
> > register region and TCSR Base register region along with the
> > interrupt entry.
> >
> > [...]
> >
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > index 839c603512403..db45337c1082c 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > [...]
> > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >                       #power-domain-cells = <1>;
> >               };
> >
> > +             eud: eud@1610000 {
> > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > +                           <0x0 0x01612000 0x0 0x1000>,
> > +                           <0x0 0x003c0000 0x0 0x40000>;
> > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>
> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> shouldn't be listed as "reg" here.
>
> Typically we describe it as syscon and then reference it from other
> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> same use case as you have. It also uses this to write something with
> qcom_scm_io_writel() at the end.

That was discussed a bit during v1 patchset review. Basically, if we
use a tcsr syscon approach here, we will need to define a 'qcom,xx'
vendor specific dt-property and use something like this in the eud
node:

qcom,eud-sec-reg = <&tcsr_reg yyyy>

which would be then used by the eud driver (via
syscon_regmap_lookup_by_phandle()).

But for sm6115 / qcm2290 this would be an over complicated solution as
normally the eud driver (say sc7280) doesn't need tcsr based secure
mode manager access. So defining a new soc / vendor specific
dt-property might be an overkill.

Thanks,
Bhupesh
Stephan Gerhold July 17, 2023, 6:26 p.m. UTC | #2 
On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > > Add the Embedded USB Debugger(EUD) device tree node for
> > > SM6115 / SM4250 SoC.
> > >
> > > The node contains EUD base register region, EUD mode manager
> > > register region and TCSR Base register region along with the
> > > interrupt entry.
> > >
> > > [...]
> > >
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > index 839c603512403..db45337c1082c 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > [...]
> > > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> > >                       #power-domain-cells = <1>;
> > >               };
> > >
> > > +             eud: eud@1610000 {
> > > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > > +                           <0x0 0x01612000 0x0 0x1000>,
> > > +                           <0x0 0x003c0000 0x0 0x40000>;
> > > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >
> > TCSR is a separate hardware block unrelated to the EUD. IMHO it
> > shouldn't be listed as "reg" here.
> >
> > Typically we describe it as syscon and then reference it from other
> > nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> > "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> > same use case as you have. It also uses this to write something with
> > qcom_scm_io_writel() at the end.
> 
> That was discussed a bit during v1 patchset review. Basically, if we
> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> vendor specific dt-property and use something like this in the eud
> node:
> 
> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> 
> which would be then used by the eud driver (via
> syscon_regmap_lookup_by_phandle()).
> 
> But for sm6115 / qcm2290 this would be an over complicated solution as
> normally the eud driver (say sc7280) doesn't need tcsr based secure
> mode manager access. So defining a new soc / vendor specific
> dt-property might be an overkill.
> 

IMO a vendor-specific DT property is still better than messing up the
device separation in the device tree. The same "tcsr-base" reg would
also appear on the actual tcsr syscon device tree node. Having two
device tree nodes with the same reg region is generally not valid.

Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
clear that this points into a region that is shared between multiple
different devices, while adding it as reg suggests that TCSR belongs
exclusively to EUD.

Thanks,
Stephan
Bhupesh Sharma July 17, 2023, 8:09 p.m. UTC | #3 
On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> > On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> > >
> > > On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > > > Add the Embedded USB Debugger(EUD) device tree node for
> > > > SM6115 / SM4250 SoC.
> > > >
> > > > The node contains EUD base register region, EUD mode manager
> > > > register region and TCSR Base register region along with the
> > > > interrupt entry.
> > > >
> > > > [...]
> > > >
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > index 839c603512403..db45337c1082c 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > [...]
> > > > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> > > >                       #power-domain-cells = <1>;
> > > >               };
> > > >
> > > > +             eud: eud@1610000 {
> > > > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > > > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > > > +                           <0x0 0x01612000 0x0 0x1000>,
> > > > +                           <0x0 0x003c0000 0x0 0x40000>;
> > > > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> > >
> > > TCSR is a separate hardware block unrelated to the EUD. IMHO it
> > > shouldn't be listed as "reg" here.
> > >
> > > Typically we describe it as syscon and then reference it from other
> > > nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> > > "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> > > same use case as you have. It also uses this to write something with
> > > qcom_scm_io_writel() at the end.
> >
> > That was discussed a bit during v1 patchset review. Basically, if we
> > use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> > vendor specific dt-property and use something like this in the eud
> > node:
> >
> > qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >
> > which would be then used by the eud driver (via
> > syscon_regmap_lookup_by_phandle()).
> >
> > But for sm6115 / qcm2290 this would be an over complicated solution as
> > normally the eud driver (say sc7280) doesn't need tcsr based secure
> > mode manager access. So defining a new soc / vendor specific
> > dt-property might be an overkill.
> >
>
> IMO a vendor-specific DT property is still better than messing up the
> device separation in the device tree. The same "tcsr-base" reg would
> also appear on the actual tcsr syscon device tree node. Having two
> device tree nodes with the same reg region is generally not valid.
>
> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> clear that this points into a region that is shared between multiple
> different devices, while adding it as reg suggests that TCSR belongs
> exclusively to EUD.

I understand your point but since for sm6115 / qcm2290 devices TCSR is
not used for any other purpose than EUD, I still think introducing a
new soc / vendor specific dt-property might be an overkill for this
changeset.

Thanks,
Bhupesh
Konrad Dybcio July 17, 2023, 8:19 p.m. UTC | #4 
On 17.07.2023 22:09, Bhupesh Sharma wrote:
> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>>
>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>
>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
>>>>> Add the Embedded USB Debugger(EUD) device tree node for
>>>>> SM6115 / SM4250 SoC.
>>>>>
>>>>> The node contains EUD base register region, EUD mode manager
>>>>> register region and TCSR Base register region along with the
>>>>> interrupt entry.
>>>>>
>>>>> [...]
>>>>>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
>>>>>  1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> index 839c603512403..db45337c1082c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> [...]
>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
>>>>>                       #power-domain-cells = <1>;
>>>>>               };
>>>>>
>>>>> +             eud: eud@1610000 {
>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>>>>
>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
>>>> shouldn't be listed as "reg" here.
>>>>
>>>> Typically we describe it as syscon and then reference it from other
>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
>>>> same use case as you have. It also uses this to write something with
>>>> qcom_scm_io_writel() at the end.
>>>
>>> That was discussed a bit during v1 patchset review. Basically, if we
>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
>>> vendor specific dt-property and use something like this in the eud
>>> node:
>>>
>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
>>>
>>> which would be then used by the eud driver (via
>>> syscon_regmap_lookup_by_phandle()).
>>>
>>> But for sm6115 / qcm2290 this would be an over complicated solution as
>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
>>> mode manager access. So defining a new soc / vendor specific
>>> dt-property might be an overkill.
>>>
>>
>> IMO a vendor-specific DT property is still better than messing up the
>> device separation in the device tree. The same "tcsr-base" reg would
>> also appear on the actual tcsr syscon device tree node. Having two
>> device tree nodes with the same reg region is generally not valid.
>>
>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
>> clear that this points into a region that is shared between multiple
>> different devices, while adding it as reg suggests that TCSR belongs
>> exclusively to EUD.
> 
> I understand your point but since for sm6115 / qcm2290 devices TCSR is
> not used for any other purpose than EUD, I still think introducing a
> new soc / vendor specific dt-property might be an overkill for this
> changeset.
Untrue, there's some mumblings around the PHY properties and PSHOLD.
I think Stephan may be correct here.

Konrad
> 
> Thanks,
> Bhupesh
Bhupesh Sharma July 17, 2023, 8:22 p.m. UTC | #5 
On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 17.07.2023 22:09, Bhupesh Sharma wrote:
> > On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>
> >> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> >>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>
> >>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> >>>>> Add the Embedded USB Debugger(EUD) device tree node for
> >>>>> SM6115 / SM4250 SoC.
> >>>>>
> >>>>> The node contains EUD base register region, EUD mode manager
> >>>>> register region and TCSR Base register region along with the
> >>>>> interrupt entry.
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >>>>>  1 file changed, 50 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> index 839c603512403..db45337c1082c 100644
> >>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> [...]
> >>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >>>>>                       #power-domain-cells = <1>;
> >>>>>               };
> >>>>>
> >>>>> +             eud: eud@1610000 {
> >>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> >>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
> >>>>> +                           <0x0 0x01612000 0x0 0x1000>,
> >>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
> >>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >>>>
> >>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> >>>> shouldn't be listed as "reg" here.
> >>>>
> >>>> Typically we describe it as syscon and then reference it from other
> >>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> >>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> >>>> same use case as you have. It also uses this to write something with
> >>>> qcom_scm_io_writel() at the end.
> >>>
> >>> That was discussed a bit during v1 patchset review. Basically, if we
> >>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> >>> vendor specific dt-property and use something like this in the eud
> >>> node:
> >>>
> >>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >>>
> >>> which would be then used by the eud driver (via
> >>> syscon_regmap_lookup_by_phandle()).
> >>>
> >>> But for sm6115 / qcm2290 this would be an over complicated solution as
> >>> normally the eud driver (say sc7280) doesn't need tcsr based secure
> >>> mode manager access. So defining a new soc / vendor specific
> >>> dt-property might be an overkill.
> >>>
> >>
> >> IMO a vendor-specific DT property is still better than messing up the
> >> device separation in the device tree. The same "tcsr-base" reg would
> >> also appear on the actual tcsr syscon device tree node. Having two
> >> device tree nodes with the same reg region is generally not valid.
> >>
> >> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> >> clear that this points into a region that is shared between multiple
> >> different devices, while adding it as reg suggests that TCSR belongs
> >> exclusively to EUD.
> >
> > I understand your point but since for sm6115 / qcm2290 devices TCSR is
> > not used for any other purpose than EUD, I still think introducing a
> > new soc / vendor specific dt-property might be an overkill for this
> > changeset.
> Untrue, there's some mumblings around the PHY properties and PSHOLD.
> I think Stephan may be correct here.

Can you share the links to those discussions?

Thanks,
Bhupesh
Konrad Dybcio July 17, 2023, 8:24 p.m. UTC | #6 
On 17.07.2023 22:22, Bhupesh Sharma wrote:
> On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 17.07.2023 22:09, Bhupesh Sharma wrote:
>>> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>
>>>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
>>>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>>>
>>>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
>>>>>>> Add the Embedded USB Debugger(EUD) device tree node for
>>>>>>> SM6115 / SM4250 SoC.
>>>>>>>
>>>>>>> The node contains EUD base register region, EUD mode manager
>>>>>>> register region and TCSR Base register region along with the
>>>>>>> interrupt entry.
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 50 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> index 839c603512403..db45337c1082c 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> [...]
>>>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
>>>>>>>                       #power-domain-cells = <1>;
>>>>>>>               };
>>>>>>>
>>>>>>> +             eud: eud@1610000 {
>>>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
>>>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
>>>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
>>>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
>>>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>>>>>>
>>>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
>>>>>> shouldn't be listed as "reg" here.
>>>>>>
>>>>>> Typically we describe it as syscon and then reference it from other
>>>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
>>>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
>>>>>> same use case as you have. It also uses this to write something with
>>>>>> qcom_scm_io_writel() at the end.
>>>>>
>>>>> That was discussed a bit during v1 patchset review. Basically, if we
>>>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
>>>>> vendor specific dt-property and use something like this in the eud
>>>>> node:
>>>>>
>>>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
>>>>>
>>>>> which would be then used by the eud driver (via
>>>>> syscon_regmap_lookup_by_phandle()).
>>>>>
>>>>> But for sm6115 / qcm2290 this would be an over complicated solution as
>>>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
>>>>> mode manager access. So defining a new soc / vendor specific
>>>>> dt-property might be an overkill.
>>>>>
>>>>
>>>> IMO a vendor-specific DT property is still better than messing up the
>>>> device separation in the device tree. The same "tcsr-base" reg would
>>>> also appear on the actual tcsr syscon device tree node. Having two
>>>> device tree nodes with the same reg region is generally not valid.
>>>>
>>>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
>>>> clear that this points into a region that is shared between multiple
>>>> different devices, while adding it as reg suggests that TCSR belongs
>>>> exclusively to EUD.
>>>
>>> I understand your point but since for sm6115 / qcm2290 devices TCSR is
>>> not used for any other purpose than EUD, I still think introducing a
>>> new soc / vendor specific dt-property might be an overkill for this
>>> changeset.
>> Untrue, there's some mumblings around the PHY properties and PSHOLD.
>> I think Stephan may be correct here.
> 
> Can you share the links to those discussions?
It just seemed off to me that TCSR was not used by anything else (even
from Linux, it would obviously be used by something else higher up in
the boot chain as it contains various configuration registers), so I
took a glance at the downstream device tree and I noticed there are
more users.

Konrad
Bhupesh Sharma July 17, 2023, 8:41 p.m. UTC | #7 
On Tue, 18 Jul 2023 at 01:54, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 17.07.2023 22:22, Bhupesh Sharma wrote:
> > On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 17.07.2023 22:09, Bhupesh Sharma wrote:
> >>> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>
> >>>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> >>>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>>>
> >>>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> >>>>>>> Add the Embedded USB Debugger(EUD) device tree node for
> >>>>>>> SM6115 / SM4250 SoC.
> >>>>>>>
> >>>>>>> The node contains EUD base register region, EUD mode manager
> >>>>>>> register region and TCSR Base register region along with the
> >>>>>>> interrupt entry.
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>
> >>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>>>>>> ---
> >>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >>>>>>>  1 file changed, 50 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> index 839c603512403..db45337c1082c 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> [...]
> >>>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >>>>>>>                       #power-domain-cells = <1>;
> >>>>>>>               };
> >>>>>>>
> >>>>>>> +             eud: eud@1610000 {
> >>>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> >>>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
> >>>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
> >>>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
> >>>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >>>>>>
> >>>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> >>>>>> shouldn't be listed as "reg" here.
> >>>>>>
> >>>>>> Typically we describe it as syscon and then reference it from other
> >>>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> >>>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> >>>>>> same use case as you have. It also uses this to write something with
> >>>>>> qcom_scm_io_writel() at the end.
> >>>>>
> >>>>> That was discussed a bit during v1 patchset review. Basically, if we
> >>>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> >>>>> vendor specific dt-property and use something like this in the eud
> >>>>> node:
> >>>>>
> >>>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >>>>>
> >>>>> which would be then used by the eud driver (via
> >>>>> syscon_regmap_lookup_by_phandle()).
> >>>>>
> >>>>> But for sm6115 / qcm2290 this would be an over complicated solution as
> >>>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
> >>>>> mode manager access. So defining a new soc / vendor specific
> >>>>> dt-property might be an overkill.
> >>>>>
> >>>>
> >>>> IMO a vendor-specific DT property is still better than messing up the
> >>>> device separation in the device tree. The same "tcsr-base" reg would
> >>>> also appear on the actual tcsr syscon device tree node. Having two
> >>>> device tree nodes with the same reg region is generally not valid.
> >>>>
> >>>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> >>>> clear that this points into a region that is shared between multiple
> >>>> different devices, while adding it as reg suggests that TCSR belongs
> >>>> exclusively to EUD.
> >>>
> >>> I understand your point but since for sm6115 / qcm2290 devices TCSR is
> >>> not used for any other purpose than EUD, I still think introducing a
> >>> new soc / vendor specific dt-property might be an overkill for this
> >>> changeset.
> >> Untrue, there's some mumblings around the PHY properties and PSHOLD.
> >> I think Stephan may be correct here.
> >
> > Can you share the links to those discussions?
> It just seemed off to me that TCSR was not used by anything else (even
> from Linux, it would obviously be used by something else higher up in
> the boot chain as it contains various configuration registers), so I
> took a glance at the downstream device tree and I noticed there are
> more users.

Ok, let me recheck the downstream code and come back.

Thanks,
Bhupesh
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index 839c603512403..db45337c1082c 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -260,6 +260,18 @@  CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
 		};
 	};
 
+	eud_typec: connector {
+		compatible = "usb-c-connector";
+
+		ports {
+			port@0 {
+				con_eud: endpoint {
+					remote-endpoint = <&eud_con>;
+				};
+			};
+		};
+	};
+
 	firmware {
 		scm: scm {
 			compatible = "qcom,scm-sm6115", "qcom,scm";
@@ -789,6 +801,37 @@  gcc: clock-controller@1400000 {
 			#power-domain-cells = <1>;
 		};
 
+		eud: eud@1610000 {
+			compatible = "qcom,sm6115-eud", "qcom,eud";
+			reg = <0x0 0x01610000 0x0 0x2000>,
+			      <0x0 0x01612000 0x0 0x1000>,
+			      <0x0 0x003c0000 0x0 0x40000>;
+			reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
+			interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					eud_ep: endpoint {
+						remote-endpoint = <&usb2_role_switch>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					eud_con: endpoint {
+						remote-endpoint = <&con_eud>;
+					};
+				};
+			};
+		};
+
 		usb_hsphy: phy@1613000 {
 			compatible = "qcom,sm6115-qusb2-phy";
 			reg = <0x0 0x01613000 0x0 0x180>;
@@ -1322,6 +1365,13 @@  usb_dwc3: usb@4e00000 {
 				snps,has-lpm-erratum;
 				snps,hird-threshold = /bits/ 8 <0x10>;
 				snps,usb3_lpm_capable;
+				usb-role-switch;
+
+				port {
+					usb2_role_switch: endpoint {
+						remote-endpoint = <&eud_ep>;
+					};
+				};
 			};
 		};