| Message ID | 20250212-mst_qcs8300-v1-1-38a8aa08394b@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MST support for qcs8300 platform | expand |
On 12/02/2025 08:12, Yongxing Mou wrote: > We need to enable mst for qcs8300, dp0 controller will support 2 streams > output. So not reuse sm8650 dp controller driver and will add a new driver > patch for qcs8300 mst feature. Modify the corresponding dt-bingding file > to compatible with the qcs8300-dp. > > Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> NAK. You just said qcs8300 is compatible with sm8650. I did not ask about drivers, I asked about hardware. This is messy approach. Describe properly the hardware first, instead of sending two conflicting patchsets. Best regards, Krzysztof
On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: > We need to enable mst for qcs8300, dp0 controller will support 2 streams > output. So not reuse sm8650 dp controller driver and will add a new driver > patch for qcs8300 mst feature. Modify the corresponding dt-bingding file > to compatible with the qcs8300-dp. NAK for a different reason: QCS8300 is still compatible with SM8650. Enable features instead of randomly reshuffle compats. In this case, enable MST for both architectures. > > Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index 359e364d79b20469d41cd8416a55b6a5d5c7d8ce..59075d7f05147f1f477f236a76fee6ec5d8c5ad8 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -18,6 +18,7 @@ properties: > compatible: > oneOf: > - enum: > + - qcom,qcs8300-dp > - qcom,sa8775p-dp > - qcom,sc7180-dp > - qcom,sc7280-dp > @@ -37,10 +38,6 @@ properties: > - qcom,sm8450-dp > - qcom,sm8550-dp > - const: qcom,sm8350-dp > - - items: > - - enum: > - - qcom,qcs8300-dp > - - const: qcom,sm8650-dp > > reg: > minItems: 4 > > -- > 2.34.1 >
On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: > We need to enable mst for qcs8300, dp0 controller will support 2 streams > output. So not reuse sm8650 dp controller driver and will add a new driver > patch for qcs8300 mst feature. Modify the corresponding dt-bingding file > to compatible with the qcs8300-dp. Forgot to mention that in the quick response: please fix usage of capital or lowercase letters in the commit message. If you are unusure, 'git log' will help you. > > Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) >
On 12/02/2025 11:41, Dmitry Baryshkov wrote: > On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: >> We need to enable mst for qcs8300, dp0 controller will support 2 streams >> output. So not reuse sm8650 dp controller driver and will add a new driver >> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >> to compatible with the qcs8300-dp. > > NAK for a different reason: QCS8300 is still compatible with SM8650. > Enable features instead of randomly reshuffle compats. In this case, > enable MST for both architectures. > So the original patch was probably correct... Best regards, Krzysztof
On 2025/2/12 16:35, Krzysztof Kozlowski wrote: > On 12/02/2025 08:12, Yongxing Mou wrote: >> We need to enable mst for qcs8300, dp0 controller will support 2 streams >> output. So not reuse sm8650 dp controller driver and will add a new driver >> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >> to compatible with the qcs8300-dp. >> >> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> > NAK. You just said qcs8300 is compatible with sm8650. I did not ask > about drivers, I asked about hardware. > > This is messy approach. Describe properly the hardware first, instead of > sending two conflicting patchsets. > > Best regards, > Krzysztof Hi, Krzysztof, thanks for reviewing, i want to explain why i submitted this patch. Patch https://lore.kernel.org/all/20250114-dts_qcs8300-v3-1-d114cc5e4af9@quicinc.com/ and https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-2-1687e7842125@quicinc.com/ is the qcs8300 display enablement changes. It base on current linux base code and it only support SST mode, so in the SST mode, qcs8300 dp controller driver is quite same with sm8650, struct msm_dp_desc only have 3 members(io_start, id and wide_bus_supported) and they are same both in qcs8300 and sm8650, so we reuse it. BTW, for dp phy hardware version, qcs8300 and sm8650 is different. For this patch series, https://lore.kernel.org/all/20250212-mst_qcs8300-v1-0-38a8aa08394b@quicinc.com/ , it is made on top of https://lore.kernel.org/all/20241205-dp_mst-v1-0-f8618d42a99a@quicinc.com/ which is a new feature for msm platform and not fully reviewed all the code. Currently patch series enable the MST feature for qcs8300, so the dp controller need to support stream 1, and we need add max_streams and intf_map in the struct msm_dp_desc to support MST. So we don't reuse the sm8650 drivers in this patch series. For my understanding, qcs8300 SST changes should merge first(https://lore.kernel.org/all/20250114-dts_qcs8300-v3-0-d114cc5e4af9@quicinc.com/ and https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-0-1687e7842125@quicinc.com/), and next is the MST base commits (https://lore.kernel.org/all/20241205-dp_mst-v1-0-f8618d42a99a@quicinc.com/), and this patch series should merge at last. why i post qcs8300 enablement changes first and then post the MST changes instead of post them together? Because our dependency, i mean this patch series (https://lore.kernel.org/all/20241205-dp_mst-v1-0-f8618d42a99a@quicinc.com/), it has about 45 changes, so it might take lots of time to merge. we just want to make sure that display can work on DP SST mode first. So I am just understand that the previous commit was not an incorrect commit, but rather two functionalities were changed in the same place. Thanks, Yongxing
On 12/02/2025 12:13, Yongxing Mou wrote: > > > On 2025/2/12 16:35, Krzysztof Kozlowski wrote: >> On 12/02/2025 08:12, Yongxing Mou wrote: >>> We need to enable mst for qcs8300, dp0 controller will support 2 streams >>> output. So not reuse sm8650 dp controller driver and will add a new driver >>> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >>> to compatible with the qcs8300-dp. >>> >>> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> >> NAK. You just said qcs8300 is compatible with sm8650. I did not ask >> about drivers, I asked about hardware. >> >> This is messy approach. Describe properly the hardware first, instead of >> sending two conflicting patchsets. >> >> Best regards, >> Krzysztof > > Hi, Krzysztof, thanks for reviewing, i want to explain why i submitted > this patch. Patch > https://lore.kernel.org/all/20250114-dts_qcs8300-v3-1-d114cc5e4af9@quicinc.com/ > and > https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-2-1687e7842125@quicinc.com/ > is the qcs8300 display enablement changes. It base on current linux base > code and it only support SST mode, so in the SST mode, qcs8300 dp > controller driver is quite same with sm8650, struct msm_dp_desc only > have 3 members(io_start, id and wide_bus_supported) and they are same > both in qcs8300 and sm8650, so we reuse it. BTW, for dp phy hardware > version, qcs8300 and sm8650 is different. No. In one patchset you claim hardware is like that, in other patchset you say hardware is different. Sorry, hardware does not change based on your patchsets. Sort out this before posting new versions. Best regards, Krzysztof
On Wed, 12 Feb 2025 at 12:54, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 12/02/2025 11:41, Dmitry Baryshkov wrote: > > On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: > >> We need to enable mst for qcs8300, dp0 controller will support 2 streams > >> output. So not reuse sm8650 dp controller driver and will add a new driver > >> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file > >> to compatible with the qcs8300-dp. > > > > NAK for a different reason: QCS8300 is still compatible with SM8650. > > Enable features instead of randomly reshuffle compats. In this case, > > enable MST for both architectures. > > > So the original patch was probably correct... I have no idea. I'd let Yongxing Mou to comment on this. It would be nice instead of getting a lengthy explanation of obvious items to get an answer to an actual question: is QCS8300 compatible with SM8650 or not. In other words whether they can support the same number of MST streams on each controller or not.
On 12.02.2025 12:28 PM, Krzysztof Kozlowski wrote: > On 12/02/2025 12:13, Yongxing Mou wrote: >> >> >> On 2025/2/12 16:35, Krzysztof Kozlowski wrote: >>> On 12/02/2025 08:12, Yongxing Mou wrote: >>>> We need to enable mst for qcs8300, dp0 controller will support 2 streams >>>> output. So not reuse sm8650 dp controller driver and will add a new driver >>>> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >>>> to compatible with the qcs8300-dp. >>>> >>>> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> >>> NAK. You just said qcs8300 is compatible with sm8650. I did not ask >>> about drivers, I asked about hardware. >>> >>> This is messy approach. Describe properly the hardware first, instead of >>> sending two conflicting patchsets. >>> >>> Best regards, >>> Krzysztof >> >> Hi, Krzysztof, thanks for reviewing, i want to explain why i submitted >> this patch. Patch >> https://lore.kernel.org/all/20250114-dts_qcs8300-v3-1-d114cc5e4af9@quicinc.com/ >> and >> https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-2-1687e7842125@quicinc.com/ >> is the qcs8300 display enablement changes. It base on current linux base >> code and it only support SST mode, so in the SST mode, qcs8300 dp >> controller driver is quite same with sm8650, struct msm_dp_desc only >> have 3 members(io_start, id and wide_bus_supported) and they are same >> both in qcs8300 and sm8650, so we reuse it. BTW, for dp phy hardware >> version, qcs8300 and sm8650 is different. > > No. In one patchset you claim hardware is like that, in other patchset > you say hardware is different. > > Sorry, hardware does not change based on your patchsets. > > Sort out this before posting new versions. In other words, fallback compatibles must be chosen with features that are present in hardware, but not yet supported upstream in mind. It's totally fine (and even preferred/expected) to describe hardware resources (such as MST clocks here) when initially creating bindings for a piece of hw, even though the drivers don't use them yet at that moment. dt-bindings are supposed to give the OS a complete representation of the hardware and ideally be immutable (which is a struggle, but we're getting better). Driver specifics should not influence your decisions (or at least do so very minimally) when adding these. Now you're in a """good""" position as the display bindings haven't been merged yet, so you can still upload a new patchset where the description is more accurate. If it was merged, we'd have to break the ABI or add some crazy workarounds.. Please coalesce this patchset with the "add 8300 display support" one. Please also describe all 4 MST clocks and whatever other clocks/resets that may be necessary down the line. Konrad
On 2025/2/12 21:44, Konrad Dybcio wrote: > On 12.02.2025 12:28 PM, Krzysztof Kozlowski wrote: >> On 12/02/2025 12:13, Yongxing Mou wrote: >>> >>> >>> On 2025/2/12 16:35, Krzysztof Kozlowski wrote: >>>> On 12/02/2025 08:12, Yongxing Mou wrote: >>>>> We need to enable mst for qcs8300, dp0 controller will support 2 streams >>>>> output. So not reuse sm8650 dp controller driver and will add a new driver >>>>> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >>>>> to compatible with the qcs8300-dp. >>>>> >>>>> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> >>>> NAK. You just said qcs8300 is compatible with sm8650. I did not ask >>>> about drivers, I asked about hardware. >>>> >>>> This is messy approach. Describe properly the hardware first, instead of >>>> sending two conflicting patchsets. >>>> >>>> Best regards, >>>> Krzysztof >>> >>> Hi, Krzysztof, thanks for reviewing, i want to explain why i submitted >>> this patch. Patch >>> https://lore.kernel.org/all/20250114-dts_qcs8300-v3-1-d114cc5e4af9@quicinc.com/ >>> and >>> https://lore.kernel.org/all/20250120-mdssdt_qcs8300-v4-2-1687e7842125@quicinc.com/ >>> is the qcs8300 display enablement changes. It base on current linux base >>> code and it only support SST mode, so in the SST mode, qcs8300 dp >>> controller driver is quite same with sm8650, struct msm_dp_desc only >>> have 3 members(io_start, id and wide_bus_supported) and they are same >>> both in qcs8300 and sm8650, so we reuse it. BTW, for dp phy hardware >>> version, qcs8300 and sm8650 is different. >> >> No. In one patchset you claim hardware is like that, in other patchset >> you say hardware is different. >> >> Sorry, hardware does not change based on your patchsets. >> >> Sort out this before posting new versions. > > In other words, fallback compatibles must be chosen with features that > are present in hardware, but not yet supported upstream in mind. > > It's totally fine (and even preferred/expected) to describe hardware resources > (such as MST clocks here) when initially creating bindings for a piece of hw, > even though the drivers don't use them yet at that moment. > > dt-bindings are supposed to give the OS a complete representation of the > hardware and ideally be immutable (which is a struggle, but we're getting > better). Driver specifics should not influence your decisions (or at least > do so very minimally) when adding these. > > Now you're in a """good""" position as the display bindings haven't been merged > yet, so you can still upload a new patchset where the description is more > accurate. If it was merged, we'd have to break the ABI or add some crazy > workarounds.. > > Please coalesce this patchset with the "add 8300 display support" one. > > Please also describe all 4 MST clocks and whatever other clocks/resets that > may be necessary down the line. > > Konrad Thanks, will update it in this patch "add 8300 display support". Because this will depend on this change: https://patchwork.freedesktop.org/series/142016/. we will first fix our dependecy comments and then repost it.
On 2025/2/12 20:26, Dmitry Baryshkov wrote: > On Wed, 12 Feb 2025 at 12:54, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 12/02/2025 11:41, Dmitry Baryshkov wrote: >>> On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: >>>> We need to enable mst for qcs8300, dp0 controller will support 2 streams >>>> output. So not reuse sm8650 dp controller driver and will add a new driver >>>> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >>>> to compatible with the qcs8300-dp. >>> >>> NAK for a different reason: QCS8300 is still compatible with SM8650. >>> Enable features instead of randomly reshuffle compats. In this case, >>> enable MST for both architectures. >>> >> So the original patch was probably correct... > > I have no idea. I'd let Yongxing Mou to comment on this. It would be > nice instead of getting a lengthy explanation of obvious items to get > an answer to an actual question: is QCS8300 compatible with SM8650 or > not. In other words whether they can support the same number of MST > streams on each controller or not. > Hi, in hardware, the SM8650's DP controller supports 2 INTFs, while the QCS8300's DP0 controller supports 4 INTFs.In sst mode, only one INTF will be used, they are same, but for MST, sm8650 supports 2 streams while qcs8300 support 4 streams. Thanks.
On 2025/2/12 18:42, Dmitry Baryshkov wrote: > On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: >> We need to enable mst for qcs8300, dp0 controller will support 2 streams >> output. So not reuse sm8650 dp controller driver and will add a new driver >> patch for qcs8300 mst feature. Modify the corresponding dt-bingding file >> to compatible with the qcs8300-dp. > > Forgot to mention that in the quick response: please fix usage of > capital or lowercase letters in the commit message. If you are unusure, > 'git log' will help you. > >> >> Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> >> --- >> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> > Got it , thanks. Will correct it in new patch.
On Wed, Feb 19, 2025 at 05:56:14PM +0800, Yongxing Mou wrote: > > > On 2025/2/12 20:26, Dmitry Baryshkov wrote: > > On Wed, 12 Feb 2025 at 12:54, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > On 12/02/2025 11:41, Dmitry Baryshkov wrote: > > > > On Wed, Feb 12, 2025 at 03:12:24PM +0800, Yongxing Mou wrote: > > > > > We need to enable mst for qcs8300, dp0 controller will support 2 streams > > > > > output. So not reuse sm8650 dp controller driver and will add a new driver > > > > > patch for qcs8300 mst feature. Modify the corresponding dt-bingding file > > > > > to compatible with the qcs8300-dp. > > > > > > > > NAK for a different reason: QCS8300 is still compatible with SM8650. > > > > Enable features instead of randomly reshuffle compats. In this case, > > > > enable MST for both architectures. > > > > > > > So the original patch was probably correct... > > > > I have no idea. I'd let Yongxing Mou to comment on this. It would be > > nice instead of getting a lengthy explanation of obvious items to get > > an answer to an actual question: is QCS8300 compatible with SM8650 or > > not. In other words whether they can support the same number of MST > > streams on each controller or not. > > > Hi, in hardware, the SM8650's DP controller supports 2 INTFs, while the > QCS8300's DP0 controller supports 4 INTFs.In sst mode, only one INTF will be > used, they are same, but for MST, sm8650 supports 2 streams while qcs8300 > support 4 streams. Thanks. So, they are not compatible. Please use separate compatible.
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 359e364d79b20469d41cd8416a55b6a5d5c7d8ce..59075d7f05147f1f477f236a76fee6ec5d8c5ad8 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -18,6 +18,7 @@ properties: compatible: oneOf: - enum: + - qcom,qcs8300-dp - qcom,sa8775p-dp - qcom,sc7180-dp - qcom,sc7280-dp @@ -37,10 +38,6 @@ properties: - qcom,sm8450-dp - qcom,sm8550-dp - const: qcom,sm8350-dp - - items: - - enum: - - qcom,qcs8300-dp - - const: qcom,sm8650-dp reg: minItems: 4
We need to enable mst for qcs8300, dp0 controller will support 2 streams output. So not reuse sm8650 dp controller driver and will add a new driver patch for qcs8300 mst feature. Modify the corresponding dt-bingding file to compatible with the qcs8300-dp. Signed-off-by: Yongxing Mou <quic_yongmou@quicinc.com> --- Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)