Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Sergey Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	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 <linux-i2c@vger.kernel.org>,
	devicetree <devicetree@vger>
Subject: Re: [PATCH 0/6] i2c: designeware: Add Baikal-T1 SoC DW I2C specifics support
Date: Tue, 31 Mar 2020 20:17:28 +0300
Message-ID: <CAHp75VfOxRdEZ81-rFUrAiQDdruKqhJn_Wh+PJnyB5WL2vMUCQ@mail.gmail.com> (raw)
In-Reply-To: <20200331162813.dnpmyzs35tvkeavx@ubsrv2.baikal.int>

On Tue, Mar 31, 2020 at 7:29 PM Sergey Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Tue, Mar 31, 2020 at 05:25:30PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 31, 2020 at 02:48:24PM +0300, Sergey Semin wrote:
> > > On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:

...

> > > 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).

Whiskey Cove case provides an I²C controller with specific access. That's it.
In your case you will need similar glue driver which will utilize
DesignWare core driver.

> > > So if you are Ok with this, I'll send a v2 patchset with corresponding
> > > alteration implemented.
> >
> > I was thinking about something like this:
> >
>
> > 1/ core driver (library + master + slave) is converted to use regmap
>
> Yes. I also intended to do this just by altering the dw_readl() and
> dw_writel() methods to work over regmap IO methods if regmap is
> available.

Why? Simple always use regmap API calls. Moreover, regmap provides
update() which may give a leverage in some cases by dropping r-m-w
code.

Core part take a pointer to regmap when instantiate (probe) a driver
on the device.

> > 2/ platform and PCI driver may provide regmap MMIO
>
> Regmap pointer will be also a part of "struct dw_i2c_dev".

Yes.

> So if PCI
> code intends the regmap-based access to the controller registers, then
> it shall just initialize the regmap pointer in the private i2c-designware data
> instance of the dw_i2c_dev structure.

Why data is being involved here?

> So, yes, this is also covered by
> my solution. Though the PCI code will be left untouched, since I can't
> predict a particular regmap-related use-case of it.

> > 3/ your glue driver will provide different regmap accessors
>
> I was thinking of developing a more generic version so any platform
> with a specific access to the DW I2C register could use it just by supplying
> the regmap pointer in the dw_i2c_platform_data structure.

regmap is not a platform data.

> Our DW I2C
> controller also perfectly fits to the generic i2c-designware-platdrv.c
> driver, so implementing an additional glue-layer would be too much seeing
> the difference only in the registers mapping.

Register accessing, mapping as far as I understood is the same.
However, with regmap layer it just an implementation detail of certain
glue driver.

> Let me explain the difference of our solutions. In case of implementing
> the glue layer, as you suggest, I would have to do it in a way like the DW PCIe
> driver is designed. I would need to move the code of current dw_i2c_plat_probe()
> function to a dedicated method named like dw_i2c_plat_init(pdev, !regmap!),
> while former method dw_i2c_plat_probe() would just call
> dw_i2c_plat_init(pdev, !NULL!). Then I would have to create a dedicated
> glue-driver - i2c-designware-bt1drv.c, which would be bound to a
> "baikal,t1-sys-i2c" device, try to find a Baikal-T1 System Controller
> device node (though this would be just a parent device), then would get
> it' syscon regmap handler, then would initialize a dedicated regmap handler to
> indirectly access the DW I2C controller register, then it would call the
> dw_i2c_plat_init(pdev, !regmap!) method with new regmap handler passed
> (though the new regmap passing could be also implemented over the
> platform_data pointer). Also seeing you already have a platform-specific
> parts in the generic i2c-designware-platdrv.c driver (like ACPI-based
> platforms and Microsemi Ocelot SoC), there might raise a necessity to
> unpin that specifics to a dedicated method, since my glue-layer
> wouldn't need that checks and initializations. Such alteration won't
> be that easy to implement and regression errors prone, since I don't have
> other platforms to test it.
>
> In case of my solution the whole glue-layer part would be moved to
> the MFD-based Baikal-T1 System Controller driver and a generic
> platform_data-based interface would be implemented, which would just
> need to alter the registers mapping part of the i2c-designware-platdrv.c
> driver. Note that that part would need to be fixed in case of any solution.
>
> So comparing these too approaches, I would select a one, which would
> need less common code modifications and would provide a generic
> solution. As I see it would be a platform_data-based design. What do you
> think?

I think you may look at the existing examples how drivers are
utilizing regmap layer.
Along with that look how glue (or let's say quirk in your case)
drivers are implemented, for example USB or SDHCI comes to my mind.

-- 
With Best Regards,
Andy Shevchenko

  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
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 [this message]
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=CAHp75VfOxRdEZ81-rFUrAiQDdruKqhJn_Wh+PJnyB5WL2vMUCQ@mail.gmail.com \
    --to=andy.shevchenko@gmail.com \
    --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=Sergey.Semin@baikalelectronics.ru \
    --cc=V.Vlasov@baikalelectronics.ru \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=devicetree@vger \
    --cc=jarkko.nikula@linux.intel.com \
    --cc=linux-i2c@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