Re: [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

[Michael Riesch <michael.riesch@collabora.com>]

Hi Jason,

On 8/4/26 07:38, 楊智成 wrote:
> [...]
>> This approach with the "first" variable seems unintuitive. If you need
>> reference counting on the BIAS block, then use a state variable
>> "bias_powered", use a mutex that protects it, and call
>> samsung_mipi_dcphy_bias_block_{en,dis}able (from
>> samsung_mipi_dcphy_power{on, off}) unconditionally. Those methods shall
>> acquire the mutex, check the state variable, {en,dis}able the BIAS
>> block, set the state variable accordingly, release the mutex.
>>
>> [...]
>>
>> Not sure whether you actually need to track the power status of the
>> individual PHYs (maybe you actually need to track the BIAS block status
>> alone) but anyway I would appreciate if
>> "samsung_mipi_dphy_tx_power_{on,off}" would set the "pd->powered"
>> variable.
> 
> Both of these lead to the same change. You are right to be unsure
> about the per-PHY flag - I checked, and its only readers are the two
> "first" computations, which only decide about the shared block. So the
> two flags become one count in the parent, taken and dropped inside
> samsung_mipi_dphy_{tx,rx}_power_{on,off}, and the "first" computations
> go away. I went for a count rather than a single flag because the
> release side has to distinguish "the other direction is still using
> it" from "nobody is left".
> 
> The shared-block helper will use lockdep_assert_held() rather than
> take the mutex itself, since samsung_mipi_dcphy_power_{on,off} already
> hold it, as you noted.
> 
> One thing I am unsure about is the enable/disable pairing. I could not
> find an enable for this block - the four BIAS registers only select
> bias currents, reference voltages, the HS output level and the BGR
> chopper divider, while PLL_CON0 in the same common block does have a
> PLL_EN. If that reading is right, there is nothing to turn off and the
> release side would only drop the count - in which case maybe the
> "enable" name is worth revisiting as well. Does that match your
> understanding of this block?

My understanding of this block comes from the TRM and the downstream
driver. TRM states that the BIAS block is common (= shared between RX
and TX), but Downstream driver does not care and just writes the
settings for RX and TX unconditionally.

There is also something weird: the register description in the GRF
indicates that RX could be in CPHY and TX in DPHY mode. But CPHY and
DPHY require different values for the DCPHY_M0_BIAS_CON4. I'll try to
ask Rockchip people about this.

BTW no objections to renaming this to e.g. "*_configure()" and use a
flag "bool configured;" if required.

> Separately, I noticed "first" also guards the APB reset, and only in
> the transmitter path - once 4/4 enables the receiver, a receiver-first
> power-on does not get one. I plan to move it into the same shared-block
> path so both directions behave the same, and check both power-on
> orders on the board before sending v3.
> 
> I would like to fold all of this into v3 together with whatever comes
> out of the PHY_TYPE_CSI/PHY_TYPE_DSI discussion in patch 1/4, since
> that decides how the two PHYs are told apart.

OK, makes sense. Looking forward to v3!

Thanks and best regards,
Michael

> [...]