From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3327DC7EE37 for ; Thu, 8 Jun 2023 13:49:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=V6LipUg92nVI0lWt6WKtzlHRtu9dITcHtKh7R9Otq+U=; b=dn4d8zIAx2+aO2zJHl7qa2EjAw WPK8igx0w5FI64AjxFVcuTARMwYa5a7lZP4n3pVnAWTMeW2UsHTKoBZivMIZgqPyPCkOqhvtWjpzy Z6r5KcaAuiGDTW8AMljrDdDzoZA3WnZe9xJpKTHbP5ahRz0DTI2R5FcCygjAZibuF2+w3KqEvUVUj k6+HRsTnMZZKz5mIX3GUsUwEeH4hcA99g9uR0NyqNY9NZfXwVvJgFfqpOWfD+z9UvPYNap6u4JI58 wylORPDZwfJfwbNt1t4vtFKO+h2xJb/fWhqnW5m2xskT9oD/Yzm/jfJADxIQZykRfN1YH5MJk/9Tn tEPpRg9w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q7G0r-009Vsv-0c; Thu, 08 Jun 2023 13:49:17 +0000 Received: from mail-io1-f53.google.com ([209.85.166.53]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q7G0o-009VrM-0E; Thu, 08 Jun 2023 13:49:15 +0000 Received: by mail-io1-f53.google.com with SMTP id ca18e2360f4ac-77acb944bdfso22387039f.0; Thu, 08 Jun 2023 06:49:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686232151; x=1688824151; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=V6LipUg92nVI0lWt6WKtzlHRtu9dITcHtKh7R9Otq+U=; b=ippHyIn1q49MA9lX+3JfYATnuBf/loW5Vx9+SsNid6MbGd/WI3q8Y6I0i763jfyVNk GW6mMtGqHZ9iF5uF6W0Psd+zE7LhqLjJ5PBjtsBlSuObrqtCDioa7wVjtI2M8uQkDNk3 OaPV/n1QU0l+BKIrXZu440FYFWqlrzmhZSYSq9lab61R1VJAbORo8dbwUSib5hnIgoOn yIFecASKQqR+fFxIlqW+g3FcS9TItK3C+IFeK+og1TUz3OgYXCNC3s3hpL4LVYVu83w8 J8tVlihpE/q5CL41xpJ/Qgmp0Qf3oYGP3uUbeQNGmSc/gsJnxrR2AFZMDOdDc5JK5Q6j 0yDw== X-Gm-Message-State: AC+VfDxM7BCnpvk2gMh692SpLdxTa/c6dmqpJ4qmRVBw7Z8eoHpc21dq FVo20/zImB2LO3cLah2TrAFXwNl2wQ== X-Google-Smtp-Source: ACHHUZ5nOHAcbMwWIfFekipe5EvJXvai7cR/i6yNTIG5gTlkQTOFwBj2u3/RcAGVuQNZWItYWIHadQ== X-Received: by 2002:a5e:c74d:0:b0:776:fc29:d965 with SMTP id g13-20020a5ec74d000000b00776fc29d965mr11721408iop.10.1686232151090; Thu, 08 Jun 2023 06:49:11 -0700 (PDT) Received: from robh_at_kernel.org ([64.188.179.250]) by smtp.gmail.com with ESMTPSA id k4-20020a02a704000000b004168295d33esm294154jam.47.2023.06.08.06.49.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jun 2023 06:49:10 -0700 (PDT) Received: (nullmailer pid 2476705 invoked by uid 1000); Thu, 08 Jun 2023 13:49:08 -0000 Date: Thu, 8 Jun 2023 07:49:08 -0600 From: Rob Herring To: Julien Stephan Cc: Krzysztof Kozlowski , Kevin Hilman , chunkuang.hu@kernel.org, linux-mediatek@lists.infradead.org, Florian Sylvestre , Chunfeng Yun , Andy Hsieh , Vinod Koul , Kishon Vijay Abraham I , Krzysztof Kozlowski , Matthias Brugger , AngeloGioacchino Del Regno , "moderated list:ARM/Mediatek USB3 PHY DRIVER" , "open list:GENERIC PHY FRAMEWORK" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list Subject: Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5 Message-ID: <20230608134908.GA2463843-robh@kernel.org> References: <4yppinkucchwnwtnnpbqdn4bejmntjq3q6mx6es55f2pwyce3c@qdhdks47lpyt> <1853f049-4f00-b7f0-973a-2c4e7b0b2634@linaro.org> <7h353w2oug.fsf@baylibre.com> <7hwn18yndq.fsf@baylibre.com> <7hcz2snpnw.fsf@baylibre.com> <10074d67-394b-3ddb-8bd1-fc051bdb7f79@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230608_064914_110166_F640B5D6 X-CRM114-Status: GOOD ( 55.41 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Mon, Jun 05, 2023 at 10:44:22AM +0200, Julien Stephan wrote: > On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote: > > On 22/05/2023 21:15, Kevin Hilman wrote: > > > Krzysztof Kozlowski writes: > > > > > >> On 16/05/2023 23:31, Kevin Hilman wrote: > > >> > > >>>> Third is to use versioned IP blocks. > > >>>> > > >>>> The second case also would work, if it is applicable to you (you really > > >>>> have fallback matching all devices). Third solution depends on your > > >>>> versioning and Rob expressed dislike about it many times. > > >>>> > > >>>> We had many discussions on mailing lists, thus simplifying the review - > > >>>> I recommend the first choice. For a better recommendation you should say > > >>>> a bit more about the block in different SoCs. > > >>> > > >>> I'll try to say a bit more about the PHY block, but in fact, it's not > > >>> just about differences between SoCs. On the same SoC, 2 different PHYs > > >>> may have different features/capabilities. > > >>> > > >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1. CSI0 can > > >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY > > >>> (used as the example in the binding patch[1].) On another related SoC, > > >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D. > > >>> > > >>> So that's why it seems (at least to me) that while we need SoC > > >>> compatible, it's not enough. We also need properties to describe > > >>> PHY-specific features (e.g. C-D PHY) > > >> > > >> I recall the same or very similar case... It bugs me now, but > > >> unfortunately I cannot find it. > > >> > > >>> > > >>> Of course, we could rely only on SoC-specific compatibles describe this. > > >>> But then driver will need an SoC-specific table with the number of PHYs > > >>> and per-PHY features for each SoC encoded in the driver. Since the > > >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many > > >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional > > >>> propery was the better solution. > > >> > > >> Phys were modeled as separate device instances, so you would need > > >> difference in compatible to figure out which phy is it. > > >> > > >> Other way could be to create device for all phys and use phy-cells=1. > > >> Whether it makes sense, depends on the actual datasheet - maybe the > > >> split phy per device is artificial? There is one PHY block with two > > >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one > > >> block? You should carefully check this because once design is chosen, > > >> you won't be able to go back to other and it might be a problem (e.g. > > >> there is some top-level block for powering on all CSI instances). > > > > > > We're pretty sure these are multiple instances of the IP block as they > > > can operate completely independently. > > > > > >>> > > >>> To me it seems redundant to have the driver encode PHYs-per-SoC info, > > >>> when the per-SoC DT is going to have the same info, so my suggestion was > > >>> to simplify the driver and have this kind of hardware description in the > > >>> DT, and keep the driver simple, but we are definitely open to learning > > >>> the "right way" of doing this. > > >> > > >> The property then is reasonable. It should not be bool, though, because > > >> it does not scale. There can be next block which supports only D-PHY on > > >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible > > >> configurations. > > > > > > OK, looks like include/dt-bindings/phy/phy.y already has > > > > > > #define PHY_TYPE_DPHY 10 > > > #define PHY_TYPE_CPHY 11 > > > > > > we'll add a PHY_TYPE_CDPHY and use that. Sound reasonable? No, because these defines are the 1 mode used for a specific connection, not the set of supported modes. > > > > Yes. Currently it is usually used as phy-cells argument (after the phy > > number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as > > property in provider. In both cases they have a bit different meaning > > than yours. You want to list all supported modes or narrow/restrict > > them. Maybe hisilicon,fixed-mode fits your purpose? There is also 'phy-type' to set the mode/type in the provider. If we need to list all possible modes, then you need to justify why that's needed and then define a property which is a mask of the existing type defines. > > > > Hi Krzysztof, > > Thanks for the suggestion, using something like hisilicon,fixed-node > looks like a good fit. > With mediatek,fixed-node, by default CSI node will be considered as > CD-PHY capable (unless the fixed-mode property is set.) so I won't need > anymore the new define PHY_TYPE_CDPHY introduced in v3. > > Also introducing mediatek,fixed-mode suggests that PHYs not declaring > the fixed mode property support mode selection, so I suggest to add a > phy argument (#phy-cells = <1>) to select the mode (D or C mode). > Exactly what is done by the hsilicon driver. > > How does that sound to you? I don't follow the need for fixed-mode, but agree you should use a phy arg. Can't you just assume the D-PHY only instance will only have D-PHY for the arg? Rob