Re: [PATCH v7 07/10] dt-bindings: reset: s700: Add binding constants for mmc
Hi Stephen , Could you please help get this merged ? Thanks -Amit On Fri, Sep 11, 2020 at 10:59 AM Amit Singh Tomar wrote: > > This commit adds device tree binding reset constants for mmc controller > present on Actions S700 Soc. > > Reviewed-by: Manivannan Sadhasivam > Acked-by: Rob Herring > Signed-off-by: Amit Singh Tomar > --- > Changes since v6: > * No change. > Changes since v5: > * Added Mani's Reviewed-by: tag. > Changes since v4: > * No change. > Changes since v3: > * No change. > Changes since v2: > * No change. > Changes since v1: > * No change. > Changes since RFC: > * added Rob's acked-by tag > --- > include/dt-bindings/reset/actions,s700-reset.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/dt-bindings/reset/actions,s700-reset.h > b/include/dt-bindings/reset/actions,s700-reset.h > index 5e3b16b8ef53..a3118de6d7aa 100644 > --- a/include/dt-bindings/reset/actions,s700-reset.h > +++ b/include/dt-bindings/reset/actions,s700-reset.h > @@ -30,5 +30,8 @@ > #define RESET_UART420 > #define RESET_UART521 > #define RESET_UART622 > +#define RESET_SD0 23 > +#define RESET_SD1 24 > +#define RESET_SD2 25 > > #endif /* __DT_BINDINGS_ACTIONS_S700_RESET_H */ > -- > 2.7.4 >
Re: [PATCH v5 10/10] arm64: dts: actions: Add uSD support for Cubieboard7
Hi, > Fixed regulators are used to nicely model the regulators which aren't tied to > any PMIC. But for some cases we use them to represent supplies when there is > no support for the specific PMIC present in the kernel and they are turned > on/configured by the bootloader (this is what happening here). > > And there is no use of declaring fixed regulators when there is no consumer. > Even if you don't define these, the corresponding supplies in the board will > always be in the same state configured by the bootloader. So I'd suggest you > to remove this for now. Checked the schematics and regulator name is the same for both eMMC and uSD Shall we keep uSD regulator sd_vcc to be consistent across ACTIONS platform? > Since I don't have the schematics to check, please make sure you name the > regulators as mentioned in the schematics (this could vary from board to > board, > so don't just copy from others). > Sure, point noted. Thanks -Amit.
Re: [PATCH v5 10/10] arm64: dts: actions: Add uSD support for Cubieboard7
Hi, > But this regulator is only used for the eMMC there, which we apparently > don't have on the Cubieboard 7? We do have eMMC present on Cubieboard 7 (both the versions of Cubieboard7), and the regulator name is similar to what is used in "s900-bubblegum-96.dts" .i.e. "vcc_3v1". But Since this patch doesn't enable eMMC, it does make sense to remove this "vcc_3v1" regulator and keep the other one. > > Also, another day tested it without having these regulators in , and > > still it seems to > > work. So should these be removed ? > > If there are not even referenced in the .dts, then fixed regulators are > rather pointless. So yes, please remove this vcc-3v1 one. Sure, I would do this. > What is the story with the other regulator? Is there a PMIC or a power > switch for the SD card? Or is the power supply actually hardwired? SD_VCC is connected to SWITCH/LDO which gets input from ATM2603C PMIC. This seems to be enabled by default ( in early bootloaders I guess). Thanks -Amit.
Re: [PATCH v5 10/10] arm64: dts: actions: Add uSD support for Cubieboard7
Hi,
On Sun, Jul 12, 2020 at 11:00 PM Manivannan Sadhasivam
wrote:
>
> On Thu, Jul 02, 2020 at 08:22:56PM +0530, Amit Singh Tomar wrote:
> > This commit adds uSD support for Cubieboard7 board based on Actions Semi
> > S700 SoC. SD0 is connected to uSD slot. Since there is no PMIC support
> > added yet, fixed regulator has been used as a regulator node.
> >
> > Signed-off-by: Amit Singh Tomar
> > ---
> > Changes since v4:
> > * No change.
> > Changes since v3:
> > * No change.
> > Changes since v2:
> > * No change.
> > Changes since v1:
> > * No change.
> > Changes since RFC:
> > * No change.
> > ---
> > arch/arm64/boot/dts/actions/s700-cubieboard7.dts | 41
> >
> > arch/arm64/boot/dts/actions/s700.dtsi| 1 +
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
> > b/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
> > index 63e375cd9eb4..ec117eb12f3a 100644
> > --- a/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
> > +++ b/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
> > @@ -13,6 +13,7 @@
> >
> > aliases {
> > serial3 =
> > + mmc0 =
> > };
> >
> > chosen {
> > @@ -28,6 +29,23 @@
> > device_type = "memory";
> > reg = <0x1 0xe000 0x0 0x0>;
> > };
> > +
> > + /* Fixed regulator used in the absence of PMIC */
> > + vcc_3v1: vcc-3v1 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "fixed-3.1V";
> > + regulator-min-microvolt = <310>;
> > + regulator-max-microvolt = <310>;
> > + };
>
> Is this regulator used somewhere?
This is something I copied from bubblegum dts as I wasn't sure what is right way
to include these regulators.
Also, another day tested it without having these regulators in , and
still it seems to
work. So should these be removed ?
Thanks
-Amit
Re: [PATCH v5 01/10] dt-bindings: dmaengine: convert Actions Semi Owl SoCs bindings to yaml
On Fri, Jul 3, 2020 at 12:48 PM Amit Tomer wrote: > > Hi Rob, > > > > If you already ran 'make dt_binding_check' and didn't see the above > > error(s), then make sure dt-schema is up to date: > > > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master > > --upgrade > > > > Please check and re-submit. > > I wasn't able to reproduce it, even after updating the dt-schema. > Kindly have a look at logs: > > https://pastebin.ubuntu.com/p/xTBNNyBdFv/ Looks like, dtschema even after upgrade pointing to older commit "6a941d46b9f5". Wondering why it has not been pointing to latest commit "6a941d46b9f5" After upgrading the pip version for python3, and upgrading the dt-schema again https://pastebin.ubuntu.com/p/Rd9knQgvKH/ Issue is still reproduced. Thanks -Amit
Re: [PATCH v5 01/10] dt-bindings: dmaengine: convert Actions Semi Owl SoCs bindings to yaml
Hi Rob, > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure dt-schema is up to date: > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master > --upgrade > > Please check and re-submit. I wasn't able to reproduce it, even after updating the dt-schema. Kindly have a look at logs: https://pastebin.ubuntu.com/p/xTBNNyBdFv/ Thanks, -Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi, On Tue, Jun 30, 2020 at 7:54 PM Vinod Koul wrote: > > On 30-06-20, 15:17, Amit Tomer wrote: > > Hi Vinod, > > > > On Mon, Jun 29, 2020 at 3:22 PM Vinod Koul wrote: > > > > > If you use of_device_get_match_data() you will not fall into this :) > > > > But again, of_device_get_match_data() returns void *, and we need > > "uintptr_t" in order to type cast it properly (at-least without > > warning). > > Not really, you can cast from void * to you own structure.. btw why do > you need uintptr_t? uintptr_t allows us to cast to an integer type that matches with enum in terms of size, and clang is happy about it (no more such warnings). > The problem is a pointer to enum conversion :) I think the right way > would be to do would be below > > soc_type = (enum foo)of_device_get_match_data(dev); > > or > soc_type = (unsigned long) of_device_get_match_data(dev); > > which I think should be fine in gcc, but possibly give you above warning Yeah, GCC is always fine with it. > in clang.. but i thought that was fixed in clang > https://reviews.llvm.org/D75758 Thanks for pointing this out. To be honest, I thought clang had brought something important which is missed by GCC (via emitting this warning) that needed to be fixed in Kernel code. But looking at this commit[1], feeling that CLANG people just wanted to be compatible with GCC, and in that situation why should one believe the clang ? [1]: https://github.com/ClangBuiltLinux/llvm-project/commit/4fd4438882cc7f78e56e147d52d9a1f63b58ba81 Thanks -Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi Vinod, On Mon, Jun 29, 2020 at 3:22 PM Vinod Koul wrote: > If you use of_device_get_match_data() you will not fall into this :) But again, of_device_get_match_data() returns void *, and we need "uintptr_t" in order to type cast it properly (at-least without warning). Also, while looking around found the similar warning for other file where it uses " of_device_get_match_data()" drivers/pci/controller/pcie-iproc-platform.c:56:15: warning: cast to smaller integer type 'enum iproc_pcie_type' from 'const void *' [-Wvoid-pointer-to-enum-cast] pcie->type = (enum iproc_pcie_type) of_device_get_match_data(dev); Thanks -Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi Vinod,
Thanks for having a look and providing the comments.
> Is the .compatible documented, Documentation patch should come before
> the driver use patch in a series
Yes, this new compatible string is documented in patch (05/10).
I would make it as a patch (1/10).
> > static int owl_dma_probe(struct platform_device *pdev)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > struct owl_dma *od;
> > int ret, i, nr_channels, nr_requests;
> > + const struct of_device_id *of_id =
> > + of_match_device(owl_dma_match, >dev);
>
> You care about driver_data rather than of_id, so using
> of_device_get_match_data() would be better..
Okay. would take care of it in next version.
> > od = devm_kzalloc(>dev, sizeof(*od), GFP_KERNEL);
> > if (!od)
> > @@ -1083,6 +1116,8 @@ static int owl_dma_probe(struct platform_device *pdev)
> > dev_info(>dev, "dma-channels %d, dma-requests %d\n",
> >nr_channels, nr_requests);
> >
> > + od->devid = (enum owl_dma_id)(uintptr_t)of_id->data;
>
> Funny casts, I dont think you need uintptr_t!
But without this cast, clang compiler emits following warning:
warning: cast to smaller integer type 'enum owl_dma_id' from 'const void *'
[-Wvoid-pointer-to-enum-cast]
Thanks
-Amit
Re: [PATCH v4 02/10] dmaengine: Actions: Add support for S700 DMA engine
Hi,
On Wed, Jun 24, 2020 at 3:06 PM André Przywara wrote:
>
> On 24/06/2020 07:15, Vinod Koul wrote:
>
> Hi,
>
> > On 09-06-20, 15:47, Amit Singh Tomar wrote:
> >
> >> @@ -372,6 +383,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan
> >> *vchan,
> >>struct dma_slave_config *sconfig,
> >>bool is_cyclic)
> >> {
> >> +struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> >> u32 mode, ctrlb;
> >>
> >> mode = OWL_DMA_MODE_PW(0);
> >> @@ -427,14 +439,26 @@ static inline int owl_dma_cfg_lli(struct
> >> owl_dma_vchan *vchan,
> >> lli->hw[OWL_DMADESC_DADDR] = dst;
> >> lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >> lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >> -/*
> >> - * Word starts from offset 0xC is shared between frame length
> >> - * (max frame length is 1MB) and frame count, where first 20
> >> - * bits are for frame length and rest of 12 bits are for frame
> >> - * count.
> >> - */
> >> -lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> -lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >> +
> >> +if (od->devid == S700_DMA) {
> >> +/* Max frame length is 1MB */
> >> +lli->hw[OWL_DMADESC_FLEN] = len;
> >> +/*
> >> + * On S700, word starts from offset 0x1C is shared between
> >> + * frame count and ctrlb, where first 12 bits are for frame
> >> + * count and rest of 20 bits are for ctrlb.
> >> + */
> >> +lli->hw[OWL_DMADESC_CTRLB] = FCNT_VAL | ctrlb;
> >> +} else {
> >> +/*
> >> + * On S900, word starts from offset 0xC is shared between
> >> + * frame length (max frame length is 1MB) and frame count,
> >> + * where first 20 bits are for frame length and rest of
> >> + * 12 bits are for frame count.
> >> + */
> >> +lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
> >> +lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> >
> > Unfortunately this wont scale, we will keep adding new conditions for
> > newer SoC's! So rather than this why not encode max frame length in
> > driver_data rather than S900_DMA/S700_DMA.. In future one can add values
> > for newer SoC and not code above logic again.
>
> What newer SoCs? I don't think we should try to guess the future here.
> We can always introduce further abstractions later, once we actually
> *know* what we are looking at.
>
Apart from it , we have *one* more SoC from Actions .i.e. S500 where
the DMA controller is
identical to S900, and requires *no* additional code to work properly.
So, I think we are safe to have the changes proposed in this patch.
Thanks
-Amit
Re: [PATCH v3 01/10] dmaengine: Actions: get rid of bit fields from dma descriptor
Hi,
Thanks for having a look.
On Wed, Jun 3, 2020 at 12:52 PM Manivannan Sadhasivam
wrote:
> Individual comments for these enums?
I was expecting this comment , and thought these fields are self explanatory
But if you prefer to have description about it, I would have it in next version.
> >+enum owl_dmadesc_offsets {
> >+ OWL_DMADESC_NEXT_LLI = 0,
> >+ OWL_DMADESC_SADDR,
> >+ OWL_DMADESC_DADDR,
> >+ OWL_DMADESC_FLEN,
> >+ OWL_DMADESC_SRC_STRIDE,
> >+ OWL_DMADESC_DST_STRIDE,
> >+ OWL_DMADESC_CTRLA,
> >+ OWL_DMADESC_CTRLB,
> >+ OWL_DMADESC_CONST_NUM,
> >+ OWL_DMADESC_SIZE
> > };
> >
> > /**
> >@@ -153,7 +144,7 @@ struct owl_dma_lli_hw {
> > * @node: node for txd's lli_list
> > */
> > struct owl_dma_lli {
> >- struct owl_dma_lli_hw hw;
> >+ u32 hw[OWL_DMADESC_SIZE];
> > dma_addr_t phys;
> > struct list_headnode;
> > };
> >@@ -320,6 +311,11 @@ static inline u32 llc_hw_ctrlb(u32 int_ctl)
> > return ctl;
> > }
> >
> >+static u32 llc_hw_flen(struct owl_dma_lli *lli)
> >+{
> >+ return lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
> >+}
> >+
> > static void owl_dma_free_lli(struct owl_dma *od,
> >struct owl_dma_lli *lli)
> > {
> >@@ -351,8 +347,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct
> >owl_dma_txd *txd,
> > list_add_tail(>node, >lli_list);
> >
> > if (prev) {
> >- prev->hw.next_lli = next->phys;
> >- prev->hw.ctrla |= llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> >+ prev->hw[OWL_DMADESC_NEXT_LLI] = next->phys;
> >+ prev->hw[OWL_DMADESC_CTRLA] |=
> >+ llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> > }
> >
> > return next;
> >@@ -365,8 +362,7 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> > struct dma_slave_config *sconfig,
> > bool is_cyclic)
> > {
> >- struct owl_dma_lli_hw *hw = >hw;
> >- u32 mode;
> >+ u32 mode, ctrlb;
> >
> > mode = OWL_DMA_MODE_PW(0);
> >
> >@@ -407,22 +403,22 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> > return -EINVAL;
> > }
> >
> >- hw->next_lli = 0; /* One link list by default */
> >- hw->saddr = src;
> >- hw->daddr = dst;
> >-
> >- hw->fcnt = 1; /* Frame count fixed as 1 */
> >- hw->flen = len; /* Max frame length is 1MB */
> >- hw->src_stride = 0;
> >- hw->dst_stride = 0;
> >- hw->ctrla = llc_hw_ctrla(mode,
> >- OWL_DMA_LLC_SAV_LOAD_NEXT |
> >- OWL_DMA_LLC_DAV_LOAD_NEXT);
> >+ lli->hw[OWL_DMADESC_CTRLA] = llc_hw_ctrla(mode,
> >+OWL_DMA_LLC_SAV_LOAD_NEXT |
> >+OWL_DMA_LLC_DAV_LOAD_NEXT);
> >
> > if (is_cyclic)
> >- hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> >+ ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> > else
> >- hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+ ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+
> >+ lli->hw[OWL_DMADESC_NEXT_LLI] = 0;
>
> Again, please preserve the old comments.
Sure, would do it.
>
> >+ lli->hw[OWL_DMADESC_SADDR] = src;
> >+ lli->hw[OWL_DMADESC_DADDR] = dst;
> >+ lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >+ lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >+ lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
>
> Please explain what you're doing here.
Actually , in next the patch 2/10 there is comment that explains a bit
about it.
/*
* S700 put flen and fcnt at offset 0x0c and 0x1c respectively,
* whereas S900 put flen and fcnt at offset 0x0c.
*/
Shall I add more details to it in the next patch 02/10 ?
Thanks
-Amit.
Re: [PATCH v2 07/10] dt-bindings: reset: s700: Add binding constants for mmc
Hi, On Wed, May 20, 2020 at 12:03 AM Rob Herring wrote: > > On Tue, May 19, 2020 at 11:49:25PM +0530, Amit Singh Tomar wrote: > > This commit adds device tree binding reset constants for mmc controller > > present on Actions S700 Soc. > > > > Signed-off-by: Amit Singh Tomar > > --- > > Changes since v1: > > * No change. > > Changes since RFC: > > * added Rob's acked-by tag > > And dropped?? Sorry, I just forgot to add it. Thanks -Amit
Re: [PATCH v1 2/3] armv8: gpio: add gpio feature
> Oh, and... u-b...@linux.nxdi.nxp.com bounces because that domain is > not resolvable - I guess that is internal to NXP, and this patch > should have remained within NXP and not been posted publically. Yeah, realized it just after sending my reply. It is for internal NXP list. Had it been for open source U-boot, Author would have chosen "u-b...@lists.denx.de" ? But, I still believe Author did it unknowingly and we all should be easy on him/her. Thanks -Amit
Re: [PATCH v1 2/3] armv8: gpio: add gpio feature
> From what I can tell, these patches are not for the kernel. The > filenames don't match th kernel layout. These files looks to be from U-boot, and must be intended for U-boot as I see U-boot mailing address in recipient's address?
Re: [PATCH 2/3] phy: sun4i: add support for A64 usb phy
Hello ,
> @@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> val = readl(phy->pmu + REG_PMU_UNK_H3);
> writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> } else {
> + /* A64 needs also this unknown bit */
> + if (data->cfg->type == sun50i_a64_phy) {
> + val = readl(phy->pmu + REG_PMU_UNK_H3);
> + writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> + }
> +
This bit is also set for H3, shall we reuse it or we should enclose it
in else-if part ?
Thanks
Amit.
Re: [PATCH 2/3] phy: sun4i: add support for A64 usb phy
Hello ,
> @@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
> val = readl(phy->pmu + REG_PMU_UNK_H3);
> writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> } else {
> + /* A64 needs also this unknown bit */
> + if (data->cfg->type == sun50i_a64_phy) {
> + val = readl(phy->pmu + REG_PMU_UNK_H3);
> + writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> + }
> +
This bit is also set for H3, shall we reuse it or we should enclose it
in else-if part ?
Thanks
Amit.
Re: [PATCH v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Sorry to intervene but just trying to learn from your comments. > You have plenty, and that's the whole of your device space. *All of it*. So > just take the base address of your PCIe controller, and be done with > it. but isn't few of PCIe controller's registers itself are mapped here(base address). So, how can we use this address for MSI? Or you said from base address of PCIe controller, find an offset that can be used as MSI address? Thanks Amit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Sorry to intervene but just trying to learn from your comments. > You have plenty, and that's the whole of your device space. *All of it*. So > just take the base address of your PCIe controller, and be done with > it. but isn't few of PCIe controller's registers itself are mapped here(base address). So, how can we use this address for MSI? Or you said from base address of PCIe controller, find an offset that can be used as MSI address? Thanks Amit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> Without #ifdefs if we compile driver for legacy, MSI structures will not be > available and we get compile time error. Sorry for nitpicking but at least can we use elegant version of #ifdefs .i.e. #if IS_ENABLED() here ? Thanks, Amit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> Without #ifdefs if we compile driver for legacy, MSI structures will not be > available and we get compile time error. Sorry for nitpicking but at least can we use elegant version of #ifdefs .i.e. #if IS_ENABLED() here ? Thanks, Amit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
