| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191205151237.GA30195@bogus>
Date: Thu, 5 Dec 2019 09:12:37 -0600
From: Rob Herring <robh@...nel.org>
To: Orson Zhai <orson.zhai@...eadtrum.com>
Cc: Arnd Bergmann <arnd@...db.de>, Orson Zhai <orson.zhai@...soc.com>,
Lee Jones <lee.jones@...aro.org>,
Mark Rutland <mark.rutland@....com>,
DTML <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
kevin.tang@...soc.com, baolin.wang@...soc.com,
Chunyan Zhang <chunyan.zhang@...soc.com>
Subject: Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to
syscon easily
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@...nel.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@...nel.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
Powered by blists - more mailing lists