The SATA PHY in Exynos5250 SoCs has two interfaces and two device nodes:
1. sata-phy@12170000
2. i2c-9/i2c@38
The first node represents the actual SATA PHY device with phy-cells.
The second represents additional I2C interface, needed by the driver
to communicate with the SATA PHY device. It is not a PHY-provider in
the terms of dtschema so rename it to silence dtbs_check warning:
arch/arm/boot/dts/exynos5250-arndale.dt.yaml: sata-phy@38: '#phy-cells' is a required property
From schema: lib/python3.6/site-packages/dtschema/schemas/phy/phy-provider.yaml
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
arch/arm/boot/dts/exynos5250-arndale.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index f2bcce167b2d..3c401c82905c 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -544,7 +544,7 @@
samsung,i2c-max-bus-freq = <40000>;
samsung,i2c-slave-addr = <0x38>;
- sata_phy_i2c: sata-phy@38 {
+ sata_phy_i2c: sata-phy-i2c@38 {
compatible = "samsung,exynos-sataphy-i2c";
reg = <0x38>;
};
--
2.17.1
Hi Krzysztof,
On 01.09.2020 09:54, Krzysztof Kozlowski wrote:
> The SATA PHY in Exynos5250 SoCs has two interfaces and two device nodes:
> 1. sata-phy@12170000
> 2. i2c-9/i2c@38
>
> The first node represents the actual SATA PHY device with phy-cells.
> The second represents additional I2C interface, needed by the driver
> to communicate with the SATA PHY device. It is not a PHY-provider in
> the terms of dtschema so rename it to silence dtbs_check warning:
>
> arch/arm/boot/dts/exynos5250-arndale.dt.yaml: sata-phy@38: '#phy-cells' is a required property
> From schema: lib/python3.6/site-packages/dtschema/schemas/phy/phy-provider.yaml
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> arch/arm/boot/dts/exynos5250-arndale.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
> index f2bcce167b2d..3c401c82905c 100644
> --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> @@ -544,7 +544,7 @@
> samsung,i2c-max-bus-freq = <40000>;
> samsung,i2c-slave-addr = <0x38>;
>
> - sata_phy_i2c: sata-phy@38 {
> + sata_phy_i2c: sata-phy-i2c@38 {
> compatible = "samsung,exynos-sataphy-i2c";
> reg = <0x38>;
> };
I'm not against the rename, but frankly, the above node and all i2c
parameters should be moved to exynos5250.dtsi. This is a SoC internal
things (the same way as hdmiphy in exynos4.dtsi), so the board dts
should only contain information like status = "enabled" for i2c_8 and
hdmi_i2c_phy nodes. No need to duplicate it here and in smdk5250.dts.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, 1 Sep 2020 at 10:13, Marek Szyprowski <[email protected]> wrote:
>
> Hi Krzysztof,
>
> On 01.09.2020 09:54, Krzysztof Kozlowski wrote:
> > The SATA PHY in Exynos5250 SoCs has two interfaces and two device nodes:
> > 1. sata-phy@12170000
> > 2. i2c-9/i2c@38
> >
> > The first node represents the actual SATA PHY device with phy-cells.
> > The second represents additional I2C interface, needed by the driver
> > to communicate with the SATA PHY device. It is not a PHY-provider in
> > the terms of dtschema so rename it to silence dtbs_check warning:
> >
> > arch/arm/boot/dts/exynos5250-arndale.dt.yaml: sata-phy@38: '#phy-cells' is a required property
> > From schema: lib/python3.6/site-packages/dtschema/schemas/phy/phy-provider.yaml
> >
> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> > ---
> > arch/arm/boot/dts/exynos5250-arndale.dts | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
> > index f2bcce167b2d..3c401c82905c 100644
> > --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> > @@ -544,7 +544,7 @@
> > samsung,i2c-max-bus-freq = <40000>;
> > samsung,i2c-slave-addr = <0x38>;
> >
> > - sata_phy_i2c: sata-phy@38 {
> > + sata_phy_i2c: sata-phy-i2c@38 {
> > compatible = "samsung,exynos-sataphy-i2c";
> > reg = <0x38>;
> > };
>
> I'm not against the rename, but frankly, the above node and all i2c
> parameters should be moved to exynos5250.dtsi. This is a SoC internal
> things (the same way as hdmiphy in exynos4.dtsi), so the board dts
> should only contain information like status = "enabled" for i2c_8 and
> hdmi_i2c_phy nodes. No need to duplicate it here and in smdk5250.dts.
Good point, the I2C bus used here is an internal part of SoC.
I will squash these two changes into a new one. Thanks for the review!
Best regards,
Krzysztof
On Tue, 1 Sep 2020 at 10:17, Krzysztof Kozlowski <[email protected]> wrote:
>
> On Tue, 1 Sep 2020 at 10:13, Marek Szyprowski <[email protected]> wrote:
> >
> > Hi Krzysztof,
> >
> > On 01.09.2020 09:54, Krzysztof Kozlowski wrote:
> > > The SATA PHY in Exynos5250 SoCs has two interfaces and two device nodes:
> > > 1. sata-phy@12170000
> > > 2. i2c-9/i2c@38
> > >
> > > The first node represents the actual SATA PHY device with phy-cells.
> > > The second represents additional I2C interface, needed by the driver
> > > to communicate with the SATA PHY device. It is not a PHY-provider in
> > > the terms of dtschema so rename it to silence dtbs_check warning:
> > >
> > > arch/arm/boot/dts/exynos5250-arndale.dt.yaml: sata-phy@38: '#phy-cells' is a required property
> > > From schema: lib/python3.6/site-packages/dtschema/schemas/phy/phy-provider.yaml
> > >
> > > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> > > ---
> > > arch/arm/boot/dts/exynos5250-arndale.dts | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
> > > index f2bcce167b2d..3c401c82905c 100644
> > > --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> > > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> > > @@ -544,7 +544,7 @@
> > > samsung,i2c-max-bus-freq = <40000>;
> > > samsung,i2c-slave-addr = <0x38>;
> > >
> > > - sata_phy_i2c: sata-phy@38 {
> > > + sata_phy_i2c: sata-phy-i2c@38 {
> > > compatible = "samsung,exynos-sataphy-i2c";
> > > reg = <0x38>;
> > > };
> >
> > I'm not against the rename, but frankly, the above node and all i2c
> > parameters should be moved to exynos5250.dtsi. This is a SoC internal
> > things (the same way as hdmiphy in exynos4.dtsi), so the board dts
> > should only contain information like status = "enabled" for i2c_8 and
> > hdmi_i2c_phy nodes. No need to duplicate it here and in smdk5250.dts.
>
> Good point, the I2C bus used here is an internal part of SoC.
>
> I will squash these two changes into a new one. Thanks for the review!
While at it, I wonder about the samsung,i2c-slave-addr property. Is it
really needed? Are there multiple masters on this bus?
Best regards,
Krzysztof
On 01.09.2020 10:19, Krzysztof Kozlowski wrote:
> On Tue, 1 Sep 2020 at 10:17, Krzysztof Kozlowski <[email protected]> wrote:
>> On Tue, 1 Sep 2020 at 10:13, Marek Szyprowski <[email protected]> wrote:
>>> On 01.09.2020 09:54, Krzysztof Kozlowski wrote:
>>>> The SATA PHY in Exynos5250 SoCs has two interfaces and two device nodes:
>>>> 1. sata-phy@12170000
>>>> 2. i2c-9/i2c@38
>>>>
>>>> The first node represents the actual SATA PHY device with phy-cells.
>>>> The second represents additional I2C interface, needed by the driver
>>>> to communicate with the SATA PHY device. It is not a PHY-provider in
>>>> the terms of dtschema so rename it to silence dtbs_check warning:
>>>>
>>>> arch/arm/boot/dts/exynos5250-arndale.dt.yaml: sata-phy@38: '#phy-cells' is a required property
>>>> From schema: lib/python3.6/site-packages/dtschema/schemas/phy/phy-provider.yaml
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <[email protected]>
>>>> ---
>>>> arch/arm/boot/dts/exynos5250-arndale.dts | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
>>>> index f2bcce167b2d..3c401c82905c 100644
>>>> --- a/arch/arm/boot/dts/exynos5250-arndale.dts
>>>> +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
>>>> @@ -544,7 +544,7 @@
>>>> samsung,i2c-max-bus-freq = <40000>;
>>>> samsung,i2c-slave-addr = <0x38>;
>>>>
>>>> - sata_phy_i2c: sata-phy@38 {
>>>> + sata_phy_i2c: sata-phy-i2c@38 {
>>>> compatible = "samsung,exynos-sataphy-i2c";
>>>> reg = <0x38>;
>>>> };
>>> I'm not against the rename, but frankly, the above node and all i2c
>>> parameters should be moved to exynos5250.dtsi. This is a SoC internal
>>> things (the same way as hdmiphy in exynos4.dtsi), so the board dts
>>> should only contain information like status = "enabled" for i2c_8 and
>>> hdmi_i2c_phy nodes. No need to duplicate it here and in smdk5250.dts.
>> Good point, the I2C bus used here is an internal part of SoC.
>>
>> I will squash these two changes into a new one. Thanks for the review!
> While at it, I wonder about the samsung,i2c-slave-addr property. Is it
> really needed? Are there multiple masters on this bus?
This might be some cargo-cult, probably copied from HDMI DDC (I'm not
sure if it is needed there). I've removed it and SATA still works fine
on Exynos5250 Arndale board.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, 1 Sep 2020 at 10:36, Marek Szyprowski <[email protected]> wrote:
> >>> I'm not against the rename, but frankly, the above node and all i2c
> >>> parameters should be moved to exynos5250.dtsi. This is a SoC internal
> >>> things (the same way as hdmiphy in exynos4.dtsi), so the board dts
> >>> should only contain information like status = "enabled" for i2c_8 and
> >>> hdmi_i2c_phy nodes. No need to duplicate it here and in smdk5250.dts.
> >> Good point, the I2C bus used here is an internal part of SoC.
> >>
> >> I will squash these two changes into a new one. Thanks for the review!
> > While at it, I wonder about the samsung,i2c-slave-addr property. Is it
> > really needed? Are there multiple masters on this bus?
>
> This might be some cargo-cult, probably copied from HDMI DDC (I'm not
> sure if it is needed there). I've removed it and SATA still works fine
> on Exynos5250 Arndale board.
Cool, thanks!
Best regards,
Krzysztof