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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4BFD6C433F5 for ; Fri, 19 Nov 2021 14:12:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2CD5B619E3 for ; Fri, 19 Nov 2021 14:12:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235762AbhKSOPa (ORCPT ); Fri, 19 Nov 2021 09:15:30 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55606 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234551AbhKSOP3 (ORCPT ); Fri, 19 Nov 2021 09:15:29 -0500 Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com [IPv6:2607:f8b0:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F2EBDC06173E for ; Fri, 19 Nov 2021 06:12:27 -0800 (PST) Received: by mail-pg1-x52d.google.com with SMTP id m15so8733098pgu.11 for ; Fri, 19 Nov 2021 06:12:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=UgAqbpopy6HklYietNPfXdJKLyBN7yZp/5A4MJwHDTY=; b=CEnYbYaY8rBd1Cif8k8YZBWlBHeTr8pPZsol+HzbDQrXPK/E4j+NS/7uSTu9qqqItA 2On7bI0TRuDAVBAKwMmp/c0Ou3YdqgsmNbZs6zsK12njRtpkGd4b81MLNLzrCpItUurk i6m5en2CybWucPpNjfnQ+XB3zctBJP4rISr0zsrxNjS45kjGEWbqjU0215wzs15PVyAL 6JFuv2WjRSbcTnHA7Id7/qaKlrraA97HiL0D6o/GrpQrtSbTq39o8Iy9G9HQmCDQJv6t E4Gc7NitF2C75qKmYSt54NOfKCYsfgGU7wYML0cKsUTGgdErHh/GkapONPo2lVZCMQZp 5hvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=UgAqbpopy6HklYietNPfXdJKLyBN7yZp/5A4MJwHDTY=; b=t81aCF1lS4orFVIfhQ2dv2+olnMmD+fI6EeQOpPcP0+zWl0WcfJXNNkyhXKzblvwD/ KHcBE9C9pioJMp1JFUKe6SPDxgtC1dQ/lXTcnIOOroo9F2qPQrHlC7d6w7UNtI9JHWYn E7dROKuWZJ/6HbA5gyazVMvOB7Z1yGFRSTssHsifntbHywv1XRnee4YbgxL9MaBwa6SA KAOZDYAwu1by+drlpgEdSrVgJnyeL6xmLrlLD7q0sQaZXHUTDAF/LKCfqpITgvQyuM8n t+DEVmDMWy2iOr7+KoC6iykYJdyEScLPvUmtrPLcbdxckLyI1H8jSQG7atGtoIsaC1wV pa8A== X-Gm-Message-State: AOAM5309s2++iK7CU337Jwf73otLByZt9t7CZQ7UP9VbLTpaz4ZG+JKS j0rf/CfV1gE71FIeZIaT7Drep3ykwV9daCwZhL7ybI7/9vZa0Q== X-Google-Smtp-Source: ABdhPJz1hwMPGGN4/nlVyGD+fO+dHHNAi/WPWrzoLWlJ/hnc5Yu87VoDBBCzIB/CWsfpcbrD9oqWIftLk7Q6tGO9mSs= X-Received: by 2002:a05:6122:1813:: with SMTP id ay19mr119204327vkb.24.1637331135963; Fri, 19 Nov 2021 06:12:15 -0800 (PST) MIME-Version: 1.0 References: <20211112010137.149174-1-jaewon02.kim@samsung.com> <20211112010137.149174-3-jaewon02.kim@samsung.com> <001401d7da86$f7ebd660$e7c38320$@samsung.com> <773110c9-fc74-6cab-68c0-1c771a3be104@canonical.com> In-Reply-To: <773110c9-fc74-6cab-68c0-1c771a3be104@canonical.com> From: Sam Protsenko Date: Fri, 19 Nov 2021 16:12:04 +0200 Message-ID: Subject: Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC To: Jaewon Kim Cc: Chanho Park , Wolfram Sang , Rob Herring , linux-samsung-soc@vger.kernel.org, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Krzysztof Kozlowski Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski wrote: > > On 18/11/2021 20:59, Sam Protsenko wrote: > > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski > > wrote: > >> > >> On 16/11/2021 02:12, Chanho Park wrote: > >>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick > >>>> from my side (just a food for thought): do we want to configure USI > >>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would > >>>> be better design to implement some platform driver for that, so we can > >>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this > >>>> series is good to be merged as is, but we should probably consider all > >>>> upsides and downsides of each option, for the future work. > >>> > >>> I'm also considering how to support this USI configuration gracefully. > >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far. > >>> But, there is a possibility that the USI hw version can be bumped for future SoCs. > >>> > >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree. > >>> > >>> Option1) Make a USI driver under soc/samsung/ like [1]. > >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver. > >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree. > >>> > >>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c > >> > >> I don't have user manuals, so all my knowledge here is based on > >> Exynos9825 vendor source code, therefore it is quite limited. In > >> devicetree the USI devices have their own nodes - but does it mean it's > >> separate SFR range dedicated to USI? Looks like that, especially that > >> address space is just for one register (4 bytes). > >> > >> In such case having separate dedicated driver makes sense and you would > >> only have to care about driver ordering (e.g. via device links or phandles). > >> > >> Option 2 looks interesting - reusing reset framework to set proper USI > >> mode, however this looks more like a hack. As you said Chanho, if there > >> is a USI version 3, this reset framework might not be sufficient. > >> > >> In option 3 each driver (UART/I2C/SPI) would need to receive second IO > >> range and toggle some registers, which could be done via shared > >> function. If USI v3 is coming, all such drivers could get more complicated. > >> > >> I think option 1 is the cleanest and extendable in future. It's easy to > >> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It > >> also nicely encapsulates USI-related stuff in separate driver. Probe > >> ordering should not be a problem now. > >> > >> But as I said, I don't have even the big picture here, so I rely on your > >> opinions more. > >> > > > > Hi Krzysztof, > > > > Can you please let me know if you're going to apply this series as is, > > or if you want me to submit USIv2 driver first, and then rework this > > patch on top of it? I'm working on some HSI2C related patches right > > now, and thus it'd nice to know about your decision on this series > > beforehand, as some of my patches (like bindings doc patches) might > > depend on it. Basically I'd like to base my patches on the proper > > baseline, so we don't have to rebase those later. > > This set won't go via my tree anyway, but I am against it. David pointed > out that his USIv1 is a little bit different and embedding in each of > I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too > big. The solution with a dedicated driver looks to me more flexible and > encapsulated/cleaner. > > Therefore after the discussions I am against this solution, so a > soft-NAK from my side. > Hi Jaewon, I'm going to submit USI driver soon, and also some more HSI2C patches. Do you mind if I rework your patches to rely on USI drver (instead of modifying System Register in HSI2C driver), and include those in my patch series? Of course, I'll preserve your authorship. Just think that would be easier and faster this way. Thanks! > > Best regards, > Krzysztof