Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
From: Sergey Semin <Sergey.Semin@baikalelectronics.ru>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>,
	Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
	Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>,
	Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>,
	Vadim Vlasov <V.Vlasov@baikalelectronics.ru>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Paul Burton <paulburton@kernel.org>,
	Ralf Baechle <ralf@linux-mips.org>,
	Jarkko Nikula <jarkko.nikula@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/6] i2c: designeware: Add Baikal-T1 SoC DW I2C specifics support
Date: Tue, 31 Mar 2020 14:48:24 +0300
Message-ID: <20200331114824.e3uljdymvsjuh6wh@ubsrv2.baikal.int> (raw)
In-Reply-To: <20200306135451.4AF0480307C4@mail.baikalelectronics.ru>

Hello Andy,

Finally I've thought this through reasonably conformed with the changes
requested in the framework of the other patchsets. My comments are
below.

On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
> First of all, I got only 3 out of 6 patches. Are you sure you properly prepared
> the series?
> 
> On Fri, Mar 06, 2020 at 04:19:49PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <fancer.lancer@gmail.com>
> 
> Same comment as per DMA series, try next time to link the cover letter to the
> series correctly.
> 
> > There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
> > of them are normal with standard DW I2C IP-core configurations and registers
> > accessible over normal MMIO space - so they are acceptable by the available
> > DW I2C driver with no modification.
> 
> > But there is a third, which is a bit
> > different. Its registers are indirectly accessed be means of "command/data
> > in/data out" registers tuple. In order to have it also supported by the DW
> > I2C driver, we must modify the code a bit. This is a main purpose of this
> > patchset.
> > 
> > First of all traditionally we replaced the legacy plain text-based dt-binding
> > file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
> > detection algorithm which tried to do it too early before dw_readl/dw_writel
> > methods could be used.
> 
> So far so good (looks like, I think colleagues of mine and myself will review
> individual patches later on).
> 
> > Finally we introduced a platform-specific flag
> > ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
> > implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
> > message of the corresponding patch for details.
> 
> This is quite questionable. In Intel SoCs we have indirect I²C controllers to
> access (inside PMIC, for example). The approach used to do that is usually to
> have an IPC mechanism and specific bus controller driver. See i2c-cht-wc.c for
> instance.
> 
> I'm not sure if it makes a lot of duplication and if actually switching I²C
> DesignWare driver to regmap API will solve it. At least that is the second
> approach I would consider.
> 
> But I'll wait others to comment on this. We have to settle the solution before
> going further.
> 

As I see the others have not comments.) Anyway I see your point and having the
regmap-based interface might be better than the approach I've suggested
in this patchset particularly seeing that our DW i2c IP registers are
hidden behind a system controller register space.

In order to follow your proposition to create a dedicated regmap and to supply
it to the DW i2c driver, I have to redevelop not only this patchset, but
also an adjacent drivers. In particular the changes will concern the
MFD-based System Controller driver (which will instantiate this DW i2c
controller device), Clocks Control Unit drivers set, and a few
others. The whole alteration I described in the RFC:
https://lkml.org/lkml/2020/3/22/393
You've been in Cc there, so fill free to send your comments regarding
the changes I suggested. Though this time I hope the solution will
satisfy everyone, who had issues with patchsets I've recently sent.

Getting back to your comment in the framework of this patchset. The approach
used for CHT Whiskey Cove i2c isn't fully suitable in our case for
the reason of the DW I2C controller nature. DW I2C controller is a generic
controller and used on many different platforms, while AFAICS CHT Whiskey Cove
I2C is the SoC-specific used to access a charger-IC. So in the former case we
may have an arbitrary set of i2c-slaves connected to the controller on
different platforms, while on the latter one - there is a fixed set of
slaves. In addition due to the same reason the DW I2C IP might be
embedded into different sub-blocks on different platforms, while the CHT
Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
For instance Baikal-T1 SoC has one DW I2C controller embedded into the
System Controller with indirectly accessible registers and two DW I2C
interfaces with normal memory mapped registers. Due to this in case of DW I2C
driver we can't just "suck" the regmap out from a parental MFD or
anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
we should somehow supply a regmap pointer to the driver.

Taking into account all of these we can utilize a combined approach
implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
"struct dw_i2c_platform_data" structure, so in case if there is no
IORESOURCE_MEM resources available (platform_get_resource() fails), we
try to get a regmap pointer from the platform data. If there is no valid
regmap available, then completely fail the driver probe procedure. Though
due to this alteration I'll have to change the
dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
platforms, but will let us cover a case when regmap is set while i2c
clock frequency is supposed to be taken from the kernel firmware (like
dtb, etc).

So if you are Ok with this, I'll send a v2 patchset with corresponding
alteration implemented.

Regards,
-Sergey

> > This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> > commit 98d54f81e36b ("Linux 5.6-rc4").
> 
> `git format-patch --base ...` should do the job.
> 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> 
> Same comment as per UART patch. Who is the Alexey in relation to the work done?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

  parent reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 13:19 Sergey.Semin-UN2wsyeM1qLJ45LvJ/SUn8gurn75+9Lz
2020-03-06 13:54 ` Andy Shevchenko
     [not found] ` <20200306135451.4AF0480307C4@mail.baikalelectronics.ru>
2020-03-31 11:48   ` Sergey Semin [this message]
2020-03-31 14:25     ` Andy Shevchenko
     [not found]       ` <20200331142530.GM1922688-XvqNBM/wLWRrdx17CPfAsdBPR1lH4CV8@public.gmane.org>
2020-03-31 16:28         ` Sergey Semin
2020-03-31 17:17           ` Andy Shevchenko
2020-03-31 19:14             ` Sergey Semin
2020-05-10  9:50 ` [PATCH v2 00/12] i2c: designeware: Add Baikal-T1 System I2C support Serge Semin
2020-05-10  9:50   ` [PATCH v2 01/12] scripts/dtc: check: Add 10bit/slave i2c reg flags support Serge Semin
2020-05-10  9:50   ` [PATCH v2 02/12] dt-bindings: i2c: Convert DW I2C binding to DT schema Serge Semin
2020-05-11 16:09     ` Rob Herring
2020-05-11 19:50       ` Serge Semin
2020-05-10  9:50   ` [PATCH v2 03/12] dt-bindings: i2c: dw: Add Baikal-T1 SoC I2C controller Serge Semin
2020-05-18 20:33     ` Rob Herring
2020-05-18 21:39       ` Serge Semin
2020-05-10  9:50   ` [PATCH v2 04/12] i2c: designware: Convert driver to using regmap API Serge Semin
2020-05-20 12:16     ` Jarkko Nikula
2020-05-21  2:02       ` Serge Semin
2020-05-10  9:50   ` [PATCH v2 05/12] i2c: designware: Use `-y` to build multi-object modules Serge Semin
2020-05-20 12:16     ` Jarkko Nikula
2020-05-10  9:50   ` [PATCH v2 06/12] i2c: designware: slave: Set DW I2C core module dependency Serge Semin
2020-05-20 12:15     ` Jarkko Nikula
2020-05-10  9:50   ` [PATCH v2 07/12] i2c: designware: Move Baytrail sem config to the platform if-clause Serge Semin
2020-05-20 12:16     ` Jarkko Nikula
2020-05-21  2:22       ` Serge Semin
2020-05-25 13:01         ` Jarkko Nikula
2020-05-26 18:40           ` Serge Semin
2020-05-10  9:50   ` [PATCH v2 08/12] i2c: designware: Introduce platform drivers glue layer interface Serge Semin
2020-05-20 12:46     ` Jarkko Nikula
2020-05-21  2:37       ` Serge Semin
2020-05-25 13:16         ` Jarkko Nikula
2020-05-25 13:42           ` Andy Shevchenko
2020-05-26 20:38           ` Serge Semin
2020-05-10  9:50   ` [PATCH v2 09/12] i2c: designware: Unpin Microsemi Ocelot I2C into a glue driver Serge Semin
2020-05-10  9:50   ` [PATCH v2 10/12] i2c: designware: Discard Cherry Trail model flag Serge Semin
2020-05-20 12:17     ` Jarkko Nikula
2020-05-10  9:50   ` [PATCH v2 11/12] i2c: designware: Use provided regmap instead of reg resource Serge Semin
2020-05-10  9:50   ` [PATCH v2 12/12] i2c: designware: Add Baikal-T1 System I2C glue driver Serge Semin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200331114824.e3uljdymvsjuh6wh@ubsrv2.baikal.int \
    --to=sergey.semin@baikalelectronics.ru \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Ekaterina.Skachko@baikalelectronics.ru \
    --cc=Maxim.Kaurkin@baikalelectronics.ru \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=Ramil.Zaripov@baikalelectronics.ru \
    --cc=V.Vlasov@baikalelectronics.ru \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jarkko.nikula@linux.intel.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.org \
    --cc=tsbogend@alpha.franken.de \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git