Re: [PATCH 2/4] drm/bridge: add Microchip DSI controller support for sam9x7 SoC series

From: Manikandan.M
Date: Thu Jul 11 2024 - 04:34:17 EST

Hi Krzysztof,

On 04/07/24 4:27 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 04/07/2024 10:48, Manikandan Muralidharan wrote:
>> Add the Microchip's DSI controller wrapper driver that uses
>> the Synopsys DesignWare MIPI DSI host controller bridge.
>>
>> Signed-off-by: Manikandan Muralidharan <manikandan.m@xxxxxxxxxxxxx>
>> ---
>
>
> ...
>
>> +
>> +#define HSTT(_maxfreq, _c_lp2hs, _c_hs2lp, _d_lp2hs, _d_hs2lp) \
>> +{ \
>> + .maxfreq = _maxfreq, \
>> + .timing = { \
>> + .clk_lp2hs = _c_lp2hs, \
>> + .clk_hs2lp = _c_hs2lp, \
>> + .data_lp2hs = _d_lp2hs, \
>> + .data_hs2lp = _d_hs2lp, \
>> + } \
>> +}
>> +
>> +struct hstt hstt_table[] = {
>
> So more globals? No.

In the sam9x7 datasheet, the high speed transition time for data and
clock lane at different freq for the DSI controller ranges are tabulated
with constant values.
I referred other similar platforms for the functionality and found
similar way of implementation, only a few had equations to derive the
low power and high speed timings.I am not able to come up with a more
efficient method. If there is something I am missing, please suggest.
TIA
>
>> + HSTT(90, 32, 20, 26, 13),
>> + HSTT(100, 35, 23, 28, 14),
>> + HSTT(110, 32, 22, 26, 13),
>> + HSTT(130, 31, 20, 27, 13),
>> + HSTT(140, 33, 22, 26, 14),
>> + HSTT(150, 33, 21, 26, 14),
>> + HSTT(170, 32, 20, 27, 13),
>> + HSTT(180, 36, 23, 30, 15),
>> + HSTT(200, 40, 22, 33, 15),
>> + HSTT(220, 40, 22, 33, 15),
>> + HSTT(240, 44, 24, 36, 16),
>> + HSTT(250, 48, 24, 38, 17),
>> + HSTT(270, 48, 24, 38, 17),
>> + HSTT(300, 50, 27, 41, 18),
>> + HSTT(330, 56, 28, 45, 18),
>> + HSTT(360, 59, 28, 48, 19),
>> + HSTT(400, 61, 30, 50, 20),
>> + HSTT(450, 67, 31, 55, 21),
>> + HSTT(500, 73, 31, 59, 22),
>> + HSTT(550, 79, 36, 63, 24),
>> + HSTT(600, 83, 37, 68, 25),
>> + HSTT(650, 90, 38, 73, 27),
>> + HSTT(700, 95, 40, 77, 28),
>> + HSTT(750, 102, 40, 84, 28),
>> + HSTT(800, 106, 42, 87, 30),
>> + HSTT(850, 113, 44, 93, 31),
>> + HSTT(900, 118, 47, 98, 32),
>> + HSTT(950, 124, 47, 102, 34),
>> + HSTT(1000, 130, 49, 107, 35),
>> +};
>> +
>
> ...
>
>> +
>> +static void dw_mipi_dsi_mchp_power_on(void *priv_data)
>> +{
>> + struct dw_mipi_dsi_mchp *dsi = priv_data;
>> +
>> + /* Enable the DSI wrapper */
>> + dsi_write(dsi, DSI_PWR_UP, HOST_PWRUP);
>> +}
>> +
>> +static void dw_mipi_dsi_mchp_power_off(void *priv_data)
>> +{
>> + struct dw_mipi_dsi_mchp *dsi = priv_data;
>> +
>> + /* Disable the DSI wrapper */
>> + dsi_write(dsi, DSI_PWR_UP, HOST_RESET);
>> +}
>> +
>> +struct dw_mipi_dsi_phy_ops dw_mipi_dsi_mchp_phy_ops = {
>
> Why this is not static?
>
> Why this is not const?
>
>> + .init = dw_mipi_dsi_mchp_init,
>> + .power_on = dw_mipi_dsi_mchp_power_on,
>> + .power_off = dw_mipi_dsi_mchp_power_off,
>> + .get_lane_mbps = dw_mipi_dsi_mchp_get_lane_mbps,
>> + .get_timing = dw_mipi_dsi_mchp_get_timing,
>> +};
>> +
>> +static int dw_mipi_dsi_mchp_probe(struct platform_device *pdev)
>> +{
>> + struct dw_mipi_dsi_mchp *dsi;
>> + struct resource *res;
>> + struct regmap *sfr;
>> + const struct dw_mipi_dsi_mchp_chip_data *cdata;
>> + int ret;
>> +
>> + dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>> + if (!dsi)
>> + return -ENOMEM;
>> +
>> + dsi->dev = &pdev->dev;
>> + cdata = of_device_get_match_data(dsi->dev);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + dsi->base = devm_ioremap_resource(&pdev->dev, res);
>
> There is a helper for these two.
>
>> + if (IS_ERR(dsi->base)) {
>> + ret = PTR_ERR(dsi->base);
>> + dev_err(dsi->dev, "Unable to get DSI Base address: %d\n", ret);
>
> return dev_err_probe
>
>> + return ret;
>> + }
>> +
>> + dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
>> + if (IS_ERR(dsi->pclk)) {
>> + ret = PTR_ERR(dsi->pclk);
>> + dev_err(dsi->dev, "Unable to get pclk: %d\n", ret);
>
> return dev_err_probe
>
> You are upstreaming some old code, aren't you?
>
>> + return ret;
>> + }
>> +
>> + dsi->pllref_clk = devm_clk_get(&pdev->dev, "refclk");
>> + if (IS_ERR(dsi->pllref_clk)) {
>> + ret = PTR_ERR(dsi->pllref_clk);
>> + dev_err(dsi->dev, "Unable to get DSI PHY PLL reference clock: %d\n",
>
> return dev_err_probe
>
>
>> + ret);
>> + return ret;
>> + }
>> +
>> + clk_set_rate(dsi->pllref_clk, DSI_PLL_REF_CLK);
>> + if (clk_get_rate(dsi->pllref_clk) != DSI_PLL_REF_CLK) {
>> + dev_err(dsi->dev, "Failed to set DSI PHY PLL reference clock\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = clk_prepare_enable(dsi->pllref_clk);
>
> Enable clock later, so your error paths will be simpler.
>
>> + if (ret) {
>> + dev_err(dsi->dev, "Failed to enable DSI PHY PLL reference clock: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + sfr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "microchip,sfr");
>> + if (IS_ERR_OR_NULL(sfr)) {
>
> NULL? Can it be NULL?
>
>> + ret = PTR_ERR(sfr);
>> + dev_err(dsi->dev, "Failed to get handle on Special Function Register: %d\n",
>> + ret);
>
> ret = dev_err_probe
>
>> + goto err_dsi_probe;
>> + }
>> + /* Select DSI in SFR's ISS Configuration Register */
>> + ret = regmap_write(sfr, SFR_ISS_CFG, ISS_CFG_DSI_MODE);
>> + if (ret) {
>> + dev_err(dsi->dev, "Failed to enable DSI in SFR ISS configuration register: %d\n",
>> + ret);
>> + goto err_dsi_probe;
>> + }
>
>
>
> Best regards,
> Krzysztof
>

--
Thanks and Regards,
Manikandan M.