| Message ID | 20200613024706.27975-1-alim.akhtar@samsung.com |
|---|---|
| Headers | show |
| Series | exynos-ufs: Add support for UFS HCI | expand |
Hi Alim, On Sat, Jun 13, 2020 at 08:17:00AM +0530, Alim Akhtar wrote: > Some UFS host controllers like Exynos uses granularities of PRDT length and > offset as bytes, whereas others uses actual segment count. > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++++++++++------- > drivers/scsi/ufs/ufshcd.h | 6 ++++++ > 2 files changed, 29 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index ee30ed6cc805..ba093d0d0942 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2151,8 +2151,14 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) > return sg_segments; > > if (sg_segments) { > - lrbp->utr_descriptor_ptr->prd_table_length = > - cpu_to_le16((u16)sg_segments); > + > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) > + lrbp->utr_descriptor_ptr->prd_table_length = > + cpu_to_le16((sg_segments * > + sizeof(struct ufshcd_sg_entry))); > + else > + lrbp->utr_descriptor_ptr->prd_table_length = > + cpu_to_le16((u16) (sg_segments)); > > prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr; > > @@ -3500,11 +3506,21 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba) > cpu_to_le32(upper_32_bits(cmd_desc_element_addr)); > > /* Response upiu and prdt offset should be in double words */ > - utrdlp[i].response_upiu_offset = > - cpu_to_le16(response_offset >> 2); > - utrdlp[i].prd_table_offset = cpu_to_le16(prdt_offset >> 2); > - utrdlp[i].response_upiu_length = > - cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) { > + utrdlp[i].response_upiu_offset = > + cpu_to_le16(response_offset); > + utrdlp[i].prd_table_offset = > + cpu_to_le16(prdt_offset); > + utrdlp[i].response_upiu_length = > + cpu_to_le16(ALIGNED_UPIU_SIZE); > + } else { > + utrdlp[i].response_upiu_offset = > + cpu_to_le16(response_offset >> 2); > + utrdlp[i].prd_table_offset = > + cpu_to_le16(prdt_offset >> 2); > + utrdlp[i].response_upiu_length = > + cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > + } > > ufshcd_init_lrb(hba, &hba->lrb[i], i); > } Isn't this patch missing an update to ufshcd_print_trs()? It uses ->prd_table_length as the number of segments, not the number of bytes. - Eric
Hi Eric, > -----Original Message----- > From: Eric Biggers <ebiggers@kernel.org> > Sent: 12 August 2020 05:59 > To: Alim Akhtar <alim.akhtar@samsung.com> > Cc: robh@kernel.org; devicetree@vger.kernel.org; linux-scsi@vger.kernel.org; > krzk@kernel.org; avri.altman@wdc.com; martin.petersen@oracle.com; > kwmad.kim@samsung.com; stanley.chu@mediatek.com; > cang@codeaurora.org; linux-samsung-soc@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; kishon@ti.com > Subject: Re: [RESEND PATCH v10 04/10] scsi: ufs: introduce > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk > > Hi Alim, > > On Sat, Jun 13, 2020 at 08:17:00AM +0530, Alim Akhtar wrote: > > Some UFS host controllers like Exynos uses granularities of PRDT > > length and offset as bytes, whereas others uses actual segment count. > > > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++++++++++------- > > drivers/scsi/ufs/ufshcd.h | 6 ++++++ > > 2 files changed, 29 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index ee30ed6cc805..ba093d0d0942 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -2151,8 +2151,14 @@ static int ufshcd_map_sg(struct ufs_hba *hba, > struct ufshcd_lrb *lrbp) > > return sg_segments; > > > > if (sg_segments) { > > - lrbp->utr_descriptor_ptr->prd_table_length = > > - cpu_to_le16((u16)sg_segments); > > + > > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) > > + lrbp->utr_descriptor_ptr->prd_table_length = > > + cpu_to_le16((sg_segments * > > + sizeof(struct ufshcd_sg_entry))); > > + else > > + lrbp->utr_descriptor_ptr->prd_table_length = > > + cpu_to_le16((u16) (sg_segments)); > > > > prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr; > > > > @@ -3500,11 +3506,21 @@ static void > ufshcd_host_memory_configure(struct ufs_hba *hba) > > > cpu_to_le32(upper_32_bits(cmd_desc_element_addr)); > > > > /* Response upiu and prdt offset should be in double words */ > > - utrdlp[i].response_upiu_offset = > > - cpu_to_le16(response_offset >> 2); > > - utrdlp[i].prd_table_offset = cpu_to_le16(prdt_offset >> 2); > > - utrdlp[i].response_upiu_length = > > - cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) { > > + utrdlp[i].response_upiu_offset = > > + cpu_to_le16(response_offset); > > + utrdlp[i].prd_table_offset = > > + cpu_to_le16(prdt_offset); > > + utrdlp[i].response_upiu_length = > > + cpu_to_le16(ALIGNED_UPIU_SIZE); > > + } else { > > + utrdlp[i].response_upiu_offset = > > + cpu_to_le16(response_offset >> 2); > > + utrdlp[i].prd_table_offset = > > + cpu_to_le16(prdt_offset >> 2); > > + utrdlp[i].response_upiu_length = > > + cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > > + } > > > > ufshcd_init_lrb(hba, &hba->lrb[i], i); > > } > > Isn't this patch missing an update to ufshcd_print_trs()? It uses > ->prd_table_length as the number of segments, not the number of bytes. > prd_table_length will be populated before it reaches ufshcd_print_trs() based on UFSHCD_QUIRK_PRDT_BYTE_GRAN. > - Eric
Hi Rob Anything else needs to be done for this patch? On Sat, Jun 13, 2020 at 8:36 AM Alim Akhtar <alim.akhtar@samsung.com> wrote: > > This patch adds DT bindings for Samsung ufs hci > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > --- > .../bindings/ufs/samsung,exynos-ufs.yaml | 89 +++++++++++++++++++ > 1 file changed, 89 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml > > diff --git a/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml b/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml > new file mode 100644 > index 000000000000..38193975c9f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml > @@ -0,0 +1,89 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/ufs/samsung,exynos-ufs.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Samsung SoC series UFS host controller Device Tree Bindings > + > +maintainers: > + - Alim Akhtar <alim.akhtar@samsung.com> > + > +description: | > + Each Samsung UFS host controller instance should have its own node. > + This binding define Samsung specific binding other then what is used > + in the common ufshcd bindings > + [1] Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > + > +properties: > + > + compatible: > + enum: > + - samsung,exynos7-ufs > + > + reg: > + items: > + - description: HCI register > + - description: vendor specific register > + - description: unipro register > + - description: UFS protector register > + > + reg-names: > + items: > + - const: hci > + - const: vs_hci > + - const: unipro > + - const: ufsp > + > + clocks: > + items: > + - description: ufs link core clock > + - description: unipro main clock > + > + clock-names: > + items: > + - const: core_clk > + - const: sclk_unipro_main > + > + interrupts: > + maxItems: 1 > + > + phys: > + maxItems: 1 > + > + phy-names: > + const: ufs-phy > + > +required: > + - compatible > + - reg > + - interrupts > + - phys > + - phy-names > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/exynos7-clk.h> > + > + ufs: ufs@15570000 { > + compatible = "samsung,exynos7-ufs"; > + reg = <0x15570000 0x100>, > + <0x15570100 0x100>, > + <0x15571000 0x200>, > + <0x15572000 0x300>; > + reg-names = "hci", "vs_hci", "unipro", "ufsp"; > + interrupts = <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clock_fsys1 ACLK_UFS20_LINK>, > + <&clock_fsys1 SCLK_UFSUNIPRO20_USER>; > + clock-names = "core_clk", "sclk_unipro_main"; > + pinctrl-names = "default"; > + pinctrl-0 = <&ufs_rst_n &ufs_refclk_out>; > + phys = <&ufs_phy>; > + phy-names = "ufs-phy"; > + }; > +... > -- > 2.17.1 >
On Tue, 13 Jul 2021 at 14:36, Alim Akhtar <alim.akhtar@gmail.com> wrote: > > Hi Rob > Anything else needs to be done for this patch? > > On Sat, Jun 13, 2020 at 8:36 AM Alim Akhtar <alim.akhtar@samsung.com> wrote: > > > > This patch adds DT bindings for Samsung ufs hci > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> It has Rob's ack, so it can be taken directly via SCSI tree. Chanho, I guess here is the answer why exynos7-ufs compatible was not documented, so you can build on top of it. Best regards, Krzysztof
> > Hi Rob > > Anything else needs to be done for this patch? > > > > On Sat, Jun 13, 2020 at 8:36 AM Alim Akhtar <alim.akhtar@samsung.com> > wrote: > > > > > > This patch adds DT bindings for Samsung ufs hci > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com> > > It has Rob's ack, so it can be taken directly via SCSI tree. > > Chanho, > I guess here is the answer why exynos7-ufs compatible was not documented, > so you can build on top of it. > Great. I'll update my compatibles on top of this patch. Anyway, who will take this patch? Best Regards, Chanho Park
Chanho, >> It has Rob's ack, so it can be taken directly via SCSI tree. > Anyway, who will take this patch? I can't remember the rationale behind only taking a subset of the exynos series through SCSI. However, it's been over a year. A resend is a prerequisite. -- Martin K. Petersen Oracle Linux Engineering