[PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver

Thu Mar 7 00:35:13 PST 2013

> -----Original Message-----
> From: 김승우 [mailto:sw0312.kim at samsung.com]
> Sent: Thursday, March 07, 2013 4:04 PM
> To: Rahul Sharma
> Cc: Inki Dae; linux-samsung-soc at vger.kernel.org; Sean Paul; sunil joshi;
> dri-devel at lists.freedesktop.org; Rahul Sharma; sw0312.kim at samsung.com
> Subject: Re: [PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to
> hdmiphy driver
> 
> 
> 
> On 2013년 03월 07일 15:45, Rahul Sharma wrote:
> > Thanks Seung Woo, Mr. Dae,
> >
> > On Thu, Mar 7, 2013 at 10:13 AM, Inki Dae <inki.dae at samsung.com> wrote:
> >> 2013/3/7 김승우 <sw0312.kim at samsung.com>:
> >>>
> >>>
> >>> On 2013년 03월 04일 23:05, Rahul Sharma wrote:
> >>>> Thanks Sean,
> >>>>
> >>>> On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul <seanpaul at google.com>
> wrote:
> >>>>> On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma
> <rahul.sharma at samsung.com> wrote:
> >>>>>> Right now hdmiphy operations and configs are kept inside hdmi
> driver. hdmiphy related
> >>>>>> code is tightly coupled with hdmi ip driver. Physicaly they are
> different devices and
> >>>>>
> >>>>> s/Physicaly/Physically/
> >>>>>
> >>>>>> should be instantiated independently.
> >>>>>>
> >>>>>> In terms of versions/mapping/configurations Hdmi and hdmiphy are
> independent of each
> >>>>>> other. It is preferred to isolate them and maintained
independently.
> >>>>>>
> >>>>>> This implementations is tested for:
> >>>>>> 1) Resolutions supported by exynos4 and 5 hdmi.
> >>>>>> 2) Runtime PM and S2R scenarions for exynos5.
> >>>>>>
> >>>>>
> >>>>> I don't like the idea of spawning off yet another driver in here. It
> >>>>> adds more globals, more suspend/resume ordering issues, and more
> >>>>> implicit dependencies. I understand, however, that this is the
> Chosen
> >>>>> Way for the exynos driver, so I will save my rant.
> >>>>>
> >>>>
> >>>> I agree to it. splitting phy to a new driver will complicate the
> power related
> >>>> scenarios. But in upcoming SoC,s, Phy is changing considerably in
> terms of
> >>>> config values, mapping (i2c/platform bus) etc. Handling this
> diversity
> >>>> inside hdmi driver is complicating it with unrelated changes.
> >>>
> >>> Basically, I agree with the idea to split hdmiphy from hdmi. And it
> >>> seems that already existing hdmiphy i2c device is just reused and
> >>> hdmiphy_power_on is reorganized to hdmiphy dpms operation: even
> calling
> >>> flow of power operations is reordered.
> >>>
> >>> But I'm not sure exynos_hdmiphy_driver_register() really need to be
> >>> called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough
> to
> >>> call exynos_hdmiphy_driver_register() from hdmi_probe() because
> hdmiphy
> >>> is only used from hdmi.
> >>>
> >>
> >> I agree with Seung-Woo. The hdmiphy is just one part of HDMI subsystem.
> >>
> >
> > I agree to the Seung Woo's point that hdmi-phy used to be solely
> accessed by
> > hdmi driver.  But in this RFC, hdmi-phy is not called by hdmi driver
> > anymore. Phy
> > ops will be called from drm-common-hdmi platform driver along with mixer
> and
> > hdmi ops.
> 
> Considering this, exynos_drm_hdmi_probe() is more proper position.
> 
> >
> > The rational behind my implementation is that I am projecting hdmi-phy
> as
> > a device which is peer to hdmi ip and mixer. These 3 devices together
> makes
> > DRM HDMI subsystem.
> >
> > Even physically hdmi-phy doesn't seems to be a part of hdmi ip though
> > configurations are listed under hdmi ip user manual. It looks like a
> > isolated module accessed by i2c.
> >
> > Though I don't find anything wrong with Seung Woo suggestion but above
> > placement of hdmi-phy (parallel to hdmi and mixer) makes more sense
> > to me.
> >
> > Please have a another look at it and let me know your opinion.
> >
> > Another things which bothers me is registering mixer, hdmi driver inside
> > exynos_drm_init(). If we strictly follow the hierarchy inside drm,
> > exynos_drm_init()
> > should register drm-common-hdmi only. drm-common-hdmi should register
> > mixer and hdmi (or hdmi-phy as well).
> 
> Yes, it makes sense. All real hw blocks for hdmi including mixer, hdmi,
> and hdmiphy shoulde be registered in exynos_drm_hdmi (drm-common-hdmi
> for exynos).
> 

Ideally, right. But the reason I designed and implemented hdmi subsystem
framework like this, is that there was one issue that
platform_device_register() can't be called at probe(). On other words, when
one platform device driver is being probed, anyone can't be probed. Anyway,
existing way needs to be improved. So let's find better way and improve the
hdmi subsystem framework this time. :)

Thanks,
Inki Dae

> Thanks and Regards,
> - Seung-Woo Kim
> 
> >
> > regards,
> > Rahul Sharma.
> >
> >> Thanks,
> >> Inki Dae
> >>
> >>> Thanks and Regards,
> >>> - Seung-Woo Kim
> >>>
> >>>>
> >>>> I have tested this RFC for Runtime PM / S2R. But if we see any major
> roadblock
> >>>> we should re-factor this by explicitly calling power related
> callbacks
> >>>> of mixer, phy,
> >>>> hdmi drivers in a required order. We can call them from exynos-drm-
> hdmi plf
> >>>> device. AFAIR something like this is already in place in chrome-
> kernel.
> >>>>
> >>>>> I've made some comments below.
> >>>>>
> >>>>>> This patch is dependent on
> >>>>>> http://www.mail-archive.com/dri-
> devel at lists.freedesktop.org/msg34733.html
> >>>>>> http://www.mail-archive.com/dri-
> devel at lists.freedesktop.org/msg34861.html
> >>>>>> http://www.mail-archive.com/dri-
> devel at lists.freedesktop.org/msg34862.html
> >>>>>>
> >>>>>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
> >>>>>> ---
> >>>>>> It is based on exynos-drm-next-todo branch at
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-
> exynos.git
> >>>>>>
> >>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |   8 +
> >>>>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |   6 +
> >>>>>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c |  58 ++-
> >>>>>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h |  11 +
> >>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c     | 375 ++-----------------
-
> >>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.h     |   1 -
> >>>>>>  drivers/gpu/drm/exynos/exynos_hdmiphy.c  | 586
> ++++++++++++++++++++++++++++++-
> >>>>>>  drivers/gpu/drm/exynos/regs-hdmiphy.h    |  61 ++++
> >>>>>>  8 files changed, 738 insertions(+), 368 deletions(-)
> >>>>>>  create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h
> >>>>>>
> >>>
> >>> <snip>
> >>>
> >>> --
> >>> Seung-Woo Kim
> >>> Samsung Software R&D Center
> >>> --
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel at lists.freedesktop.org
> >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> 
> --
> Seung-Woo Kim
> Samsung Software R&D Center
> --