lkml.org 
[lkml]   [2018]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 02/10] phy: Add configuration interface
    From
    Date
    Hi Maxime,

    On Wednesday 12 September 2018 02:12 PM, Maxime Ripard wrote:
    > Hi!
    >
    > On Wed, Sep 12, 2018 at 01:12:31PM +0530, Kishon Vijay Abraham I wrote:
    >> On Thursday 06 September 2018 08:26 PM, Maxime Ripard wrote:
    >>> Hi Kishon,
    >>>
    >>> On Thu, Sep 06, 2018 at 02:57:58PM +0530, Kishon Vijay Abraham I wrote:
    >>>> On Wednesday 05 September 2018 02:46 PM, Maxime Ripard wrote:
    >>>>> The phy framework is only allowing to configure the power state of the PHY
    >>>>> using the init and power_on hooks, and their power_off and exit
    >>>>> counterparts.
    >>>>>
    >>>>> While it works for most, simple, PHYs supported so far, some more advanced
    >>>>> PHYs need some configuration depending on runtime parameters. These PHYs
    >>>>> have been supported by a number of means already, often by using ad-hoc
    >>>>> drivers in their consumer drivers.
    >>>>>
    >>>>> That doesn't work too well however, when a consumer device needs to deal
    >>>>> multiple PHYs, or when multiple consumers need to deal with the same PHY (a
    >>>>> DSI driver and a CSI driver for example).
    >>>>>
    >>>>> So we'll add a new interface, through two funtions, phy_validate and
    >>>>> phy_configure. The first one will allow to check that a current
    >>>>> configuration, for a given mode, is applicable. It will also allow the PHY
    >>>>> driver to tune the settings given as parameters as it sees fit.
    >>>>>
    >>>>> phy_configure will actually apply that configuration in the phy itself.
    >>>>>
    >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
    >>>>> ---
    >>>>> drivers/phy/phy-core.c | 62 ++++++++++++++++++++++++++++++++++++++++++-
    >>>>> include/linux/phy/phy.h | 42 ++++++++++++++++++++++++++++-
    >>>>> 2 files changed, 104 insertions(+)
    >>>>>
    >>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
    >>>>> index 35fd38c5a4a1..6eaf655e370f 100644
    >>>>> --- a/drivers/phy/phy-core.c
    >>>>> +++ b/drivers/phy/phy-core.c
    >>>>> @@ -408,6 +408,68 @@ int phy_calibrate(struct phy *phy)
    >>>>> EXPORT_SYMBOL_GPL(phy_calibrate);
    >>>>>
    >>>>> /**
    >>>>> + * phy_configure() - Changes the phy parameters
    >>>>> + * @phy: the phy returned by phy_get()
    >>>>> + * @mode: phy_mode the configuration is applicable to.
    >>>>
    >>>> mode should be used if the same PHY can be configured in multiple modes. But
    >>>> with phy_set_mode() and phy_calibrate() we could achieve the same.
    >>>
    >>> So you would change the prototype to have a configuration applying
    >>> only to the current mode set previously through set_mode?
    >>
    >> yeah.
    >> With phy_configure, if the PHY is not in @mode, it should return an error? Or
    >> will it set the PHY to @mode and apply the configuration in @opts?
    >
    > I wanted to have it return an error either if it was configured in
    > another mode or if the mode was unsupported yes.
    >
    >>> Can we have PHY that operate in multiple modes at the same time?
    >>
    >> Not at the same time. But the same PHY can operate in multiple modes (For
    >> example we have PHYs that can be used either with PCIe or USB3)
    >
    > Ok, that makes sense. I guess we could rely on phy_set_mode then if
    > you prefer.
    >
    >>>>> + * @opts: New configuration to apply
    >>>>
    >>>> Should these configuration come from the consumer driver?
    >>>
    >>> Yes
    >>
    >> How does the consumer driver get these configurations? Is it from user space or
    >> dt associated with consumer device.
    >
    > It really depends on multiple factors (and I guess on what mode the
    > PHY is actually supposed to support), but in the case covered by this
    > serie, the info mostly come from multiple places:
    > - The resolutions supported by the panel
    > - The resolutions supported by the phy consumer (and its
    > integration, for things like the clock rates it can output)
    > - The resolutions and timings supported by the phy itself (once
    > again, the integration is mostly involved here since it really
    > only depends on which clock rates can be achieved)
    > - The timings boundaries that the specification has
    > - The resolution selected by the user
    >
    > So we'd have that information coming from multiple places: the
    > userspace would select the resolution, drivers would be able to filter
    > out unsupported resolutions, and the DT will provide the integration
    > details to help them do so.
    >
    > But I guess from an API standpoint, it really is expected to be
    > assembled by the phy consumer driver.
    >
    >>>>> +/**
    >>>>> + * phy_validate() - Checks the phy parameters
    >>>>> + * @phy: the phy returned by phy_get()
    >>>>> + * @mode: phy_mode the configuration is applicable to.
    >>>>> + * @opts: Configuration to check
    >>>>> + *
    >>>>> + * Used to check that the current set of parameters can be handled by
    >>>>> + * the phy. Implementations are free to tune the parameters passed as
    >>>>> + * arguments if needed by some implementation detail or
    >>>>> + * constraints. It will not change any actual configuration of the
    >>>>> + * PHY, so calling it as many times as deemed fit will have no side
    >>>>> + * effect.
    >>>>> + *
    >>>>> + * Returns: 0 if successful, an negative error code otherwise
    >>>>> + */
    >>>>> +int phy_validate(struct phy *phy, enum phy_mode mode,
    >>>>> + union phy_configure_opts *opts)
    >>>>
    >>>> IIUC the consumer driver will pass configuration options (or PHY parameters)
    >>>> which will be validated by the PHY driver and in some cases the PHY driver can
    >>>> modify the configuration options? And these modified configuration options will
    >>>> again be given to phy_configure?
    >>>>
    >>>> Looks like it's a round about way of doing the same thing.
    >>>
    >>> Not really. The validate callback allows to check whether a particular
    >>> configuration would work, and try to negotiate a set of configurations
    >>> that both the consumer and the PHY could work with.
    >>
    >> Maybe the PHY should provide the list of supported features to the consumer
    >> driver and the consumer should select a supported feature?
    >
    > It's not really about the features it supports, but the boundaries it
    > might have on those features. For example, the same phy integrated in
    > two different SoCs will probably have some limit on the clock rate it
    > can output because of the phy design itself, but also because of the
    > clock that is fed into that phy, and that will be different from one
    > SoC to the other.
    >
    > This integration will prevent us to use some clock rates on the first
    > SoC, while the second one would be totally fine with it.

    If there's a clock that is fed to the PHY from the consumer, then the consumer
    driver should model a clock provider and the PHY can get a reference to it
    using clk_get(). Rockchip and Arasan eMMC PHYs has already used something like
    that.

    Assuming the PHY can get a reference to the clock provided by the consumer,
    what are the parameters we'll be able to get rid of in struct
    phy_configure_opts_mipi_dphy?

    I'm sorry but I'm not convinced a consumer driver should have all the details
    that are added in phy_configure_opts_mipi_dphy.
    >
    > Obviously, the consumer driver shouldn't care about the phy
    > integration details, especially since some of those consumer drivers
    > need to interact with multiple phy designs (or the same phy design can
    > be used by multiple consumers).
    >
    > So knowing that a feature is supported is really not enough.
    >
    > With MIPI-DPHY at least, the API is generic enough so that another
    > mode where the features would make sense could implement a feature
    > flag if that makes sense.
    >
    >>> For example, DRM requires this to filter out display modes (ie,
    >>> resolutions) that wouldn't be achievable by the PHY so that it's never
    >>
    >> Can't the consumer driver just tell the required resolution to the PHY and PHY
    >> figuring out all the parameters for the resolution or an error if that
    >> resolution cannot be supported?
    >
    > Not really either. With MIPI D-PHY, the phy is fed a clock that is
    > generated by the phy consumer, which might or might not be an exact
    > fit for the resolution. There's so many resolutions that in most case,
    > the clock factors don't allow you to have a perfect match. And
    > obviously, this imprecision should be taken into account by the PHY as
    > well.
    >
    > And then, there's also the matter than due to design constraints, some
    > consumers would have fixed timings that are not at the spec default
    > value, but still within the acceptable range. We need to communicate
    > that to the PHY.

    Here do you mean videomode timings?

    Thanks
    Kishon

    \
     
     \ /
      Last update: 2018-09-14 10:50    [W:2.069 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site