| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <96f8310f-93f1-4bcb-8637-137e1159ff83@rock-chips.com>
Date: Tue, 7 Jan 2025 11:02:15 +0800
From: Damon Ding <damon.ding@...k-chips.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: heiko@...ech.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, rfoss@...nel.org, vkoul@...nel.org,
sebastian.reichel@...labora.com, cristian.ciocaltea@...labora.com,
l.stach@...gutronix.de, andy.yan@...k-chips.com, hjc@...k-chips.com,
algea.cao@...k-chips.com, kever.yang@...k-chips.com,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org
Subject: Re: [PATCH v4 07/17] phy: phy-rockchip-samsung-hdptx: Add support for
eDP mode
Hi Dmitry,
On 2024/12/30 20:45, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 02:33:03PM +0800, Damon Ding wrote:
>> Add basic support for RBR/HBR/HBR2 link rates, and the voltage swing and
>> pre-emphasis configurations of each link rate have been verified according
>> to the eDP 1.3 requirements.
>
> Well... Please describe what's happening here. That the HDMI PHY on your
> platform also provides support for DP / eDP. Please document any design
> decisions that you had to make.
>
Yes, I will add more relevant descriptions in the next version.
>>
>> Signed-off-by: Damon Ding <damon.ding@...k-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - Add the module author
>>
>> Changes in v3:
>> - Split this patch into two, one for correction and the other for
>> extension
>>
>> Changes in v4:
>> - Add link_rate and lanes parameters in struct rk_hdptx_phy to store the
>> phy_configure() result for &phy_configure_opts.dp.link_rate and
>> &phy_configure_opts.dp.lanes
>> ---
>> .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 896 +++++++++++++++++-
>> 1 file changed, 889 insertions(+), 7 deletions(-)
>>
>> @@ -933,9 +1484,339 @@ static int rk_hdptx_phy_power_off(struct phy *phy)
>> return rk_hdptx_phy_consumer_put(hdptx, false);
>> }
>>
>> +static int rk_hdptx_phy_set_mode(struct phy *phy, enum phy_mode mode,
>> + int submode)
>> +{
>> + return 0;
>> +}
>
> No need for the stub, please drop it. The host controller driver can
> still call phy_set_mode() / _ext(), the call will return 0.
Without the &phy_ops.set_mode(), the phy driver can not get phy_mode to
distinguish between HDMI and DP mode via the phy_get_mode(), even if the
host driver calls phy_set_mode() / _ext(). Additionally, the previous
discussion [0] also mentioned future considerations for dynamic
switching. Indeed, I should add a related comment before the 'return 0;'
to enhance understandability.
>
>> +
>> +static int rk_hdptx_phy_verify_config(struct rk_hdptx_phy *hdptx,
>> + struct phy_configure_opts_dp *dp)
>> +{
>> + int i;
>> +
>> + if (dp->set_rate) {
>> + switch (dp->link_rate) {
>> + case 1620:
>> + case 2700:
>> + case 5400:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + if (dp->set_lanes) {
>> + switch (dp->lanes) {
>> + case 0:
>
> Is it really a valid config to have 0 lanes?
The case of 0 lanes is indeed unnecessary.
>
>> + case 1:
>> + case 2:
>> + case 4:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + if (dp->set_voltages) {
>> + for (i = 0; i < hdptx->lanes; i++) {
>> + if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>> + return -EINVAL;
>> +
>> + if (dp->voltage[i] + dp->pre[i] > 3)
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> [..]
>
>> +
>> +static int rk_hdptx_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>> +{
>> + struct rk_hdptx_phy *hdptx = phy_get_drvdata(phy);
>> + enum phy_mode mode = phy_get_mode(phy);
>> + int ret;
>> +
>> + if (mode != PHY_MODE_DP)
>> + return -EINVAL;
>
> I'd say, return 0;
Yes, it is really not an error.
>
>> +
>> + ret = rk_hdptx_phy_verify_config(hdptx, &opts->dp);
>> + if (ret) {
>> + dev_err(hdptx->dev, "invalid params for phy configure\n");
>> + return ret;
>> + }
>> +
>> + if (opts->dp.set_rate) {
>> + ret = rk_hdptx_phy_set_rate(hdptx, &opts->dp);
>> + if (ret) {
>> + dev_err(hdptx->dev, "failed to set rate: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + if (opts->dp.set_lanes) {
>> + ret = rk_hdptx_phy_set_lanes(hdptx, &opts->dp);
>> + if (ret) {
>> + dev_err(hdptx->dev, "failed to set lanes: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + if (opts->dp.set_voltages) {
>> + ret = rk_hdptx_phy_set_voltages(hdptx, &opts->dp);
>> + if (ret) {
>> + dev_err(hdptx->dev, "failed to set voltages: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static const struct phy_ops rk_hdptx_phy_ops = {
>> .power_on = rk_hdptx_phy_power_on,
>> .power_off = rk_hdptx_phy_power_off,
>> + .set_mode = rk_hdptx_phy_set_mode,
>> + .configure = rk_hdptx_phy_configure,
>> .owner = THIS_MODULE,
>> };
>>
>> @@ -1149,5 +2030,6 @@ module_platform_driver(rk_hdptx_phy_driver);
>>
>> MODULE_AUTHOR("Algea Cao <algea.cao@...k-chips.com>");
>> MODULE_AUTHOR("Cristian Ciocaltea <cristian.ciocaltea@...labora.com>");
>> +MODULE_AUTHOR("Damon Ding <damon.ding@...k-chips.com>");
>> MODULE_DESCRIPTION("Samsung HDMI/eDP Transmitter Combo PHY Driver");
>> MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
>
Best regards
Damon
[0]https://patchwork.kernel.org/project/linux-rockchip/patch/20241127075157.856029-5-damon.ding@rock-chips.com/
Powered by blists - more mailing lists