by Rob Herring

[permalink] [raw]

On 08/04/2021 11:13, Chunfeng Yun wrote:
> On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote:
>> On 31/03/21 05:24PM, Chunfeng Yun wrote:
>>> On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote:
>>>> Some platforms like TI's J721E can have the CSI2RX paired with an
>>>> external DPHY. Add support to enable and configure the DPHY using the
>>>> generic PHY framework.
>>>>
>>>> Get the pixel rate and bpp from the subdev and pass them on to the DPHY
>>>> along with the number of lanes. All other settings are left to their
>>>> default values.
>>>>
>>>> Signed-off-by: Pratyush Yadav <[email protected]>
>>>> ---
>>>> drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++--
>>>> 1 file changed, 137 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>>>> index c68a3eac62cd..31bd80e3f780 100644
>>>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>>>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>>>> @@ -30,6 +30,12 @@
>>>> #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4))
>>>> #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8)
>>>>
>>>> +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40
>>>> +#define CSI2RX_DPHY_CL_RST BIT(16)
>>>> +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12)
>>>> +#define CSI2RX_DPHY_CL_EN BIT(4)
>>>> +#define CSI2RX_DPHY_DL_EN(i) BIT(i)
>>>> +
>>>> #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
>>>>
>>>> #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
>>>> @@ -54,6 +60,11 @@ enum csi2rx_pads {
>>>> CSI2RX_PAD_MAX,
>>>> };
>>>>
>>>> +struct csi2rx_fmt {
>>>> + u32 code;
>>>> + u8 bpp;
>>>> +};
>>>> +
>>>> struct csi2rx_priv {
>>>> struct device *dev;
>>>> unsigned int count;
>>>> @@ -85,6 +96,52 @@ struct csi2rx_priv {
>>>> int source_pad;
>>>> };
>>>>
>>>> +static const struct csi2rx_fmt formats[] = {
>>>> + {
>>>> + .code = MEDIA_BUS_FMT_YUYV8_2X8,
>>>> + .bpp = 16,
>>>> + },
>>>> + {
>>>> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
>>>> + .bpp = 16,
>>>> + },
>>>> + {
>>>> + .code = MEDIA_BUS_FMT_YVYU8_2X8,
>>>> + .bpp = 16,
>>>> + },
>>>> + {
>>>> + .code = MEDIA_BUS_FMT_VYUY8_2X8,
>>>> + .bpp = 16,
>>>> + },
>>>> +};
>>>> +
>>>> +static u8 csi2rx_get_bpp(u32 code)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(formats); i++) {
>>>> + if (formats[i].code == code)
>>>> + return formats[i].bpp;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx)
>>>> +{
>>>> + struct v4l2_ctrl *ctrl;
>>>> +
>>>> + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler,
>>>> + V4L2_CID_PIXEL_RATE);
>>>> + if (!ctrl) {
>>>> + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n",
>>>> + csi2rx->source_subdev->name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + return v4l2_ctrl_g_ctrl_int64(ctrl);
>>>> +}
>>>> +
>>>> static inline
>>>> struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
>>>> {
>>>> @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
>>>> writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
>>>> }
>>>>
>>>> +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
>>>> +{
>>>> + union phy_configure_opts opts = { };
>>>> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>>>> + struct v4l2_subdev_format sd_fmt;
>>>> + s64 pixel_rate;
>>>> + int ret;
>>>> + u8 bpp;
>>>> +
>>>> + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>> + sd_fmt.pad = 0;
>>>> +
>>>> + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
>>>> + &sd_fmt);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + bpp = csi2rx_get_bpp(sd_fmt.format.code);
>>>> + if (!bpp)
>>>> + return -EINVAL;
>>>> +
>>>> + pixel_rate = csi2rx_get_pixel_rate(csi2rx);
>>>> + if (pixel_rate < 0)
>>>> + return pixel_rate;
>>>> +
>>>> + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp,
>>>> + csi2rx->num_lanes, cfg);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY,
>>>> + PHY_MIPI_DPHY_SUBMODE_RX);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = phy_power_on(csi2rx->dphy);
>>>> + if (ret)
>>>> + return ret;
>>> Seems phy_power_on, then phy_set_mode_ext?
>>
>> Shouldn't the mode be set before the PHY is powered on so the correct
>> power on procedure can be performed based on the mode of operation?
> Of course, it is fine for cnds-dphy.
> But it depends on HW design and also phy driver;
> if the mode is controlled in PHY IP register, we can't access it before
> phy_power_on if no phy_init called (e.g. clock/power is not enabled).
>
> Just let you pay attention on the phy sequence.

I don't think the phy configuration should depend on phy_power_on, but
the runtime PM.

I guess this could be solved with:

phy_pm_runtime_get_sync();
phy_set_mode_ext();
phy_power_on();
phy_pm_runtime_put();

Tomi