Message-ID: <c9e0e9b838cc046637059e771755f995.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <6ff60914-0c39-9916-2e3a-a906b4cdef0d@kernel.org>
References: <20230307115022.12846-2-zhuyinbo@loongson.cn> <202303082037.QPfBP64A-lkp@intel.com> <b94ee1d2-b224-f9d5-3f3c-0096634f4c93@loongson.cn> <ec1fb4d134181a1b1859bcb884dcd494.sboyd@kernel.org> <c03e47f7-bb26-0114-b300-357634b0e581@kernel.org> <61eee19400e9a45ce9543bfd92a27eaa.sboyd@kernel.org> <6ff60914-0c39-9916-2e3a-a906b4cdef0d@kernel.org>
Subject: Re: [PATCH v13 2/2] clk: clk-loongson2: add clock controller driver support
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     oe-kbuild-all@lists.linux.dev, Jianmin Lv <lvjianmin@loongson.cn>,
        Liu Peibao <liupeibao@loongson.cn>, wanghongliang@loongson.cn,
        loongson-kernel@lists.loongnix.cn
To:     Krzysztof Kozlowski <krzk@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        kernel test robot <lkp@intel.com>, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, zhuyinbo <zhuyinbo@loongson.cn>
Date:   Tue, 14 Mar 2023 17:26:21 -0700
User-Agent: alot/0.10
Precedence: bulk

Quoting Krzysztof Kozlowski (2023-03-13 23:49:40)
> On 13/03/2023 19:20, Stephen Boyd wrote:
> >>>> The CONFIG_64BIT not enabled in your config file, I will add a depen=
d on=20
> >>>> "CONFIG_64BIT" in my clock driver to fix this compile error.
> >>>
> >>> Do you need to use readq() here? Can you read two 32-bit registers wi=
th
> >>> readl() and put them together for a 64-bit number?
> >>
> >> If the platform supports 64-bit reads and these are actually one
> >> register, then readq makes sense - code is more readable, smaller, more
> >> efficient.
> >>
> >=20
> > Please read the section in Documentation/driver-api/device-io.rst about
> > hi_lo_readq() and <linux/io-64-nonatomic-lo-hi.h>. We shouldn't need to
> > restrict the driver to CONFIG_64BIT. Instead, include one of these
> > header files to get the IO access primitives.
>=20
> These primitives are for 32bit access. Quoting: "on 32-bit
> architectures". What's the point of them if the code *will never* run on
> 32-bit?

They're there to make drivers portable.

> It will be a fake choice of linux/io-64-nonatomic-lo-hi.h or
> linux/io-64-nonatomic-hi-lo.h misleading users to think this was tested
> on 32-bit.
>=20

I don't think anyone is really going to care that it hasn't been tested.
It's not like the Linux kernel driver is the source of truth for
integrating IP blocks into different architectures. If it's wrong
someone will fix it when they try to use the hardware on 32-bit systems.

Can the register handle being read/written with two 32-bit accesses? I
still don't think we've had any answer to that question. If so, pick the
one that makes the most sense and move on.

In Linux, we try to write portable drivers. This way anyone can compile
the driver on any host architecture with whatever compiler they're
using. Otherwise, they have to download a cross compiler for the target
architecture to simply build test the code. Also, the Linux kernel is
fairly portable. We try to limit architecture specific code to arch/ and
so anything in drivers/ is ideally portable code.