Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Dec 2019 09:12:37 -0600
From:   Rob Herring <robh@kernel.org>
To:     Orson Zhai <orson.zhai@spreadtrum.com>
Cc:     Arnd Bergmann <arnd@arndb.de>, Orson Zhai <orson.zhai@unisoc.com>,
        Lee Jones <lee.jones@linaro.org>,
        Mark Rutland <mark.rutland@arm.com>,
        DTML <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kevin.tang@unisoc.com, baolin.wang@unisoc.com,
        Chunyan Zhang <chunyan.zhang@unisoc.com>
Subject: Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to
 syscon easily
Message-ID: <20191205151237.GA30195@bogus>
References: <20191120154148.22067-1-orson.zhai@unisoc.com>
 <20191120154148.22067-2-orson.zhai@unisoc.com>
 <20191204163830.GA25135@bogus>
 <CAK8P3a3_r6z6Qk133=4gUzJ0rYmMH7sDDqpEF8ZVXS_bc3OtkQ@mail.gmail.com>
 <20191204192639.GA15786@bogus>
 <CAK8P3a165hqB=5LmMiTPGJxvsSJqrbFf5EC9WnqtFRYFok+xKw@mail.gmail.com>
 <20191205125539.GH21613@lenovo>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191205125539.GH21613@lenovo>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Dec 05, 2019 at 08:55:39PM +0800, Orson Zhai wrote:
> Hi Arnd & Rob,
> 
> On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <robh@kernel.org> wrote:
> > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote:
> >
> > > > I think generally speaking this would be useful for random registers that
> > > > logically belong to one device but are grouped with other unrelated
> > > > registers in a syscon, and that are in a different register offset for
> > > > each chip that has them. Using named properties instead of a list
> > > > of phandle/arg tuples with names is clearly a simpler alternative
> > > > and more like we do it today, but I can also see some API simplification
> > > > with Orson's patch without the binding getting out of hand.
> > >
> > > I understand when a phandle to a syscon is used. That's nothing new.
> > > What's special about Unisoc SoC that needs something new/different?
> > > I saw there's a large number of syscons, but I don't understand what's
> > > in them.
> > >
> > > If the API is this:
> > >
> > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> > >                                        const char *name,
> > >                                        int arg_count, __u32 *out_args)
> > >
> > > How is 'name' being an entry in syscon-names simpler than just being the
> > > property name? The implementation for the latter would certainly be
> > > simpler.
> > >
> > > It also makes the property unparseable without knowledge outside of the
> > > DT (i.e. in the driver). I suppose if the number of cells for each entry
> > > is fixed, we could count the number of syscon-names entries to figure
> > > out the stride. But then if one entry needs a lot of cells, then they
> > > all have to have padding cells.
> >
> > Good point. The syscon_regmap_lookup_by_name() interface would
> > work just as well when passing a property name compared to
> > a name listed in another property, and this would still be more in
> > line with what we do on other SoCs.
> >
> 
> udx710-modem.dtsi:69:   syscons = <&pmu_apb_regs 0x18 0x2000000>,
> udx710-modem.dtsi-70-           <&pmu_apb_regs 0x544 0x1>,
> udx710-modem.dtsi-71-           <&aon_apb_regs 0x218 0x7e00>,
> udx710-modem.dtsi-72-           <&pmu_apb_regs 0xb0 0x20000>,
> udx710-modem.dtsi-73-           <&pmu_apb_regs 0xff 0x100>;
> udx710-modem.dtsi:74:   syscon-names = "shutdown", "deepsleep", "corereset",
> udx710-modem.dtsi-75-                  "sysreset", "getstatus";

Reset at least has a standard binding.


> ud710.dtsi:1268:        syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> ud710.dtsi-1269-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> ud710.dtsi-1270-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> ud710.dtsi-1271-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> ud710.dtsi:1272:        syscon-names =  "sd_detect_pol",
> ud710.dtsi-1273-                        "sd_hotplug_protect_en",
> ud710.dtsi-1274-                        "sd_hotplug_debounce_en",
> ud710.dtsi-1275-                        "sd_hotplug_debounce_cn";

This looks to me like it should be a single phandle. How many different 
register layouts across how many SoCs do you need to support?

> > The only advantage I can see in having a list of phandle/arg tuples
> > rather than a set of properties is that it is a slightly more compact
> > representation in source form, but otherwise they should be equivalent
> 
> Yes, I agree.
> They are equivalent.
> 
> But sprd SoCs have too many registers and the representation might matter.
> Here's some real code from local,
> 
> orca.dtsi:1276:         syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >,
> orca.dtsi-1277-                 <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>,
> orca.dtsi-1278-                 <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>,
> orca.dtsi-1279-                 <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>,
> orca.dtsi-1280-                 <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>,
> orca.dtsi-1281-                 <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>,
> orca.dtsi-1282-                 <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>,
> orca.dtsi-1283-                 <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>,
> orca.dtsi-1284-                 <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>,
> --
> orca.dtsi:1288:         syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset",
> orca.dtsi-1289-                 "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus",
> orca.dtsi-1290-                 "bootprotect", "bootvector", "bootaddress_sel";

Again, reset has standard binding. 

Also consider if you really need to access all of these vs. assuming a 
fixed mode that the firmware/bootloader sets up.

> ud710.dtsi:1268:        syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> ud710.dtsi-1269-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> ud710.dtsi-1270-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> ud710.dtsi-1271-                  <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> ud710.dtsi:1272:        syscon-names =  "sd_detect_pol",
> ud710.dtsi-1273-                        "sd_hotplug_protect_en",
> ud710.dtsi-1274-                        "sd_hotplug_debounce_en",
> ud710.dtsi-1275-                        "sd_hotplug_debounce_cn";
> 
> Compare to following,
> 
> sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>;
> sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>;
> sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> .....
> 
> For me, my choice would be the former.
> It looks more clear.
> 
> > and agree about this being harder to parse in an automated way.
> >
> > Orson, do you see any other reason for the combined property?
> No other reason.
> 
> > If not, could you respin the series once more with
> > syscon_regmap_lookup_by_name() replaced by something like:?
> >
> > struct regmap *
> > syscon_regmap_lookup_args_by_phandle(struct device_node *np,
> >                                         const char *property,
> >                                         int arg_count, __u32 *out_args)
> 
> I like this idea. syscon_regmap_lookup_by_phandle_args() would be better?
> 
> May I impelement them both?

No.

Rob