Message ID | 1400095043-685-2-git-send-email-rahul.sharma@samsung.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Thanks Tomasz, On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Rahul, Tomasz, [snip] >> + simplephys: simple-phys@10040000 { >> + compatible = "samsung,exynos5250-simple-phy"; > > Missing reg property or unnecessary @unit-address suffix in node name. I should have removed "@unit-address". I will change this. > >> + samsung,pmu-syscon = <&pmu_system_controller>; >> + #phy-cells = <1>; >> + }; > > In general, the idea is quite good, but I think this should rather bind > to the main PMU node, since this is just a part of the PMU, not another > device in the system. This also means that the PMU node itself should be > the PHY provider. > Please correct me if I got you wrong. You want somthing like this: pmu_system_controller: system-controller@10040000 { ... simple_phys: simple-phys { compatible = "samsung,exynos5420-simple-phy"; ... }; }; Regards, Rahul Sharma. > Otherwise looks good. > > Best regards, > Tomasz > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thierry, On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: > [...] >> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt > [...] >> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in >> +the PHY specifier identifies the PHY and the supported phys for exynos4210 > > I think the specifier is only the part after the phandle, so this should > probably be "... compatible PHYs the single cell specifier ..." or > something equivalent. > ok. I will rephrase this line. >> +are: >> + HDMI_PHY, >> + DAC_PHY, >> + ADC_PHY, >> + PCIE_PHY, >> + SATA_PHY. > > I think you need to specify the literal values here as well, since the > binding must be fully self-contained. That is you can't rely on the DT > binding to be bundled with the exynos-simple-phy.h header. > Ok. I will add that. >> @@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o >> phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.o >> obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o >> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o >> +obj-$(CONFIG_EXYNOS_SIMPLE_PHY) += exynos-simple-phy.o > > Perhaps this should be named phy-exynos-simple for consistency? Also it > may be a good idea to sort this alphabetically to reduce the potential > for conflicts. yea correct. I will use "phy-exynos-simple". I will place this addition in alphabetical order in makefile. > >> +static int exynos_phy_probe(struct platform_device *pdev) >> +{ >> + const struct of_device_id *of_id = of_match_device( >> + of_match_ptr(exynos_phy_of_match), &pdev->dev); > > Why does this need of_match_ptr()? > yea correct. Not required. >> + dev_info(dev, "probe success\n"); > > If at all this should be dev_dbg(). But in general the driver core will > already complain if the driver fails to probe, so there's in general no > need to mention when it probes successfully. > ok. >> diff --git a/include/dt-bindings/phy/exynos-simple-phy.h b/include/dt-bindings/phy/exynos-simple-phy.h > [...] >> +/* simeple phys */ > > s/simeple phys/simple PHYs/ > > Although on second thought that comment probably shouldn't be there in > the first place. > >> +#define INVALID (~1) > > This doesn't belong in this header. The value should never be used by a > DT source file, should it? I will move this to driver file. > >> +#define HDMI_PHY 0 >> +#define DAC_PHY 1 >> +#define ADC_PHY 2 >> +#define PCIE_PHY 3 >> +#define SATA_PHY 4 > > Perhaps these should be namespaced somehow to avoid potential conflicts > with other PHY providers? How about XXX_SIMPLE_PHY? > >> +#define PHY_NR 5 > > I'm not sure that this belongs here either. It's not a value that will > ever appear in a DT source file. I want it to grow along with new additions in the phy list else catastrophic. This will look unrelated in driver. Regards, Rahul Sharma. > > Thierry > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: > Hi Thierry, > > On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: [...] > >> +#define HDMI_PHY 0 > >> +#define DAC_PHY 1 > >> +#define ADC_PHY 2 > >> +#define PCIE_PHY 3 > >> +#define SATA_PHY 4 > > > > Perhaps these should be namespaced somehow to avoid potential conflicts > > with other PHY providers? > > How about XXX_SIMPLE_PHY? The indices are specific to the Exynos PHY device, so why not PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)? > >> +#define PHY_NR 5 > > > > I'm not sure that this belongs here either. It's not a value that will > > ever appear in a DT source file. > > I want it to grow along with new additions in the phy list else > catastrophic. This will look unrelated in driver. But this is in no way growing automatically as it is. Whoever adds a new type of PHY will need to manually increment this define. Furthermore the driver will need to be updated to cope with this anyway. I think this is even a reason to have this only in the driver. Otherwise you could have a situation where somebody upgrades the binding (and this file along with it) but not update the driver with the necessary support. But the driver will still pick up the PHY_NR change and will accept the new PHY type as valid, even though it has no code to handle it. If you have this in the driver, however, then as long as the driver has not yet been updated it will reject requests for the new PHY type. And that's exactly what it should be doing since it doesn't know how to handle it. Thierry
On 15 May 2014 13:12, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: >> Hi Thierry, >> >> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote: >> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: > [...] >> >> +#define HDMI_PHY 0 >> >> +#define DAC_PHY 1 >> >> +#define ADC_PHY 2 >> >> +#define PCIE_PHY 3 >> >> +#define SATA_PHY 4 >> > >> > Perhaps these should be namespaced somehow to avoid potential conflicts >> > with other PHY providers? >> >> How about XXX_SIMPLE_PHY? > > The indices are specific to the Exynos PHY device, so why not > PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)? This looks ok. I will use that. > >> >> +#define PHY_NR 5 >> > >> > I'm not sure that this belongs here either. It's not a value that will >> > ever appear in a DT source file. >> >> I want it to grow along with new additions in the phy list else >> catastrophic. This will look unrelated in driver. > > But this is in no way growing automatically as it is. Whoever adds a new > type of PHY will need to manually increment this define. Furthermore the > driver will need to be updated to cope with this anyway. Not automatically. What I meant was If keeping it at end of the list, it is not possible that somebody skip the updation of PHY_NR when adding a new phy type. If I leave a comment at the end of the list to update PHY_NR (after moving it to driver), that also serves the purpose. > > I think this is even a reason to have this only in the driver. Otherwise > you could have a situation where somebody upgrades the binding (and this > file along with it) but not update the driver with the necessary support. > But the driver will still pick up the PHY_NR change and will accept the > new PHY type as valid, even though it has no code to handle it. If you > have this in the driver, however, then as long as the driver has not yet > been updated it will reject requests for the new PHY type. And that's > exactly what it should be doing since it doesn't know how to handle it. hmn...Ok. Regards, Rahul Sharma > > Thierry
On Thu, May 15, 2014 at 01:47:33PM +0530, Rahul Sharma wrote: > On 15 May 2014 13:12, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: > >> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote: > >> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: [...] > >> >> +#define PHY_NR 5 > >> > > >> > I'm not sure that this belongs here either. It's not a value that will > >> > ever appear in a DT source file. > >> > >> I want it to grow along with new additions in the phy list else > >> catastrophic. This will look unrelated in driver. > > > > But this is in no way growing automatically as it is. Whoever adds a new > > type of PHY will need to manually increment this define. Furthermore the > > driver will need to be updated to cope with this anyway. > > Not automatically. What I meant was If keeping it at end of the list, it is not > possible that somebody skip the updation of PHY_NR when adding a new phy > type. It's perhaps not as likely, but still possible. > If I leave a comment at the end of the list to update PHY_NR (after moving it > to driver), that also serves the purpose. I don't think this is needed either. Like I said earlier, since the driver has an internal maximum number of PHYs that it supports the maximum that can be specified in the DTS is irrelevant. If it doesn't support a new one, then it will simply return an error. And I would assume that if somebody added support for a new PHY type then they probably wouldn't forget to update the driver since they're modifying it anyway and testing will fail if they don't. Thierry
Hi, On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: > From: Tomasz Stanislawski <t.stanislaws@samsung.com> > > Add exynos-simple-phy driver to support a single register > PHY interfaces present on Exynos4 SoC. > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> > Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com> > > --- > .../devicetree/bindings/phy/samsung-phy.txt | 56 ++++++ > drivers/phy/Kconfig | 5 + > drivers/phy/Makefile | 1 + > drivers/phy/exynos-simple-phy.c | 189 ++++++++++++++++++++ > include/dt-bindings/phy/exynos-simple-phy.h | 27 +++ > 5 files changed, 278 insertions(+) > create mode 100644 drivers/phy/exynos-simple-phy.c > create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h [...] > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index 16a2f06..2a13e0d 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -178,4 +178,9 @@ config PHY_XGENE > help > This option enables support for APM X-Gene SoC multi-purpose PHY. > > +config EXYNOS_SIMPLE_PHY > + tristate "Exynos Simple PHY driver" Please limit this driver to EXYNOS platforms with: depends on ARCH_EXYNOS or (optionally) depends on ARCH_EXYNOS || COMPILE_TEST Moreover the generic PHY dependency is missing. It can be fixed with: select GENERIC_PHY > + help > + Support for 1-bit PHY controllers on SoCs from Exynos family. > + > endmenu Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Hi, On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > Hi, > > On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: >> From: Tomasz Stanislawski <t.stanislaws@samsung.com> >> >> Add exynos-simple-phy driver to support a single register >> PHY interfaces present on Exynos4 SoC. >> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com> >> >> --- >> .../devicetree/bindings/phy/samsung-phy.txt | 56 ++++++ >> drivers/phy/Kconfig | 5 + >> drivers/phy/Makefile | 1 + >> drivers/phy/exynos-simple-phy.c | 189 ++++++++++++++++++++ >> include/dt-bindings/phy/exynos-simple-phy.h | 27 +++ >> 5 files changed, 278 insertions(+) >> create mode 100644 drivers/phy/exynos-simple-phy.c >> create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h > > [...] > >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index 16a2f06..2a13e0d 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -178,4 +178,9 @@ config PHY_XGENE >> help >> This option enables support for APM X-Gene SoC multi-purpose PHY. >> >> +config EXYNOS_SIMPLE_PHY >> + tristate "Exynos Simple PHY driver" > > Please limit this driver to EXYNOS platforms with: > > depends on ARCH_EXYNOS > > or (optionally) > > depends on ARCH_EXYNOS || COMPILE_TEST > > Moreover the generic PHY dependency is missing. It can be fixed with: > > select GENERIC_PHY > Yea, I will fix this part. Regards, Rahul Sharma. >> + help >> + Support for 1-bit PHY controllers on SoCs from Exynos family. >> + >> endmenu > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote: > Hi, > > On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: >> >> Hi, >> >> On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: >>> From: Tomasz Stanislawski <t.stanislaws@samsung.com> >>> >>> Add exynos-simple-phy driver to support a single register >>> PHY interfaces present on Exynos4 SoC. >>> >>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com> >>> >>> --- >>> .../devicetree/bindings/phy/samsung-phy.txt | 56 ++++++ >>> drivers/phy/Kconfig | 5 + >>> drivers/phy/Makefile | 1 + >>> drivers/phy/exynos-simple-phy.c | 189 ++++++++++++++++++++ >>> include/dt-bindings/phy/exynos-simple-phy.h | 27 +++ >>> 5 files changed, 278 insertions(+) >>> create mode 100644 drivers/phy/exynos-simple-phy.c >>> create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h >> >> [...] >> >>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >>> index 16a2f06..2a13e0d 100644 >>> --- a/drivers/phy/Kconfig >>> +++ b/drivers/phy/Kconfig >>> @@ -178,4 +178,9 @@ config PHY_XGENE >>> help >>> This option enables support for APM X-Gene SoC multi-purpose PHY. >>> >>> +config EXYNOS_SIMPLE_PHY >>> + tristate "Exynos Simple PHY driver" >> >> Please limit this driver to EXYNOS platforms with: >> >> depends on ARCH_EXYNOS >> >> or (optionally) >> >> depends on ARCH_EXYNOS || COMPILE_TEST >> >> Moreover the generic PHY dependency is missing. It can be fixed with: >> >> select GENERIC_PHY >> > > Yea, I will fix this part. While at that, change the header of the driver file to *2014* Thanks Kishon > > Regards, > Rahul Sharma. > >>> + help >>> + Support for 1-bit PHY controllers on SoCs from Exynos family. >>> + >>> endmenu >> >> Best regards, >> -- >> Bartlomiej Zolnierkiewicz >> Samsung R&D Institute Poland >> Samsung Electronics >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 May 2014 19:11, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > > On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote: >> Hi, >> >> On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz >> <b.zolnierkie@samsung.com> wrote: >>> >>> Hi, >>> >>> On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: >>>> From: Tomasz Stanislawski <t.stanislaws@samsung.com> >>>> >>>> Add exynos-simple-phy driver to support a single register >>>> PHY interfaces present on Exynos4 SoC. >>>> >>>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >>>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com> >>>> >>>> --- >>>> .../devicetree/bindings/phy/samsung-phy.txt | 56 ++++++ >>>> drivers/phy/Kconfig | 5 + >>>> drivers/phy/Makefile | 1 + >>>> drivers/phy/exynos-simple-phy.c | 189 ++++++++++++++++++++ >>>> include/dt-bindings/phy/exynos-simple-phy.h | 27 +++ >>>> 5 files changed, 278 insertions(+) >>>> create mode 100644 drivers/phy/exynos-simple-phy.c >>>> create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h >>> >>> [...] >>> >>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >>>> index 16a2f06..2a13e0d 100644 >>>> --- a/drivers/phy/Kconfig >>>> +++ b/drivers/phy/Kconfig >>>> @@ -178,4 +178,9 @@ config PHY_XGENE >>>> help >>>> This option enables support for APM X-Gene SoC multi-purpose PHY. >>>> >>>> +config EXYNOS_SIMPLE_PHY >>>> + tristate "Exynos Simple PHY driver" >>> >>> Please limit this driver to EXYNOS platforms with: >>> >>> depends on ARCH_EXYNOS >>> >>> or (optionally) >>> >>> depends on ARCH_EXYNOS || COMPILE_TEST >>> >>> Moreover the generic PHY dependency is missing. It can be fixed with: >>> >>> select GENERIC_PHY >>> >> >> Yea, I will fix this part. > > While at that, change the header of the driver file to *2014* > Sure. Thanks, Rahul Sharma. > Thanks > Kishon >> >> Regards, >> Rahul Sharma. >> >>>> + help >>>> + Support for 1-bit PHY controllers on SoCs from Exynos family. >>>> + >>>> endmenu >>> >>> Best regards, >>> -- >>> Bartlomiej Zolnierkiewicz >>> Samsung R&D Institute Poland >>> Samsung Electronics >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15.05.2014 06:01, Rahul Sharma wrote: > Thanks Tomasz, > > On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> Hi Rahul, Tomasz, > [snip] >>> + simplephys: simple-phys@10040000 { >>> + compatible = "samsung,exynos5250-simple-phy"; >> >> Missing reg property or unnecessary @unit-address suffix in node name. > > I should have removed "@unit-address". I will change this. > >> >>> + samsung,pmu-syscon = <&pmu_system_controller>; >>> + #phy-cells = <1>; >>> + }; >> >> In general, the idea is quite good, but I think this should rather bind >> to the main PMU node, since this is just a part of the PMU, not another >> device in the system. This also means that the PMU node itself should be >> the PHY provider. >> > > Please correct me if I got you wrong. You want somthing like this: > > pmu_system_controller: system-controller@10040000 { > ... > simple_phys: simple-phys { > compatible = "samsung,exynos5420-simple-phy"; > ... > }; > }; Not exactly. What I meant is that the PMU node itself should be the PHY provider, e.g. pmu_system_controller: system-controller@10040000 { /* ... */ #phy-cells = <1>; }; and then the PMU node should instantiate the Exynos simple PHY driver, as this is a driver for a facility existing entirely inside of the PMU. Moreover, the driver should be rather called Exynos PMU PHY. I know this isn't really possible at the moment, but with device tree we must design things carefully, so it's better to take a bit more time and do things properly. So my opinion on this is that there should be a central Exynos PMU driver that claims the IO region and instantiates necessary subdrivers, such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle driver and more, similar to what is being done in drivers/mfd. Now, there is already ongoing effort to convert current freestanding PMU configuration code in mach-exynos into a full-fledged PMU driver, but not exactly in the same direction as I stated above. I'll try to cooperate with Pankaj, who is responsible for this work to make this go into the right track. Best regards, Tomasz
On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On 15.05.2014 06:01, Rahul Sharma wrote: >> Thanks Tomasz, >> >> On 15 May 2014 01:31, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>> Hi Rahul, Tomasz, >> [snip] >>>> + simplephys: simple-phys@10040000 { >>>> + compatible = "samsung,exynos5250-simple-phy"; >>> >>> Missing reg property or unnecessary @unit-address suffix in node name. >> >> I should have removed "@unit-address". I will change this. >> >>> >>>> + samsung,pmu-syscon = <&pmu_system_controller>; >>>> + #phy-cells = <1>; >>>> + }; >>> >>> In general, the idea is quite good, but I think this should rather bind >>> to the main PMU node, since this is just a part of the PMU, not another >>> device in the system. This also means that the PMU node itself should be >>> the PHY provider. >>> >> >> Please correct me if I got you wrong. You want somthing like this: >> >> pmu_system_controller: system-controller@10040000 { >> ... >> simple_phys: simple-phys { >> compatible = "samsung,exynos5420-simple-phy"; >> ... >> }; >> }; > > Not exactly. > > What I meant is that the PMU node itself should be the PHY provider, e.g. > > pmu_system_controller: system-controller@10040000 { > /* ... */ > #phy-cells = <1>; > }; > > and then the PMU node should instantiate the Exynos simple PHY driver, > as this is a driver for a facility existing entirely inside of the PMU. > Moreover, the driver should be rather called Exynos PMU PHY. > > I know this isn't really possible at the moment, but with device tree we > must design things carefully, so it's better to take a bit more time and > do things properly. > > So my opinion on this is that there should be a central Exynos PMU > driver that claims the IO region and instantiates necessary subdrivers, > such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle > driver and more, similar to what is being done in drivers/mfd. Hi Tomasz, > > Now, there is already ongoing effort to convert current freestanding PMU > configuration code in mach-exynos into a full-fledged PMU driver, but > not exactly in the same direction as I stated above. I'll try to > cooperate with Pankaj, who is responsible for this work to make this go > into the right track. > > Best regards, > Tomasz > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: > On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> On 15.05.2014 06:01, Rahul Sharma wrote: [snip] >>>> the PHY provider. >>>> >>> >>> Please correct me if I got you wrong. You want somthing like this: >>> >>> pmu_system_controller: system-controller@10040000 { >>> ... >>> simple_phys: simple-phys { >>> compatible = "samsung,exynos5420-simple-phy"; >>> ... >>> }; >>> }; >> >> Not exactly. >> >> What I meant is that the PMU node itself should be the PHY provider, e.g. >> >> pmu_system_controller: system-controller@10040000 { >> /* ... */ >> #phy-cells = <1>; >> }; >> >> and then the PMU node should instantiate the Exynos simple PHY driver, >> as this is a driver for a facility existing entirely inside of the PMU. >> Moreover, the driver should be rather called Exynos PMU PHY. >> >> I know this isn't really possible at the moment, but with device tree we >> must design things carefully, so it's better to take a bit more time and >> do things properly. >> >> So my opinion on this is that there should be a central Exynos PMU >> driver that claims the IO region and instantiates necessary subdrivers, >> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >> driver and more, similar to what is being done in drivers/mfd. > Hi Tomasz, These PHYs are not part of PMU as such. I am not sure if it is correct to probe them as phy provider for all these phys. Only relation of these phys with the PMU is 'enable/disable control'. Controlling this bit using regmap interface still looks better to me. IMHO Ideal method would be probing these PHYs independently and resolving the necessary dependencies like syscon handle, clocks etc. This way we will not be having any common phy provider for all these independent PHYs and it would be clean to add each of these phy nodes in DT. Please see my original comment below. http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html Regards, Rahul Sharma > >> >> Now, there is already ongoing effort to convert current freestanding PMU >> configuration code in mach-exynos into a full-fledged PMU driver, but >> not exactly in the same direction as I stated above. I'll try to >> cooperate with Pankaj, who is responsible for this work to make this go >> into the right track. >> >> Best regards, >> Tomasz >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 16.05.2014 12:35, Rahul Sharma wrote: > On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>> On 15.05.2014 06:01, Rahul Sharma wrote: > [snip] >>>>> the PHY provider. >>>>> >>>> >>>> Please correct me if I got you wrong. You want somthing like this: >>>> >>>> pmu_system_controller: system-controller@10040000 { >>>> ... >>>> simple_phys: simple-phys { >>>> compatible = "samsung,exynos5420-simple-phy"; >>>> ... >>>> }; >>>> }; >>> >>> Not exactly. >>> >>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>> >>> pmu_system_controller: system-controller@10040000 { >>> /* ... */ >>> #phy-cells = <1>; >>> }; >>> >>> and then the PMU node should instantiate the Exynos simple PHY driver, >>> as this is a driver for a facility existing entirely inside of the PMU. >>> Moreover, the driver should be rather called Exynos PMU PHY. >>> >>> I know this isn't really possible at the moment, but with device tree we >>> must design things carefully, so it's better to take a bit more time and >>> do things properly. >>> >>> So my opinion on this is that there should be a central Exynos PMU >>> driver that claims the IO region and instantiates necessary subdrivers, >>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>> driver and more, similar to what is being done in drivers/mfd. >> > > Hi Tomasz, > > These PHYs are not part of PMU as such. I am not sure if it is correct to > probe them as phy provider for all these phys. Only relation of these phys with > the PMU is 'enable/disable control'. Well, in reality what is implemented by this driver is not even a PHY, just some kind of power controllers, which are contained entirely in the PMU. > Controlling this bit using regmap interface > still looks better to me. Well, when there is a choice between using regmap and not using regmap, I'd rather choose the latter. Why would you want to introduce additional abstraction layer if there is no need for such? > > IMHO Ideal method would be probing these PHYs independently and resolving > the necessary dependencies like syscon handle, clocks etc. This way we will > not be having any common phy provider for all these independent PHYs and it > would be clean to add each of these phy nodes in DT. Please see my original > comment below. > > http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html With the solution I proposed, you don't need any kind of dependencies for those simple power controllers. They are just single bits that don't need anything special to operate, except PMU clock running. Best regards, Tomasz
On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote: > On 16.05.2014 12:35, Rahul Sharma wrote: >> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>> On 15.05.2014 06:01, Rahul Sharma wrote: >> [snip] >>>>>> the PHY provider. >>>>>> >>>>> >>>>> Please correct me if I got you wrong. You want somthing like this: >>>>> >>>>> pmu_system_controller: system-controller@10040000 { >>>>> ... >>>>> simple_phys: simple-phys { >>>>> compatible = "samsung,exynos5420-simple-phy"; >>>>> ... >>>>> }; >>>>> }; >>>> >>>> Not exactly. >>>> >>>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>>> >>>> pmu_system_controller: system-controller@10040000 { >>>> /* ... */ >>>> #phy-cells = <1>; >>>> }; >>>> >>>> and then the PMU node should instantiate the Exynos simple PHY driver, >>>> as this is a driver for a facility existing entirely inside of the PMU. >>>> Moreover, the driver should be rather called Exynos PMU PHY. >>>> >>>> I know this isn't really possible at the moment, but with device tree we >>>> must design things carefully, so it's better to take a bit more time and >>>> do things properly. >>>> >>>> So my opinion on this is that there should be a central Exynos PMU >>>> driver that claims the IO region and instantiates necessary subdrivers, >>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>>> driver and more, similar to what is being done in drivers/mfd. >>> >> >> Hi Tomasz, >> >> These PHYs are not part of PMU as such. I am not sure if it is correct to >> probe them as phy provider for all these phys. Only relation of these phys with >> the PMU is 'enable/disable control'. > > Well, in reality what is implemented by this driver is not even a PHY, > just some kind of power controllers, which are contained entirely in the > PMU. > I agree. Actually the role of generic phy framework for these 'simple' phys is only that much. >> Controlling this bit using regmap interface >> still looks better to me. > > Well, when there is a choice between using regmap and not using regmap, > I'd rather choose the latter. Why would you want to introduce additional > abstraction layer if there is no need for such? > >> >> IMHO Ideal method would be probing these PHYs independently and resolving >> the necessary dependencies like syscon handle, clocks etc. This way we will >> not be having any common phy provider for all these independent PHYs and it >> would be clean to add each of these phy nodes in DT. Please see my original >> comment below. >> >> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html > > With the solution I proposed, you don't need any kind of dependencies > for those simple power controllers. They are just single bits that don't > need anything special to operate, except PMU clock running. In that case we can further trim it down and let the drivers use the regmap interface to control this bit. Many drivers including HDMI, DP just need that much functionality from the phy provider. Regards, Rahul Sharma > > Best regards, > Tomasz > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 16.05.2014 16:30, Rahul Sharma wrote: > On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote: >> On 16.05.2014 12:35, Rahul Sharma wrote: >>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>>> On 15.05.2014 06:01, Rahul Sharma wrote: >>> [snip] >>>>>>> the PHY provider. >>>>>>> >>>>>> >>>>>> Please correct me if I got you wrong. You want somthing like this: >>>>>> >>>>>> pmu_system_controller: system-controller@10040000 { >>>>>> ... >>>>>> simple_phys: simple-phys { >>>>>> compatible = "samsung,exynos5420-simple-phy"; >>>>>> ... >>>>>> }; >>>>>> }; >>>>> >>>>> Not exactly. >>>>> >>>>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>>>> >>>>> pmu_system_controller: system-controller@10040000 { >>>>> /* ... */ >>>>> #phy-cells = <1>; >>>>> }; >>>>> >>>>> and then the PMU node should instantiate the Exynos simple PHY driver, >>>>> as this is a driver for a facility existing entirely inside of the PMU. >>>>> Moreover, the driver should be rather called Exynos PMU PHY. >>>>> >>>>> I know this isn't really possible at the moment, but with device tree we >>>>> must design things carefully, so it's better to take a bit more time and >>>>> do things properly. >>>>> >>>>> So my opinion on this is that there should be a central Exynos PMU >>>>> driver that claims the IO region and instantiates necessary subdrivers, >>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>>>> driver and more, similar to what is being done in drivers/mfd. >>>> >>> >>> Hi Tomasz, >>> >>> These PHYs are not part of PMU as such. I am not sure if it is correct to >>> probe them as phy provider for all these phys. Only relation of these phys with >>> the PMU is 'enable/disable control'. >> >> Well, in reality what is implemented by this driver is not even a PHY, >> just some kind of power controllers, which are contained entirely in the >> PMU. >> > > I agree. Actually the role of generic phy framework for these 'simple' phys is > only that much. > >>> Controlling this bit using regmap interface >>> still looks better to me. >> >> Well, when there is a choice between using regmap and not using regmap, >> I'd rather choose the latter. Why would you want to introduce additional >> abstraction layer if there is no need for such? >> >>> >>> IMHO Ideal method would be probing these PHYs independently and resolving >>> the necessary dependencies like syscon handle, clocks etc. This way we will >>> not be having any common phy provider for all these independent PHYs and it >>> would be clean to add each of these phy nodes in DT. Please see my original >>> comment below. >>> >>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html >> >> With the solution I proposed, you don't need any kind of dependencies >> for those simple power controllers. They are just single bits that don't >> need anything special to operate, except PMU clock running. > > In that case we can further trim it down and let the drivers use the regmap > interface to control this bit. Many drivers including HDMI, DP just need that > much functionality from the phy provider. Well, this is what several drivers already do, like USB PHY (dedicated IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block too) or will do, like I2C (for configuration of I2C mux on Exynos5). At least this would be consistent with them and wouldn't be an API abuse, so I'd be inclined to go this way more than introducing abstractions like this patch does. Best regards, Tomasz
On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote: > On 16.05.2014 16:30, Rahul Sharma wrote: >> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote: >>> On 16.05.2014 12:35, Rahul Sharma wrote: >>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>>>> On 15.05.2014 06:01, Rahul Sharma wrote: >>>> [snip] >>>>>>>> the PHY provider. >>>>>>>> >>>>>>> >>>>>>> Please correct me if I got you wrong. You want somthing like this: >>>>>>> >>>>>>> pmu_system_controller: system-controller@10040000 { >>>>>>> ... >>>>>>> simple_phys: simple-phys { >>>>>>> compatible = "samsung,exynos5420-simple-phy"; >>>>>>> ... >>>>>>> }; >>>>>>> }; >>>>>> >>>>>> Not exactly. >>>>>> >>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>>>>> >>>>>> pmu_system_controller: system-controller@10040000 { >>>>>> /* ... */ >>>>>> #phy-cells = <1>; >>>>>> }; >>>>>> >>>>>> and then the PMU node should instantiate the Exynos simple PHY driver, >>>>>> as this is a driver for a facility existing entirely inside of the PMU. >>>>>> Moreover, the driver should be rather called Exynos PMU PHY. >>>>>> >>>>>> I know this isn't really possible at the moment, but with device tree we >>>>>> must design things carefully, so it's better to take a bit more time and >>>>>> do things properly. >>>>>> >>>>>> So my opinion on this is that there should be a central Exynos PMU >>>>>> driver that claims the IO region and instantiates necessary subdrivers, >>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>>>>> driver and more, similar to what is being done in drivers/mfd. >>>>> >>>> >>>> Hi Tomasz, >>>> >>>> These PHYs are not part of PMU as such. I am not sure if it is correct to >>>> probe them as phy provider for all these phys. Only relation of these phys with >>>> the PMU is 'enable/disable control'. >>> >>> Well, in reality what is implemented by this driver is not even a PHY, >>> just some kind of power controllers, which are contained entirely in the >>> PMU. >>> >> >> I agree. Actually the role of generic phy framework for these 'simple' phys is >> only that much. >> >>>> Controlling this bit using regmap interface >>>> still looks better to me. >>> >>> Well, when there is a choice between using regmap and not using regmap, >>> I'd rather choose the latter. Why would you want to introduce additional >>> abstraction layer if there is no need for such? >>> >>>> >>>> IMHO Ideal method would be probing these PHYs independently and resolving >>>> the necessary dependencies like syscon handle, clocks etc. This way we will >>>> not be having any common phy provider for all these independent PHYs and it >>>> would be clean to add each of these phy nodes in DT. Please see my original >>>> comment below. >>>> >>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html >>> >>> With the solution I proposed, you don't need any kind of dependencies >>> for those simple power controllers. They are just single bits that don't >>> need anything special to operate, except PMU clock running. >> >> In that case we can further trim it down and let the drivers use the regmap >> interface to control this bit. Many drivers including HDMI, DP just need that >> much functionality from the phy provider. > > Well, this is what several drivers already do, like USB PHY (dedicated > IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block > too) or will do, like I2C (for configuration of I2C mux on Exynos5). > > At least this would be consistent with them and wouldn't be an API > abuse, so I'd be inclined to go this way more than introducing > abstractions like this patch does. Ok. I had already posted a patch for this at http://www.spinics.net/lists/linux-samsung-soc/msg28049.html I will revive that thread. @Tomasz Stanislawski, Do you have different opinion here? Regards, Rahul Sharma. > > Best regards, > Tomasz > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 19.05.2014 09:10, Rahul Sharma wrote: > On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote: >> On 16.05.2014 16:30, Rahul Sharma wrote: >>> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote: >>>> On 16.05.2014 12:35, Rahul Sharma wrote: >>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote: >>>>> [snip] >>>>>>>>> the PHY provider. >>>>>>>>> >>>>>>>> >>>>>>>> Please correct me if I got you wrong. You want somthing like this: >>>>>>>> >>>>>>>> pmu_system_controller: system-controller@10040000 { >>>>>>>> ... >>>>>>>> simple_phys: simple-phys { >>>>>>>> compatible = "samsung,exynos5420-simple-phy"; >>>>>>>> ... >>>>>>>> }; >>>>>>>> }; >>>>>>> >>>>>>> Not exactly. >>>>>>> >>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>>>>>> >>>>>>> pmu_system_controller: system-controller@10040000 { >>>>>>> /* ... */ >>>>>>> #phy-cells = <1>; >>>>>>> }; >>>>>>> >>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver, >>>>>>> as this is a driver for a facility existing entirely inside of the PMU. >>>>>>> Moreover, the driver should be rather called Exynos PMU PHY. >>>>>>> >>>>>>> I know this isn't really possible at the moment, but with device tree we >>>>>>> must design things carefully, so it's better to take a bit more time and >>>>>>> do things properly. >>>>>>> >>>>>>> So my opinion on this is that there should be a central Exynos PMU >>>>>>> driver that claims the IO region and instantiates necessary subdrivers, >>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>>>>>> driver and more, similar to what is being done in drivers/mfd. >>>>>> >>>>> >>>>> Hi Tomasz, >>>>> >>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to >>>>> probe them as phy provider for all these phys. Only relation of these phys with >>>>> the PMU is 'enable/disable control'. >>>> >>>> Well, in reality what is implemented by this driver is not even a PHY, >>>> just some kind of power controllers, which are contained entirely in the >>>> PMU. >>>> >>> >>> I agree. Actually the role of generic phy framework for these 'simple' phys is >>> only that much. >>> >>>>> Controlling this bit using regmap interface >>>>> still looks better to me. >>>> >>>> Well, when there is a choice between using regmap and not using regmap, >>>> I'd rather choose the latter. Why would you want to introduce additional >>>> abstraction layer if there is no need for such? >>>> >>>>> >>>>> IMHO Ideal method would be probing these PHYs independently and resolving >>>>> the necessary dependencies like syscon handle, clocks etc. This way we will >>>>> not be having any common phy provider for all these independent PHYs and it >>>>> would be clean to add each of these phy nodes in DT. Please see my original >>>>> comment below. >>>>> >>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html >>>> >>>> With the solution I proposed, you don't need any kind of dependencies >>>> for those simple power controllers. They are just single bits that don't >>>> need anything special to operate, except PMU clock running. >>> >>> In that case we can further trim it down and let the drivers use the regmap >>> interface to control this bit. Many drivers including HDMI, DP just need that >>> much functionality from the phy provider. >> >> Well, this is what several drivers already do, like USB PHY (dedicated >> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block >> too) or will do, like I2C (for configuration of I2C mux on Exynos5). >> >> At least this would be consistent with them and wouldn't be an API >> abuse, so I'd be inclined to go this way more than introducing >> abstractions like this patch does. > > Ok. I had already posted a patch for this at > http://www.spinics.net/lists/linux-samsung-soc/msg28049.html > I will revive that thread. Looks good to me. > > @Tomasz Stanislawski, Do you have different opinion here? I'm afraid Tomasz might not be very responsive during next few days, as he is on a business trip. You might be able to reach him on our internal communicator, though. Best regards, Tomasz
On 19 May 2014 16:24, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On 19.05.2014 09:10, Rahul Sharma wrote: >> On 16 May 2014 20:19, Tomasz Figa <t.figa@samsung.com> wrote: >>> On 16.05.2014 16:30, Rahul Sharma wrote: >>>> On 16 May 2014 16:20, Tomasz Figa <t.figa@samsung.com> wrote: >>>>> On 16.05.2014 12:35, Rahul Sharma wrote: >>>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@samsung.com> wrote: >>>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@gmail.com> wrote: >>>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote: >>>>>> [snip] >>>>>>>>>> the PHY provider. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Please correct me if I got you wrong. You want somthing like this: >>>>>>>>> >>>>>>>>> pmu_system_controller: system-controller@10040000 { >>>>>>>>> ... >>>>>>>>> simple_phys: simple-phys { >>>>>>>>> compatible = "samsung,exynos5420-simple-phy"; >>>>>>>>> ... >>>>>>>>> }; >>>>>>>>> }; >>>>>>>> >>>>>>>> Not exactly. >>>>>>>> >>>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g. >>>>>>>> >>>>>>>> pmu_system_controller: system-controller@10040000 { >>>>>>>> /* ... */ >>>>>>>> #phy-cells = <1>; >>>>>>>> }; >>>>>>>> >>>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver, >>>>>>>> as this is a driver for a facility existing entirely inside of the PMU. >>>>>>>> Moreover, the driver should be rather called Exynos PMU PHY. >>>>>>>> >>>>>>>> I know this isn't really possible at the moment, but with device tree we >>>>>>>> must design things carefully, so it's better to take a bit more time and >>>>>>>> do things properly. >>>>>>>> >>>>>>>> So my opinion on this is that there should be a central Exynos PMU >>>>>>>> driver that claims the IO region and instantiates necessary subdrivers, >>>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle >>>>>>>> driver and more, similar to what is being done in drivers/mfd. >>>>>>> >>>>>> >>>>>> Hi Tomasz, >>>>>> >>>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to >>>>>> probe them as phy provider for all these phys. Only relation of these phys with >>>>>> the PMU is 'enable/disable control'. >>>>> >>>>> Well, in reality what is implemented by this driver is not even a PHY, >>>>> just some kind of power controllers, which are contained entirely in the >>>>> PMU. >>>>> >>>> >>>> I agree. Actually the role of generic phy framework for these 'simple' phys is >>>> only that much. >>>> >>>>>> Controlling this bit using regmap interface >>>>>> still looks better to me. >>>>> >>>>> Well, when there is a choice between using regmap and not using regmap, >>>>> I'd rather choose the latter. Why would you want to introduce additional >>>>> abstraction layer if there is no need for such? >>>>> >>>>>> >>>>>> IMHO Ideal method would be probing these PHYs independently and resolving >>>>>> the necessary dependencies like syscon handle, clocks etc. This way we will >>>>>> not be having any common phy provider for all these independent PHYs and it >>>>>> would be clean to add each of these phy nodes in DT. Please see my original >>>>>> comment below. >>>>>> >>>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html >>>>> >>>>> With the solution I proposed, you don't need any kind of dependencies >>>>> for those simple power controllers. They are just single bits that don't >>>>> need anything special to operate, except PMU clock running. >>>> >>>> In that case we can further trim it down and let the drivers use the regmap >>>> interface to control this bit. Many drivers including HDMI, DP just need that >>>> much functionality from the phy provider. >>> >>> Well, this is what several drivers already do, like USB PHY (dedicated >>> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block >>> too) or will do, like I2C (for configuration of I2C mux on Exynos5). >>> >>> At least this would be consistent with them and wouldn't be an API >>> abuse, so I'd be inclined to go this way more than introducing >>> abstractions like this patch does. >> >> Ok. I had already posted a patch for this at >> http://www.spinics.net/lists/linux-samsung-soc/msg28049.html >> I will revive that thread. > > Looks good to me. > >> >> @Tomasz Stanislawski, Do you have different opinion here? > > I'm afraid Tomasz might not be very responsive during next few days, as > he is on a business trip. You might be able to reach him on our internal > communicator, though. Thanks Tomasz, I will contact him over communicator. Regards. > > Best regards, > Tomasz > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt index 2049261..12ad9cf 100644 --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt @@ -161,3 +161,59 @@ Example: usbdrdphy0 = &usb3_phy0; usbdrdphy1 = &usb3_phy1; }; + +Samsung S5P/EXYNOS SoC series SIMPLE PHY +------------------------------------------------- + +Required properties: +- compatible : should be one of the listed compatibles: + - "samsung,exynos4210-simple-phy" + - "samsung,exynos4412-simple-phy" + - "samsung,exynos5250-simple-phy" + - "samsung,exynos5420-simple-phy" +- samsung,pmureg-phandle - handle to syscon to control PMU registers +- #phy-cells : from the generic phy bindings, must be 1; + +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in +the PHY specifier identifies the PHY and the supported phys for exynos4210 +are: + HDMI_PHY, + DAC_PHY, + ADC_PHY, + PCIE_PHY, + SATA_PHY. + +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in +the PHY specifier identifies the PHY and the supported phys for exynos4412 +are: + HDMI_PHY, + ADC_PHY. + +For "samsung,exynos5250-simple-phy" compatible PHYs the second cell in +the PHY specifier identifies the PHY and the supported phys for exynos5250 +are: + HDMI_PHY, + ADC_PHY, + SATA_PHY. + +For "samsung,exynos5420-simple-phy" compatible PHYs the second cell in +the PHY specifier identifies the PHY and the supported phys for exynos5420 +are: + HDMI_PHY, + ADC_PHY. + +Example: +Simple PHY provider node: + + simplephys: simple-phys@10040000 { + compatible = "samsung,exynos5250-simple-phy"; + samsung,pmu-syscon = <&pmu_system_controller>; + #phy-cells = <1>; + }; + +Other nodes accessing simple PHYs: + + hdmi { + phys = <&simplephys HDMI_PHY>; + phy-names = "hdmiphy"; + }; diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f06..2a13e0d 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -178,4 +178,9 @@ config PHY_XGENE help This option enables support for APM X-Gene SoC multi-purpose PHY. +config EXYNOS_SIMPLE_PHY + tristate "Exynos Simple PHY driver" + help + Support for 1-bit PHY controllers on SoCs from Exynos family. + endmenu diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index b4f1d57..81b6efa 100644 --- a/drivers/phy/Makefile +++ b/drivers/phy/Makefile @@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.o obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o obj-$(CONFIG_PHY_XGENE) += phy-xgene.o +obj-$(CONFIG_EXYNOS_SIMPLE_PHY) += exynos-simple-phy.o diff --git a/drivers/phy/exynos-simple-phy.c b/drivers/phy/exynos-simple-phy.c new file mode 100644 index 0000000..792e9bc --- /dev/null +++ b/drivers/phy/exynos-simple-phy.c @@ -0,0 +1,189 @@ +/* + * Exynos Simple PHY driver + * + * Copyright (C) 2013 Samsung Electronics Co., Ltd. + * Author: Tomasz Stanislawski <t.stanislaws@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/phy/phy.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> + +#include <dt-bindings/phy/exynos-simple-phy.h> + +struct phy_driver_priv { + struct phy *phys[PHY_NR]; + struct regmap *pmureg; + u32 offsets[PHY_NR]; +}; + +struct phy_driver_data { + u32 index; + u32 offset; +}; + +struct phy_private { + struct regmap *pmureg; + u32 offset; +}; + +#define EXYNOS_PHY_ENABLE (1 << 0) + +static int exynos_phy_power_on(struct phy *phy) +{ + struct phy_private *phy_private = phy_get_drvdata(phy); + + return regmap_update_bits(phy_private->pmureg, phy_private->offset, + EXYNOS_PHY_ENABLE, 1); +} + +static int exynos_phy_power_off(struct phy *phy) +{ + struct phy_private *phy_private = phy_get_drvdata(phy); + + return regmap_update_bits(phy_private->pmureg, phy_private->offset, + EXYNOS_PHY_ENABLE, 0); +} + +static struct phy_ops exynos_phy_ops = { + .power_on = exynos_phy_power_on, + .power_off = exynos_phy_power_off, + .owner = THIS_MODULE, +}; + +static const struct phy_driver_data exynos4210_offsets[] = { + { HDMI_PHY, 0x0700 }, /* HDMI_PHY */ + { DAC_PHY, 0x070C }, /* DAC_PHY */ + { ADC_PHY, 0x0718 }, /* ADC_PHY */ + { PCIE_PHY, 0x071C }, /* PCIE_PHY */ + { SATA_PHY, 0x0720 }, /* SATA_PHY */ + { INVALID, 0 }, /* End Mark */ +}; + +static const struct phy_driver_data exynos4412_offsets[] = { + { HDMI_PHY, 0x0700 }, /* HDMI_PHY */ + { ADC_PHY, 0x0718 }, /* ADC_PHY */ + { INVALID, 0 }, /* End Mark */ +}; + +static const struct phy_driver_data exynos5250_offsets[] = { + { HDMI_PHY, 0x0700 }, /* HDMI_PHY */ + { ADC_PHY, 0x0718 }, /* ADC_PHY */ + { SATA_PHY, 0x0724 }, /* SATA_PHY */ + { INVALID, 0 }, /* End Mark */ +}; + +static const struct phy_driver_data exynos5420_offsets[] = { + { HDMI_PHY, 0x0700 }, /* HDMI_PHY */ + { ADC_PHY, 0x0720 }, /* ADC_PHY */ + { INVALID, 0 }, /* End Mark */ +}; + +static const struct of_device_id exynos_phy_of_match[] = { + { .compatible = "samsung,exynos4210-simple-phy", + .data = exynos4210_offsets}, + { .compatible = "samsung,exynos4412-simple-phy", + .data = exynos4412_offsets}, + { .compatible = "samsung,exynos5250-simple-phy", + .data = exynos5250_offsets}, + { .compatible = "samsung,exynos5420-simple-phy", + .data = exynos5420_offsets}, + { }, +}; +MODULE_DEVICE_TABLE(of, exynos_phy_of_match); + +static struct phy *exynos_phy_xlate(struct device *dev, + struct of_phandle_args *args) +{ + struct phy_driver_priv *priv = dev_get_drvdata(dev); + struct phy_private *phy_private; + int index = args->args[0]; + + /* verify if index and corresponding offset are valid */ + if (index >= PHY_NR || priv->offsets[index] == INVALID) + return ERR_PTR(-ENODEV); + + /* return phy if already allocated */ + if (!IS_ERR_OR_NULL(priv->phys[index])) + return priv->phys[index]; + + priv->phys[index] = devm_phy_create(dev, &exynos_phy_ops, NULL); + if (IS_ERR(priv->phys[index])) { + dev_err(dev, "failed to create PHY %d\n", index); + return priv->phys[index]; + } + + phy_private = devm_kzalloc(dev, sizeof(*phy_private), GFP_KERNEL); + if (!phy_private) + return ERR_PTR(-ENOMEM); + + phy_private->pmureg = priv->pmureg; + phy_private->offset = priv->offsets[index]; + phy_set_drvdata(priv->phys[index], phy_private); + + return priv->phys[index]; +} + +static int exynos_phy_probe(struct platform_device *pdev) +{ + const struct of_device_id *of_id = of_match_device( + of_match_ptr(exynos_phy_of_match), &pdev->dev); + const struct phy_driver_data *drv_data = of_id->data; + struct device *dev = &pdev->dev; + struct phy_driver_priv *priv; + struct phy_provider *phy_provider; + int i; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + dev_set_drvdata(dev, priv); + + priv->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, + "samsung,pmu-syscon"); + if (IS_ERR(priv->pmureg)) { + dev_err(dev, "Failed to map PMU register (via syscon)\n"); + return PTR_ERR(priv->pmureg); + } + + /* make all offsets invalid */ + for (i = 0; i < PHY_NR; i++) + priv->offsets[i] = INVALID; + + /* initialize offsets only if available in drv data */ + for (i = 0; drv_data[i].index != INVALID; i++) + priv->offsets[drv_data[i].index] = drv_data[i].offset; + + phy_provider = devm_of_phy_provider_register(dev, exynos_phy_xlate); + if (IS_ERR(phy_provider)) { + dev_err(dev, "failed to register PHY provider\n"); + return PTR_ERR(phy_provider); + } + + dev_info(dev, "probe success\n"); + + return 0; +} + +static struct platform_driver exynos_phy_driver = { + .probe = exynos_phy_probe, + .driver = { + .of_match_table = exynos_phy_of_match, + .name = "exynos-simple-phy", + .owner = THIS_MODULE, + } +}; +module_platform_driver(exynos_phy_driver); + +MODULE_DESCRIPTION("Exynos Simple PHY driver"); +MODULE_AUTHOR("Tomasz Stanislawski <t.stanislaws@samsung.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/include/dt-bindings/phy/exynos-simple-phy.h b/include/dt-bindings/phy/exynos-simple-phy.h new file mode 100644 index 0000000..30bb341 --- /dev/null +++ b/include/dt-bindings/phy/exynos-simple-phy.h @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2013 Samsung Electronics Co., Ltd. + * Author: Tomasz Stanislawski <t.stanislaws@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * Device Tree binding constants for Exynos Simple PHY driver + * + */ + +#ifndef _DT_BINDINGS_PHY_EXYNOS_SIMPLE_PHY_H +#define _DT_BINDINGS_PHY_EXYNOS_SIMPLE_PHY_H + +/* simeple phys */ + +#define INVALID (~1) + +#define HDMI_PHY 0 +#define DAC_PHY 1 +#define ADC_PHY 2 +#define PCIE_PHY 3 +#define SATA_PHY 4 +#define PHY_NR 5 + +#endif