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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91BB2C433FE for ; Tue, 16 Nov 2021 15:31:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7694361507 for ; Tue, 16 Nov 2021 15:31:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238301AbhKPPez (ORCPT ); Tue, 16 Nov 2021 10:34:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54354 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238278AbhKPPex (ORCPT ); Tue, 16 Nov 2021 10:34:53 -0500 Received: from mail-ua1-x92b.google.com (mail-ua1-x92b.google.com [IPv6:2607:f8b0:4864:20::92b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0F647C061766 for ; Tue, 16 Nov 2021 07:31:56 -0800 (PST) Received: by mail-ua1-x92b.google.com with SMTP id v3so43163461uam.10 for ; Tue, 16 Nov 2021 07:31:56 -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=tsrV+hfFCVu/lguKxyW5H34AzEuQICjVq9Wuxqx+30s=; b=t5/2NAXadqXAMwAiI9RIP3gOoQY4iP2IRi9cf5GQoc3LIoPav3xQzx9O6dh8IWGd+B S4VutQCpR3V9WIfTCmveVjh3KKjL7jILRXRqcRBgqReRwrHVt2uwRQJhtE+YbeXvdIcH f2K1j30ztG6CwRB/8CgVoxgqEsZqtWFz1GIr7klc9YT1NDQqqUEZXQv4qTAVKo1vUeJ2 hReZoa+BfBUX7Um1+ZKMr27iclbkoSqEZQ8hk1szDHC6UDhAn9DnMLgoH50ozs4hHzQf uEVcJma99XK7xK8meSQ/l9ZRmPmEBq5ylKKP3aGi5tXxKiN7Dk6Zp4qj2lxGtw3r4SQN jnwA== 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=tsrV+hfFCVu/lguKxyW5H34AzEuQICjVq9Wuxqx+30s=; b=X5+4XWlXVtdFHEXla0kM3GOzuQC5Md2kLKjnyqnN4sq2IorQhy9igqISyKINTxumXz t4paw4RY7KgGykBfoAgvsyp/R+m2UxFmT+0o1wIrgV+fJj8jeFnN2LfKOjzKHiPWbQ0x BMs8DkeqprNhS/9z1jmwbVsSGPeXeMauXjQ1psOwSkfarZ6m1UuH/yRrk5/SXGN0AKrc 9BtOWwsjXT7NoXWlN44FbAW0JsQVHc5Ugr5ls+8N7UP/SN2XHsGMTpUe4mfqlZYZiZOz EGvF4nAHpyZAPFLmeDjxiXkM2znzGZaDC/glLPRYyhCrP+3mRkYqvdC+yuTwdRXlSL1H LU1Q== X-Gm-Message-State: AOAM531x1BbpxZKa6Xf8/9b6MAaDg0a9+DL2X78kmqUd5O0/EB1V5Viu /rBH/uAH4iHRv4KOnWxVb/yTiYHeqR2tmVEDufljEA== X-Google-Smtp-Source: ABdhPJzI1vYHs3IlbeTAF3zLZ2iugMLFFYsb7zRv/5mdhBpZbcKWUCT8T42LC150nqoSu9alCPYFu2j907jPq57oocE= X-Received: by 2002:ab0:458e:: with SMTP id u14mr12043487uau.104.1637076715001; Tue, 16 Nov 2021 07:31:55 -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> In-Reply-To: From: Sam Protsenko Date: Tue, 16 Nov 2021 17:31:43 +0200 Message-ID: Subject: Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC To: Krzysztof Kozlowski Cc: Chanho Park , Jaewon Kim , 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > To provide more context, USI registers are split across two different register maps: - USI protocol configuration (where we choose which protocol to use: HSI2C, SPI or UART) is done via *_SW_CONF registers, from System Register SFR range. To access those SW_CONF registers we need to either: (Option 3) pass System Register registers to corresponding driver (HSI2C/SPI/UART) via syscon (Option 1) or implement separate USI driver, so we can choose desired protocol in device tree; in that case I guess System Register registers should be passed via syscon to USI driver - USI registers (like USI_CON register, which contains USI_RESET bit) are contained in the same SFR range as corresponding HSI2C/SPI/UART IP-core. Or rather HSI2C/SPI/UART IP-cores are encapsulated within USI block(s). So to access registers like USI_CON we only need to use memory already passed to corresponding HSI2C/SPI/UART driver via "reg" property. So I'd say ideally we should do next: - modify USI registers (like USI_CON) in corresponding HSI2C/SPI/UART drivers; but because all HSI2C/SPI/UART drivers share the same USI registers, we might want to pull USI register offsets and bits to some common header file, for example (to not duplicate that code in drivers) - implement separate USI driver to control SW_CONF registers from System Register module (Option 1), so we can choose desired protocol in device tree (in some USI node, not in HSI2C node) - also, it probably makes sense to group all USI related nodes in some separate *-usi.dtsi file; that would reduce confusion, given that we have even more encapsulation in Exynos850 thanks to CMGP (Common GPIO) block My suggestion is to take this patch as is, and then we can work on USI driver implementation/upstreaming. Right now we don't have much of device tree files that use USI HSI2C driver, so it'll be fairly easy to fix those dts's once we implemented USI driver. That will also unblock me with submitting dev board support (dts files) I'm working on right now. Krzysztof, please let me know if I'm wrong and if we shouldn't change dts API too much, so it's mandatory to implement USI driver before accepting this patch. Thanks! > Best regards, > Krzysztof 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19437C433EF for ; Tue, 16 Nov 2021 15:33:32 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id D428861507 for ; Tue, 16 Nov 2021 15:33:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D428861507 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=fGToQiNuFhWg5wFRjDHd2cfB4VF7snMwJguRtEcBfVo=; b=CgBLW7tS1GJOQD aRX05DtUOzHg2lY5DO5UAF2d8BXBMKW5HlCxRDNN9LHpwKUMk6N/1u9WEoidI10W12jtcCe1NrHeB UtLPsC0DGMQCrjO7nfD+ovhKGF8iApmmHNlq3D+AZWSVgTa17dtd3J6b36NniZd7W/CPt5sDlKD80 NI2buMRXmH2DFJawsT0yYFsS3AQ54U/8thqG9eBXT9d5nKC2LEKTyW+b7343FuHn2akTgaVKcZ2cy PTWY/Vbu+W+q0nka3rxIJiXjthxh9JJDYAY1HEfD4nhuemWPHIQeWJvy9z3fL/bpa3cyr6zYuEw1n d8LP+gMykw1e7Ws8GIKg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mn0RF-0028si-9M; Tue, 16 Nov 2021 15:32:01 +0000 Received: from mail-ua1-x930.google.com ([2607:f8b0:4864:20::930]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mn0RB-0028s2-NY for linux-arm-kernel@lists.infradead.org; Tue, 16 Nov 2021 15:31:59 +0000 Received: by mail-ua1-x930.google.com with SMTP id l24so39015238uak.2 for ; Tue, 16 Nov 2021 07:31:56 -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=tsrV+hfFCVu/lguKxyW5H34AzEuQICjVq9Wuxqx+30s=; b=t5/2NAXadqXAMwAiI9RIP3gOoQY4iP2IRi9cf5GQoc3LIoPav3xQzx9O6dh8IWGd+B S4VutQCpR3V9WIfTCmveVjh3KKjL7jILRXRqcRBgqReRwrHVt2uwRQJhtE+YbeXvdIcH f2K1j30ztG6CwRB/8CgVoxgqEsZqtWFz1GIr7klc9YT1NDQqqUEZXQv4qTAVKo1vUeJ2 hReZoa+BfBUX7Um1+ZKMr27iclbkoSqEZQ8hk1szDHC6UDhAn9DnMLgoH50ozs4hHzQf uEVcJma99XK7xK8meSQ/l9ZRmPmEBq5ylKKP3aGi5tXxKiN7Dk6Zp4qj2lxGtw3r4SQN jnwA== 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=tsrV+hfFCVu/lguKxyW5H34AzEuQICjVq9Wuxqx+30s=; b=8M9RmMxJqcc3ghBf+NNrrEIjjlvqkN9CYsQbK30RuPivkZmJPK94Sx99TVfcE0hm6H 7Pp0qlOee9bRUp4+9p4hi975629USnuYdlJG/02eGXuRauCE1/OVZ9HDF34hLK/Ek/6h yD3VAhtq+QxEVyqTAlxAAxfB4USAtoFc89xewEdKaB1CvyeRBx+4rtTMeTaeFS7chPVt kVuCHgT8S8OoaFEtlQepxcsj2oQtGKTeg0jCDnBCv3KzL6w+R71V9gPxt//mx6zstB6Y DaXNWk+8FEKPHIxFDBsR3HgqgIr1WZsmx/eqi/gGS4Ia4xzS+QMWDGOkPlKpztQIw+W/ B9lQ== X-Gm-Message-State: AOAM530dscI+o0S8MjXqafjFnEpZUyoAdBbv6uNAxXxxI98f0nJrZOcN t01xykNEm+f3804olnl1cAiSqas6HowXv82uDUNFeQ== X-Google-Smtp-Source: ABdhPJzI1vYHs3IlbeTAF3zLZ2iugMLFFYsb7zRv/5mdhBpZbcKWUCT8T42LC150nqoSu9alCPYFu2j907jPq57oocE= X-Received: by 2002:ab0:458e:: with SMTP id u14mr12043487uau.104.1637076715001; Tue, 16 Nov 2021 07:31:55 -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> In-Reply-To: From: Sam Protsenko Date: Tue, 16 Nov 2021 17:31:43 +0200 Message-ID: Subject: Re: [PATCH v3 2/2] i2c: exynos5: add support for ExynosAutov9 SoC To: Krzysztof Kozlowski Cc: Chanho Park , Jaewon Kim , 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211116_073157_875765_216F8B41 X-CRM114-Status: GOOD ( 39.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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. > To provide more context, USI registers are split across two different register maps: - USI protocol configuration (where we choose which protocol to use: HSI2C, SPI or UART) is done via *_SW_CONF registers, from System Register SFR range. To access those SW_CONF registers we need to either: (Option 3) pass System Register registers to corresponding driver (HSI2C/SPI/UART) via syscon (Option 1) or implement separate USI driver, so we can choose desired protocol in device tree; in that case I guess System Register registers should be passed via syscon to USI driver - USI registers (like USI_CON register, which contains USI_RESET bit) are contained in the same SFR range as corresponding HSI2C/SPI/UART IP-core. Or rather HSI2C/SPI/UART IP-cores are encapsulated within USI block(s). So to access registers like USI_CON we only need to use memory already passed to corresponding HSI2C/SPI/UART driver via "reg" property. So I'd say ideally we should do next: - modify USI registers (like USI_CON) in corresponding HSI2C/SPI/UART drivers; but because all HSI2C/SPI/UART drivers share the same USI registers, we might want to pull USI register offsets and bits to some common header file, for example (to not duplicate that code in drivers) - implement separate USI driver to control SW_CONF registers from System Register module (Option 1), so we can choose desired protocol in device tree (in some USI node, not in HSI2C node) - also, it probably makes sense to group all USI related nodes in some separate *-usi.dtsi file; that would reduce confusion, given that we have even more encapsulation in Exynos850 thanks to CMGP (Common GPIO) block My suggestion is to take this patch as is, and then we can work on USI driver implementation/upstreaming. Right now we don't have much of device tree files that use USI HSI2C driver, so it'll be fairly easy to fix those dts's once we implemented USI driver. That will also unblock me with submitting dev board support (dts files) I'm working on right now. Krzysztof, please let me know if I'm wrong and if we shouldn't change dts API too much, so it's mandatory to implement USI driver before accepting this patch. Thanks! > Best regards, > Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel